Bug 112075 - De-flake fast/dom/title-directionality.html
Summary: De-flake fast/dom/title-directionality.html
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: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-11 16:01 PDT by James Robinson
Modified: 2013-03-11 16:42 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.18 KB, patch)
2013-03-11 16:03 PDT, James Robinson
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2013-03-11 16:01:45 PDT
De-flake fast/dom/title-directionality.html
Comment 1 James Robinson 2013-03-11 16:03:34 PDT
Created attachment 192584 [details]
Patch
Comment 2 James Robinson 2013-03-11 16:11:10 PDT
This patch does de-flake the test, but I don't fully understand how.  Without this patch:

Found 1 test; running 1 (500 times each: --repeat-each=100 --iterations=5), skipping 0.

Running 1 DumpRenderTree.       

[17/500] fast/dom/title-directionality.html failed unexpectedly (text diff)
...
                                        
478 tests ran as expected, 22 didn't:

With this patch:

Found 1 test; running 1 (500 times each: --repeat-each=100 --iterations=5), skipping 0.

Running 1 DumpRenderTree.       

All 500 tests ran as expected. 

I've seen similar tests flake in the same way (differing in a trailing newline) because the HTML parser was at a different point in the main document when the test code ran, usually because of a setTimeout(..., 0) in an inline script.  There's no setTimeout here, but the parser is pumped synchronously on an iframe by doc.open()/write()/close().
Comment 3 Darin Adler 2013-03-11 16:16:37 PDT
Comment on attachment 192584 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        De-flake fast/dom/title-directionality.html
> +        https://bugs.webkit.org/show_bug.cgi?id=112075
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * fast/dom/title-directionality-expected.txt:
> +        * fast/dom/title-directionality.html:

Patch would be much better if this said what the fix was, and why it works.
Comment 4 Tony Chang 2013-03-11 16:18:22 PDT
Comment on attachment 192584 [details]
Patch

Seems worth trying.  I guess there could be a race between the email iframe load and the first test case.  Not sure what type of error that would cause.
Comment 5 Elliott Sprehn 2013-03-11 16:19:26 PDT
This smells like lazyAttach + Node::attach logic and the whitespace fixup stuff in there. eseidel had mentioned that code doesn't look right recently.
Comment 6 James Robinson 2013-03-11 16:28:28 PDT
(In reply to comment #3)
> (From update of attachment 192584 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192584&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        De-flake fast/dom/title-directionality.html
> > +        https://bugs.webkit.org/show_bug.cgi?id=112075
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        * fast/dom/title-directionality-expected.txt:
> > +        * fast/dom/title-directionality.html:
> 
> Patch would be much better if this said what the fix was, and why it works.

Agreed - I don't fully understand why it works yet, so I can't populate a description.  I can put a guess in.
Comment 7 James Robinson 2013-03-11 16:37:25 PDT
Elliot pointed out that the lazy attachment logic works by considering all nodes 'attached' and then setting the style recalc timer.  This style recalc races against the HTML parser hitting the end of the document.  If the HTML parser yields after attaching some nodes before the end of the test (i.e. after running the inline script but before hitting the end of the main document), the collapsing logic in Node::attach() behaves in a different way than if the rest of the document is parsed without any interleaving style recalculations.

Since this particular test isn't intended to exercise the whitespace collapsing path and we have other (non-flaky) tests for this behavior, this just moves the test logic into an onload handler which fires after the document is fully parsed.

I'll land this patch with a description in the ChangeLog based on ^^.
Comment 8 James Robinson 2013-03-11 16:42:45 PDT
Committed r145429: <http://trac.webkit.org/changeset/145429>