1428694 - Fix unexpected assertion in toolkit/components/extensions/test/mochitest/test_ext_new_tab_processType.html on debug build of Win7/Win10 if bug 1193394 is fixed

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

I can see this failure constantly during these 2 week on the debug build of windows 7/10:
Win7(debug):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7b7ba9086f5524823ea95f717f4bfbe0430313a&selectedJob=154671122
Win10(debug):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7b7ba9086f5524823ea95f717f4bfbe0430313a&selectedJob=154671194


1655 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_new_tab_processType.html | assertion count 2 is more than expected 0 assertions
1885 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_new_tab_processType.html | assertion count 2 is more than expected 0 assertions

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

After checking the logs:
the ASSERTIONS printed from layout/base/PresShell.cpp during this test are:
04:41:06     INFO -  1652 INFO None1653 INFO TEST-START | toolkit/components/extensions/test/mochitest/test_ext_new_tab_processType.html
04:41:08     INFO -  GECKO(1604) | [Child 4716, Main Thread] ###!!! ASSERTION: Should be in mDirtyRoots now: 'mDirtyRoots.Contains(rootFrame)', file z:/build/build/src/layout/base/PresShell.cpp, line 1851
04:41:08     INFO -  GECKO(1604) | [Child 4716, Main Thread] ###!!! ASSERTION: Why no reflow scheduled?: 'mObservingLayoutFlushes', file z:/build/build/src/layout/base/PresShell.cpp, line 1852
04:41:08     INFO -  1654 INFO TEST-OK | toolkit/components/extensions/test/mochitest/test_ext_new_tab_processType.html | took 2743ms
04:41:09    ERROR -  1655 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_new_tab_processType.html | assertion count 2 is more than expected 0 assertions

Note: these assertions are only printed on windows debug builds.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Bevis Tseng[:bevis][:btseng] from comment #1)
> After checking the logs:
> the ASSERTIONS printed from layout/base/PresShell.cpp during this test are:
> 04:41:06     INFO -  1652 INFO None1653 INFO TEST-START |
> toolkit/components/extensions/test/mochitest/test_ext_new_tab_processType.
> html
> 04:41:08     INFO -  GECKO(1604) | [Child 4716, Main Thread] ###!!!
> ASSERTION: Should be in mDirtyRoots now: 'mDirtyRoots.Contains(rootFrame)',
> file z:/build/build/src/layout/base/PresShell.cpp, line 1851
> 04:41:08     INFO -  GECKO(1604) | [Child 4716, Main Thread] ###!!!
> ASSERTION: Why no reflow scheduled?: 'mObservingLayoutFlushes', file
> z:/build/build/src/layout/base/PresShell.cpp, line 1852

This smells the same issue of as bug 1425833.

Oh wait, the assertion happened on Win10 too, it's not the same.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

CCing :tnikkel since I guess this might be related to https://hg.mozilla.org/mozilla-central/rev/4d0bd19cf77dc72ce57aa83daa2b06a40c169ad6

I mean the commit made the XBL stuff asynchronous so that the expected reflow hasn't yet happen there?

Let's see what happens if the commit is reverted.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a69237768fd1d7d6d5c66034591814e4a1fdb63

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Do'h!  Reverting the commit caused another assertion;

 Assertion failure: !presContext->HasPendingMediaQueryUpdates() (Someone forgot to update media queries?), at z:/build/build/src/layout/base/ServoRestyleManager.cpp:1100

Comment 5

[Hiroyuki Ikezoe (:hiro)]

A try with debug logging [1] showed me that the pres context in question was destroying when the assertions happened.  So FrameNeedsReflow() bailed out at [2].

Let's see a try with modifications for the assertions; 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=35be4295b7db975448e555d1bc064c2db544e25c

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cfc5f5351d405d99decf45a13323237fa0a6e63
[2] https://hg.mozilla.org/mozilla-central/file/c1154ebbe3fa/layout/base/PresShell.cpp#l2686

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I've started looking at this.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

CCing Mats who introduced the assertions in question in bug 688044.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

