Assertion failure: false (We had an exception; we should not have)

RESOLVED FIXED in Firefox 57

Status

()

Core
DOM
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: jkratzer, Assigned: bz)

Tracking

(Blocks: 1 bug, {assertion, testcase})

unspecified
mozilla57
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

9 months ago
Created attachment 8901223 [details]
trigger.html

Testcase found while fuzzing mozilla-central rev 20170825-2306e153fba9.

Assertion failure: false (We had an exception; we should not have), at /home/worker/workspace/build/src/dom/script/ScriptSettings.cpp:446
#01: mozilla::dom::AutoEntryScript::AutoEntryScript at dom/script/ScriptSettings.cpp:674
#02: nsXPCWrappedJSClass::DelegatedQueryInterface at js/xpconnect/src/XPCWrappedJSClass.cpp:606
#03: nsQueryReferent::operator() at xpcom/base/nsWeakReference.cpp:78
#04: nsCOMPtr<nsIWebProgressListener>::assign_from_helper at xpcom/base/nsCOMPtr.h:1185
#05: nsCOMPtr<nsIWebProgressListener>::operator= at xpcom/base/nsCOMPtr.h:675
#06: nsDocLoader::DoFireOnStateChange at uriloader/base/nsDocLoader.cpp:1320
#07: nsDocLoader::doStopDocumentLoad at uriloader/base/nsDocLoader.cpp:860
#08: nsDocLoader::DocLoaderIsEmpty at uriloader/base/nsDocLoader.cpp:752
#09: nsDocLoader::OnStopRequest at uriloader/base/nsDocLoader.cpp:625
#10: mozilla::net::nsLoadGroup::RemoveRequest at netwerk/base/nsLoadGroup.cpp:634
#11: mozilla::net::nsLoadGroup::Cancel at netwerk/base/nsLoadGroup.cpp:268
#12: nsDocLoader::Stop at uriloader/base/nsDocLoader.cpp:246
#13: nsDocShell::Stop at xpcom/ds/nsTObserverArray.h:315
#14: nsDocShell::Destroy at docshell/base/nsDocShell.cpp:5906
#15: nsFrameLoader::DestroyDocShell at dom/base/nsFrameLoader.cpp:2298
#16: nsFrameLoaderDestroyRunnable::Run at dom/base/nsFrameLoader.cpp:2241
#17: nsDocument::MaybeInitializeFinalizeFrameLoaders at dom/base/nsDocument.cpp:7504
#18: mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0u>::Run at xpcom/threads/nsThreadUtils.h:1196
#19: nsContentUtils::RemoveScriptBlocker at xpcom/base/nsCOMPtr.h:600
#20: nsAutoScriptBlocker::~nsAutoScriptBlocker at dom/base/nsContentUtils.h:3392
#21: nsIDocument::AdoptNode at dom/base/nsDocument.cpp:7960
#22: mozilla::dom::DocumentBinding::adoptNode at s3:gecko-generated-sources:a746e4a05cf930d0a6e4dc0a4cde5ff89df2f552cf08c7024cc5a4c11cadc3e8aaaa0fcf690761bf828e83f5360beb173f542c5323fb238ace3a8bc4ff2547c9/dom/bindings/DocumentBinding.cpp::1424
#23: mozilla::dom::GenericBindingMethod at dom/bindings/BindingUtils.cpp:3055
#24: ??? (???:???)
Flags: in-testsuite?
(Reporter)

Comment 1

9 months ago
Testcase may also return the following assertion due to timing issues:

