Bug 48077

Created attachment 71457 [details] test case part 2

Created attachment 71459 [details] the patch

Created attachment 71485 [details] solve conflict

Comment on attachment 71485 [details] solve conflict We need a test. Also, the ChangeLog doesn't explain why we're making this change.

(In reply to comment #4) > (From update of attachment 71485 [details]) > We need a test. Also, the ChangeLog doesn't explain why we're making this change. Maybe we can trigger the bug without turning up the parser granularity so high by making the test page REALLY big (several megs). Probably better would be to expose the ChunkSize setting through the layouttest harness so it can be adjusted automatically by the test.

(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 71485 [details] [details]) > > We need a test. Also, the ChangeLog doesn't explain why we're making this change. > > Maybe we can trigger the bug without turning up the parser granularity so high by making the test page REALLY big (several megs). > > Probably better would be to expose the ChunkSize setting through the layouttest harness so it can be adjusted automatically by the test. The problem is networking layer usually reads only a small chunk from file every time, which makes it almost impossible to trigger m_continueNextChunkTimer without changing code. I'll try to get something for manual testing.

Created attachment 71775 [details] with a manual test case

Created attachment 71776 [details] with test case fix a typo in comment

Attachment 71776 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/html/parser/HTMLParserScheduler.h:74: Place brace on its own line for function definitions. [whitespace/braces] [4] WebCore/html/parser/HTMLParserScheduler.h:78: Place brace on its own line for function definitions. [whitespace/braces] [4] WebCore/html/parser/HTMLParserScheduler.h:86: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 3 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 71782 [details] fix style errors

Comment on attachment 71782 [details] fix style errors View in context: https://bugs.webkit.org/attachment.cgi?id=71782&action=review > WebCore/ChangeLog:1 > + This blank line at the top of the changelog will cause problems for the tools.

Created attachment 71895 [details] remove the blank line from ChangeLog

Created attachment 71896 [details] Updated

Comment on attachment 71896 [details] Updated View in context: https://bugs.webkit.org/attachment.cgi?id=71896&action=review Something about this change doesn't seem very elegant. I think the problem is we're plumbing this even though the DocumentParser interface even though it's only relevant for an HTMLDocumentParser. Maybe Eric has some thoughts. R- for nits below. > WebCore/dom/DocumentParser.h:95 > + virtual void willDeferLoading() {} > + virtual void didResumeLoading() {} Please don't put implementations of virtual methods in the header. That leads to binary bloat. What about all the other implementations of this interface? Do they not care about these events? > WebCore/html/parser/HTMLParserScheduler.h:95 > + void suspend() > + { > + ASSERT(!m_isSuspendedWithActiveTimer); > + if (!m_continueNextChunkTimer.isActive()) > + return; > + m_isSuspendedWithActiveTimer = true; > + m_continueNextChunkTimer.stop(); > + } > + > + void resume() > + { > + ASSERT(!m_continueNextChunkTimer.isActive()); > + if (!m_isSuspendedWithActiveTimer) > + return; > + m_isSuspendedWithActiveTimer = false; > + m_continueNextChunkTimer.startOneShot(0); > + } These should go in the cpp file. > WebCore/page/PageGroupLoadDeferrer.cpp:54 > frame->document()->suspendActiveDOMObjects(ActiveDOMObject::WillShowDialog); > frame->document()->asyncScriptRunner()->suspend(); > + if (DocumentParser* parser = frame->document()->parser()) > + parser->willDeferLoading(); These operations feel grouped together. Is there some higher-level concept they're expressing? Maybe the document should know how to do the details of that operation.

I don't understand what this patch is trying to do. I don't really understand what PageGroupLoadDeferrer is. I guess if I had understood these things this "bug" wouldn't have existed. Can you explain again what you're trying to fix here?

> + virtual void willDeferLoading() {} > + virtual void didResumeLoading() {} > Please don't put implementations of virtual methods in the header. That leads to binary bloat. This is only accurate for virtual destructors. The above can make linking slower and increases intermediate file sizes, but doesn't have any effect on resulting binaries.

> This is only accurate for virtual destructors. The above can make linking slower and increases intermediate file sizes, but doesn't have any effect on resulting binaries. Fair enough, but I'd prefer linking to be fast and to have smaller intermediate file sizes. :)

