Bug 107987 – Make unload-reparent-sibling-frame.html wait for completion

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Sprehn]

Make unload-reparent-sibling-frame.html wait for completion

[Elliott Sprehn]

Created attachment 184819 [details]
Patch

[Ojan Vafai]

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.

[Eric Seidel (no email)]

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?

[Elliott Sprehn]

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

[Elliott Sprehn]

(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

[Ojan Vafai]

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

[Ojan Vafai]

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

[Elliott Sprehn]

Created attachment 184838 [details]
Patch

Address review comments

[Elliott Sprehn]

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

[WebKit Review Bot]

Comment on attachment 184838 [details]
Patch

Clearing flags on attachment: 184838

Committed r140885: <http://trac.webkit.org/changeset/140885>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.