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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jason Kratzer [:jkratzer]]

Attachment: 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: ??? (???:???)

Comment 1

[Jason Kratzer [:jkratzer]]

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

Comment 2

[Andrew Overholt [:overholt]]

Boris, you seem to have written this code. Do you know what's up here?

Comment 3

[Boris Zbarsky [: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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Don't leave dangling exceptions on the JSContext when wrapper reparenting fails in nsNodeUtils::CloneAndAdopt

Comment 7

[Boris Zbarsky [:bzbarsky]]

Jason, would you be able to check whether those patches fix the problem for you?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Don't leave dangling exceptions on the JSContext when wrapper reparenting fails in nsNodeUtils::CloneAndAdopt

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Jason Kratzer [:jkratzer]]

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

Comment 13

[Peter Van der Beken [:peterv]]

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.

Comment 14

[Peter Van der Beken [:peterv]]

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.

Comment 15

[Peter Van der Beken [:peterv]]

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?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> 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

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 19

[Jason Kratzer [:jkratzer]]

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

Comment 20

[Jason Kratzer [:jkratzer]]

The original assertion, "Assertion failure: false (We had an exception; we should not have)" appears to have been fixed.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I filed bug 1399922 on the IsIdle(oldState) assertion.