1371246 - Crash in InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | mozilla::dom::`anonymous namespace''::ReadBlob

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

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

This bug was filed from the Socorro interface and is 
report bp-e98e3306-637b-45ce-a796-958640170608.
=============================================================

0 	mozglue.dll 	MOZ_CrashPrintf 	mfbt/Assertions.cpp:63
1 	xul.dll 	InvalidArrayIndex_CRASH(unsigned __int64, unsigned __int64) 	xpcom/ds/nsTArray.cpp:26
2 	xul.dll 	nsTArray_Impl<void*, nsTArrayInfallibleAllocator>::ElementAt(unsigned __int64) 	obj-firefox/dist/include/nsTArray.h:1048
3 	xul.dll 	mozilla::dom::`anonymous namespace'::ReadBlob 	dom/base/StructuredCloneHolder.cpp:522
4 	xul.dll 	mozilla::dom::StructuredCloneHolder::CustomReadHandler(JSContext*, JSStructuredCloneReader*, unsigned int, unsigned int) 	dom/base/StructuredCloneHolder.cpp:975
5 	xul.dll 	mozilla::dom::`anonymous namespace'::StructuredCloneCallbacksRead 	dom/base/StructuredCloneHolder.cpp:64
6 	xul.dll 	JSStructuredCloneReader::startRead(JS::MutableHandle<JS::Value>) 	js/src/vm/StructuredClone.cpp:2234
7 	xul.dll 	JSStructuredCloneReader::read(JS::MutableHandle<JS::Value>) 	js/src/vm/StructuredClone.cpp:2537
8 	xul.dll 	ReadStructuredClone(JSContext*, JSStructuredCloneData&, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JSStructuredCloneCallbacks const*, void*) 	js/src/vm/StructuredClone.cpp:626
9 	xul.dll 	JS_ReadStructuredClone(JSContext*, JSStructuredCloneData&, unsigned int, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JSStructuredCloneCallbacks const*, void*) 	js/src/vm/StructuredClone.cpp:2578
10 	xul.dll 	JSAutoStructuredCloneBuffer::read(JSContext*, JS::MutableHandle<JS::Value>, JSStructuredCloneCallbacks const*, void*) 	js/src/vm/StructuredClone.cpp:2742
11 	xul.dll 	mozilla::dom::StructuredCloneHolderBase::Read(JSContext*, JS::MutableHandle<JS::Value>) 	dom/base/StructuredCloneHolder.cpp:206
12 	xul.dll 	mozilla::dom::StructuredCloneHolder::Read(nsISupports*, JSContext*, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) 	dom/base/StructuredCloneHolder.cpp:301
13 	xul.dll 	mozilla::dom::StructuredCloneBlob::Deserialize(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) 	dom/base/StructuredCloneBlob.cpp:81
14 	xul.dll 	mozilla::dom::StructuredCloneHolderBinding::deserialize 	obj-firefox/dom/bindings/StructuredCloneHolderBinding.cpp:34
15 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:470
16 	xul.dll 	js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) 	js/src/proxy/Wrapper.cpp:166
17 	xul.dll 	js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) 	js/src/proxy/CrossCompartmentWrapper.cpp:353
18 	xul.dll 	js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) 	js/src/proxy/Proxy.cpp:479
19 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:452
20 	xul.dll 	Interpret 	js/src/vm/Interpreter.cpp:3028
21 	xul.dll 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp:410
22 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:488
23 	xul.dll 	js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) 	js/src/proxy/Wrapper.cpp:166
24 	xul.dll 	js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) 	js/src/proxy/CrossCompartmentWrapper.cpp:353
25 	xul.dll 	js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) 	js/src/proxy/Proxy.cpp:479
26 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:452
27 	xul.dll 	Interpret 	js/src/vm/Interpreter.cpp:3028
28 	xul.dll 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp:410
29 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:488
30 	xul.dll 	Interpret 	js/src/vm/Interpreter.cpp:3028
31 	xul.dll 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp:410
32 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:488
33 	xul.dll 	js::jit::DoCallFallback 	js/src/jit/BaselineIC.cpp:2453
34 		@0x338d24f6609 	

When I save a Blob data which came from <canvas> (with my private add-on), I always meet this crash. I tested with mozregression, this is a regression of bug 1356546.

Comment 1

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

My addon save a Blob data to file with:

>   let url = URL.createObjectURL(msg.blob);
>   browser.downloads.download({ "url": url, "filename": filename });

Comment 3

[Kris Maglione [:kmag]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #1)
> My addon save a Blob data to file with:
> 
> >   let url = URL.createObjectURL(msg.blob);
> >   browser.downloads.download({ "url": url, "filename": filename });

This is clearly an issue with cloning a Blob object, but the issue is not with this code, it's with the code that sends a message containing a Blob object.

Comment 4

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

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #3)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #1)
> > My addon save a Blob data to file with:
> > 
> > >   let url = URL.createObjectURL(msg.blob);
> > >   browser.downloads.download({ "url": url, "filename": filename });
> 
> This is clearly an issue with cloning a Blob object, but the issue is not
> with this code, it's with the code that sends a message containing a Blob
> object.

The sender is,

canvas.toBlob((blob)=>{
  browser.runtime.sendMessage({ "blob": blob, "filename": filename }).then(...)
});

Comment 5

[Marcia Knous [:marcia]]

Makes sense to track this for 55, P1 critical crash bug.

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1371246: Handle serializing Blobs in StructuredCloneHolder instances.

Review commit: https://reviewboard.mozilla.org/r/148236/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148236/

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8876910 [details]
Bug 1371246: Handle serializing Blobs in StructuredCloneHolder instances.

https://reviewboard.mozilla.org/r/148236/#review153098

Are we going to have similar problems for the other weird stuff that can be structured cloned (ports, WASM, etc.)?

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8876910 [details]
Bug 1371246: Handle serializing Blobs in StructuredCloneHolder instances.

https://reviewboard.mozilla.org/r/148236/#review153098

No. Ports can only be transferred, not cloned, and we don't support transferring. WasmModules and surfaces can only be cloned when the scope is SameProcess, but we use DifferentProcess. InputStreams could potentially be an issue if someone started using this for chrome code, so it might be worth failing if someone tries to store one, but it shouldn't be a problem for any of our current uses.

Comment 9

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff83c9de8a7c75d38eb04e262209ac08a0c38f0
Bug 1371246: Handle serializing Blobs in StructuredCloneHolder instances. r=billm

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ff83c9de8a7

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8876910 [details]
Bug 1371246: Handle serializing Blobs in StructuredCloneHolder instances.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1356546
[User impact if declined]: This bug causes crashes when extensions try to send Blob or File objects using extension messaging APIs.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: No, automated tests should be sufficient. 
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This is a relatively simple change to copy missing blob objects during structured clone reads and writes. It should have no effect except in cases where we're currently crashing.
[String changes made/needed]: None.

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8876910 [details]
Bug 1371246: Handle serializing Blobs in StructuredCloneHolder instances.

fix a crash in beta55

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/64d650c2c2ac