|Product:||WebKit||Reporter:||Adam Barth <abarth>|
|Component:||New Bugs||Assignee:||Adam Barth <abarth>|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
|Bug Depends on:|
Description Adam Barth 2010-07-25 13:45:34 PDT
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 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.