(In reply to comment #15) > I don't understand what this patch is trying to do. I don't really understand what PageGroupLoadDeferrer is. I guess if I had understood these things this "bug" wouldn't have existed. > > Can you explain again what you're trying to fix here? PageGroupLoadDeferrer prevents reentrance into the parser (and thus the JS interpreter). When a PageGroupLoadDeferrer exists, all incoming network data intended for a given page group is buffered and not forwarded to the parser until the PageGroupLoadDeferrer goes out of scope. (I'm not sure why this is done on the page group level rather than per-page.) Also, timers that have already been set which trigger parsing should not fire until the PageGroupLoadDeferrer goes out of scope. The simplest use case is when a JS alert box is up - the JS function is synchronous, so the interpreter is paused on the alert call, but the app message pump needs to keep running to update the UI. During this time we need to block further parsing since sync JS is "running" even though the interpreter won't actually be active again until the user hits OK. So, Chrome::runJavaScriptAlert creates a PageGroupLoadDeferrer to avoid parsing any data that comes in while the function is running. The problem in this bug is that when a PageLoadDeferrer object exists, in some cases the m_parserScheduler timer can still fire, which will restart the parser.

OK. Feels a bit strange to suspend the thing which does the suspends. :) But I can see where you're going here. I think the general concept of this patch is OK then. I wonder if instead of adding methods to this internal-detail of the parser, if instead we should add to the DocumentParser API and call a pause() resume() functions on the DocumentParser, which in the case of the HTMLDocumentParser know how to pause the scheduler as well.

It would be best to keep PageGroupLoadDeferrer from having to know about too many subsystems. Ideally it should just call a single API on Page. Page can pass the pause/resume messages to subsystems it knows about (like Frame), and on down. That way we don't have strange dependency graphs.

