Bug 62066 – XHRs and preloads cause DocumentLoader::isLoadingInAPISense() to return true

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

(Original report at http://code.google.com/p/chromium/issues/detail?id=41726)

DocumentLoader::isLoadingInAPISense()'s meaning is a little fuzzy, but it seems like the intent is to indicate something similar to what the Load event does.  Given that isLoadingInAPISense() affects when the loading spinner stops in at least Chromium and Safari, it seems like requests that are hiding from CachedResourceLoader::requestCount() shouldn't cause it to return true.  So I think it makes sense to make two changes:
* Remove the check against m_subresourceLoaders.isEmpty(), since this includes all SubresourceLoaders and not just CachedResources.
* Ensure the load event has finished before isLoadingInAPISense() starts returning false (and try to merge this and the load event firing logic in the long term).

The WebCore piece of this will be pretty trivial, but a bunch of XHR tests will need to be rewritten to use waitUntilDone()/notifyDone(), since XHRs will no longer prevent DRT from ending the test.

Does this seem sane?

[Adam Barth]

If we can change this to be just after the load event fires, we can change the name to DocumentLoader::hasLoadEventFired(), which might mean something more concrete.

[Alexey Proskuryakov]

It seems reasonable to keep spinning a load indicator for dynamic requests that are started during load, but probably not for preloads.

I don't know what exactly client expectations are. In WebCore, it affects a large number of code paths.

[Adam Barth]

The spinning is super annoying on Gmail.  Apparently working around the spinning on the server side is painful (and makes things slow).  Another possible solution is to change how we trigger the throbber.

[Nate Chapin]

Created attachment 96106 [details]
Test updates

[Nate Chapin]

Created attachment 96108 [details]
patch - depends on test updates

This includes a test change that adds a setTimeout().  I'm very sorry.  I couldn't figure out how to avoid it :(

[Adam Barth]

Comment on attachment 96108 [details]
patch - depends on test updates

View in context: https://bugs.webkit.org/attachment.cgi?id=96108&action=review

> LayoutTests/http/tests/loading/preload-append-scan.php:14
> +setTimeout(function() { layoutTestController.notifyDone(); }, 5000);

5000 !

[Nate Chapin]

(In reply to comment #6)
> (From update of attachment 96108 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96108&action=review
> 
> > LayoutTests/http/tests/loading/preload-append-scan.php:14
> > +setTimeout(function() { layoutTestController.notifyDone(); }, 5000);
> 
> 5000 !

Yeah.......I tried it with 2000 and it was failing locally, which doesn't bode well.

I crave advice on how to avoid that setTimeout().

[Adam Barth]

Comment on attachment 96108 [details]
patch - depends on test updates

View in context: https://bugs.webkit.org/attachment.cgi?id=96108&action=review

>>> LayoutTests/http/tests/loading/preload-append-scan.php:14
>>> +setTimeout(function() { layoutTestController.notifyDone(); }, 5000);
>> 
>> 5000 !
> 
> Yeah.......I tried it with 2000 and it was failing locally, which doesn't bode well.
> 
> I crave advice on how to avoid that setTimeout().

Yeah, the problem is you need to wait for the preload to finish...  Maybe you can ask DRT to wait until the resource loads?  It gets called (that's how it prints out the type).  You could even give a MIME type that it could wait for.

[WebKit Review Bot]

Comment on attachment 96108 [details]
patch - depends on test updates

Attachment 96108 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8776611

New failing tests:
http/tests/xmlhttprequest/access-control-preflight-async-not-supported.html
http/tests/xmlhttprequest/interactive-state.html
http/tests/xmlhttprequest/cross-origin-preflight-get.html
html5lib/runner.html
http/tests/xmlhttprequest/response-encoding.html
http/tests/xmlhttprequest/access-control-preflight-async-header-denied.html
http/tests/xmlhttprequest/access-control-basic-non-simple-allow-async.html
fast/preloader/scan-body-from-head-import.html
fast/xmlhttprequest/xmlhttprequest-html-response-encoding.html
http/tests/xmlhttprequest/event-listener-gc.html
http/tests/xmlhttprequest/access-control-preflight-async-method-denied.html
html5lib/webkit-resumer.html
fast/xmlhttprequest/xmlhttprequest-gc.html
http/tests/xmlhttprequest/access-control-basic-denied-preflight-cache.html
fast/xmlhttprequest/xmlhttprequest-get.xhtml

[WebKit Review Bot]

Created attachment 96400 [details]
Archive of layout-test-results from cr-jail-8

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: cr-jail-8  Port: Mac  Platform: Mac OS X 10.6.7

[Nate Chapin]

Created attachment 96445 [details]
patch + less crazy timeouts (still depends on test updates)

[Gustavo Noronha (kov)]

Comment on attachment 96445 [details]
patch + less crazy timeouts (still depends on test updates)

Attachment 96445 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8788593

[WebKit Review Bot]

Comment on attachment 96445 [details]
patch + less crazy timeouts (still depends on test updates)

Attachment 96445 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8786662

[Nate Chapin]

Created attachment 96460 [details]
+ a #include

[Gustavo Noronha (kov)]

Comment on attachment 96460 [details]
+ a #include

Attachment 96460 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8823063

[Collabora GTK+ EWS bot]

Comment on attachment 96460 [details]
+ a #include

Attachment 96460 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8812501

[Nate Chapin]

Created attachment 96994 [details]
Build file updates for win + gtk

[Adam Barth]

Comment on attachment 96994 [details]
Build file updates for win + gtk

View in context: https://bugs.webkit.org/attachment.cgi?id=96994&action=review

> Source/WebCore/testing/Internals.cpp:53
> +    Document* document = m_frame->document();
> +    if (!document)
> +        return false;

m_frame->document() can't ever be null.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:690
> +bool CachedResourceLoader::isPreload(const KURL& url) const

isPreloaded ?  isPreloading ?  willBePreloaded ?

> Source/WebCore/loader/DocumentLoader.cpp:439
> +        if ((!m_primaryLoadComplete || !m_documentLoadTiming.loadEventEnd) && isLoading())

Is it legit to use m_documentLoadTiming to affect behavior?  I'm not sure where m_documentLoadTiming is just for measurement...

[Nate Chapin]

(In reply to comment #18)
> (From update of attachment 96994 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96994&action=review
> 
> > Source/WebCore/testing/Internals.cpp:53
> > +    Document* document = m_frame->document();
> > +    if (!document)
> > +        return false;
> 
> m_frame->document() can't ever be null.
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:690
> > +bool CachedResourceLoader::isPreload(const KURL& url) const
> 
> isPreloaded ?  isPreloading ?  willBePreloaded ?

As implemented, it should be isPreloaded().  Will rename.

> 
> > Source/WebCore/loader/DocumentLoader.cpp:439
> > +        if ((!m_primaryLoadComplete || !m_documentLoadTiming.loadEventEnd) && isLoading())
> 
> Is it legit to use m_documentLoadTiming to affect behavior?  I'm not sure where m_documentLoadTiming is just for measurement...

I assumed so based on http://trac.webkit.org/browser/trunk/Source/WebCore/page/DOMWindow.cpp#L1565, but I'm willing to consider otherwise.

[Adam Barth]

+tonyg

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 96994 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=96994&action=review
> > 
> > > Source/WebCore/loader/DocumentLoader.cpp:439
> > > +        if ((!m_primaryLoadComplete || !m_documentLoadTiming.loadEventEnd) && isLoading())
> > 
> > Is it legit to use m_documentLoadTiming to affect behavior?  I'm not sure where m_documentLoadTiming is just for measurement...
> 
> I assumed so based on http://trac.webkit.org/browser/trunk/Source/WebCore/page/DOMWindow.cpp#L1565, but I'm willing to consider otherwise.

Ok.  I've added tonyg just in case.

[Nate Chapin]

A(In reply to comment #20)
> +tonyg
> 
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (From update of attachment 96994 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=96994&action=review
> > > 
> > > > Source/WebCore/loader/DocumentLoader.cpp:439
> > > > +        if ((!m_primaryLoadComplete || !m_documentLoadTiming.loadEventEnd) && isLoading())
> > > 
> > > Is it legit to use m_documentLoadTiming to affect behavior?  I'm not sure where m_documentLoadTiming is just for measurement...
> > 
> > I assumed so based on http://trac.webkit.org/browser/trunk/Source/WebCore/page/DOMWindow.cpp#L1565, but I'm willing to consider otherwise.
> 
> Ok.  I've added tonyg just in case.

Are we sufficiently confident that this patch is the Right Way that the "Test updates" patch can be reviewed?

[Adam Barth]

Comment on attachment 96106 [details]
Test updates

View in context: https://bugs.webkit.org/attachment.cgi?id=96106&action=review

This is fine, assuming you've checked that other browsers don't hold the load event for XHRs.

> LayoutTests/http/tests/xmlhttprequest/resources/uri-resolution-opera-open-008-iframe.html:20
> +            if (window.layoutTestController)
> +                        layoutTestController.notifyDone();

Crazy indent.

[Nate Chapin]

(In reply to comment #22)
> (From update of attachment 96106 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96106&action=review
> 
> This is fine, assuming you've checked that other browsers don't hold the load event for XHRs.

I think I did, but I'll double check before landing.

> 
> > LayoutTests/http/tests/xmlhttprequest/resources/uri-resolution-opera-open-008-iframe.html:20
> > +            if (window.layoutTestController)
> > +                        layoutTestController.notifyDone();
> 
> Crazy indent.

[Tony Gentilcore]

Comment on attachment 96994 [details]
Build file updates for win + gtk

View in context: https://bugs.webkit.org/attachment.cgi?id=96994&action=review

>>>>> Source/WebCore/loader/DocumentLoader.cpp:439
>>>>> +        if ((!m_primaryLoadComplete || !m_documentLoadTiming.loadEventEnd) && isLoading())
>>>> 
>>>> Is it legit to use m_documentLoadTiming to affect behavior?  I'm not sure where m_documentLoadTiming is just for measurement...
>>> 
>>> I assumed so based on http://trac.webkit.org/browser/trunk/Source/WebCore/page/DOMWindow.cpp#L1565, but I'm willing to consider otherwise.
>> 
>> Ok.  I've added tonyg just in case.
> 
> Are we sufficiently confident that this patch is the Right Way that the "Test updates" patch can be reviewed?

Is FrameLoader's m_isComplete any good for this purpose? Maybe not, it becomes true when the load event is ready to fire not after it has been handled.

While I can't think of anything that will break, something does feel wrong about using these times to affect behavior. Do you think it is worth adding another bool for code cleanliness or is it best to just re-purpose this?

FWIW, L1565 in DOMWindow is only used to decide whether to set other times in the timing structure, not for any more serious decisions like this.

[Nate Chapin]

(In reply to comment #24)
> (From update of attachment 96994 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96994&action=review
> 
> >>>>> Source/WebCore/loader/DocumentLoader.cpp:439
> >>>>> +        if ((!m_primaryLoadComplete || !m_documentLoadTiming.loadEventEnd) && isLoading())
> >>>> 
> >>>> Is it legit to use m_documentLoadTiming to affect behavior?  I'm not sure where m_documentLoadTiming is just for measurement...
> >>> 
> >>> I assumed so based on http://trac.webkit.org/browser/trunk/Source/WebCore/page/DOMWindow.cpp#L1565, but I'm willing to consider otherwise.
> >> 
> >> Ok.  I've added tonyg just in case.
> > 
> > Are we sufficiently confident that this patch is the Right Way that the "Test updates" patch can be reviewed?
> 
> Is FrameLoader's m_isComplete any good for this purpose? Maybe not, it becomes true when the load event is ready to fire not after it has been handled.
> 
> While I can't think of anything that will break, something does feel wrong about using these times to affect behavior. Do you think it is worth adding another bool for code cleanliness or is it best to just re-purpose this?
> 
> FWIW, L1565 in DOMWindow is only used to decide whether to set other times in the timing structure, not for any more serious decisions like this.

That's true,.  Now that I look at it, whenever we have something from DocumentLoadTiming in an if statement, it's to make a decision about something else timing-related.

Adam, do you think an m_loadEventFired bool is the way to go?

[Adam Barth]

> Adam, do you think an m_loadEventFired bool is the way to go?

I'm sure there's already a bool for whether the load event has fired.  Maybe in FrameLoader?

[Nate Chapin]

(In reply to comment #26)
> > Adam, do you think an m_loadEventFired bool is the way to go?
> 
> I'm sure there's already a bool for whether the load event has fired.  Maybe in FrameLoader?

FrameLoader::m_didCallImplicitClose is the closest thing I'm aware of.

[Nate Chapin]

Comment on attachment 96106 [details]
Test updates

Test updates landed: http://trac.webkit.org/changeset/88709

[Gustavo Noronha (kov)]

Comment on attachment 96994 [details]
Build file updates for win + gtk

Attachment 96994 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8835418

[Collabora GTK+ EWS bot]

Comment on attachment 96994 [details]
Build file updates for win + gtk

Attachment 96994 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8841035

[Nate Chapin]

Created attachment 97330 [details]
Don't depend in DocumentLoadTiming, and more gtk build fixes.

[WebKit Review Bot]

Comment on attachment 97330 [details]
Don't depend in DocumentLoadTiming, and more gtk build fixes.

Attachment 97330 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8865238

New failing tests:
fast/preloader/scan-body-from-head-import.html
fast/preloader/scan-body-from-head-script.html

[WebKit Review Bot]

Created attachment 97339 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Gustavo Noronha (kov)]

Comment on attachment 97330 [details]
Don't depend in DocumentLoadTiming, and more gtk build fixes.

Attachment 97330 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8865257

[Collabora GTK+ EWS bot]

Comment on attachment 97330 [details]
Don't depend in DocumentLoadTiming, and more gtk build fixes.

Attachment 97330 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8843804

[Nate Chapin]

Created attachment 97450 [details]
Moar symbols changes, simpler tests, etc.

[WebKit Review Bot]

Comment on attachment 97450 [details]
Moar symbols changes, simpler tests, etc.

Attachment 97450 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8877267

New failing tests:
fast/preloader/scan-body-from-head-script.html

[WebKit Review Bot]

Created attachment 97467 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nate Chapin]

Created attachment 97480 [details]
Fix Windows Build. Again.

[WebKit Review Bot]

Comment on attachment 97480 [details]
Fix Windows Build. Again.

Attachment 97480 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8872293

New failing tests:
fast/preloader/scan-body-from-head-script.html

[WebKit Review Bot]

Created attachment 97485 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

Comment on attachment 97480 [details]
Fix Windows Build. Again.

View in context: https://bugs.webkit.org/attachment.cgi?id=97480&action=review

> LayoutTests/http/tests/loading/preload-append-scan.php:15
> +    var result;
> +    if (internals.isPreloaded("resources/preload-test.jpg"))

Should we check for the existence of window.internals before using it?

> Source/WebCore/testing/Internals.h:46
> +    Frame* m_frame;

Is there a risk of this pointer becoming garbage since Internals is refcounted and could out-live Frame?

[Nate Chapin]

(In reply to comment #42)
> (From update of attachment 97480 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97480&action=review
> 
> > LayoutTests/http/tests/loading/preload-append-scan.php:15
> > +    var result;
> > +    if (internals.isPreloaded("resources/preload-test.jpg"))
> 
> Should we check for the existence of window.internals before using it?

The test is by definition DRT-only if it's using window.internals, and I don't have a strong opinion about how we should fail if this is opened manually.

> 
> > Source/WebCore/testing/Internals.h:46
> > +    Frame* m_frame;
> 
> Is there a risk of this pointer becoming garbage since Internals is refcounted and could out-live Frame?

Hmm.....I don't remember enough of the Frame detach timing to be sure.  Am I right in thinking that all scripts will stop during detach?  If so, then it comes down to whether any script objects can remain un-gced past script shutdown, since internals should only be ref'ed in JS.

[Adam Barth]

> > (From update of attachment 97480 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=97480&action=review
> > 
> > > Source/WebCore/testing/Internals.h:46
> > > +    Frame* m_frame;
> > 
> > Is there a risk of this pointer becoming garbage since Internals is refcounted and could out-live Frame?
> 
> Hmm.....I don't remember enough of the Frame detach timing to be sure.  Am I right in thinking that all scripts will stop during detach?  If so, then it comes down to whether any script objects can remain un-gced past script shutdown, since internals should only be ref'ed in JS.

So, this is actually a problem.  The internals JS object can live arbitrarily long, but the Frame will eventually die and you'll have a garbage pointer.  Most of the other objects that hold Frame pointers have a "disconnect" type function that sets their Frame pointer to zero.

[Nate Chapin]

(In reply to comment #44)
> > > (From update of attachment 97480 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=97480&action=review
> > > 
> > > > Source/WebCore/testing/Internals.h:46
> > > > +    Frame* m_frame;
> > > 
> > > Is there a risk of this pointer becoming garbage since Internals is refcounted and could out-live Frame?
> > 
> > Hmm.....I don't remember enough of the Frame detach timing to be sure.  Am I right in thinking that all scripts will stop during detach?  If so, then it comes down to whether any script objects can remain un-gced past script shutdown, since internals should only be ref'ed in JS.
> 
> So, this is actually a problem.  The internals JS object can live arbitrarily long, but the Frame will eventually die and you'll have a garbage pointer.  Most of the other objects that hold Frame pointers have a "disconnect" type function that sets their Frame pointer to zero.

Ok.....maybe we should have a  RefPtr<Frame>, and just clear it when Internals is deleted?  It means the Frame will keep living until Internals goes away, but it handles our lifetime concerns and we don't need to teach Frame to disconnect something that is only there in DRT.

[Alexey Proskuryakov]

> The test is by definition DRT-only if it's using window.internals, and I don't have a strong opinion about how we should fail if this is opened manually.

A test that can not work in browser should have text explaining why it doesn't work. Something like 'if (!window.internals) document.write("This test only works as an automated one")'.

[Adam Barth]

> Ok.....maybe we should have a  RefPtr<Frame>, and just clear it when Internals is deleted?  It means the Frame will keep living until Internals goes away, but it handles our lifetime concerns and we don't need to teach Frame to disconnect something that is only there in DRT.

It seems like a bad idea to have a different Frame lifetime during testing than in production...

[Darin Adler]

(In reply to comment #44)
> Most of the other objects that hold Frame pointers have a "disconnect" type function that sets their Frame pointer to zero.

I think that’s the right pattern to use here.

[Nate Chapin]

Created attachment 97626 [details]
Pass a Document* into isPreloaded()

At dglazkov's suggestion, I've removed Internals::m_frame and instead am passing a Document* into isPreloaded() to solve our lifetime issues.

[Nate Chapin]

Created attachment 97632 [details]
Merged to trunk

[Collabora GTK+ EWS bot]

Comment on attachment 97632 [details]
Merged to trunk

Attachment 97632 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8874708

[Gustavo Noronha (kov)]

Comment on attachment 97632 [details]
Merged to trunk

Attachment 97632 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8876729

[Eric Seidel (no email)]

Comment on attachment 96106 [details]
Test updates

Cleared Adam Barth's review+ from obsolete attachment 96106 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Nate Chapin]

Created attachment 98074 [details]
Merged to trunk. Again.

[Adam Barth]

Comment on attachment 98074 [details]
Merged to trunk. Again.

This patch is slightly scary, but I think we should move forward cautiously.

[WebKit Review Bot]

Comment on attachment 98074 [details]
Merged to trunk. Again.

Clearing flags on attachment: 98074

Committed r89503: <http://trac.webkit.org/changeset/89503>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nate Chapin]

The functional part of this patch was reverted in http://trac.webkit.org/changeset/89598.

[Brent Fulgham]

This patch broke the WinCairo build.  The WebKit2.def changes were not also made to the WebKit2CFLite.def file.

Corrected and landed in http://trac.webkit.org/changeset/89757.

No further action is needed.

[Nate Chapin]

Created attachment 99532 [details]
Version for re-landing

This is basically what was reverted before.  http://trac.webkit.org/changeset/90284 takes care of the WebKit2/mac crashes.

[Nate Chapin]

(In reply to comment #60)
> Created an attachment (id=99532) [details]
> Version for re-landing
> 
> This is basically what was reverted before.  http://trac.webkit.org/changeset/90284 takes care of the WebKit2/mac crashes.

(In reply to comment #60)
> Created an attachment (id=99532) [details]
> Version for re-landing
> 
> This is basically what was reverted before.  http://trac.webkit.org/changeset/90284 takes care of the WebKit2/mac crashes.

Lhttp://trac.webkit.org/changeset/90471.  Since this patch is identical to what was reverted and the crash was fixed elsewhere, this was landed based on abarth's previous r+ and his offline approval.