1752322 - Assertion failure from ReadableByteStreamController#enqueue

Status: RESOLVED FIXED

(Core :: DOM: Streams, defect)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

MOZ_ASSERT(mMightHaveUnreportedJSException, "Why didn't you tell us you planned to throw a JS exception?");

Dear assertion, I'm sorry.

xul.dll!mozilla::binding_danger::TErrorResult<mozilla::binding_danger::AssertAndSuppressCleanupPolicy>::StealExceptionFromJSContext(JSContext * cx) Line 652 (c:\Users\sasch\Documents\GitHub\gecko-dev\dom\bindings\BindingUtils.cpp:652)
xul.dll!mozilla::dom::ReadableByteStreamControllerEnqueue(JSContext * aCx, mozilla::dom::ReadableByteStreamController * aController, JS::Handle<JSObject *> aChunk, mozilla::ErrorResult & aRv) Line 675 (c:\Users\sasch\Documents\GitHub\gecko-dev\dom\streams\ReadableByteStreamController.cpp:675)
xul.dll!mozilla::dom::ReadableByteStreamController::Enqueue(JSContext * aCx, const mozilla::dom::ArrayBufferView_base<&JS_GetArrayBufferViewType> & aChunk, mozilla::ErrorResult & aRv) Line 813 (c:\Users\sasch\Documents\GitHub\gecko-dev\dom\streams\ReadableByteStreamController.cpp:813)
xul.dll!mozilla::dom::ReadableByteStreamController_Binding::enqueue(JSContext * cx_, JS::Handle<JSObject *> obj, void * void_self, const JSJitMethodCallArgs & args) Line 193 (c:\Users\sasch\Documents\GitHub\gecko-dev\obj-domstreams-dbg\dom\bindings\ReadableByteStreamControllerBinding.cpp:193)
xul.dll!mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions>(JSContext * cx, unsigned int argc, JS::Value * vp) Line 3306 (c:\Users\sasch\Documents\GitHub\gecko-dev\dom\bindings\BindingUtils.cpp:3306)
xul.dll!CallJSNative(JSContext * cx, bool(*)(JSContext *, unsigned int, JS::Value *) native, js::CallReason reason, const JS::CallArgs & args) Line 425 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\Interpreter.cpp:425)
xul.dll!js::InternalCallOrConstruct(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct, js::CallReason reason) Line 512 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\Interpreter.cpp:512)
xul.dll!InternalCall(JSContext * cx, const js::AnyInvokeArgs & args, js::CallReason reason) Line 572 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\Interpreter.cpp:572)
xul.dll!js::CallFromStack(JSContext * cx, const JS::CallArgs & args) Line 576 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\Interpreter.cpp:576)
xul.dll!Interpret(JSContext * cx, js::RunState & state) Line 3309 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\Interpreter.cpp:3309)
xul.dll!js::RunScript(JSContext * cx, js::RunState & state) Line 394 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\Interpreter.cpp:394)
xul.dll!js::InternalCallOrConstruct(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct, js::CallReason reason) Line 544 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\Interpreter.cpp:544)
xul.dll!InternalCall(JSContext * cx, const js::AnyInvokeArgs & args, js::CallReason reason) Line 572 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\Interpreter.cpp:572)
xul.dll!js::Call(JSContext * cx, JS::Handle<JS::Value> fval, JS::Handle<JS::Value> thisv, const js::AnyInvokeArgs & args, JS::MutableHandle<JS::Value> rval, js::CallReason reason) Line 589 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\Interpreter.cpp:589)
xul.dll!js::fun_call(JSContext * cx, unsigned int argc, JS::Value * vp) Line 962 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\JSFunction.cpp:962)
xul.dll!CallJSNative(JSContext * cx, bool(*)(JSContext *, unsigned int, JS::Value *) native, js::CallReason reason, const JS::CallArgs & args) Line 425 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\Interpreter.cpp:425)
xul.dll!js::InternalCallOrConstruct(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct, js::CallReason reason) Line 512 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\Interpreter.cpp:512)
xul.dll!InternalCall(JSContext * cx, const js::AnyInvokeArgs & args, js::CallReason reason) Line 572 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\Interpreter.cpp:572)
xul.dll!js::CallFromStack(JSContext * cx, const JS::CallArgs & args) Line 576 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\Interpreter.cpp:576)
xul.dll!Interpret(JSContext * cx, js::RunState & state) Line 3309 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\Interpreter.cpp:3309)

Comment 1

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

So, this one is pretty clearly an error to flag "MightThrowJSException" at the start of the IDL implementing method ReadableByteStreamController.cpp

I'm sort of surprised that the bindings code doesn't do this for us.

I have a patch to fix the above instance of this, but I should spend a bit of time too looking and seeing what the state is for other throwing IDL methods; we might want to just defensively mark everything, given that the binding code subsquently calls MaybeSetPendingException (which will clear the bit appropriately by calling WouldReportJSException.

Comment 2

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

It sure looks like fixing this in the binding generation code would be easy, but I don't entirely understand the design constraint of the mMightHaveUnreportedJSException assertions.

I'll post a patch and see what the WebIDL codegen folk think.

Comment 3

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

Attachment: Bug 1752322 - Make sure binding generated ErrorResults are ready for the possibility of JS exception throw r?edgar,peterv

Comment 4

[Tom S [:evilpie]]

Attachment: Bug 1752322 - Sprinkle around MightThrowJSException in ReadableByteStreamController. r?mgaudet

I still had this patch so I thought I might as well upload it.

My process for adding the calls was roughly:

• Cover everything that uses TransferArrayBuffer
• Look for uses of JS_ and JS::
• If there a multiple uses in a function put MightThrowJSException at the top

Overall I kind of doubt the usefulness of this assertion for our uses.

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to Tom Schuster [:evilpie] from comment #4)

Overall I kind of doubt the usefulness of this assertion for our uses.

It's mostly to catch non-binding code dropping JS exceptions on the floor through ErrorResult, so it might not be (although it also double-checks the codegen). I wish we had a better way to check it, but for now it's all we got.

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

Will something like [ThrowsJS] help here? (That won't do anything more than just calling rv.MightThrowJSException though...)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7b916a1d433
Sprinkle around MightThrowJSException in ReadableByteStreamController. r=saschanaz

Comment 8

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/f7b916a1d433

Comment 9

[Peter Van der Beken [:peterv]]

(In reply to Kagami :saschanaz from comment #6)

Will something like [ThrowsJS] help here? (That won't do anything more than just calling rv.MightThrowJSException though...)

Not really, since it doesn't help if we end up in the code that calls StealExceptionFromJSContext/ThrowJSException from non-binding code. I know it's annoying if you're sure that that won't happen. But then there have been situations like this before where we were sure, and then subsequent code changes made it not be true anymore. To avoid issues like that in the first place I think it's better to keep the MightThrowJSException close to the calls to StealExceptionFromJSContext/ThrowJSException.

I think the only real solution here would be to replace MightThrowJSException with a static analysis based on calls to APIs that store an exception in an ErrorResult. IIRC that was considered, and it turned out to be pretty complicated. It would be nicer because right now we rely on reviews, and also actually hitting the assertions when an exception is thrown (which is often hard to provoke in a test).