(In reply to comment #20) > It would be best to keep PageGroupLoadDeferrer from having to know about too many subsystems. Ideally it should just call a single API on Page. Page can pass the pause/resume messages to subsystems it knows about (like Frame), and on down. That way we don't have strange dependency graphs. Agreed. (In principle - I haven't looked at the relationship between Page and HTMLParserScheduler to see how hard this would be.) Also either Page or PageGroupLoadDeferrer directly should call a client callback so that clients that do things that might cause JS execution (such as injecting custom JS) can hold off if loading is deferred.

(In reply to comment #14) > (From update of attachment 71896 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71896&action=review > > Something about this change doesn't seem very elegant. I think the problem is we're plumbing this even though the DocumentParser interface even though it's only relevant for an HTMLDocumentParser. Maybe Eric has some thoughts. R- for nits below. > > > WebCore/dom/DocumentParser.h:95 > > + virtual void willDeferLoading() {} > > + virtual void didResumeLoading() {} > > Please don't put implementations of virtual methods in the header. That leads to binary bloat. What about all the other implementations of this interface? Do they not care about these events? > I also prefer to keep virtual methods' implementations in cpp file. But there are tons of virtual methods implemented in webkit header files. I'm not sure which is more webkit style. Other DocumentParser don't have this problem yet. > > WebCore/html/parser/HTMLParserScheduler.h:95 > > + void suspend() > > + { > > + ASSERT(!m_isSuspendedWithActiveTimer); > > + if (!m_continueNextChunkTimer.isActive()) > > + return; > > + m_isSuspendedWithActiveTimer = true; > > + m_continueNextChunkTimer.stop(); > > + } > > + > > + void resume() > > + { > > + ASSERT(!m_continueNextChunkTimer.isActive()); > > + if (!m_isSuspendedWithActiveTimer) > > + return; > > + m_isSuspendedWithActiveTimer = false; > > + m_continueNextChunkTimer.startOneShot(0); > > + } > > These should go in the cpp file. > Totally agree. But almost all other methods of the scheduler are implemented in the header file including those containing many lines. I think they should be kept in consistent style. Should I move some of them to the cpp file?

(In reply to comment #22) > (In reply to comment #14) > > (From update of attachment 71896 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=71896&action=review > > > > Something about this change doesn't seem very elegant. I think the problem is we're plumbing this even though the DocumentParser interface even though it's only relevant for an HTMLDocumentParser. Maybe Eric has some thoughts. R- for nits below. > > > > > WebCore/dom/DocumentParser.h:95 > > > + virtual void willDeferLoading() {} > > > + virtual void didResumeLoading() {} > > > > Please don't put implementations of virtual methods in the header. That leads to binary bloat. What about all the other implementations of this interface? Do they not care about these events? > > > > I also prefer to keep virtual methods' implementations in cpp file. But there are tons of virtual methods implemented in webkit header files. I'm not sure which is more webkit style. Other DocumentParser don't have this problem yet. We've been cleaning this up recently. > > > WebCore/html/parser/HTMLParserScheduler.h:95 > > > + void suspend() > > > + { > > > + ASSERT(!m_isSuspendedWithActiveTimer); > > > + if (!m_continueNextChunkTimer.isActive()) > > > + return; > > > + m_isSuspendedWithActiveTimer = true; > > > + m_continueNextChunkTimer.stop(); > > > + } > > > + > > > + void resume() > > > + { > > > + ASSERT(!m_continueNextChunkTimer.isActive()); > > > + if (!m_isSuspendedWithActiveTimer) > > > + return; > > > + m_isSuspendedWithActiveTimer = false; > > > + m_continueNextChunkTimer.startOneShot(0); > > > + } > > > > These should go in the cpp file. > > Totally agree. But almost all other methods of the scheduler are implemented in the header file including those containing many lines. I think they should be kept in consistent style. Should I move some of them to the cpp file? There are comments explaining why the other methods are inline. They have a measurable impact on performance. If you move around existing code, please run the html-parser benchmark to ensure you're not causing a performance regression. (I recommend leaving the existing code alone.)

Created attachment 73422 [details] Try again Updated according to Adam's comments. Hasn't addressed Eric's suggestions: 1) let document parser manage this state: This requires good knowledge about the html document parser. I don't feel I'm qualified for this change. 2) merge those sub system jobs into a single API in Page or Frame: Not logically related to this bug. should it be another patch?

Comment on attachment 73422 [details] Try again View in context: https://bugs.webkit.org/attachment.cgi?id=73422&action=review > WebCore/dom/DocumentParser.h:95 > + virtual void willDeferLoading(); > + virtual void didResumeLoading(); These really want to be named suspend and resume, respectively. The only problem is that calling suspend doesn't actually suspend the parser. It just stops the parser from running its scheduled callback. I think the code content of this patch is fine, I'm just struggling to find the right names for these functions. I don't want to hold your patch hostage any longer, but we might want to come back and think of better names here. Maybe add a FIXME comment? > WebCore/page/PageGroupLoadDeferrer.cpp:54 > frame->document()->suspendActiveDOMObjects(ActiveDOMObject::WillShowDialog); > frame->document()->asyncScriptRunner()->suspend(); > + if (DocumentParser* parser = frame->document()->parser()) > + parser->willDeferLoading(); I understand that you don't want to add a FIXME for grouping these operations, but can you add a FIXME about moving this work to Document at some point? > WebCore/page/PageGroupLoadDeferrer.cpp:76 > frame->document()->resumeActiveDOMObjects(); > frame->document()->asyncScriptRunner()->resume(); > + if (DocumentParser* parser = frame->document()->parser()) > + parser->didResumeLoading(); Same here.

(In reply to comment #25) > (From update of attachment 73422 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73422&action=review > > WebCore/dom/DocumentParser.h:95 > > + virtual void willDeferLoading(); > > + virtual void didResumeLoading(); > These really want to be named suspend and resume, respectively. The only problem is that calling suspend doesn't actually suspend the parser. It just stops the parser from running its scheduled callback. > I think the code content of this patch is fine, I'm just struggling to find the right names for these functions. I don't want to hold your patch hostage any longer, but we might want to come back and think of better names here. Maybe add a FIXME comment? How about suspendScheduledTasks()? Like what is being used in Bug 49401's patch > > WebCore/page/PageGroupLoadDeferrer.cpp:54 > > frame->document()->suspendActiveDOMObjects(ActiveDOMObject::WillShowDialog); > > frame->document()->asyncScriptRunner()->suspend(); > > + if (DocumentParser* parser = frame->document()->parser()) > > + parser->willDeferLoading(); > I understand that you don't want to add a FIXME for grouping these operations, but can you add a FIXME about moving this work to Document at some point? Would you also like to take a look at Bug 49401? In that patch, I moved these calls to FrameLoader::setDefersLoading()

Created attachment 77123 [details] name changed hope it can trigger buildbots?

No, only r? will trigger the EWS bots.

It's possible to manually submit a patch at http://queues.webkit.org/submit-to-ews (which I just did for your patch).

Comment on attachment 77123 [details] name changed Clearing flags on attachment: 77123 Committed r74420: <http://trac.webkit.org/changeset/74420>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/74420 might have broken Qt Linux Release The following tests are not passing: fast/events/attribute-listener-cloned-from-frameless-doc.xhtml

(In reply to comment #32) > http://trac.webkit.org/changeset/74420 might have broken Qt Linux Release > The following tests are not passing: > fast/events/attribute-listener-cloned-from-frameless-doc.xhtml Not very possible because the patch is supposed to affect only when PageGroupLoadDeferrer is used

Sorry, I had to roll this back due to the following crash. It repro'd on chromium, gtk and qt. 2010-12-21 17:11:20,796 20348 dump_render_tree_thread.py:98 DEBUG worker-1 Stacktrace for editing/pasteboard/paste-noscript-xhtml.xhtml: ASSERTION FAILED: m_parserPaused (c:\b\build\slave\webkit-win-dbg\build\src\third_party\WebKit\WebCore\dom\XMLDocumentParserLibxml2.cpp:1420 WebCore::XMLDocumentParser::resumeParsing) Backtrace: (No symbol) [0x0141F833] (No symbol) [0x0109E328] (No symbol) [0x012335F1] (No symbol) [0x011D5451] (No symbol) [0x01D945D7] (No symbol) [0x005E3A3A] (No symbol) [0x005E0660] (No symbol) [0x005E05D8] (No symbol) [0x36BA028E] (No symbol) [0x09463589] (No symbol) [0x36BAE919] (No symbol) [0x36BA3E42] (No symbol) [0x004D4C06] (No symbol) [0x004D4A72] (No symbol) [0x0047C52A] (No symbol) [0x0103B3CD] (No symbol) [0x0103AD65] (No symbol) [0x011CCB86] (No symbol) [0x011E03E1] (No symbol) [0x015D8ADC] (No symbol) [0x01F4A6B5] (No symbol) [0x01F49D4C] (No symbol) [0x01EE015B] (No symbol) [0x01EE041C] (No symbol) [0x01EDFEFF] (No symbol) [0x01EE0B98] (No symbol) [0x015F3817] (No symbol) [0x010F55D7] (No symbol) [0x010F569C] (No symbol) [0x010F5617] (No symbol) [0x011FBA55] (No symbol) [0x010BB272] (No symbol) [0x014FF31C] (No symbol) [0x014F8417] (No symbol) [0x01F6DB10] (No symbol) [0x00B4834B] (No symbol) [0x008A7C89] (No symbol) [0x008AED61] (No symbol) [0x008AD585] (No symbol) [0x01B75F90] (No symbol) [0x01B760C5] (No symbol) [0x01B7661C] (No symbol) [0x01BC3344] (No symbol) [0x01BC2B92] (No symbol) [0x01BC2DFC] (No symbol) [0x01B75546] (No symbol) [0x01B7530E] (No symbol) [0x01B751FA] (No symbol) [0x0089D59F] (No symbol) [0x0044BFCD] (No symbol) [0x0045979B] (No symbol) [0x00469D42] (No symbol) [0x0046950E] (No symbol) [0x00A56967] (No symbol) [0x00A5683F] RegisterWaitForInputIdle [0x7C817077+73]

fast/events/attribute-listener-cloned-from-frameless-doc.xhtml also seemed to be failing on qt after this patch. http://build.webkit.org/results/Qt%20Linux%20Release/r74419%20(25509)/results.html http://build.webkit.org/results/Qt%20Linux%20Release/r74420%20(25511)/results.html

(In reply to comment #34) > Sorry, I had to roll this back due to the following crash. It repro'd on chromium, gtk and qt. > > 2010-12-21 17:11:20,796 20348 dump_render_tree_thread.py:98 DEBUG worker-1 Stacktrace for editing/pasteboard/paste-noscript-xhtml.xhtml: > ASSERTION FAILED: m_parserPaused > (c:\b\build\slave\webkit-win-dbg\build\src\third_party\WebKit\WebCore\dom\XMLDocumentParserLibxml2.cpp:1420 WebCore::XMLDocumentParser::resumeParsing) > Backtrace: Seems the name "resumeParsing" is already used :(

will update the patch

Created attachment 77218 [details] solved naming conflict

> Seems the name "resumeParsing" is already used :( Crazy! I guess that's why folks like the override keyword from C#. :)

Comment on attachment 77218 [details] solved naming conflict Rejecting attachment 77218 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2 Last 500 characters of output: OR 0320 setenv YACC /Developer/usr/bin/yacc /bin/sh -c /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh ** BUILD FAILED ** The following build commands failed: WebCore: CompileC /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/HTMLDocumentParser.o /mnt/git/webkit-commit-queue/WebCore/html/parser/HTMLDocumentParser.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2 (1 failure) Full output: http://queues.webkit.org/results/7314090

Created attachment 77893 [details] fix build error

Comment on attachment 77893 [details] fix build error Clearing flags on attachment: 77893 Committed r74992: <http://trac.webkit.org/changeset/74992>

All reviewed patches have been landed. Closing bug.

Comment on attachment 77893 [details] fix build error View in context: https://bugs.webkit.org/attachment.cgi?id=77893&action=review > WebCore/html/parser/HTMLDocumentParser.cpp:525 > +{ > + m_parserScheduler->suspend(); > +} m_parserScheduler can be empty. A test for Chromium (not a LayoutTest) crashed here. So, I'll roll r74992 out.

Rolled out by r75048.

m_parserScheduler is NULL when parsing fragments. We'll need a test. What was the chromium test that crashed? And why is the test chromium-only?

(In reply to comment #46) > m_parserScheduler is NULL when parsing fragments. We'll need a test. > > What was the chromium test that crashed? And why is the test chromium-only? It's memory_test of Chromium, and a renderer process crashed when loading the following three pages quickly. "http://www.nytimes.com/pages/technology/index.html", "http://pogue.blogs.nytimes.com/2008/07/17/a-candy-store-for-the-iphone/", "http://www.nytimes.com/2008/07/21/technology/21pc.html?_r=1&ref=technology&oref=slogin", I don't have a reduction yet.

That's fine. Should be easy to write one given that Yong knows now to test with fragment parsing.

(In reply to comment #47) > (In reply to comment #46) > > m_parserScheduler is NULL when parsing fragments. We'll need a test. > > > > What was the chromium test that crashed? And why is the test chromium-only? > > It's memory_test of Chromium, and a renderer process crashed when loading the following three pages quickly. > > "http://www.nytimes.com/pages/technology/index.html", > "http://pogue.blogs.nytimes.com/2008/07/17/a-candy-store-for-the-iphone/", > "http://www.nytimes.com/2008/07/21/technology/21pc.html?_r=1&ref=technology&oref=slogin", > > I don't have a reduction yet. Interesting. The function is called only when a JS modal dialog like alert shows up. Do you know why page loading is deferred in chromium when running the tests?

Created attachment 78002 [details] check the scheduler because it can be null

Created attachment 78003 [details] another try

Created attachment 78004 [details] darn...

Comment on attachment 78004 [details] darn... Clearing flags on attachment: 78004 Committed r75066: <http://trac.webkit.org/changeset/75066>

All reviewed patches have been landed. Closing bug.

Why was this committed w/o a test or further review? I would have r-'d this w/o testing or explanation of why testing is impossible.

(In reply to comment #55) > Why was this committed w/o a test or further review? I would have r-'d this w/o testing or explanation of why testing is impossible. The patch contains a manual test. PageGroupLoadDeferrer is only used when a JS modal dialog pops up. So the test requires user action.

(In reply to comment #55) > Why was this committed w/o a test or further review? I would have r-'d this w/o testing or explanation of why testing is impossible. oops. I missed the test case in updated patch

Created attachment 78016 [details] Test case missed in the previous commit

Created attachment 78017 [details] the test case

> So the test requires user action. Can this be tested with layoutTestController.setDeferMainResourceDataLoad()?

(In reply to comment #60) > > So the test requires user action. > Can this be tested with layoutTestController.setDeferMainResourceDataLoad()? Just took a look. first it won't create PageGroupLoadDeferrer, so manually clicking a JS modal dialog is still needed. Second, it is still hard to trigger the timeout in HTML parser scheduler.

Can DumpRenderTree be further improved to exercise deferring? This area is too important and fragile to rely on manual tests (I strongly doubt that anyone ever runs these - I tried running some a few months ago, and found many regressions).

(In reply to comment #59) > Created an attachment (id=78017) [details] > the test case Please submit your patch for a review. I don't think it's appropriate for a test to be committed without a formal review.

(In reply to comment #63) > (In reply to comment #59) > > Created an attachment (id=78017) [details] [details] > > the test case > > Please submit your patch for a review. I don't think it's appropriate for a test to be committed without a formal review. The test case was included in the original patch and reviewed by Adam Barth. It was just missed in the subsequent patches. If Eric is OK, I'll commit-queue+.

(In reply to comment #64) > The test case was included in the original patch and reviewed by Adam Barth. It was just missed in the subsequent patches. If Eric is OK, I'll commit-queue+. Ok, that's fine then.

(In reply to comment #62) > Can DumpRenderTree be further improved to exercise deferring? This area is too important and fragile to rely on manual tests (I strongly doubt that anyone ever runs these - I tried running some a few months ago, and found many regressions). We could probably let DumpRenderTree run an event loop for 5 seconds (or a configurable number) in the implementation of JS modal dialogs. Currently each port handles DRT JS modal dialogs in platform code, and most of them simply print the message to console. Another way might be add a new function to DRT that takes an argument as timeout.

to commit the test case

The commit-queue encountered the following flaky tests while processing attachment 78017 [details]: http/tests/navigation/ping-cross-origin-from-https.html bug 51314 (author: japhet@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 78017 [details] the test case Clearing flags on attachment: 78017 Committed r75172: <http://trac.webkit.org/changeset/75172>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/75172 might have broken GTK Linux 32-bit Release

(In reply to comment #71) > http://trac.webkit.org/changeset/75172 might have broken GTK Linux 32-bit Release not possible