Closed Bug 1440523 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: DOM: Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: rforbes, Assigned: hiro)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [fuzzblocker])

Attachments

(3 files)

Attached file 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
Attached file prefs.js
We are seeing this very frequently when fuzzing.

Andrew, who would be able to have a look at this?
Flags: needinfo?(overholt)
Whiteboard: [fuzzblocker]
OT: are we able to these traces more readable? :)
Flags: needinfo?(rforbes)
(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.
Flags: needinfo?(overholt)
Flags: needinfo?(hikezoe)
Flags: needinfo?(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...
Flags: needinfo?(bzbarsky)
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.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
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
Flags: needinfo?(hikezoe)
Masayuki ^
Flags: needinfo?(masayuki)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e7aa2c45a65c14bfe5f1a2989f171a0e1cc0a15

I will try to request review instead of waiting for the NI.
Flags: needinfo?(masayuki)
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.
Attachment #8961991 - Flags: review?(masayuki) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46216d5f85a4
Bail out from nsHTMLDocument::EditingStateChanged if the docshell is being destroyed by FlushPendingNotifications call. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/46216d5f85a4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Is there a user impact here which justifies an uplift request to Beta?
Flags: needinfo?(hikezoe)
Flags: in-testsuite+
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
Flags: needinfo?(hikezoe)
Attachment #8961991 - Flags: approval-mozilla-beta?
(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.
Attachment #8961991 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Flags: needinfo?(rforbes)

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

Keywords: regression
You need to log in before you can comment on or make changes to this bug.