1220688 - nsGeolocationSettings has broken JSAPI usage

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Dave Huseby [: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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> 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.  :(

Comment 3

[Dave Huseby [:huseby]]

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()?

Comment 4

[Dave Huseby [:huseby]]

Attachment: Bug_1220688.patch

Is this what you had in mind?

Comment 5

[Boris Zbarsky [: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.

Comment 6

[Boris Zbarsky [: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.

Comment 7

[Dave Huseby [:huseby]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7e16b34f210

Comment 8

[Dave Huseby [:huseby]]

Oops, forgot to fix the JS_GetElement caller.  Doing that now.

Comment 9

[Dave Huseby [:huseby]]

Attachment: Bug_1220688.patch

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.

Comment 10

[Dave Huseby [:huseby]]

try push for latest version of the patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ebafb4d392f

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Dave Huseby [:huseby]]

Attachment: Bug_1220688.patch already

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

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22d372a893d7

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/22d372a893d7