Assertion failure: IsIdle(oldState), at /home/worker/workspace/build/src/xpcom/ds/PLDHashTable.h:132
#01: PLDHashTable::RemoveEntry at xpcom/ds/PLDHashTable.cpp:644
#02: nsDOMAttributeMap::DropAttribute at dom/base/nsDOMAttributeMap.cpp:125
#03: mozilla::dom::Element::UnsetAttr at dom/base/Element.cpp:2877
#04: nsDOMAttributeMap::BlastSubtreeToPieces at dom/base/nsDocument.cpp:7754
#05: nsDOMAttributeMap::BlastSubtreeToPieces at dom/base/nsDocument.cpp:7762
#06: nsDOMAttributeMap::BlastSubtreeToPieces at dom/base/nsDocument.cpp:7762
#07: nsIDocument::AdoptNode at dom/base/nsDocument.cpp:7923
#08: mozilla::dom::DocumentBinding::adoptNode at s3:gecko-generated-sources:a746e4a05cf930d0a6e4dc0a4cde5ff89df2f552cf08c7024cc5a4c11cadc3e8aaaa0fcf690761bf828e83f5360beb173f542c5323fb238ace3a8bc4ff2547c9/dom/bindings/DocumentBinding.cpp::1424
#09: mozilla::dom::GenericBindingMethod at dom/bindings/BindingUtils.cpp:3055
#10: ??? (???:???)
Summary: Assertion failure: false (We had an exception; we should not have) → Assertion failure: false (We had an exception; we should not have) || Assertion failure: IsIdle(oldState)
Boris, you seem to have written this code. Do you know what's up here?
Flags: needinfo?(bzbarsky)
The other relevant part of the output here is:

  PREEXISTING EXCEPTION OBJECT: 'InternalError: too much recursion'

OK, so what's happening?  The testcase does:

      function fun_0() {
        document.implementation.createDocument('', '', null).adoptNode(o2);
      }
      document.addEventListener('DOMNodeRemoved', fun_0, false);

and then performs a node removal (via setting o1.innerText).  The node removal calls the DOMNodeRemoved listener _before_ the actual removal has happened, because that's how DOMNodeRemoved works.  The listener calls adoptNode, which tries to remove the node being adopted, which also calls the DOMNodeRemoved listener, etc.  Eventually we hit the "too much recursion" exception.

Which is all fine, but why is the exception sticking around on the JSContext until the next time we try to run script?  We're supposed to report the exception and move on; we call EventListener::HandleEvent with aExceptionHandling=eReportExceptions.  And we do report the exception there, and clear it from the JSContext.  So this part is working 

So we proceed with the adoption and call into nsNodeUtils::CloneAndAdopt.  This tries to dom::ReparentWrapper, which does a over-recursion check.  This check fails and throws _another_ "too much recursion" exception on the JSContext.  It's getting that JSContext via the usual "AutoJSContext" smuggling route, unfortunately.  That means that it does NOT report exceptions on it, but rather just leaves them hanging out there.

Then we unwind the stack from the failing CloneAndAdopt, get back into nsIDocument::AdoptNode, and BlastSubtreeToPieces to handle the failure.  At this point there's still an exception on the JSContext that we're not really aware of, because nsIDocument::AdoptNode does nothing with JSContexts.

Then the scriptblocker AdoptNode puts on the stack comes off the stack.  Since we actually removed the iframe from the DOM as part of BlastSubtreeToPieces, we have a frameloader finalization pending, which we run, and that gives us the stack in comment 0.

It's not really clear to me what a _good_ behavior here would be.  But having CloneAndAdopt either suppress or report the exception on its JSContext when there is one are the two obvious options here.  I guess reporting would be better, generally, if we can figure out a sane way to do it.  I'll poke at this a bit tomorrow.
Blocks: 1396904
So... I haven't been able to reproduce the assert with the testcase.  I can write a patch based on comment 3, but have no way to test it...
Created attachment 8905659 [details] [diff] [review]
part 1.  Change nsNodeUtils cloning/adopting stuff to use an ErrorResult for errors

This will allow us to propagate out more informative errors in some cases.
Attachment #8905659 - Flags: review?(peterv)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8905660 [details] [diff] [review]
part 2.  Don't leave dangling exceptions on the JSContext when wrapper reparenting fails in nsNodeUtils::CloneAndAdopt
Attachment #8905660 - Flags: review?(peterv)
Jason, would you be able to check whether those patches fix the problem for you?
Flags: needinfo?(bzbarsky) → needinfo?(jkratzer)
Created attachment 8905786 [details] [diff] [review]
part 2.  Don't leave dangling exceptions on the JSContext when wrapper reparenting fails in nsNodeUtils::CloneAndAdopt
Attachment #8905786 - Flags: review?(peterv)
Attachment #8905660 - Attachment is obsolete: true
Attachment #8905660 - Flags: review?(peterv)
Created attachment 8906026 [details] [diff] [review]
part 2.  Remove the XPCOM version of Document.adoptNode/importNode

