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

RESOLVED FIXED in Firefox 45

Status

()

--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jruderman, Assigned: bzbarsky)

Tracking

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

Trunk
mozilla45
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
Created attachment 8693155 [details]
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().
(Reporter)

Comment 1

3 years ago
Created attachment 8693156 [details]
stack+
(Reporter)

Updated

3 years ago
Blocks: 306663

Comment 2

3 years ago
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.
Created attachment 8693168 [details] [diff] [review]
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
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.

Comment 7

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5be86ea3a376
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.