1908466 - Crash [@ Aborted]

Status: VERIFIED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Testcase found while fuzzing mozilla-central rev 9abfb7fcb613 (built with: --enable-debug --enable-fuzzing).

Testcase can be reproduced using the following commands:

$ pip install fuzzfetch grizzly-framework --upgrade
$ python -m fuzzfetch --build 9abfb7fcb613 --debug --fuzzing  -n firefox
$ python -m grizzly.replay.bugzilla ./firefox/firefox <bugid>

[@ Aborted]

    ==1045767==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x0000000000a0 (pc 0x70274bb5f5fe bp 0x70272e12fdc0 sp 0x70272e12fd40 T1045819)
    ==1045767==The signal is caused by a READ memory access.
    ==1045767==Hint: address points to the zero page.
        #0 0x70274bb5f5fe in Aborted /dom/abort/AbortSignal.cpp:31:48
        #1 0x70274bb5f5fe in mozilla::dom::AbortSignal::Any(mozilla::dom::GlobalObject&, mozilla::dom::Sequence<mozilla::OwningNonNull<mozilla::dom::AbortSignal>> const&) /dom/abort/AbortSignal.cpp:296:9
        #2 0x70274c15f08e in mozilla::dom::AbortSignal_Binding::any(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/obj-build/dom/bindings/./AbortSignalBinding.cpp:179:57
        #3 0x7027503ff6c4 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /js/src/vm/Interpreter.cpp:491:13
        #4 0x7027503feeaf in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /js/src/vm/Interpreter.cpp:585:12
        #5 0x702750ebe836 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /js/src/jit/BaselineIC.cpp:1670:10
        #6 0x17cdab5bca5e  ([anon:js-executable-memory]+0xba5e)
    
    UndefinedBehaviorSanitizer can not provide additional info.
    SUMMARY: UndefinedBehaviorSanitizer: SEGV /dom/abort/AbortSignal.cpp:31:48 in Aborted
    ==1045767==ABORTING

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Comment 2

[Andrew McCreight [:mccr8]]

From the stack, looks like maybe sourceSignal is null here?

Comment 3

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20240717212306-e2109b806cd9.
The bug appears to have been introduced in the following build range:

Start: 2437c2ca5bec35fe4ab02eb938e0e02457cd079b (20240123142542)
End: f3efca74da0f43269bd8ac07e2a5d27e89c4d7c3 (20240123145016)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2437c2ca5bec35fe4ab02eb938e0e02457cd079b&tochange=f3efca74da0f43269bd8ac07e2a5d27e89c4d7c3

Comment 4

[Andrew McCreight [:mccr8]]

Bug 1830781 is where AbortSignal.any() was implemented, and the test case includes that, so that seems like a likely regressor.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1830781

:vhilla, since you are the author of the regressor, bug 1830781, could you take a look?

For more information, please visit BugBot documentation.

Comment 6

[Bugmon [:jkratzer for issues]]

Successfully recorded a pernosco session. A link to the pernosco session will be added here shortly.

Comment 7

[Bugmon [:jkratzer for issues]]

A pernosco session for this bug can be found here.

Comment 8

[Vincent Hilla [:vhilla]]

Here is a simplified test case

function test() {
    let controller = new AbortController()
    let signal = AbortSignal.any([controller.signal])
    setInterval(async () => {
        AbortSignal.any([signal])
        console.log("step")
    }, 100)
}
test()

The closure keeps signal alive, but not controller. After several intervals, controller is garbage collected and thus the source signal of signal is null.

I think we can simply check here for sourceSignal to be null.

I understand the spec as source signals being weak references. Afaik nothing ensures the source signal is not garbage collected before the dependent signals are. The AbortSignal::Any algorithm does not check for the source signals to be null. Maybe this is a spec issue?

Comment 9

[Vincent Hilla [:vhilla]]

Attachment: Bug 1908466 - Ensure source signal is not null when creating a dependent signal. r=#dom-core,saschanaz

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/289709006a92
Ensure source signal is not null when creating a dependent signal. r=saschanaz

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/47227 for changes under testing/web-platform/tests

Comment 12

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/289709006a92

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 14

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20240722214643-ac4a1f84adfa.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:vhilla, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox129 to wontfix.

For more information, please visit BugBot documentation.

Comment 16

[Vincent Hilla [:vhilla]]

Attachment: Bug 1908466 - Ensure source signal is not null when creating a dependent signal. r=#dom-core,saschanaz

Original Revision: https://phabricator.services.mozilla.com/D217233

Comment 17

[Vincent Hilla [:vhilla]]

Attachment: Bug 1908466 - Ensure source signal is not null when creating a dependent signal. r=#dom-core,saschanaz

Original Revision: https://phabricator.services.mozilla.com/D217233

Comment 18

[Vincent Hilla [:vhilla]]

Attachment: Simplified testcase

Comment 19

[Vincent Hilla [:vhilla]]

Attachment: simplified testcase.html

Comment 20

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: small chance this crash starts appearing in the wild
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: open attached test case, wait around 10 seconds for tab to crash
• Risk associated with taking this patch: minimal
• Explanation of risk level: patch only adds a null check
• String changes made/needed: none
• Is Android affected?: yes

Comment 21

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: small chance this crash starts appearing in the wild
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: open attached test case, wait around 10 seconds for tab to crash
• Risk associated with taking this patch: minimal
• Explanation of risk level: patch only adds a null check
• String changes made/needed: none
• Is Android affected?: yes

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/cfa834c6d8a5

Comment 23

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/8ab8cee6727c