After some more thought I think adding 'NS_ENSURE_STATE(!mHaveShutDown)' after nsContentUtils::AddScriptRunner(new XBLConstructorRunner(mDocument)) is a fix for this failure.

Here is a try run;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=71d974d812b110686d1a1e6f4703e487f9709c61

One thing is not yet clear to me is that this is the case whether we need nsAutoScriptBlocker or not.

FWIW, here is a call stack in the case where the pres context was destroyed in the |nsContentUtils::AddScriptRunner(new XBLConstructorRunner(mDocument))|.

#01: nsContentSink::StartLayout(bool) [dom/base/nsContentSink.cpp:1271]
#02: nsHtml5TreeOpExecutor::StartLayout(bool *) [parser/html/nsHtml5TreeOpExecutor.cpp:675]
#03: nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor *,nsIContent * *,bool *,bool *) [parser/html/nsHtml5TreeOperation.cpp:1215]
#04: nsHtml5TreeOpExecutor::RunFlushLoop() [parser/html/nsHtml5TreeOpExecutor.cpp:494]
#05: nsHtml5ExecutorFlusher::Run() [parser/html/nsHtml5StreamParser.cpp:133]
#06: mozilla::SchedulerGroup::Runnable::Run() [xpcom/threads/SchedulerGroup.cpp:395]
#07: nsThread::ProcessNextEvent(bool,bool *) [xpcom/threads/nsThread.cpp:1041]

No nsDocumentViewer involved there, so this is not a case that needs nsAutoScriptBlocker?

:tnikkel, any insight on this?

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> After some more thought I think adding 'NS_ENSURE_STATE(!mHaveShutDown)'
> after nsContentUtils::AddScriptRunner(new XBLConstructorRunner(mDocument))
> is a fix for this failure.

I think that is the right fix.

> One thing is not yet clear to me is that this is the case whether we need
> nsAutoScriptBlocker or not.
> 
> FWIW, here is a call stack in the case where the pres context was destroyed
> in the |nsContentUtils::AddScriptRunner(new
> XBLConstructorRunner(mDocument))|.
> 
> #01: nsContentSink::StartLayout(bool) [dom/base/nsContentSink.cpp:1271]
> #02: nsHtml5TreeOpExecutor::StartLayout(bool *)
> [parser/html/nsHtml5TreeOpExecutor.cpp:675]
> #03: nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor *,nsIContent *
> *,bool *,bool *) [parser/html/nsHtml5TreeOperation.cpp:1215]
> #04: nsHtml5TreeOpExecutor::RunFlushLoop()
> [parser/html/nsHtml5TreeOpExecutor.cpp:494]
> #05: nsHtml5ExecutorFlusher::Run() [parser/html/nsHtml5StreamParser.cpp:133]
> #06: mozilla::SchedulerGroup::Runnable::Run()
> [xpcom/threads/SchedulerGroup.cpp:395]
> #07: nsThread::ProcessNextEvent(bool,bool *)
> [xpcom/threads/nsThread.cpp:1041]
> 
> No nsDocumentViewer involved there, so this is not a case that needs
> nsAutoScriptBlocker?
> 
> :tnikkel, any insight on this?

I don't think we need a script blocker here because nsContentSink::StartLayout isn't going to get confused if the Initialize called re-enters that function, it will just skip the Initialize call the second time if it is the same shell.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Thank you!  A patch is coming.  Here is a try with that fix.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a04869cdb9ae818f078bb4cfb1c357c8ae5565c7

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1428694 - Bail out from PresShell::Initialize if the pres shell is being destroyed in XBLConstructorRunner.

Review commit: https://reviewboard.mozilla.org/r/216822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216822/

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8947006 [details]
Bug 1428694 - Bail out from PresShell::Initialize if the pres shell is being destroyed in XBLConstructorRunner.

https://reviewboard.mozilla.org/r/216822/#review222640

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Thank you for the review!

A try based on the patches for bug 1193394 just in case.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7127e25a5b09c2980d911621613c87434dd49a9a

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9eac4abdb4c0
Bail out from PresShell::Initialize if the pres shell is being destroyed in XBLConstructorRunner. r=tnikkel

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/9eac4abdb4c0