Bug 105330

WebArchives and application cache both use substitute data, so this may well affect them, too.

Created attachment 180134 [details] Patch Notify the frame loader client about main resources loaded from substitute data.

Comment on attachment 180134 [details] Patch Attachment 180134 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15411596 New failing tests: fast/frames/frame-unload-crash2.html http/tests/appcache/crash-when-navigating-away-then-back.html fast/forms/number/number-spinbutton-crash.html

Comment on attachment 180134 [details] Patch Attachment 180134 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15411644 New failing tests: platform/chromium/fast/forms/suggestion-picker/month-suggestion-picker-reset-value-after-reload.html fast/forms/week-multiple-fields/week-multiple-fields-spinbutton-change-and-input-events.html platform/chromium/fast/forms/suggestion-picker/week-suggestion-picker-reset-value-after-reload.html platform/chromium/fast/forms/suggestion-picker/datetime-suggestion-picker-reset-value-after-reload.html fast/forms/month-multiple-fields/month-multiple-fields-spinbutton-change-and-input-events.html fast/forms/time-multiple-fields/time-multiple-fields-state-change-on-focus-or-blur.html fast/forms/time-multiple-fields/time-multiple-fields-spinbutton-change-and-input-events.html platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-reset-value-after-reload.html fast/forms/number/number-spinbutton-crash.html fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-spinbutton-change-and-input-events.html fast/frames/frame-unload-crash2.html fast/forms/date-multiple-fields/date-multiple-fields-spinbutton-change-and-input-events.html platform/chromium/fast/forms/suggestion-picker/datetimelocal-suggestion-picker-reset-value-after-reload.html

Created attachment 180188 [details] japhet's attempt

Comment on attachment 180188 [details] japhet's attempt View in context: https://bugs.webkit.org/attachment.cgi?id=180188&action=review > Source/WebCore/loader/MainResourceLoader.cpp:697 > unsigned long MainResourceLoader::identifier() const > { > + if (m_identifier) > + return m_identifier; > if (ResourceLoader* resourceLoader = loader()) > return resourceLoader->identifier(); This breaks the "setIdentifier" API on ResourceLoader that WebKit2 relies on.

