Closed Bug 1228707 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files)

Attached file 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().
Attached file stack+
Blocks: stirdom
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.
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
Attachment #8693168 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8693168 - Flags: review?(bugs) → review+
At least RefreshDriver calling animation frame callbacks could now drop 
ErrorResult ignored; and ignored.SuppressException();
> At least RefreshDriver calling animation frame callbacks could now drop

Yep, done.
https://hg.mozilla.org/mozilla-central/rev/5be86ea3a376
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.