1688585 - Crash in [@ mozilla::dom::(anonymous namespace)::PromiseNativeHandlerShim::RejectedCallback]

Gabriele Svelto [:gsvelto] (PTO)

Reporter

Description

•

3 years ago

Crash report: https://crash-stats.mozilla.org/report/index/2ae0cb7c-4831-49c9-9ce2-811210210123

Reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS

Top 10 frames of crashing thread:

0 XUL mozilla::dom:: dom/promise/Promise.cpp:391
1 XUL mozilla::dom::NativeHandlerCallback dom/promise/Promise.cpp:341
2 XUL js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:594
3 XUL js::Call js/src/vm/Interpreter.cpp:664
4 XUL PromiseReactionJob js/src/builtin/Promise.cpp:1904
5 XUL js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:594
6 XUL JS::Call js/src/jsapi.cpp:2861
7 XUL mozilla::dom::PromiseJobCallback::Call dom/bindings/PromiseBinding.cpp:31
8 XUL mozilla::PromiseJobRunnable::Run xpcom/base/CycleCollectedJSContext.cpp:211
9 XUL mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint xpcom/base/CycleCollectedJSContext.cpp:644

I found this during nightly crash triage but it's not a recent regression given the first version with significant volume is release 84. The crash is caused by a NULL pointer access and it's happening on shutdown - at least two user comments mention this happening when they tried to quit Firefox. I can't tell from the stack what's the affected component unfortunately.

Andrew McCreight [:mccr8]

Comment 1

•

3 years ago

I think DOM is a reasonable starting component for a crash involving DOM promises.

Component: General → DOM: Core & HTML

Masayuki Nakano [:masayuki] (he/him)(JST, +0900)

Comment 2

•

3 years ago

It seems that the crash reports are coming after landing of bug 1679094.

djg: Could you take a look?

Flags: needinfo?(dan.glastonbury)

Steven Michaud [:smichaud] (Retired)

Comment 3

•

3 years ago

A recent crash report shows potentially interesting mac_crash_info:

bp-170a9dba-d442-4564-b8fc-ef7b30210718

    {
      "num_records": 1,
      "records": [
        {
          "message": "Performing @selector(menuItemHit:) from sender NSMenuItem 0x12b0957b0",
          "module": "/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit"
        }
      ]
    }

This is consistent with the crash having been triggered by pressing Cmd-Q or choosing Quit Firefox from the menu.

Hsin-Yi Tsai (she/her) [:hsinyi]

Updated

•

2 years ago

Severity: -- → S3

Hsin-Yi Tsai (she/her) [:hsinyi]

Updated

•

2 years ago

Flags: needinfo?(u480271)

Andrew McCreight [:mccr8]

Comment 4

•

2 years ago

Looks like maybe mInner was unlinked or maybe previously resolved or rejected. I'm not sure how any of that could happen. We could probably just return if mInner is null, though maybe we'd still be in a bad state.

Hsin-Yi Tsai (she/her) [:hsinyi]

Comment 5

•

2 years ago

•

Edited

Hey Kagami,
The crash volume increased on nightly since April. From the crash stack, it's not very clear where we got this problematic promise from. However, you've touched Promise code recently for Streams, and we've started landed/enabled Streams recently. It could be that the crash increment on nightly is coming from unexpected resolved/rejected promises in this new feature. So I think it makes sense to start from asking for your help to take a first look. Thank you.

Flags: needinfo?(krosylight)

Kagami Rosylight [:saschanaz] (they/them)

Comment 6

•

2 years ago

Hmm, indeed Streams heavily uses Promise::AppendNativeHandler and thus the function. But I'm off this week, NI'ing :smaug in case he can get some idea before I return.

Flags: needinfo?(krosylight) → needinfo?(bugs)

Peter Van der Beken [:peterv]

Assignee

Comment 7

•

2 years ago

Smaug and I looked at it earlier this week. I found one problematic call to Promise::AppendNativeHandler, and we're also going to convert the assert at https://searchfox.org/mozilla-central/rev/ecd91b104714a8b2584a4c03175be50ccb3a7c67/dom/promise/Promise.cpp#405 to a diagnostic assert.

Assignee: nobody → peterv

Status: NEW → ASSIGNED

Flags: needinfo?(bugs)

Peter Van der Beken [:peterv]

Assignee

Comment 8

•

2 years ago

Attached file Bug 1688585 - extensions::RequestWorkerRunnable::Init should propagate failure of dom::PromiseWorkerProxy::Create. r?rpl! — Details

Peter Van der Beken [:peterv]

Assignee

Comment 9

•

2 years ago

Attached file Bug 1688585 - Make PromiseNativeHandlerShim diagnostic assert that the PromiseNativeHandler is non-null. r?smaug! — Details

Depends on D145066

Peter Van der Beken [:peterv]

Assignee

Updated

•

2 years ago

Keywords: leave-open

Pulsebot

Comment 10

•

2 years ago

Pushed by pvanderbeken@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d2b018a42f9
extensions::RequestWorkerRunnable::Init should propagate failure of dom::PromiseWorkerProxy::Create. r=rpl
https://hg.mozilla.org/integration/autoland/rev/5e2758073f60
Make PromiseNativeHandlerShim diagnostic assert that the PromiseNativeHandler is non-null. r=smaug

Peter Van der Beken [:peterv]

Assignee

Updated

•

2 years ago

Crash Signature: [@ mozilla::dom::(anonymous namespace)::PromiseNativeHandlerShim::RejectedCallback] → [@ mozilla::dom::(anonymous namespace)::PromiseNativeHandlerShim::RejectedCallback][@ mozilla::dom::(anonymous namespace)::PromiseNativeHandlerShim::ResolvedCallback]

Atila Butkovits

Comment 11

•

2 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/4d2b018a42f9
https://hg.mozilla.org/mozilla-central/rev/5e2758073f60

Peter Van der Beken [:peterv]

Assignee

Comment 12

•

2 years ago

Attached file Bug 1688585 - Log the reason for nulling out PromiseNativeHandlerShim::mInner. r?smaug! — Details

Pulsebot

Comment 13

•

2 years ago

Pushed by pvanderbeken@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/912fe57e9efb
Log the reason for nulling out PromiseNativeHandlerShim::mInner. r=smaug

bszekely

Comment 14

•

2 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/912fe57e9efb

Andrew McCreight [:mccr8]

Updated

•

2 years ago

Regressions: 1768823

Peter Van der Beken [:peterv]

Assignee

Updated

•

2 years ago

No longer regressions: 1768823

Peter Van der Beken [:peterv]

Assignee