43423 2010-08-03 08:43:24 -0700 HTML5 parser may cause onload not to fire 2010-08-04 02:31:58 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 41115 1 benm benm abarth eric oldest_to_newest 259481 0 benm 2010-08-03 08:43:24 -0700 If a sufficiently complex page causes the HTML5 parser to yield (i.e. > 4096 tokens and > 0.5 parsing time), when we return to parsing and complete the document, onload is not fired. Patch to follow. 259596 1 benm 2010-08-03 11:48:51 -0700 Attaching the code change that I propose to fix the bug. I haven't set r? yet or created a ChangeLog as I've been trying to write a layout test for this. It's tricky as we need to make the parser stall (without creating a new PumpSession) so that it triggers the timeout on HTMLParserScheduler.h:59, i.e. elapsedTime needs to be greater than 0.5 seconds. Thanks to Adam's advice on trying to write a PHP script that sleeps/flushes, I managed to write a layout test that works on Windows, but natch when moving it to my Mac to try it doesn't trigger the bug. I fear this is going to be very tricky to reliably test due to the timing based nature of what we are testing. If for example the parser speeds up due to a hardware change we might not trigger the timeout anymore and this test would become useless. Any ideas folks? I can attach the layout test that worked on Windows if it might help. 259597 2 63362 benm 2010-08-03 11:50:37 -0700 Created attachment 63362 Fix and potential layout test ideas The Windows layout test is actually attached to the patch as well. 259607 3 63362 eric 2010-08-03 12:04:48 -0700 Comment on attachment 63362 Fix and potential layout test ideas You may be fighting the layout timer. The two constants you're trying to trip in your test are: // defaultParserChunkSize is used to define how many tokens the parser will // process before checking against parserTimeLimit and possibly yielding. // This is a performance optimization to prevent checking after every token. static const int defaultParserChunkSize = 4096; // defaultParserTimeLimit is the seconds the parser will run in one write() call // before yielding. Inline <script> execution can cause it to excede the limit. // FIXME: We would like this value to be 0.2. static const double defaultParserTimeLimit = 0.500; Why do you ahve this section: +<!-- +<?php +print str_repeat("A", 2048); +?> +--> To get around some buffering delay at the network layer? What do you mean by "start a new PumpSession"? Isn't the parser in full control until it yields (by being blocked by a script). I guess you have to be careful that the final pumping will be done inside resumeParsingAfterYield, so you need to send more than 4096 tokens, but fewer than 8192, right? Ideally you'd want the tokens as small as possible. You might try sending <a></a> instead (two tokens). You could send <a>a</a>a to get 4 tokens per bunch instead. Of course you have to send the 4096th token after 0.5 seconds to hav it yield. And then you need to have sent all of the rest of the document before it ends up resuming from its yield. 259608 4 eric 2010-08-03 12:05:11 -0700 I agree, this is complicated to test and may not be reliably testable. 259609 5 benm 2010-08-03 12:09:17 -0700 (In reply to comment #3) Thanks for the ideas Eric! > Why do you ahve this section: > +<!-- > +<?php > +print str_repeat("A", 2048); > +?> > +--> > > To get around some buffering delay at the network layer? Exactly. > > What do you mean by "start a new PumpSession"? Well, it seemed to me that the startTime that we compare with when we decide to yield or not is set in the PumpSession constructor. I'm not exactly sure what constitutes a PumpSession though? > > Isn't the parser in full control until it yields (by being blocked by a script). > > I guess you have to be careful that the final pumping will be done inside resumeParsingAfterYield, so you need to send more than 4096 tokens, but fewer than 8192, right? > > Ideally you'd want the tokens as small as possible. > > You might try sending <a></a> instead (two tokens). You could send <a>a</a>a to get 4 tokens per bunch instead. > > Of course you have to send the 4096th token after 0.5 seconds to hav it yield. And then you need to have sent all of the rest of the document before it ends up resuming from its yield. Good suggestions, I'll give it a shot. Thanks! 259634 6 benm 2010-08-03 12:46:08 -0700 Still no luck. I think the problem is that as soon as the pump loop in HTMLDocumentParser doesn't see a token, we stop pumping (and also presumably then the PumpingSession?). Trying to stall by sleeping in the php script causes the tokenizer to not get a token, so we break out of the loop and so when the PHP resumes, we need to send another 4096 tokens, etc... Test aside, does this look like a sane fix? 259641 7 eric 2010-08-03 12:55:53 -0700 The fix looks great. I think you should upload a patch which explains that this seems impossible to test. And then I'll happily r+ it. 259656 8 63371 benm 2010-08-03 13:14:54 -0700 Created attachment 63371 Patch. Awesome, thanks Eric! 259664 9 63371 darin 2010-08-03 13:26:31 -0700 Comment on attachment 63371 Patch. Typo: "dependant" 259937 10 benm 2010-08-04 02:31:58 -0700 Fixed Changelog typo. Sending WebCore/ChangeLog Sending WebCore/html/HTMLDocumentParser.cpp Transmitting file data .. Committed revision 64638. 63362 2010-08-03 11:50:37 -0700 2010-08-03 13:14:54 -0700 Fix and potential layout test ideas patch-1.txt text/plain 1680 benm SW5kZXg6IFdlYkNvcmUvaHRtbC9IVE1MRG9jdW1lbnRQYXJzZXIuY3BwCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IFdlYkNvcmUvaHRtbC9IVE1MRG9jdW1lbnRQYXJzZXIuY3BwCShyZXZpc2lvbiA2NDM1NSkKKysr IFdlYkNvcmUvaHRtbC9IVE1MRG9jdW1lbnRQYXJzZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0x MzgsNiArMTM4LDcgQEAgdm9pZCBIVE1MRG9jdW1lbnRQYXJzZXI6OnJlc3VtZVBhcnNpbmdBZgog ICAgIC8vIFdlIHNob3VsZCBuZXZlciBiZSBoZXJlIHVubGVzcyB3ZSBjYW4gcHVtcCBpbW1lZGlh dGVseS4gIENhbGwgcHVtcFRva2VuaXplcigpCiAgICAgLy8gZGlyZWN0bHkgc28gdGhhdCBBU1NF UlRTIHdpbGwgZmlyZSBpZiB3ZSdyZSB3cm9uZy4KICAgICBwdW1wVG9rZW5pemVyKEFsbG93WWll bGQpOworICAgIGVuZElmRGVsYXllZCgpOwogfQogCiBib29sIEhUTUxEb2N1bWVudFBhcnNlcjo6 cnVuU2NyaXB0c0ZvclBhdXNlZFRyZWVCdWlsZGVyKCkKSW5kZXg6IExheW91dFRlc3RzL2h0dHAv dGVzdHMvbG9hZGluZy9vbmxvYWQtZmlyZXMtYWZ0ZXItcGFyc2VyLXlpZWxkcy5waHAKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gTGF5b3V0VGVzdHMvaHR0cC90ZXN0cy9sb2FkaW5nL29ubG9hZC1maXJlcy1hZnRl ci1wYXJzZXIteWllbGRzLnBocAkocmV2aXNpb24gMCkKKysrIExheW91dFRlc3RzL2h0dHAvdGVz dHMvbG9hZGluZy9vbmxvYWQtZmlyZXMtYWZ0ZXItcGFyc2VyLXlpZWxkcy5waHAJKHJldmlzaW9u IDApCkBAIC0wLDAgKzEsMjkgQEAKKzw/CitoZWFkZXIoIkNvbnRlbnQtVHlwZTogdGV4dC9odG1s OyBjaGFyc2V0PXV0Zi04Iik7Cis/PgorPGh0bWw+Cis8IS0tCis8P3BocAorcHJpbnQgc3RyX3Jl cGVhdCgiQSIsIDIwNDgpOworPz4KKy0tPgorPHNjcmlwdD4KK2lmKHdpbmRvdy5sYXlvdXRUZXN0 Q29udHJvbGxlcikKKyAgICBsYXlvdXRUZXN0Q29udHJvbGxlci5kdW1wQXNUZXh0KCk7CisKKzwv c2NyaXB0PgorPGJvZHkgb25sb2FkPSJkb2N1bWVudC5nZXRFbGVtZW50QnlJZCgncmVzdWx0Jyku aW5uZXJIVE1MID0gJ1BBU1MhJzsiPgorPHAgaWQ9InJlc3VsdCI+RkFJTDwvcD4KKzw/cGhwCitm bHVzaCgpOworIyBjaHVybmluZyBpbiB0aGlzIGxvb3AgbGlrZSB0aGlzIHNlZW1zIHRvIHNlbmQK KyMgZGF0YSBhdCB0aGUgcmlnaHQgcmF0ZSBzbyB0aGF0IHRoZSBwYXJzZXIgZG9lc24ndAorIyBz dGFydCBhIG5ldyBQdW1wU2Vzc2lvbiwgc28gd2hlbiBpdCBjb21wbGV0ZXMgd2UKKyMgdHJpZ2dl ciB0aGUgeWllbGRpbmcgY29kZSBwYXRoLgorZm9yICgkaSA9IDA7ICRpIDwgNTEyOyAkaSsrKSB7 CitwcmludCBzdHJfcmVwZWF0KCI8ZGl2PjwvZGl2PiIsIDMyKTsKK2ZsdXNoKCk7Cit9Cis/Pgor PC9ib2R5PgorCgpQcm9wZXJ0eSBjaGFuZ2VzIG9uOiBMYXlvdXRUZXN0cy9odHRwL3Rlc3RzL2xv YWRpbmcvb25sb2FkLWZpcmVzLWFmdGVyLXBhcnNlci15aWVsZHMucGhwCl9fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KQWRk ZWQ6IHN2bjpleGVjdXRhYmxlCiAgICsgKgoK 63371 2010-08-03 13:14:54 -0700 2010-08-03 13:26:31 -0700 Patch. 43423.txt text/plain 1841 benm SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2NDU4MykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjcgQEAKKzIwMTAtMDgtMDMgIEJlbiBNdXJkb2NoICA8YmVubUBnb29nbGUuY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEhUTUw1 IHBhcnNlciBtYXkgY2F1c2Ugb25sb2FkIG5vdCB0byBmaXJlCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00MzQyMworCisgICAgICAgIElmIGEgY29tcGxl eCBwYWdlIGNhdXNlcyB0aGUgSFRNTCBwYXJzZXIgdG8geWllbGQsCisgICAgICAgIHRoZW4gd2hl biBwYXJzaW5nIGNvbnRpbnVlcyBhZ2FpbiBhbmQgZXZlbnR1YWxseQorICAgICAgICBmaW5pc2hl cywgd2UgYXJlIG1pc3NpbmcgYSBjYWxsIHRvIGVuZElmRGVsYXllZCgpLiBUaGlzCisgICAgICAg IHJlc3VsdHMgaW4gb25sb2FkIG5ldmVyIGJlaW5nIGNhbGxlZC4KKworICAgICAgICBOb3QgZm9y IGxhY2sgb2YgdHJ5aW5nLCBidXQgaXQgc2VlbXMgYWxtb3N0CisgICAgICAgIGltcG9zc2libGUg dG8gd3JpdGUgYSByZWxpYWJsZSB0ZXN0IGZvciB0aGlzIGJ1ZywKKyAgICAgICAgZHVlIHRvIHRo ZSBoaWdobHkgdGltaW5nLWRlcGVuZGFudCBuYXR1cmUgb2YgdGhlCisgICAgICAgIGJ1Zy4gVGhl IGxpbmsgYWJvdmUgY29udGFpbnMgZnVydGhlciBkaXNjdXNzaW9uIGFuZAorICAgICAgICBhdHRl bXB0cyBhdCB3cml0aW5nIGEgdGVzdC4KKyAgICAgICAgCisgICAgICAgICogaHRtbC9IVE1MRG9j dW1lbnRQYXJzZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6SFRNTERvY3VtZW50UGFyc2VyOjpy ZXN1bWVQYXJzaW5nQWZ0ZXJZaWVsZCk6CisgICAgICAgIEFkZCBhIGNhbGwgdG8gZW5kSWZEZWxh eWVkKCkgYWZ0ZXIgcHVtcGluZyB0aGUgdG9rZW5pemVyCisgICAgICAgIHBvc3QgdGhlIHBhcnNl ciB5aWVsZGluZyB0byBlbnN1cmUgdGhhdCB0aGUgcGFyc2luZyBzdGVwCisgICAgICAgIGlzIGNv bXBsZXRlZCBwcm9wZXJseSBhbmQgdGhlIG9ubG9hZCBldmVudCBmaXJlcy4KKwogMjAxMC0wOC0w MyAgQWRhbSBSb2JlbiAgPGFyb2JlbkBhcHBsZS5jb20+CiAKICAgICAgICAgUmVuYW1lIExvY2Fs aXplZFN0cmluZ3NNYWMubW0gdG8gTG9jYWxpemVkU3RyaW5ncy5jcHAKSW5kZXg6IFdlYkNvcmUv aHRtbC9IVE1MRG9jdW1lbnRQYXJzZXIuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvaHRtbC9I VE1MRG9jdW1lbnRQYXJzZXIuY3BwCShyZXZpc2lvbiA2NDM1NSkKKysrIFdlYkNvcmUvaHRtbC9I VE1MRG9jdW1lbnRQYXJzZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMzgsNiArMTM4LDcgQEAg dm9pZCBIVE1MRG9jdW1lbnRQYXJzZXI6OnJlc3VtZVBhcnNpbmdBZgogICAgIC8vIFdlIHNob3Vs ZCBuZXZlciBiZSBoZXJlIHVubGVzcyB3ZSBjYW4gcHVtcCBpbW1lZGlhdGVseS4gIENhbGwgcHVt cFRva2VuaXplcigpCiAgICAgLy8gZGlyZWN0bHkgc28gdGhhdCBBU1NFUlRTIHdpbGwgZmlyZSBp ZiB3ZSdyZSB3cm9uZy4KICAgICBwdW1wVG9rZW5pemVyKEFsbG93WWllbGQpOworICAgIGVuZElm RGVsYXllZCgpOwogfQogCiBib29sIEhUTUxEb2N1bWVudFBhcnNlcjo6cnVuU2NyaXB0c0ZvclBh dXNlZFRyZWVCdWlsZGVyKCk=