Bug 48077 – HTMLParserScheduler should be suspended when page loading is deferred

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Yong Li]

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.

[Yong Li]

Created attachment 71457 [details]
test case part 2

[Yong Li]

Created attachment 71459 [details]
the patch

[Yong Li]

Created attachment 71485 [details]
solve conflict

[Adam Barth]

Comment on attachment 71485 [details]
solve conflict

We need a test.  Also, the ChangeLog doesn't explain why we're making this change.

[Joe Mason]

(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.

[Yong Li]

(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.

[Yong Li]

Created attachment 71775 [details]
with a manual test case

[Yong Li]

Created attachment 71776 [details]
with test case

fix a typo in comment

[WebKit Review Bot]

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.

[Yong Li]

Created attachment 71782 [details]
fix style errors

[Adam Barth]

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.

[Yong Li]

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

[Yong Li]

Created attachment 71896 [details]
Updated

[Adam Barth]

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.

[Eric Seidel (no email)]

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?

[Alexey Proskuryakov]

> +    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.

[Adam Barth]

> 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.  :)

[Joe Mason]

(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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Joe Mason]

(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.

[Yong Li]

(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?

[Adam Barth]

(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.)

[Yong Li]

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?

[Adam Barth]

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.

[Yong Li]

(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()

[Yong Li]

Created attachment 77123 [details]
name changed

hope it can trigger buildbots?

[Eric Seidel (no email)]

No, only r? will trigger the EWS bots.

[Eric Seidel (no email)]

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

[WebKit Commit Bot]

Comment on attachment 77123 [details]
name changed

Clearing flags on attachment: 77123

Committed r74420: <http://trac.webkit.org/changeset/74420>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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

[Yong Li]

(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

[Tony Gentilcore]

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]

[Ryosuke Niwa]

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

[Yong Li]

(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 :(

[Yong Li]

will update the patch

[Yong Li]

Created attachment 77218 [details]
solved naming conflict

[Adam Barth]

> Seems the name "resumeParsing" is already used :(

Crazy!  I guess that's why folks like the override keyword from C#.  :)

[WebKit Commit Bot]

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

[Yong Li]

Created attachment 77893 [details]
fix build error

[WebKit Commit Bot]

Comment on attachment 77893 [details]
fix build error

Clearing flags on attachment: 77893

Committed r74992: <http://trac.webkit.org/changeset/74992>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

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.

[Kent Tamura]

Rolled out by r75048.

[Eric Seidel (no email)]

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?

[Kent Tamura]

(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.

[Eric Seidel (no email)]

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

[Yong Li]

(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?

[Yong Li]

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

[Yong Li]

Created attachment 78003 [details]
another try

[Yong Li]

Created attachment 78004 [details]
darn...

[WebKit Commit Bot]

Comment on attachment 78004 [details]
darn...

Clearing flags on attachment: 78004

Committed r75066: <http://trac.webkit.org/changeset/75066>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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.

[Yong Li]

(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.

[Yong Li]

(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

[Yong Li]

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

[Yong Li]

Created attachment 78017 [details]
the test case

[Alexey Proskuryakov]

> So the test requires user action.

Can this be tested with layoutTestController.setDeferMainResourceDataLoad()?

[Yong Li]

(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.

[Alexey Proskuryakov]

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).

[Ryosuke Niwa]

(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.

[Yong Li]

(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+.

[Ryosuke Niwa]

(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.

[Yong Li]

(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.

[Yong Li]

to commit the test case

[WebKit Commit Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 78017 [details]
the test case

Clearing flags on attachment: 78017

Committed r75172: <http://trac.webkit.org/changeset/75172>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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

[Yong Li]

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

not possible