nsGeolocationSettings has broken JSAPI usage

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bzbarsky, Assigned: huseby)

Tracking

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Specifically, there are two problems I ran into (though there may be others; I was auditing nsAutoJSString::init consumers, not this file per se):

1) HandleGeolocationPerOriginSettingsChange presses on with the loop after a JS exception is thrown (e.g. from origin.init() or  JS_GetPropertyById or the various JS_GetProperty callers).  It needs to either clear the exception if it should be suppressed or return immediately and let the AutoEntryScript report the exception.

2) HandleGeolocationAlwaysPreciseChange presses on with the loop on JS_GetElement failures or origin.init() failure in a similar way.
Flags: needinfo?(martin.thomson)
Flags: needinfo?(josh)
Flags: needinfo?(huseby)
1) In HandleGeolocationPerOriginSettingsChange, any failure to init() or get a propert using an nsAutoJSString should be suppressed.  What's the correct way to do that?

2) In HandleGeolocationAlwaysPreciseChange, same thing here.  it already returns on any failures up to the start of the loop.  During the loop, any failures should be suppressed.

BTW, I'm not at all proud of this code.  It was rushed as part of a partner project and I've been intending to refactor it for some time.  See https://bgz.la/1097229  I'm interested in learning the correct way to use nsAutoJSString though.  I can work up a patch to fix these consumer sites once we decide on the right thing to do.
Flags: needinfo?(huseby)
> What's the correct way to do that?

JS_ClearPendingException(cx)

> it already returns on any failures up to the start of the loop. 

Those failures are reported to the console, note, by the AutoEntryScript.

> I'm interested in learning the correct way to use nsAutoJSString though.

Basically two options there.  Either JS_ClearPendingException after a failing init() call if you want to catch the exception and ignore it, or you need to report the exception (immediately returning under the scope of an AutoJSAPI/AutoEntryScript that had TakeOwnershipOfErrorReporting() called on it would work, as would a manual call to AutoJSAPI::ReportException() if you want to report it and keep going).

Also, no judgement passed on this code; literally half the callers of nsAutoJSString::init in the tree mishandled exceptions from it.  JSAPI is a pain.  :(
I'm writing the patch now.  AFAICT, JS_GetProperty returns false if the property doesn't exist and true if it did exist and the value was returned.  How do I know the error case that requires us catch and clear the exception?  If I get false from JS_GetProperty() should I call JS_ClearPendingException()?
Flags: needinfo?(bzbarsky)
Posted patch Bug_1220688.patch (obsolete) — Splinter Review
Is this what you had in mind?
Attachment #8682628 - Flags: review?(bzbarsky)
> AFAICT, JS_GetProperty returns false if the property doesn't exist and true if it did
> exist and the value was returned.

No, JS_GetProperty returns false if the property get threw an exception (e.g. there was a getter that threw, or a proxy trap that threw or there was some sort of internal exception inside the JS engine) and true otherwise.  If the property simply does not exist, the return value is true and the MutableHandleValue is set to undefined.
Flags: needinfo?(bzbarsky)
Comment on attachment 8682628 [details] [diff] [review]
Bug_1220688.patch

You need to fix the JS_GetElement caller too.

r=me with that done.
Attachment #8682628 - Flags: review?(bzbarsky) → review+
Assignee: nobody → huseby
Oops, forgot to fix the JS_GetElement caller.  Doing that now.
Posted patch Bug_1220688.patch (obsolete) — Splinter Review
Please review again because I reworked the logic around line 272 of nsGeolocationSettings.cpp to separate out the JS_GetPropertyById and the the isObject test on propertyValue.  I assume we only want to clear the JS exceptions when JS_GetPropertyById fails and not when propertyValue isn't an object.
Attachment #8682628 - Attachment is obsolete: true
Attachment #8682762 - Flags: review?(bzbarsky)
try push for latest version of the patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ebafb4d392f
Comment on attachment 8682762 [details] [diff] [review]
Bug_1220688.patch

r=me.  It's probably fine to clear if !isObject() too, since there is no exception there so the clear is a no-op.  But this is better, yes.
Flags: needinfo?(martin.thomson)
Flags: needinfo?(josh)
Attachment #8682762 - Flags: review?(bzbarsky) → review+
try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f547d626501

this removes the no-op that bz pointed out in Comment 11.

r+ already.  r=bz
Attachment #8682762 - Attachment is obsolete: true
Keywords: checkin-needed
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/22d372a893d7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.