Closed
Bug 1206822
Opened 9 years ago
Closed 9 years ago
NativeJSContainer has bizarre to nonexistent error handling
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox42- affected, firefox43 affected, firefox44 affected, firefox48 fixed)
RESOLVED
FIXED
Firefox 48
People
(Reporter: bzbarsky, Assigned: jchen)
References
Details
Attachments
(1 file)
3.55 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
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)
![]() |
Reporter | |
Updated•9 years ago
|
Flags: needinfo?(nchen)
![]() |
Reporter | |
Comment 1•9 years ago
|
||
[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.
tracking-firefox42:
--- → ?
Assignee | ||
Comment 2•9 years ago
|
||
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)
Updated•9 years ago
|
Comment 3•9 years ago
|
||
About the tracking request, I am unable to evaluate the impact for us.
Could you explain what would be the impact for users? Thanks
![]() |
Reporter | |
Comment 4•9 years ago
|
||
> 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.
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
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+
Comment 8•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•