Bug 62066 - XHRs and preloads cause DocumentLoader::isLoadingInAPISense() to return true
Summary: XHRs and preloads cause DocumentLoader::isLoadingInAPISense() to return true
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on: 63255 63835
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-03 16:36 PDT by Nate Chapin
Modified: 2011-07-06 11:02 PDT (History)
15 users (show)

See Also:


Attachments
Test updates (32.18 KB, patch)
2011-06-06 12:03 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
patch - depends on test updates (8.53 KB, patch)
2011-06-06 12:28 PDT, Nate Chapin
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from cr-jail-8 (273.55 KB, application/zip)
2011-06-08 02:10 PDT, WebKit Review Bot
no flags Details
patch + less crazy timeouts (still depends on test updates) (15.27 KB, patch)
2011-06-08 11:38 PDT, Nate Chapin
gns: commit-queue-
Details | Formatted Diff | Diff
+ a #include (15.29 KB, patch)
2011-06-08 12:21 PDT, Nate Chapin
gns: commit-queue-
Details | Formatted Diff | Diff
Build file updates for win + gtk (16.97 KB, patch)
2011-06-13 13:06 PDT, Nate Chapin
gns: commit-queue-
Details | Formatted Diff | Diff
Don't depend in DocumentLoadTiming, and more gtk build fixes. (22.17 KB, patch)
2011-06-15 11:18 PDT, Nate Chapin
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.56 MB, application/zip)
2011-06-15 11:53 PDT, WebKit Review Bot
no flags Details
Moar symbols changes, simpler tests, etc. (24.09 KB, patch)
2011-06-16 09:14 PDT, Nate Chapin
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.26 MB, application/zip)
2011-06-16 11:00 PDT, WebKit Review Bot
no flags Details
Fix Windows Build. Again. (24.16 KB, patch)
2011-06-16 13:08 PDT, Nate Chapin
abarth: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.18 MB, application/zip)
2011-06-16 13:47 PDT, WebKit Review Bot
no flags Details
Pass a Document* into isPreloaded() (24.00 KB, patch)
2011-06-17 11:28 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Merged to trunk (23.96 KB, patch)
2011-06-17 12:12 PDT, Nate Chapin
gustavo.noronha: commit-queue-
Details | Formatted Diff | Diff
Merged to trunk. Again. (22.85 KB, patch)
2011-06-21 16:14 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Version for re-landing (8.12 KB, patch)
2011-07-01 16:02 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2011-06-03 16:36:09 PDT
(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?
Comment 1 Adam Barth 2011-06-03 16:39:23 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.
Comment 2 Alexey Proskuryakov 2011-06-03 17:11:20 PDT
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.
Comment 3 Adam Barth 2011-06-03 17:21:48 PDT
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.
Comment 4 Nate Chapin 2011-06-06 12:03:59 PDT
Created attachment 96106 [details]
Test updates
Comment 5 Nate Chapin 2011-06-06 12:28:15 PDT
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 6 Adam Barth 2011-06-06 12:30:02 PDT
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 !
Comment 7 Nate Chapin 2011-06-06 12:39:24 PDT
(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 8 Adam Barth 2011-06-06 14:08:25 PDT
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 9 WebKit Review Bot 2011-06-08 02:10:50 PDT
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
Comment 10 WebKit Review Bot 2011-06-08 02:10:54 PDT
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
Comment 11 Nate Chapin 2011-06-08 11:38:31 PDT
Created attachment 96445 [details]
patch + less crazy timeouts (still depends on test updates)
Comment 12 Gustavo Noronha (kov) 2011-06-08 11:57:40 PDT
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 13 WebKit Review Bot 2011-06-08 11:58:16 PDT
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
Comment 14 Nate Chapin 2011-06-08 12:21:12 PDT
Created attachment 96460 [details]
+ a #include
Comment 15 Gustavo Noronha (kov) 2011-06-08 18:30:58 PDT
Comment on attachment 96460 [details]
+ a #include

Attachment 96460 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8823063
Comment 16 Collabora GTK+ EWS bot 2011-06-08 18:48:05 PDT
Comment on attachment 96460 [details]
+ a #include

Attachment 96460 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8812501
Comment 17 Nate Chapin 2011-06-13 13:06:23 PDT
Created attachment 96994 [details]
Build file updates for win + gtk
Comment 18 Adam Barth 2011-06-13 13:21:32 PDT
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...
Comment 19 Nate Chapin 2011-06-13 13:26:15 PDT
(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.
Comment 20 Adam Barth 2011-06-13 13:34:04 PDT
+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.
Comment 21 Nate Chapin 2011-06-13 13:35:33 PDT
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 22 Adam Barth 2011-06-13 13:38:59 PDT
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.
Comment 23 Nate Chapin 2011-06-13 13:46:24 PDT
(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 24 Tony Gentilcore 2011-06-13 14:02:51 PDT
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.
Comment 25 Nate Chapin 2011-06-13 14:06:36 PDT
(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?
Comment 26 Adam Barth 2011-06-13 14:10:32 PDT
> 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?
Comment 27 Nate Chapin 2011-06-13 14:14:27 PDT
(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 28 Nate Chapin 2011-06-13 15:22:45 PDT
Comment on attachment 96106 [details]
Test updates

Test updates landed: http://trac.webkit.org/changeset/88709
Comment 29 Gustavo Noronha (kov) 2011-06-13 18:52:42 PDT
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 30 Collabora GTK+ EWS bot 2011-06-13 18:57:10 PDT
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
Comment 31 Nate Chapin 2011-06-15 11:18:13 PDT
Created attachment 97330 [details]
Don't depend in DocumentLoadTiming, and more gtk build fixes.
Comment 32 WebKit Review Bot 2011-06-15 11:53:36 PDT
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
Comment 33 WebKit Review Bot 2011-06-15 11:53:43 PDT
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 34 Gustavo Noronha (kov) 2011-06-15 12:51:00 PDT
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 35 Collabora GTK+ EWS bot 2011-06-15 14:04:43 PDT
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
Comment 36 Nate Chapin 2011-06-16 09:14:05 PDT
Created attachment 97450 [details]
Moar symbols changes, simpler tests, etc.
Comment 37 WebKit Review Bot 2011-06-16 11:00:20 PDT
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
Comment 38 WebKit Review Bot 2011-06-16 11:00:26 PDT
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
Comment 39 Nate Chapin 2011-06-16 13:08:27 PDT
Created attachment 97480 [details]
Fix Windows Build. Again.
Comment 40 WebKit Review Bot 2011-06-16 13:47:45 PDT
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
Comment 41 WebKit Review Bot 2011-06-16 13:47:52 PDT
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 42 Adam Barth 2011-06-16 13:58:40 PDT
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?
Comment 43 Nate Chapin 2011-06-16 14:04:57 PDT
(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.
Comment 44 Adam Barth 2011-06-16 14:14:39 PDT
> > (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.
Comment 45 Nate Chapin 2011-06-16 14:17:37 PDT
(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.
Comment 46 Alexey Proskuryakov 2011-06-16 14:22:50 PDT
> 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")'.
Comment 47 Adam Barth 2011-06-16 14:23:40 PDT
> 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...
Comment 48 Darin Adler 2011-06-16 16:08:35 PDT
(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.
Comment 49 Nate Chapin 2011-06-17 11:28:59 PDT
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.
Comment 50 Nate Chapin 2011-06-17 12:12:09 PDT
Created attachment 97632 [details]
Merged to trunk
Comment 51 Collabora GTK+ EWS bot 2011-06-17 16:03:34 PDT
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 52 Gustavo Noronha (kov) 2011-06-17 16:13:14 PDT
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 53 Eric Seidel (no email) 2011-06-18 13:32:49 PDT
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.
Comment 54 Nate Chapin 2011-06-21 16:14:50 PDT
Created attachment 98074 [details]
Merged to trunk. Again.
Comment 55 Adam Barth 2011-06-22 10:30:43 PDT
Comment on attachment 98074 [details]
Merged to trunk. Again.

This patch is slightly scary, but I think we should move forward cautiously.
Comment 56 WebKit Review Bot 2011-06-22 17:29:10 PDT
Comment on attachment 98074 [details]
Merged to trunk. Again.

Clearing flags on attachment: 98074

Committed r89503: <http://trac.webkit.org/changeset/89503>
Comment 57 WebKit Review Bot 2011-06-22 17:29:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 58 Nate Chapin 2011-06-23 12:15:57 PDT
The functional part of this patch was reverted in http://trac.webkit.org/changeset/89598.
Comment 59 Brent Fulgham 2011-06-25 17:22:38 PDT
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.
Comment 60 Nate Chapin 2011-07-01 16:02:49 PDT
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.
Comment 61 Nate Chapin 2011-07-06 11:01:52 PDT
(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.