1228707 - "Assertion failure: !Failed(), at ErrorResult.h:103" with MutationObserver that throws

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: !Failed(), at ErrorResult.h:103

This assertion was added recently:

changeset:   https://hg.mozilla.org/mozilla-central/rev/ecb3051bba08
user:        Boris Zbarsky
date:        Fri Nov 20 16:29:41 2015 -0500
summary:     Bug 1224007 part 6.  Change MaybeSetPendingException to set the ErrorResult state to "not failed", just like SuppressException and StealNSResult already do, and assert in the destructor that the ErrorResult is not Failed().

Comment 1

[Jesse Ruderman]

Attachment: stack+

Comment 2

[Bob Clary [:bc] (inactive)]

FYI, We've seen this assertion in Bughunter on about 70 urls so far. The majority do involve nsDOMMutationObserver::HandleMutation on the stack.

At first I thought it was directly related to the Spider extension we use to load pages since it did not reproduce just loading the page manually, but realized this evening that I could also reproduce it by loading a page where it had reproduced, then opening a new tab and while the original page was still loading, closing its tab.

There is a smaller group of urls where NS_CreateJSTimeoutHandler is present on the stack without MutationObserver at all. If I can reproduce it, I'll file that separately.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Add a away to call Web IDL callbacks while ignoring any errors from them, and use it in a few places

It seems like a common pattern to have callbacks whose return value is ignored,
so I figured I should just add IDL support for that.  The idea is to add some
overloads of the callback method that _don't_ have an ErrorResult argument,
put one on the stack themselves, and suppress it after the call.  This was
easiest if I also added an IgnoredErrorResult, because then I don't have to
worry about declaring a temporary for the return type etc.

In any case, the codegen for MutationCallback used to have two public overloads
of Call() with these signatures:

  template <typename T>
  inline void
  Call(const T& thisVal, const Sequence<OwningNonNull<nsDOMMutationRecord>>& mutations, nsDOMMutationObserver& observer, ErrorResult& aRv, const char* aExecutionReason = nullptr, ExceptionHandling aExceptionHandling = eReportExceptions, JSCompartment* aCompartment = nullptr)

and

  inline void
  Call(const Sequence<OwningNonNull<nsDOMMutationRecord>>& mutations, nsDOMMutationObserver& observer, ErrorResult& aRv, const char* aExecutionReason = nullptr, ExceptionHandling aExceptionHandling = eReportExceptions, JSCompartment* aCompartment = nullptr)

This patch adds two more overloads with the following signatures and implementations:

  template <typename T>
  inline void
  Call(const T& thisVal, const Sequence<OwningNonNull<nsDOMMutationRecord>>& mutations, nsDOMMutationObserver& observer, const char* aExecutionReason = nullptr)
  {
    IgnoredErrorResult rv;
    return Call(thisVal, mutations, observer, rv, aExecutionReason);
  }

  inline void
  Call(const Sequence<OwningNonNull<nsDOMMutationRecord>>& mutations, nsDOMMutationObserver& observer, const char* aExecutionReason = nullptr)
  {
    IgnoredErrorResult rv;
    return Call(mutations, observer, rv, aExecutionReason, eReportExceptions, nullptr);
  }

Please see the comments in the codegen for why that last one passes
"eReportExceptions, nullptr" explicitly

Comment 4

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

At least RefreshDriver calling animation frame callbacks could now drop 
ErrorResult ignored; and ignored.SuppressException();

Comment 5

[Boris Zbarsky [:bzbarsky]]

> At least RefreshDriver calling animation frame callbacks could now drop

Yep, done.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5be86ea3a376

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5be86ea3a376