710412 - nsIDOMGeoPositionOptions handling looks suspicious

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

nsIDOMGeoPositionOptions is a JS implemented object and it seems to be handled
asynchronously (it is used for example in nsGeolocationRequest::Allow()).
Since the implementation relies on xpconnect, for example maximumAge could be implemented
as a getter, so random JS could run at an unexpected time. And I have no idea
which JSContext is on the stack at that moment so don't know if there is a sg:crit 
somewhere...

I believe in the spec PositionOptions should be a dictionary, not an interface.
Though Geolocation spec predates WebIDL dictionaries.

I hope the tool in bug 709569 could be useful here, since the values of
nsIDOMGeoPositionOptions could be read before using them (that is the way dictionaries
are handled).

Comment 1

[Bob Clary [:bc] (inactive)]

fwiw (which is not a lot), during the automated crash testing I get lots of instances of crashes with geolocation stuff on the stack and some times deleted or otherwise not-nice memory addresses but have never been able to reproduce them. This has been happening for a long time. If this issue is somehow related I might see a decrease if you have a fix.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

This relies on bug 709569.
The tool creates the following class (in mozilla::dom namespace) automatically

class DOMGeoPositionOptions
{
public:
  DOMGeoPositionOptions() :
    enableHighAccuracy(false),
    timeout(0),
    maximumAge(0)
  {}
  nsresult Init(JSContext* aCx, const jsval* aVal);
  bool enableHighAccuracy;
  PRInt32 timeout;
  PRInt32 maximumAge;
};

Unfortunately we don't seem to have good tests for various nsIDOMGeoPositionOptions values.
Doug, do you have any manual tests? Could you try the patch?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 581461 [details] [diff] [review]
patch

Review of attachment 581461 [details] [diff] [review]:
-----------------------------------------------------------------

I really don't understand these changes and like to defer them to someone that knows xpconnect better.  Blake?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

The idea is to read the values from the JS object right after it has been passed to the method,
not later at random time.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 581461 [details] [diff] [review]
patch

Review of attachment 581461 [details] [diff] [review]:
-----------------------------------------------------------------

This seems cleaner to me. I'm not sure I understand the motivation here, though. Do we call into the options object at times that are more weird than any other place we call into JS?

Either way, I like this patch. Olli, did you end up finding tests for this? I know that there are at least *some* tests for geolocation.

::: dom/ipc/ContentParent.cpp
@@ +1197,5 @@
>      nsCOMPtr<nsIDOMGeoGeolocation> geo = do_GetService("@mozilla.org/geolocation;1");
>      if (!geo) {
>        return true;
>      }
> +    jsval dummy;

It looks really scary to pass an uninitialized jsval around. Even though we know it isn't used because of the null context, I'd prefer seeing this initialized to JSVAL_VOID.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Blake Kaplan (:mrbkap) from comment #6)
> This seems cleaner to me. I'm not sure I understand the motivation here,
> though. Do we call into the options object at times that are more weird than
> any other place we call into JS?
In other cases when we're calling into content JS we for example explicitly push
JS context to stack. I don't recall any other place where we're calling into 
content JS this way.

Also, per spec the values of the properties in JS object should
be assigned to internal variables when the methods are called.


> > +    jsval dummy;
> 
> It looks really scary to pass an uninitialized jsval around. Even though we
> know it isn't used because of the null context, I'd prefer seeing this
> initialized to JSVAL_VOID.
Indeed.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Doug, do you know any tests (manual or automatic) for GeoPositionOptions?

Comment 9

[Daniel Veditz [:dveditz]]

guessing moderate because we don't know exactly how to trigger the issue, but it probably does happen per comment 1.

Comment 10

[Doug Turner (:dougt)]

olli, nope.  just have to write some.

Comment 11

[Doug Turner (:dougt)]

olli, checkin-needed?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Bah, forgot when waiting for possible tests.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Ok, we need to fix this to prevent a leak too :/

Comment 14

[Chris Peterson [:cpeterson]]

Olli, is this bug still relevant? Blake r+'d your patch three years ago, but you were waiting to write tests and fix a leak. :)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

This is all using webidl now, so we don't run JS at unexpected times.