Bug 107987 - Make unload-reparent-sibling-frame.html wait for completion
Summary: Make unload-reparent-sibling-frame.html wait for completion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-25 15:31 PST by Elliott Sprehn
Modified: 2013-01-25 17:14 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.87 KB, patch)
2013-01-25 15:35 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (3.09 KB, patch)
2013-01-25 16:44 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2013-01-25 15:31:22 PST
Make unload-reparent-sibling-frame.html wait for completion
Comment 1 Elliott Sprehn 2013-01-25 15:35:44 PST
Created attachment 184819 [details]
Patch
Comment 2 Ojan Vafai 2013-01-25 16:00:16 PST
Comment on attachment 184819 [details]
Patch

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

This is kind of changing what this test is testing, no? As in, it used to also test that we'd fire the onload events for both frames when they're reparented and the description seems to be that it's explicitly testing that. Adding waituntildone/notifydone makes sense, but I think you need to notifyDone in teh onload handlers.

> LayoutTests/fast/frames/unload-reparent-sibling-frame.html:40
> +            isSuccessfullyParsed();

Ideally you'd include js-test-post.js. That ensures that the test doesn't appear to pass if you accidentally introduce a JS error. I don't think this actually does what it's intended to if don't include js-test-post.js (i.e. it's supposed to spit out an error if it's not successfully parsed.
Comment 3 Eric Seidel (no email) 2013-01-25 16:03:53 PST
Comment on attachment 184819 [details]
Patch

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

> LayoutTests/fast/frames/unload-reparent-sibling-frame.html:37
> +        setTimeout(function() {

Why the timeout?
Comment 4 Elliott Sprehn 2013-01-25 16:10:03 PST
(In reply to comment #2)
> (From update of attachment 184819 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184819&action=review
> 
> This is kind of changing what this test is testing, no? As in, it used to also test that we'd fire the onload events for both frames when they're reparented and the description seems to be that it's explicitly testing that. Adding waituntildone/notifydone makes sense, but I think you need to notifyDone in teh onload handlers.

We know it fired the onload event if the text in the dump contains the value of the srcdoc. You can't depend on the onload handlers because they may fire synchronously inside appendChild.

> 
> > LayoutTests/fast/frames/unload-reparent-sibling-frame.html:40
> > +            isSuccessfullyParsed();
> 
> Ideally you'd include js-test-post.js. That ensures that the test doesn't appear to pass if you accidentally introduce a JS error. I don't think this actually does what it's intended to if don't include js-test-post.js (i.e. it's supposed to spit out an error if it's not successfully parsed.

Nah, js-test-post.js in async mode just waits for you to call finishJSTest() and then calls isSuccessfullyParsed() which is exactly equivalent to this code. If I used post.js I'd need to set jsTestIsAsync = true and then call finishJsTest() where I have isSuccessfullyParsed() right now and the output would be identical.
Comment 5 Elliott Sprehn 2013-01-25 16:15:06 PST
(In reply to comment #3)
> (From update of attachment 184819 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184819&action=review
> 
> > LayoutTests/fast/frames/unload-reparent-sibling-frame.html:37
> > +        setTimeout(function() {
> 
> Why the timeout?

We have to wait until the current ChildFrameDisconnector::disconnect() call completes before looking at the loaded state of the documents.

The bug this test is for is that we're in the middle of iterating the list of frames to disconnect in ChildFrameDisconnector::disconnectFrameOwners(), and frame at index N moves the frame at index N+k to a different location in the document but with the same parentElement() by moving some ancestor, then when we get to frame N+k we disconnect it even though it's no longer in the subtree we should be disconnecting because the old isValid() method would have returned true.

See: https://trac.webkit.org/changeset/140856
Comment 6 Ojan Vafai 2013-01-25 16:38:13 PST
Comment on attachment 184819 [details]
Patch

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

>>> LayoutTests/fast/frames/unload-reparent-sibling-frame.html:40
>>> +            isSuccessfullyParsed();
>> 
>> Ideally you'd include js-test-post.js. That ensures that the test doesn't appear to pass if you accidentally introduce a JS error. I don't think this actually does what it's intended to if don't include js-test-post.js (i.e. it's supposed to spit out an error if it's not successfully parsed.
> 
> Nah, js-test-post.js in async mode just waits for you to call finishJSTest() and then calls isSuccessfullyParsed() which is exactly equivalent to this code. If I used post.js I'd need to set jsTestIsAsync = true and then call finishJsTest() where I have isSuccessfullyParsed() right now and the output would be identical.

You're right. That said, the more code we have that digs into the internals of the js-test functions, the harder it is to refactor them later (e.g. to get rid of js-test-post).
Comment 7 Ojan Vafai 2013-01-25 16:39:05 PST
(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 184819 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=184819&action=review
> > 
> > This is kind of changing what this test is testing, no? As in, it used to also test that we'd fire the onload events for both frames when they're reparented and the description seems to be that it's explicitly testing that. Adding waituntildone/notifydone makes sense, but I think you need to notifyDone in teh onload handlers.
> 
> We know it fired the onload event if the text in the dump contains the value of the srcdoc. You can't depend on the onload handlers because they may fire synchronously inside appendChild.

Isn't this a race condition? If the onload hasn't fired yet, the text dump will have the wrong output, no?
Comment 8 Elliott Sprehn 2013-01-25 16:44:09 PST
Created attachment 184838 [details]
Patch

Address review comments
Comment 9 Elliott Sprehn 2013-01-25 16:44:40 PST
(In reply to comment #7)
> ...
> Isn't this a race condition? If the onload hasn't fired yet, the text dump will have the wrong output, no?

I think you're right, fixed in the new patch.
Comment 10 WebKit Review Bot 2013-01-25 17:14:45 PST
Comment on attachment 184838 [details]
Patch

Clearing flags on attachment: 184838

Committed r140885: <http://trac.webkit.org/changeset/140885>
Comment 11 WebKit Review Bot 2013-01-25 17:14:49 PST
All reviewed patches have been landed.  Closing bug.