Bug 42950 - fast/parser/residual-style-hang.html hangs
Summary: fast/parser/residual-style-hang.html hangs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 41123
  Show dependency treegraph
 
Reported: 2010-07-25 13:45 PDT by Adam Barth
Modified: 2010-08-04 18:10 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.40 KB, patch)
2010-07-25 13:47 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (207.88 KB, patch)
2010-08-04 17:52 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-07-25 13:45:34 PDT
fast/parser/residual-style-hang.html hangs
Comment 1 Adam Barth 2010-07-25 13:47:07 PDT
Created attachment 62530 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-07-25 13:53:57 PDT
Comment on attachment 62530 [details]
Patch

OK, but it would be better to have a test which explicitly showed this limit.  I.e. what is the resulting DOM from such a limit?

I believe you could produce this with:

<a>
<b><i>
<b><i>
<b><i>
<b><i>
<b><i>
<p>
</a>

Would be best to document.write() those and give them all unique ids of course.  Would end up being a "big" DOM, but I think seeing it in a test would help any future tweaks to this be better understood.  A test which simply hangs isn't super helpful.
Comment 3 Adam Barth 2010-07-25 14:02:11 PDT
We could.  I think the desired behavior is to pass all the other tests and to not hang that test, which is what we're doing now.
Comment 4 Eric Seidel (no email) 2010-07-25 14:11:46 PDT
Sure, but there are multiple ways to pass that test.  One of them would be to produce a full DOM, slowly.  Or to ignore the middle tags instead of the end tags.  I'm not sure that it matters, but the current testing is a pretty coarse filter for determining desired behavior.
Comment 5 Adam Barth 2010-07-25 14:15:04 PDT
We should see what James things, but I think either of those would be fine ways to pass the test.
Comment 6 Eric Seidel (no email) 2010-07-25 14:30:21 PDT
Did you mean html5lib james? or chromium/rendering james?
Comment 7 James Robinson 2010-07-25 14:33:37 PDT
I think it'd be better to have a test that checked that the output DOM is correct and requires 10 iterations - or at least 7, since we've seen pages in the wild that require 7 iterations of the adoption agency to render correctly https://bugs.webkit.org/show_bug.cgi?id=41181.

residual-style-hang is at test that we don't hang forever, not for any particular behavior beyond that.
Comment 8 Adam Barth 2010-07-25 14:35:05 PDT
I meant jamesr.  He's the one who worked on this in the old parser.
Comment 9 James Robinson 2010-07-26 16:33:57 PDT
Do we want a cap on the outter and inner loops? The legacy tree builder only caps the equivalent of the outter loop, I believe.  I don't fully grok the fixup algorithm enough to know when this will produce different behavior but I think we should still do 10 rounds of fixup even if the misnested DOM is very deep.
Comment 10 Adam Barth 2010-07-27 03:20:29 PDT
Just capping the outer loop wasn't sufficient to prevent the hang.  I'm not sure exactly how the newer and older algorithms relate.
Comment 11 Eric Seidel (no email) 2010-08-04 16:24:36 PDT
Comment on attachment 62530 [details]
Patch

Needs a layout test which shows behavior.
Comment 12 Adam Barth 2010-08-04 17:52:44 PDT
Created attachment 63522 [details]
Patch
Comment 13 Eric Seidel (no email) 2010-08-04 17:56:34 PDT
Comment on attachment 63522 [details]
Patch

Thanks.
Comment 14 Adam Barth 2010-08-04 18:10:19 PDT
Comment on attachment 63522 [details]
Patch

Clearing flags on attachment: 63522

Committed r64702: <http://trac.webkit.org/changeset/64702>
Comment 15 Adam Barth 2010-08-04 18:10:26 PDT
All reviewed patches have been landed.  Closing bug.