1689503 - Crash in [@ js::ArrayBufferObject::addSizeOfExcludingThis]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Andrew McCreight [:mccr8]]

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/14901b9c-c20f-4d07-acea-d56860210127

MOZ_CRASH Reason: MOZ_CRASH(external buffers not currently supported)

Top 10 frames of crashing thread:

0 xul.dll static js::ArrayBufferObject::addSizeOfExcludingThis js/src/vm/ArrayBufferObject.cpp:1477
1 xul.dll JS::ubi::Concrete<JSObject>::size const js/src/vm/JSObject.cpp:3755
2 xul.dll js::Debugger::appendAllocationSite js/src/debugger/Debugger.cpp:2883
3 xul.dll static js::DebugAPI::slowPathOnLogAllocationSite js/src/debugger/Debugger.cpp:2856
4 xul.dll js::SavedStacks::MetadataBuilder::build const js/src/vm/SavedStacks.cpp:1899
5 xul.dll JS::Realm::setNewObjectMetadata js/src/vm/Realm.cpp:518
6 xul.dll js::AutoSetNewObjectMetadata::~AutoSetNewObjectMetadata js/src/vm/Realm.cpp:712
7 xul.dll static js::ArrayBufferObject::createForContents js/src/vm/ArrayBufferObject.cpp:1196
8 xul.dll JS::NewExternalArrayBuffer js/src/vm/ArrayBufferObject.cpp:1789
9 xul.dll mozilla::webgpu::Buffer::GetMappedRange dom/webgpu/Buffer.cpp:134

This is an odd looking crash. Low volume. Some kind of debugger API is trying to get the size of an object via the Ubi Node interface, and hitting MOZ_CRASH("external buffers not currently supported");. I see WebGPU in the stack, so maybe that's involved, too.

Comment 1

[Jason Orendorff [:jorendorff]]

I don't think it's Fission-related. Very straightforward. I think the fuzzers would have found this except there doesn't seem to be a way to call JS::NewExternalArrayBuffer from any testing function. If there were, then something like this should hit it:

let g = newGlobal({newCompartment: true});
let dbg = new Debugger(g);
dbg.memory.trackingAllocationSites = true;
g.newExternalArrayBuffer(64);

Comment 2

[Jason Orendorff [:jorendorff]]

Matt, would you mind fixing?

I think for accounting purposes we should not count the buffer memory. For the one in-tree use of JS::NewExternalArrayBuffer, the external ArrayBuffer does not own the buffer. Instead, it serves as a view on a buffer owned by another object; if the other buffer is freed, the ArrayBuffer is automatically detached. So it should be treated like case USER_OWNED.

Comment 3

[Jason Orendorff [:jorendorff]]

I'm not sure how valuable it would be to have a newExternalArrayBuffer helper.

Comment 4

[Jason Orendorff [:jorendorff]]

In the browser, I think the steps would be:

1. open DevTools
2. go to the Memory pane
3. check "Record call stacks"
4. visit a web page that uses GPUBuffer.getMappedRange.

Comment 5

[Andrew McCreight [:mccr8]]

Thanks for investigating, Jason. I'll mark this as blocking the WebGPU meta bug, as it sounds like it is the only user of this JSAPI function.

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1689503 - Add shell function for creating external data array buffers r?jorendorff

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1689503 - Update ArrayBufferObject size accounting to handle external ArrayBuffers r?jorendorff

Depends on D104411

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f20b9fd489d0
Add shell function for creating external data array buffers r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/b1472558e0c3
Update ArrayBufferObject size accounting to handle external ArrayBuffers r=jorendorff

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f20b9fd489d0
https://hg.mozilla.org/mozilla-central/rev/b1472558e0c3

Comment 10

[Philip Chimento [:ptomato]]

Is the "wontfix" on esr78 final, or would it be a possibility to get D104412 uplifted? While it's not a huge priority (otherwise I'd have noticed it before now), it does make JS::CollectRuntimeStats crash if you have any external ArrayBuffers live, which I believe are probably used more by embedders than inside Firefox.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

We can take it if it helps embedders, assuming Matthew doesn't see an issue with doing so.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

(Please nominate for uplift if so)

Comment 13

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 9201804 [details]
Bug 1689503 - Add shell function for creating external data array buffers r?jorendorff

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Helps avoid SM embedder crashes
• User impact if declined: SM embedders likely carry their own copies of this patch to avoid crashes.
• Fix Landed on Version: 87
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Low risk: Changes a memory accounting path the correct a MOZ_CRASH path into a sensible path.
• String or UUID changes made by this patch:

Comment 14

[Matthew Gaudet (he/him) [:mgaudet]]

No issue with uplifting. Because of the test, both patches with be needed (First patch is adds shell testing function, second patch fixes bug, and then adds test case using said testing function). I'll nominate for uplift.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9201804 [details]
Bug 1689503 - Add shell function for creating external data array buffers r?jorendorff

Approved for 78.9esr, thanks for the quick response.

Comment 16

[Philip Chimento [:ptomato]]

Thanks very much!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/2ff62f191bc2
https://hg.mozilla.org/releases/mozilla-esr78/rev/ea2e04a681d4