Bug 48077 - HTMLParserScheduler should be suspended when page loading is deferred
Summary: HTMLParserScheduler should be suspended when page loading is deferred
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 51436 51917
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-21 10:39 PDT by Yong Li
Modified: 2011-01-07 09:27 PST (History)
10 users (show)

See Also:


Attachments
Test case part 1 (593.37 KB, text/html)
2010-10-21 10:39 PDT, Yong Li
no flags Details
test case part 2 (61 bytes, application/x-javascript)
2010-10-21 10:39 PDT, Yong Li
no flags Details
the patch (6.03 KB, patch)
2010-10-21 10:45 PDT, Yong Li
no flags Details | Formatted Diff | Diff
solve conflict (6.20 KB, patch)
2010-10-21 14:01 PDT, Yong Li
abarth: review-
Details | Formatted Diff | Diff
with a manual test case (10.61 KB, patch)
2010-10-25 11:35 PDT, Yong Li
no flags Details | Formatted Diff | Diff
with test case (10.61 KB, patch)
2010-10-25 11:36 PDT, Yong Li
no flags Details | Formatted Diff | Diff
fix style errors (10.61 KB, patch)
2010-10-25 12:33 PDT, Yong Li
abarth: commit-queue-
Details | Formatted Diff | Diff
remove the blank line from ChangeLog (10.61 KB, patch)
2010-10-26 08:48 PDT, Yong Li
no flags Details | Formatted Diff | Diff
Updated (10.61 KB, patch)
2010-10-26 08:52 PDT, Yong Li
abarth: review-
Details | Formatted Diff | Diff
Try again (11.09 KB, patch)
2010-11-09 14:44 PST, Yong Li
abarth: review+
Details | Formatted Diff | Diff
name changed (6.87 KB, patch)
2010-12-21 09:45 PST, Yong Li
no flags Details | Formatted Diff | Diff
solved naming conflict (6.95 KB, patch)
2010-12-22 08:15 PST, Yong Li
commit-queue: commit-queue-
Details | Formatted Diff | Diff
fix build error (6.96 KB, patch)
2011-01-04 09:17 PST, Yong Li
no flags Details | Formatted Diff | Diff
check the scheduler because it can be null (6.96 KB, patch)
2011-01-05 07:22 PST, Yong Li
yong.li.webkit: commit-queue+
Details | Formatted Diff | Diff
another try (14.33 KB, patch)
2011-01-05 07:23 PST, Yong Li
yong.li.webkit: commit-queue+
Details | Formatted Diff | Diff
darn... (6.98 KB, patch)
2011-01-05 07:24 PST, Yong Li
no flags Details | Formatted Diff | Diff
Test case missed in the previous commit (697 bytes, patch)
2011-01-05 10:49 PST, Yong Li
no flags Details | Formatted Diff | Diff
the test case (4.45 KB, patch)
2011-01-05 10:58 PST, Yong Li
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2010-10-21 10:39:09 PDT
Created attachment 71456 [details]
Test case part 1

HTMLParserScheduler should also be suspended when page load is deferred. Otherwise, when a script is executed in the ways other than by html parser (by async script runner for example) and runs a nested event loop (like showing a dialog to user), the html parser can continue parsing contents and execute JS in the same context, which can result JS errors and bad layout behaviors.

The problem is hard to reproduce without code changes, because it is hard to trigger the m_continueNextChunkTimer.

To reproduce the problem, we can try this settings:
    page->setCustomHTMLTokenizerChunkSize(10);
    page->setCustomHTMLTokenizerTimeDelay(0.001);

and then load the attached test case. After the alert "test" shows up, wait a few seconds and click "ok", then "error detected" will show up which indicates JS re-entrancy is detected.
Comment 1 Yong Li 2010-10-21 10:39:36 PDT
Created attachment 71457 [details]
test case part 2
Comment 2 Yong Li 2010-10-21 10:45:59 PDT
Created attachment 71459 [details]
the patch
Comment 3 Yong Li 2010-10-21 14:01:05 PDT
Created attachment 71485 [details]
solve conflict
Comment 4 Adam Barth 2010-10-21 14:13:50 PDT
Comment on attachment 71485 [details]
solve conflict