(In reply to comment #6) > (From update of attachment 180188 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180188&action=review > > > Source/WebCore/loader/MainResourceLoader.cpp:697 > > unsigned long MainResourceLoader::identifier() const > > { > > + if (m_identifier) > > + return m_identifier; > > if (ResourceLoader* resourceLoader = loader()) > > return resourceLoader->identifier(); > > This breaks the "setIdentifier" API on ResourceLoader that WebKit2 relies on. Now that we've made MainResourceLoader no longer be a ResourceLoader, we're going to start getting in to trouble as we diverge the two. Having two copies of the identifier seems like a great first step towards breaking things.

(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 180188 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=180188&action=review > > > > > Source/WebCore/loader/MainResourceLoader.cpp:697 > > > unsigned long MainResourceLoader::identifier() const > > > { > > > + if (m_identifier) > > > + return m_identifier; > > > if (ResourceLoader* resourceLoader = loader()) > > > return resourceLoader->identifier(); > > > > This breaks the "setIdentifier" API on ResourceLoader that WebKit2 relies on. > > Now that we've made MainResourceLoader no longer be a ResourceLoader, we're going to start getting in to trouble as we diverge the two. Having two copies of the identifier seems like a great first step towards breaking things. MainResourceLoader::m_identifier should only be used when it is a SubstituteData load. In that case, there should not be a ResourceLoader. So there should still only be one identifier per load. Should I consider an assert to that effect here?

(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > (From update of attachment 180188 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=180188&action=review > > > > > > > Source/WebCore/loader/MainResourceLoader.cpp:697 > > > > unsigned long MainResourceLoader::identifier() const > > > > { > > > > + if (m_identifier) > > > > + return m_identifier; > > > > if (ResourceLoader* resourceLoader = loader()) > > > > return resourceLoader->identifier(); > > > > > > This breaks the "setIdentifier" API on ResourceLoader that WebKit2 relies on. > > > > Now that we've made MainResourceLoader no longer be a ResourceLoader, we're going to start getting in to trouble as we diverge the two. Having two copies of the identifier seems like a great first step towards breaking things. > > MainResourceLoader::m_identifier should only be used when it is a SubstituteData load. In that case, there should not be a ResourceLoader. So there should still only be one identifier per load. > > Should I consider an assert to that effect here? Yes please. On that note, I might also feel better about it having a different name, like in cgarcia's patch.

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (In reply to comment #6) > > > > (From update of attachment 180188 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=180188&action=review > > > > > > > > > Source/WebCore/loader/MainResourceLoader.cpp:697 > > > > > unsigned long MainResourceLoader::identifier() const > > > > > { > > > > > + if (m_identifier) > > > > > + return m_identifier; > > > > > if (ResourceLoader* resourceLoader = loader()) > > > > > return resourceLoader->identifier(); > > > > > > > > This breaks the "setIdentifier" API on ResourceLoader that WebKit2 relies on. > > > > > > Now that we've made MainResourceLoader no longer be a ResourceLoader, we're going to start getting in to trouble as we diverge the two. Having two copies of the identifier seems like a great first step towards breaking things. > > > > MainResourceLoader::m_identifier should only be used when it is a SubstituteData load. In that case, there should not be a ResourceLoader. So there should still only be one identifier per load. > > > > Should I consider an assert to that effect here? > > Yes please. > > On that note, I might also feel better about it having a different name, like in cgarcia's patch. Yeah, that's a good point. Will do both.

Created attachment 180206 [details] Add an identifier ASSERT, rename m_identifier

Created attachment 180215 [details] Fix a capitalization issue

Comment on attachment 180215 [details] Fix a capitalization issue I kind of cringe at "!loader()" being the check for "is this a data load", but I guess it already has precedent.

(In reply to comment #13) > (From update of attachment 180215 [details]) > I kind of cringe at "!loader()" being the check for "is this a data load", but I guess it already has precedent. Yeah :( I had done it that way because I foresaw using the same if() statement to fake resource load callbacks for main resource cache hits (once cache hits are permitted). I can make it based on a non-zero m_substituteDataLoadIdentifier in the interim if you prefer.

(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 180215 [details] [details]) > > I kind of cringe at "!loader()" being the check for "is this a data load", but I guess it already has precedent. > > Yeah :( > > I had done it that way because I foresaw using the same if() statement to fake resource load callbacks for main resource cache hits (once cache hits are permitted). I can make it based on a non-zero m_substituteDataLoadIdentifier in the interim if you prefer. We'd still need a different check to *get* the substitute identifier, so then we'd have two types of checks... this is fine for now.

Comment on attachment 180215 [details] Fix a capitalization issue Clearing flags on attachment: 180215 Committed r138222: <http://trac.webkit.org/changeset/138222>

Comment on attachment 180134 [details] Patch Clearing flags, landed patch works much better than mine, thanks Nate!

(In reply to comment #16) > (From update of attachment 180215 [details]) > Clearing flags on attachment: 180215 > > Committed r138222: <http://trac.webkit.org/changeset/138222> It caused a regression on Qt - see https://bugs.webkit.org/show_bug.cgi?id=105529 for details.

After this patch, http/tests/appcache/main-resource-redirect.html is crashing on EFL port. The reason for this is that ResourceLoadClient::didReceiveResponseForResource() is now called with 0 identifier (which is not a valid key in our HashTable). Could you tell me if this is expected? If 0 is now a valid resource identifier, then I should probably use WTF::UnsignedWithZeroKeyHashTraits for our Resource HashTable.

(In reply to comment #19) > After this patch, http/tests/appcache/main-resource-redirect.html is crashing on EFL port. The reason for this is that ResourceLoadClient::didReceiveResponseForResource() is now called with 0 identifier (which is not a valid key in our HashTable). > > Could you tell me if this is expected? If 0 is now a valid resource identifier, then I should probably use WTF::UnsignedWithZeroKeyHashTraits for our Resource HashTable. Sounds like a bug, I don't think 0 is a valid resource identifier.

(In reply to comment #20) > (In reply to comment #19) > > After this patch, http/tests/appcache/main-resource-redirect.html is crashing on EFL port. The reason for this is that ResourceLoadClient::didReceiveResponseForResource() is now called with 0 identifier (which is not a valid key in our HashTable). > > > > Could you tell me if this is expected? If 0 is now a valid resource identifier, then I should probably use WTF::UnsignedWithZeroKeyHashTraits for our Resource HashTable. > > Sounds like a bug, I don't think 0 is a valid resource identifier. Thanks for your reply. I filed Bug 105788 to track the regression.

This patch caused a serious memory leak. See the bug 106137.