Summary: | XHRs and preloads cause DocumentLoader::isLoadingInAPISense() to return true | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nate Chapin <japhet> | ||||||||||||||||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Nate Chapin <japhet> | ||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, andersca, ap, beidson, bfulgham, darin, dglazkov, eric, gustavo.noronha, gustavo, koivisto, mjs, tonyg, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||
Bug Depends on: | 63255, 63835 | ||||||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Nate Chapin
2011-06-03 16:36:09 PDT
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. 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. 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. Created attachment 96106 [details]
Test updates
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 :(
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 ! (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(). 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. 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 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
Created attachment 96445 [details]
patch + less crazy timeouts (still depends on test updates)
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 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 Created attachment 96460 [details]
+ a #include
Comment on attachment 96460 [details] + a #include Attachment 96460 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8823063 Comment on attachment 96460 [details] + a #include Attachment 96460 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8812501 Created attachment 96994 [details]
Build file updates for win + gtk
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... (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. +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. 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? 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. (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. 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. (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, 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?
(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. Comment on attachment 96106 [details] Test updates Test updates landed: http://trac.webkit.org/changeset/88709 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 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 Created attachment 97330 [details]
Don't depend in DocumentLoadTiming, and more gtk build fixes.
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 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
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 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 Created attachment 97450 [details]
Moar symbols changes, simpler tests, etc.
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 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
Created attachment 97480 [details]
Fix Windows Build. Again.
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 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
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? (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. > > (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.
(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. > 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")'.
> 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...
(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. 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.
Created attachment 97632 [details]
Merged to trunk
Comment on attachment 97632 [details] Merged to trunk Attachment 97632 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8874708 Comment on attachment 97632 [details] Merged to trunk Attachment 97632 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8876729 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. Created attachment 98074 [details]
Merged to trunk. Again.
Comment on attachment 98074 [details]
Merged to trunk. Again.
This patch is slightly scary, but I think we should move forward cautiously.
Comment on attachment 98074 [details] Merged to trunk. Again. Clearing flags on attachment: 98074 Committed r89503: <http://trac.webkit.org/changeset/89503> All reviewed patches have been landed. Closing bug. The functional part of this patch was reverted in http://trac.webkit.org/changeset/89598. 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. 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. (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. |