These don't play nice with ErrorResult error propagation, and don't really have a reason to exist anyway.
Attachment #8906026 - Flags: review?(peterv)
Created attachment 8906027 [details] [diff] [review]
part 3.  Change dom::ReparentWrapper to take an ErrorResult

This makes it easier for its consumers to avoid leaving a dangling exception on
the JSContext.
Attachment #8906027 - Flags: review?(peterv)
Attachment #8905786 - Attachment is obsolete: true
Attachment #8905786 - Flags: review?(peterv)
Created attachment 8906207 [details] [diff] [review]
part 3.  Change dom::ReparentWrapper to take an ErrorResult

This makes it easier for its consumers to avoid leaving a dangling exception on
the JSContext.
Attachment #8906207 - Flags: review?(peterv)
Attachment #8906027 - Attachment is obsolete: true
Attachment #8906027 - Flags: review?(peterv)
(Reporter)

Comment 12

8 months ago
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #7)
> Jason, would you be able to check whether those patches fix the problem for
> you?

I'm currently unable to compile debug builds.  After speaking with Boris, we'll wait for this patch to land and retest.
Flags: needinfo?(jkratzer)
Comment on attachment 8905659 [details] [diff] [review]
part 1.  Change nsNodeUtils cloning/adopting stuff to use an ErrorResult for errors

Review of attachment 8905659 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xbl/nsXBLBinding.cpp
@@ +321,5 @@
>  
> +    IgnoredErrorResult rv;
> +    nsCOMPtr<nsINode> clonedNode =
> +      nsNodeUtils::Clone(content, true, doc->NodeInfoManager(), nullptr, rv);
> +    // XXXbz Why is this code OK assuming that nsNodeUtils::Clone never fails?

Can you file a bug? It might not be trivial to deal with an error here, but we should probably figure it out.
Attachment #8905659 - Flags: review?(peterv) → review+
Comment on attachment 8906026 [details] [diff] [review]
part 2.  Remove the XPCOM version of Document.adoptNode/importNode

Review of attachment 8906026 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsINode.cpp
@@ +1522,5 @@
>    nsIDocument *doc = aParent->OwnerDoc();
>  
> +  ErrorResult rv;
> +  nsINode* adoptedNode = doc->AdoptNode(*aNode, rv);
> +  rv.WouldReportJSException();

I don't completely understand why we need this, but you remove it in part 3.
Attachment #8906026 - Flags: review?(peterv) → review+
Comment on attachment 8906207 [details] [diff] [review]
part 3.  Change dom::ReparentWrapper to take an ErrorResult

Review of attachment 8906207 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/crashtests/1393806.html
@@ +3,5 @@
> +    <script>
> +      function fun_0() {
> +        document.implementation.createDocument('', '', null).adoptNode(o2);
> +      }
> +      

Trailing whitespace.

