1440523 - Assertion failure: !mIsBeingDestroyed, at /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp:12989

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Raymond Forbes[:rforbes]]

Attachment: testcase.txt

this was found while fuzzing REV 3f474e48db7f

Crash|SIGSEGV|0x0|0
0|0|libxul.so|nsDocShell::EnsureEditorData|hg:hg.mozilla.org/mozilla-central:docshell/base/nsDocShell.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|12989|0x18
0|1|libxul.so|nsDocShell::GetEditingSession|hg:hg.mozilla.org/mozilla-central:docshell/base/nsDocShell.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|14378|0x5
0|2|libxul.so|nsHTMLDocument::EditingStateChanged|hg:hg.mozilla.org/mozilla-central:dom/html/nsHTMLDocument.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|2510|0x10
0|3|libxul.so|nsHtml5TreeOpExecutor::DidBuildModel|hg:hg.mozilla.org/mozilla-central:parser/html/nsHtml5TreeOpExecutor.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|201|0x14
0|4|libxul.so|nsHtml5Parser::Terminate|hg:hg.mozilla.org/mozilla-central:parser/html/nsHtml5Parser.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|536|0x19
0|5|libxul.so|nsHTMLDocument::StopDocumentLoad|hg:hg.mozilla.org/mozilla-central:dom/html/nsHTMLDocument.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|825|0x8
0|6|libxul.so|nsDocumentViewer::Stop|hg:hg.mozilla.org/mozilla-central:layout/base/nsDocumentViewer.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|1826|0x15
0|7|libxul.so|nsDocShell::Stop|hg:hg.mozilla.org/mozilla-central:docshell/base/nsDocShell.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|5192|0x14
0|8|libxul.so|nsDocShell::Destroy|hg:hg.mozilla.org/mozilla-central:docshell/base/nsDocShell.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|5512|0x5
0|9|libxul.so|nsFrameLoader::DestroyDocShell|hg:hg.mozilla.org/mozilla-central:dom/base/nsFrameLoader.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|1893|0x11
0|10|libxul.so|nsFrameLoaderDestroyRunnable::Run|hg:hg.mozilla.org/mozilla-central:dom/base/nsFrameLoader.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|1831|0x14
0|11|libxul.so|nsDocument::MaybeInitializeFinalizeFrameLoaders|hg:hg.mozilla.org/mozilla-central:dom/base/nsDocument.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|6680|0x6
0|12|libxul.so|nsDocument::EndUpdate|hg:hg.mozilla.org/mozilla-central:dom/base/nsDocument.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|5144|0x8
0|13|libxul.so|nsHTMLDocument::EndUpdate|hg:hg.mozilla.org/mozilla-central:dom/html/nsHTMLDocument.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|2271|0x5
0|14|libxul.so|mozAutoDocUpdate::~mozAutoDocUpdate|hg:hg.mozilla.org/mozilla-central:dom/base/mozAutoDocUpdate.h:994a684a7564c2735d98d6910a78d079a68f0b25|40|0x14
0|15|libxul.so|nsINode::ReplaceOrInsertBefore|hg:hg.mozilla.org/mozilla-central:dom/base/nsINode.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|2320|0xc
0|16|libxul.so|mozilla::dom::NodeBinding::replaceChild|hg:hg.mozilla.org/mozilla-central:dom/base/nsINode.h:994a684a7564c2735d98d6910a78d079a68f0b25|1937|0x16
0|17|libxul.so|mozilla::dom::GenericBindingMethod|hg:hg.mozilla.org/mozilla-central:dom/bindings/BindingUtils.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|3031|0x9
0|18|libxul.so|js::CallJSNative|hg:hg.mozilla.org/mozilla-central:js/src/vm/JSContext-inl.h:994a684a7564c2735d98d6910a78d079a68f0b25|290|0x6
0|19|libxul.so|js::InternalCallOrConstruct|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|468|0xf
0|20|libxul.so|InternalCall|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|517|0xd
0|21|libxul.so|Interpret|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|523|0xf
0|22|libxul.so|js::RunScript|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|418|0xb
0|23|libxul.so|js::ExecuteKernel|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|701|0x5
0|24|libxul.so|js::Execute|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|734|0x5
0|25|libxul.so|ExecuteScript|hg:hg.mozilla.org/mozilla-central:js/src/jsapi.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|4720|0x12
0|26|libxul.so|ExecuteScript|hg:hg.mozilla.org/mozilla-central:js/src/jsapi.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|4739|0x5
0|27|libxul.so|nsJSUtils::ExecutionContext::CompileAndExec|hg:hg.mozilla.org/mozilla-central:dom/base/nsJSUtils.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|266|0xc
0|28|libxul.so|mozilla::dom::ScriptLoader::EvaluateScript|hg:hg.mozilla.org/mozilla-central:dom/script/ScriptLoader.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|2315|0x12
0|29|libxul.so|mozilla::dom::ScriptLoader::ProcessRequest|hg:hg.mozilla.org/mozilla-central:dom/script/ScriptLoader.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|1950|0xb
0|30|libxul.so|mozilla::dom::ScriptLoader::ProcessInlineScript|hg:hg.mozilla.org/mozilla-central:dom/script/ScriptLoader.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|1591|0xf
0|31|libxul.so|mozilla::dom::ScriptLoader::ProcessScriptElement|hg:hg.mozilla.org/mozilla-central:dom/script/ScriptLoader.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|1310|0xb
0|32|libxul.so|mozilla::dom::ScriptElement::MaybeProcessScript|hg:hg.mozilla.org/mozilla-central:dom/script/ScriptElement.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|147|0x13
0|33|libxul.so|nsIScriptElement::AttemptToExecute|hg:hg.mozilla.org/mozilla-central:dom/script/nsIScriptElement.h:994a684a7564c2735d98d6910a78d079a68f0b25|247|0x3
0|34|libxul.so|nsHtml5TreeOpExecutor::RunScript|hg:hg.mozilla.org/mozilla-central:parser/html/nsHtml5TreeOpExecutor.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|736|0x10
0|35|libxul.so|nsHtml5TreeOpExecutor::RunFlushLoop|hg:hg.mozilla.org/mozilla-central:parser/html/nsHtml5TreeOpExecutor.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|540|0x8
0|36|libxul.so|nsHtml5ExecutorFlusher::Run|hg:hg.mozilla.org/mozilla-central:parser/html/nsHtml5StreamParser.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|131|0x10
0|37|libxul.so|mozilla::SchedulerGroup::Runnable::Run|hg:hg.mozilla.org/mozilla-central:xpcom/threads/SchedulerGroup.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|413|0x1c
0|38|libxul.so|nsThread::ProcessNextEvent|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|1040|0x15
0|39|libxul.so|NS_ProcessNextEvent|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|517|0x11
0|40|libxul.so|mozilla::ipc::MessagePump::Run|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|97|0xa
0|41|libxul.so|MessageLoop::RunInternal|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:994a684a7564c2735d98d6910a78d079a68f0b25|326|0x17
0|42|libxul.so|MessageLoop::Run|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:994a684a7564c2735d98d6910a78d079a68f0b25|319|0x8
0|43|libxul.so|nsBaseAppShell::Run|hg:hg.mozilla.org/mozilla-central:widget/nsBaseAppShell.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|157|0xd
0|44|libxul.so|XRE_RunAppShell|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsEmbedFunctions.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|892|0x11
0|45|libxul.so|mozilla::ipc::MessagePumpForChildProcess::Run|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|269|0x5
0|46|libxul.so|MessageLoop::RunInternal|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:994a684a7564c2735d98d6910a78d079a68f0b25|326|0x17
0|47|libxul.so|MessageLoop::Run|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:994a684a7564c2735d98d6910a78d079a68f0b25|319|0x8
0|48|libxul.so|XRE_InitChildProcess|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsEmbedFunctions.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|718|0x8
0|49|firefox|content_process_main|hg:hg.mozilla.org/mozilla-central:ipc/contentproc/plugin-container.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|63|0x14
0|50|firefox|main|hg:hg.mozilla.org/mozilla-central:browser/app/nsBrowserApp.cpp:994a684a7564c2735d98d6910a78d079a68f0b25|280|0x11
0|51|libc-2.23.so||||0x20830
0|52|firefox|MOZ_ReportAssertionFailure|hg:hg.mozilla.org/mozilla-central:mfbt/Assertions.h:994a684a7564c2735d98d6910a78d079a68f0b25|164|0x5

