NativeJSContainer has bizarre to nonexistent error handling

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bzbarsky, Assigned: jchen)

Tracking

unspecified
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42- affected, firefox43 affected, firefox44 affected, firefox48 fixed)

Details

Attachments

(1 attachment)

For example, consider ArrayInValue, as mentioned in bug 1179003.

If getting the "length" property throws, a pending exception will be set on mJSContext.  Once that's happened the following will be true:

1)  In a debug build, various operations on mJSContext will fatally assert.

2)  In an opt build, various later operations will return false because they think an exception has been thrown, no matter what the _correct_ return value is.

I see nothing in this code that unsets pending exceptions as needed... there should probably be something, at least for this function.  I'm not sure what exception-handling semantics the rest of this file wants.
Flags: needinfo?(snorp)
Flags: needinfo?(nchen)
[Tracking Requested - why for this release]: Depending on how trusted these objects are and what the consequences of having all operations on mJSContext start failing, this could be something that needs to be fixed before shipping.
These functions are called by Java code. We already throw Java exceptions on failure, so seems like we should simply clear pending JS exceptions.
Flags: needinfo?(nchen)
About the tracking request, I am unable to evaluate the impact for us.
Could you explain what would be the impact for users? Thanks
> Could you explain what would be the impact for users?

I don't know this code well enough to judge.  The main failure mode is that if something fails and leaves an exception sitting around then future calls that shouldn't fail can also fail.  Whether this is a problem in practice in this case, I can't tell.
OK, I won't be tracking as I cannot help on this for now and won't block the release on this. Please resubmit when we are closer to find a solution.
Assignee: nobody → nchen
Flags: needinfo?(snorp)
Clear any JS exceptions in appropriate places in NativeJSContainer, so
the exceptions don't affect subsequent JS API calls. We don't actually
"handle" the exceptions because we throw a Java exception instead or it
is safe to ignore the JS exception.
Attachment #8739568 - Flags: review?(snorp)
Attachment #8739568 - Flags: review?(snorp) → review+
https://hg.mozilla.org/mozilla-central/rev/317bc3f69953
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.