We need a test.  Also, the ChangeLog doesn't explain why we're making this change.
Comment 5 Joe Mason 2010-10-21 15:10:52 PDT
(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.
Comment 6 Yong Li 2010-10-21 15:19:52 PDT
(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.
Comment 7 Yong Li 2010-10-25 11:35:14 PDT
Created attachment 71775 [details]
with a manual test case
Comment 8 Yong Li 2010-10-25 11:36:41 PDT
Created attachment 71776 [details]
with test case

fix a typo in comment
Comment 9 WebKit Review Bot 2010-10-25 11:38:29 PDT
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.
Comment 10 Yong Li 2010-10-25 12:33:20 PDT
Created attachment 71782 [details]
fix style errors
Comment 11 Adam Barth 2010-10-25 19:46:13 PDT
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.
Comment 12 Yong Li 2010-10-26 08:48:50 PDT
Created attachment 71895 [details]
remove the blank line from ChangeLog
Comment 13 Yong Li 2010-10-26 08:52:05 PDT
Created attachment 71896 [details]
Updated
Comment 14 Adam Barth 2010-10-31 18:26:00 PDT
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.
Comment 15 Eric Seidel (no email) 2010-10-31 18:30:01 PDT
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?
Comment 16 Alexey Proskuryakov 2010-10-31 20:38:01 PDT
> +    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.
Comment 17 Adam Barth 2010-10-31 21:08:15 PDT
> 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.  :)
Comment 18 Joe Mason 2010-11-01 06:33:06 PDT
(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.
Comment 19 Eric Seidel (no email) 2010-11-01 07:42:47 PDT
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.
Comment 20 Eric Seidel (no email) 2010-11-01 08:02:26 PDT
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.
Comment 21 Joe Mason 2010-11-01 09:13:37 PDT
(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.
Comment 22 Yong Li 2010-11-02 08:49:09 PDT
(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?
Comment 23 Adam Barth 2010-11-02 09:29:12 PDT
(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.)
Comment 24 Yong Li 2010-11-09 14:44:46 PST
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 25 Adam Barth 2010-12-21 01:15:24 PST
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.
Comment 26 Yong Li 2010-12-21 08:02:05 PST
(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()
Comment 27 Yong Li 2010-12-21 09:45:01 PST
Created attachment 77123 [details]
name changed

hope it can trigger buildbots?
Comment 28 Eric Seidel (no email) 2010-12-21 11:15:06 PST
No, only r? will trigger the EWS bots.
Comment 29 Eric Seidel (no email) 2010-12-21 11:15:50 PST
It's possible to manually submit a patch at http://queues.webkit.org/submit-to-ews (which I just did for your patch).
Comment 30 WebKit Commit Bot 2010-12-21 11:24:21 PST
Comment on attachment 77123 [details]
name changed

Clearing flags on attachment: 77123

Committed r74420: <http://trac.webkit.org/changeset/74420>
Comment 31 WebKit Commit Bot 2010-12-21 11:24:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 32 WebKit Review Bot 2010-12-21 12:19:53 PST
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
Comment 33 Yong Li 2010-12-21 13:21:11 PST
(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
Comment 34 Tony Gentilcore 2010-12-21 17:38:06 PST
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]
Comment 35 Ryosuke Niwa 2010-12-21 17:54:30 PST
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
Comment 36 Yong Li 2010-12-21 21:11:39 PST
(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 :(
Comment 37 Yong Li 2010-12-21 21:12:20 PST
will update the patch
Comment 38 Yong Li 2010-12-22 08:15:42 PST
Created attachment 77218 [details]
solved naming conflict
Comment 39 Adam Barth 2010-12-22 10:25:11 PST
> Seems the name "resumeParsing" is already used :(

Crazy!  I guess that's why folks like the override keyword from C#.  :)
Comment 40 WebKit Commit Bot 2010-12-22 14:48:30 PST
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
Comment 41 Yong Li 2011-01-04 09:17:54 PST
Created attachment 77893 [details]
fix build error
Comment 42 WebKit Commit Bot 2011-01-04 12:20:47 PST
Comment on attachment 77893 [details]
fix build error

Clearing flags on attachment: 77893

Committed r74992: <http://trac.webkit.org/changeset/74992>
Comment 43 WebKit Commit Bot 2011-01-04 12:20:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 44 Kent Tamura 2011-01-04 23:18:11 PST
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.
Comment 45 Kent Tamura 2011-01-04 23:25:41 PST
Rolled out by r75048.
Comment 46 Eric Seidel (no email) 2011-01-04 23:28:16 PST
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?
Comment 47 Kent Tamura 2011-01-04 23:32:10 PST
(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.
Comment 48 Eric Seidel (no email) 2011-01-04 23:38:48 PST
That's fine.  Should be easy to write one given that Yong knows now to test with fragment parsing.
Comment 49 Yong Li 2011-01-05 06:57:18 PST
(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?
Comment 50 Yong Li 2011-01-05 07:22:59 PST
Created attachment 78002 [details]
check the scheduler because it can be null
Comment 51 Yong Li 2011-01-05 07:23:56 PST
Created attachment 78003 [details]
another try
Comment 52 Yong Li 2011-01-05 07:24:54 PST
Created attachment 78004 [details]
darn...
Comment 53 WebKit Commit Bot 2011-01-05 07:51:55 PST
Comment on attachment 78004 [details]
darn...

Clearing flags on attachment: 78004

Committed r75066: <http://trac.webkit.org/changeset/75066>
Comment 54 WebKit Commit Bot 2011-01-05 07:52:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 55 Eric Seidel (no email) 2011-01-05 10:11:18 PST
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.
Comment 56 Yong Li 2011-01-05 10:35:39 PST
(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.
Comment 57 Yong Li 2011-01-05 10:36:34 PST
(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
Comment 58 Yong Li 2011-01-05 10:49:17 PST
Created attachment 78016 [details]
Test case missed in the previous commit
Comment 59 Yong Li 2011-01-05 10:58:30 PST
Created attachment 78017 [details]
the test case
Comment 60 Alexey Proskuryakov 2011-01-05 11:01:00 PST
> So the test requires user action.

Can this be tested with layoutTestController.setDeferMainResourceDataLoad()?
Comment 61 Yong Li 2011-01-05 11:14:36 PST
(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.
Comment 62 Alexey Proskuryakov 2011-01-05 11:47:30 PST
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).
Comment 63 Ryosuke Niwa 2011-01-05 11:57:26 PST
(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.
Comment 64 Yong Li 2011-01-05 12:58:26 PST
(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+.
Comment 65 Ryosuke Niwa 2011-01-05 13:02:40 PST
(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.
Comment 66 Yong Li 2011-01-05 13:14:47 PST
(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.
Comment 67 Yong Li 2011-01-06 10:42:20 PST
to commit the test case
Comment 68 WebKit Commit Bot 2011-01-06 11:12:59 PST
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 69 WebKit Commit Bot 2011-01-06 11:14:18 PST
Comment on attachment 78017 [details]
the test case

Clearing flags on attachment: 78017

Committed r75172: <http://trac.webkit.org/changeset/75172>
Comment 70 WebKit Commit Bot 2011-01-06 11:14:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 71 WebKit Review Bot 2011-01-06 12:18:07 PST
http://trac.webkit.org/changeset/75172 might have broken GTK Linux 32-bit Release
Comment 72 Yong Li 2011-01-07 09:27:24 PST
(In reply to comment #71)
> http://trac.webkit.org/changeset/75172 might have broken GTK Linux 32-bit Release

not possible