::: dom/base/nsINode.cpp
@@ +1570,5 @@
>    if (OwnerDoc() != aKid->OwnerDoc()) {
> +    ErrorResult error;
> +    AdoptNodeIntoOwnerDoc(this, aKid, error);
> +
> +    error.WouldReportJSException();

Hmm, I think I understand now. Because ReparentWrapper steals the exception from the context into this ErrorResult we need to deal with it here.

Can you file a bug about changing doInsertChildAt (and InsertChildAt and any other callers) to use ErrorResult?

I wondered whether it would make sense to unconditionally call StealNSResult (which always calls WouldReportJSException I think) instead of adding WouldReportJSException in various places. But I guess this way it's clearer that if we switch doInsertChildAt to ErrorResult any caller that converts the ErrorResult to nsresult would need to call WouldReportJSException itself. In any case, these WouldReportJSException calls need comments about why they're there and when we can remove them.

::: dom/html/nsHTMLDocument.cpp
@@ +1723,5 @@
>      nsCOMPtr<nsIScriptGlobalObject> newScope(do_QueryReferent(mScopeObject));
>      JS::Rooted<JSObject*> wrapper(cx, GetWrapper());
>      if (oldScope && newScope != oldScope && wrapper) {
>        JSAutoCompartment ac(cx, wrapper);
> +      mozilla::dom::ReparentWrapper(cx, wrapper, rv);

Ugh, I wish we'd used aError instead of rv. I was assuming this was a local ErrorResult, which would need WouldReportJSException, right?

::: dom/xml/XMLDocument.cpp
@@ +176,5 @@
>      options.SetAsString();
>  
>      nsCOMPtr<Element> root =
>        doc->CreateElementNS(aNamespaceURI, aQualifiedName, options, result);
> +    result.WouldReportJSException();

Shouldn't this be after the AppendChild below instead?
Attachment #8906207 - Flags: review?(peterv) → review+
> Can you file a bug? It might not be trivial to deal with an error here, but we should probably figure it out.

Bug 1399558 filed.

> I don't completely understand why we need this

Because of the assertions around JS exceptions on ErrorResult.  Those assert that:

1)  Any codepath that throws a JS exception has called MightThrowJSException.  
2)  Any codepath that calls MightThrowJSException also calls WouldReportJSException.

the idea being to not leak JS exceptions.

> Trailing whitespace.

Fixed.

> Because ReparentWrapper steals the exception from the context into this ErrorResult we need to deal with it here.

Right.

> Can you file a bug about changing doInsertChildAt (and InsertChildAt and any other callers) to use ErrorResult?

Bug 1399560 filed.

> In any case, these WouldReportJSException calls need comments about why they're there and when we can remove them.

Added:

    // Need to WouldReportJSException() if our callee can throw a JS
    // exception (which it can) and we're neither propagating the
    // error out nor unconditionally suppressing it.

> Ugh, I wish we'd used aError instead of rv.

I'll just rename it.  If it were local, it would need WouldReportJSException, yes.

> Shouldn't this be after the AppendChild below instead?

It actually doesn't matter.  Just needs to be true that we would actually report the JS exception on any codepath on which we'd reach this call.  But I agree it's clearer after the AppendChild; moved it there.

Comment 17

8 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3105c9342885
part 1.  Change nsNodeUtils cloning/adopting stuff to use an ErrorResult for errors.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/25ba920b0c5a
part 2.  Remove the XPCOM version of Document.adoptNode/importNode.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca7ea3ba0fe
part 3.  Change dom::ReparentWrapper to take an ErrorResult.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/867fe67b16a1
part 4.  Rename the ErrorResult argument of nsHTMLDocument::Open to aError.

Comment 18

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3105c9342885
https://hg.mozilla.org/mozilla-central/rev/25ba920b0c5a
https://hg.mozilla.org/mozilla-central/rev/1ca7ea3ba0fe
https://hg.mozilla.org/mozilla-central/rev/867fe67b16a1
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Reporter)

Comment 19

8 months ago
I just tested this on mozilla-central rev ef0f1085d54e and the following assertion is triggered:

Assertion failure: IsIdle(oldState), at /home/mozilla/source/firefox/xpcom/ds/PLDHashTable.h:132
(Reporter)

Comment 20

8 months ago
The original assertion, "Assertion failure: false (We had an exception; we should not have)" appears to have been fixed.
I filed bug 1399922 on the IsIdle(oldState) assertion.
Summary: Assertion failure: false (We had an exception; we should not have) || Assertion failure: IsIdle(oldState) → Assertion failure: false (We had an exception; we should not have)
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox-esr52: --- → unaffected
Flags: in-testsuite? → in-testsuite+
Depends on: 1408444
Depends on: 1413253
(Reporter)

Updated

7 months ago
Blocks: 1415761
You need to log in before you can comment on or make changes to this bug.