Comment 1

[Raymond Forbes[:rforbes]]

Attachment: prefs.js

Comment 2

[Tyson Smith [:tsmith]]

We are seeing this very frequently when fuzzing.

Andrew, who would be able to have a look at this?

Comment 3

[Andrew Overholt [:overholt]]

OT: are we able to these traces more readable? :)

Comment 4

[Andrew Overholt [:overholt]]

(In reply to Tyson Smith [:tsmith] from comment #2)
> We are seeing this very frequently when fuzzing.

Did this just start happening? Bug 1432396 (specifically https://hg.mozilla.org/mozilla-central/rev/57abd8e4ab25) seems to have added the assertion we're hitting so maybe Hiro or bz know what's up.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Right, so the key is that we have this stack:

  nsDocShell::EnsureEditorData
  nsDocShell::GetEditingSession
  nsHTMLDocument::EditingStateChanged
  nsHtml5TreeOpExecutor::DidBuildModel
  nsHtml5Parser::Terminate
  nsHTMLDocument::StopDocumentLoad
  nsDocumentViewer::Stop
  nsDocShell::Stop
  nsDocShell::Destroy

We should probably either change EnsureEditorData to not assert (since it handles that case sanely anyway) or do a "is destroying" check somewhere upstream.  Hiro, thoughts?  You looked at this stuff pretty recently...

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I will look into this later today.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> Right, so the key is that we have this stack:
> 
>   nsDocShell::EnsureEditorData
>   nsDocShell::GetEditingSession
>   nsHTMLDocument::EditingStateChanged
>   nsHtml5TreeOpExecutor::DidBuildModel
>   nsHtml5Parser::Terminate
>   nsHTMLDocument::StopDocumentLoad
>   nsDocumentViewer::Stop
>   nsDocShell::Stop
>   nsDocShell::Destroy
> 
> We should probably either change EnsureEditorData to not assert (since it
> handles that case sanely anyway) or do a "is destroying" check somewhere
> upstream.

Yep, I think we should do the check in upstream.  At first glance nsHTMLDocument::EditingStateChanged() has a FlushPendingNotifications call, we should bail out early there.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Masayuki, we are calling FlushPendingNotification [1] in nsHTMLDocument::EditingStateChanged(), after this call, is there anything that we have to do even if the presshell (docshell) is being destroyed?  If not, I will simply just bail out there.

[1] https://hg.mozilla.org/mozilla-central/file/d7b0d0e7228d/dom/html/nsHTMLDocument.cpp#l2353

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Masayuki ^

Comment 9

[Hiroyuki Ikezoe (:hiro)]

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

I will try to request review instead of waiting for the NI.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1440523 - Bail out from nsHTMLDocument::EditingStateChanged if the docshell is being destroyed by FlushPendingNotifications call.

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

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Comment on attachment 8961991 [details]
Bug 1440523 - Bail out from nsHTMLDocument::EditingStateChanged if the docshell is being destroyed by FlushPendingNotifications call.

https://reviewboard.mozilla.org/r/230840/#review236468

::: dom/html/crashtests/1440523.html:4
(Diff revision 1)
> +<html>
> +  <head>
> +    <script>
> +      try { o1 = document.createElement('frame') } catch(e) { }

If renaming variable name does not affect whether this test causes crash, please name o1 to |frame|.

::: dom/html/crashtests/1440523.html:6
(Diff revision 1)
> +<html>
> +  <head>
> +    <script>
> +      try { o1 = document.createElement('frame') } catch(e) { }
> +      try { document.documentElement.appendChild(o1) } catch(e) { }
> +      try { o2 = o1.contentDocument } catch(e) { }

Same, please name o2 to |contentDocument|, |documentOfFrame| or something.

::: dom/html/crashtests/1440523.html:8
(Diff revision 1)
> +    <script>
> +      try { o1 = document.createElement('frame') } catch(e) { }
> +      try { document.documentElement.appendChild(o1) } catch(e) { }
> +      try { o2 = o1.contentDocument } catch(e) { }
> +      try { o2.writeln("<p contenteditable='true'>") } catch(e) { }
> +      try { o4 = document.implementation.createHTMLDocument(); } catch(e) { }

Same, please rename o4 to |anotherDocument| or something.

::: dom/html/crashtests/1440523.html:9
(Diff revision 1)
> +      try { o1 = document.createElement('frame') } catch(e) { }
> +      try { document.documentElement.appendChild(o1) } catch(e) { }
> +      try { o2 = o1.contentDocument } catch(e) { }
> +      try { o2.writeln("<p contenteditable='true'>") } catch(e) { }
> +      try { o4 = document.implementation.createHTMLDocument(); } catch(e) { }
> +      try { o5 = o4.documentElement; } catch(e) { }

Same, please rename o5 to |rootOfAnotherDocument| or something similar name.

::: dom/html/nsHTMLDocument.cpp:2505
(Diff revision 1)
>    if (!docshell)
>      return NS_ERROR_FAILURE;
>  
> +  // FlushPendingNotifications might destroy our docshell.
> +  bool isBeingDestroyed = false;
> +  docshell->IsBeingDestroyed(&isBeingDestroyed);

Hmm, we still need to use XPCOM method to retrieve this kind of information, sigh.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8961991 [details]
Bug 1440523 - Bail out from nsHTMLDocument::EditingStateChanged if the docshell is being destroyed by FlushPendingNotifications call.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/230840/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46216d5f85a4
Bail out from nsHTMLDocument::EditingStateChanged if the docshell is being destroyed by FlushPendingNotifications call. r=masayuki

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/46216d5f85a4

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact here which justifies an uplift request to Beta?

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8961991 [details]
Bug 1440523 - Bail out from nsHTMLDocument::EditingStateChanged if the docshell is being destroyed by FlushPendingNotifications call.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1432396 
[User impact if declined]: nothing particular should happen on release build, an assertion happens on debug build in particular cases
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: very low
[Why is the change risky/not risky?]: This patch did just stop unnecessary works during document is being destroying
[String changes made/needed]: None

Comment 17

[Julien Cristau [:jcristau]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> Is there a user impact here which justifies an uplift request to Beta?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> [User impact if declined]: nothing particular should happen on release
> build, an assertion happens on debug build in particular cases

Sounds like that's a no to the above question.  Unless the fuzzing team needs this for 60.

Comment 18

[Intermittent Failures Robot]

4 failures in 4666 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 4

Platform breakdown:

• linux64: 1
• linux64-qr: 3

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1440523&startday=2019-08-05&endday=2019-08-11&tree=all

Comment 19

[Release mgmt bot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.