Exception handling with Dictionary.ToObject(Internal) / ToJSValue(cx, *) should be less error prone

NEW
Unassigned

Status

()

defect
P5
normal
5 years ago
3 months ago

People

(Reporter: smaug, Unassigned)

Tracking

29 Branch
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Reporter

Description

5 years ago
Currently it is black magic how exceptions should be handled if
dictionary.ToObject / ToJSValue fails. And it is not documented in the code that exceptions 
should be handled in any way.

Could we add some param to ToObject/ToJSValue which indicates the wanted
behavior in case of an exception?
We could.  WebIDL callbacks have an enum for this sort of thing; we could add one here too (with a different value set).

Fundamentally the three plausible values are:

1)  Report the exception before returning.
2)  Squelch the exception before returning.
3)  Leave the exception on the JSContext for the caller to deal with.

I assume we would have no default value for this arg for the ToJSValue overloads that can actually throw exceptions, right?
Hm, when do we want to do anything other than just returning a failure in this case? The general plan is that exceptions would be reported on the cx, and the cx's AutoJSAPI would notice this when it comes off the stack. When that happens, the AutoJSAPI will do the appropriate thing with the exception (rethrowing, squelching, or reporting) depending on how it's been set up.

See bug 981187 and the associated dependency chain. We know what we need to do here, we're just lacking the resources to do it. In theory, I might get back to this stuff late-Q2 or Q3.
> Hm, when do we want to do anything other than just returning a failure in this case?

Well, we need to report or squelch exceptions, right?  We do NOT want to leave them on the JSContext.

Or is the point that we'll make AutoJSContext either imply or also require AutoJSAPI?
(In reply to Boris Zbarsky [:bz] from comment #3)
> Or is the point that we'll make AutoJSContext either imply or also require
> AutoJSAPI?

Yes. It will eventually be replaced with something like AutoInheritJSAPI for the cases where the callee wants to inherit the caller's compartment and other state (a la today's GetCurrentJSContext). For cases with a less-clear relationship to the caller's JSAPI state, a new AutoJSAPI should be instantiated.
Hi, is there a quick solution or what do you suggest to do for those, bug 878533 and bug 916607, blocked before a complete solution is landed here?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
Flags: needinfo?(bobbyholley)
For now, those should be reporting, suppressing, or propagating the exceptions, depending on the behavior they want.
Flags: needinfo?(bzbarsky)
Reporter

Comment 7

5 years ago
And suppressing means manually calling JS_ClearPendingException() ?
Flags: needinfo?(bzbarsky)
Yes, exactly.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bobbyholley)
Reporter

Updated

5 years ago
Flags: needinfo?(bugs)
Reporter

Updated

5 years ago
Blocks: 1095728
Reporter

Updated

5 years ago
Summary: Exception handling with Dictionary.ToObject(Internal) / ToJSValue(cx, dictionary) should be less error prone → Exception handling with Dictionary.ToObject(Internal) / ToJSValue(cx, *) should be less error prone
https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.
Priority: -- → P5
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.