Bug 105330 – REGRESSION(r137607): resource load client callbacks are not called for the main resource when loading HTML string

• Status: NEW
• Component: Page Loading

[Carlos Garcia Campos]

Since r137607, the resource load client callbacks are not called when loading with substitute data. I think the problem is that MainResourceLoader is not a ResourceLoader anymore, and only main frame callback are called. This affects most of the WebKit2 GTK+ unit tests, that are timing out, because WebKit2GTK+ always expects main resource load callbacks to be emitted.

[Alexey Proskuryakov]

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

[Carlos Garcia Campos]

Created attachment 180134 [details]
Patch

Notify the frame loader client about main resources loaded from substitute data.

[Build Bot]

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

[WebKit Review Bot]

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

[Nate Chapin]

Created attachment 180188 [details]
japhet's attempt

[Brady Eidson]

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.

[Brady Eidson]

(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.

[Nate Chapin]

(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?

[Brady Eidson]

(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.

[Nate Chapin]

(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.

[Nate Chapin]

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

[Nate Chapin]

Created attachment 180215 [details]
Fix a capitalization issue

[Brady Eidson]

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.

[Nate Chapin]

(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.

[Brady Eidson]

(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.

[WebKit Review Bot]

Comment on attachment 180215 [details]
Fix a capitalization issue

Clearing flags on attachment: 180215

Committed r138222: <http://trac.webkit.org/changeset/138222>

[Carlos Garcia Campos]

Comment on attachment 180134 [details]
Patch

Clearing flags, landed patch works much better than mine, thanks Nate!

[Csaba Osztrogonác]

(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.

[Chris Dumez]

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.

[Carlos Garcia Campos]

(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.

[Chris Dumez]

(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.

[Ryosuke Niwa]

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