1073419 - [ALA] All gecko code needed to support adjustable location accuracy

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Dave Huseby [:huseby]]

We need an object that looks up, and caches the geolocation blur settings both globally and on a per-app basis.  This needs to handle the mozsetting-change observation to track changes in the geo blur settings.

Comment 1

[Dave Huseby [:huseby]]

Attachment: Bug1073419.patch

This adds a live cache for the user settable geolocation settings for both global and per-app.

Comment 2

[Dave Huseby [:huseby]]

It's late/early...I need sleep.

Comment 3

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

This patch contains all of the changes to the geolocation subsystem to support precise, user-settable, approximate, and no location reporting.  It adds a nsGeolocationSettings cache that tracks global and per-app geolocation settings as well as a list of always-precise apps.  

By default the approximate location reporting is not compiled in.  To enabled it, you must specify MOZ_APPROX_LOCATION=1 in your .mozconfig or on the command line when doing ./config.sh and ./build.sh.

I also added MOZ_GPS_DEBUG, that when enabled during compile time, will add in a bunch of verbose GPS debugging.

Comment 4

[Dave Huseby [:huseby]]

Attachment: gecko-ala-v2.1.patch

the same patch, cherry picked onto v2.1.

Comment 5

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

This is an updated and rebased patch for the gecko side of adjustable location accuracy.

Comment 6

[Doug Turner (:dougt)]

I have made it very clear that mozilla will not fuzz geolocation.

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8499890 [details] [diff] [review]
gecko-ala-master.patch

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

This sort of naive obscuring is ineffectual against any moderately motivated attempt to recover location.

Sections 13.2 and 13.3 of RFC 6772 describe the sorts of trivial attacks that are possible against this method:
https://tools.ietf.org/html/rfc6772#section-13.2   This document also describes how a motivated attacker can use side channel information to recover geolocation.

I'd further recommend reading Matt Duckham's papers on the subject, which outline the complexity of a genuinely good solution in this space.  For instance: http://dl.acm.org/citation.cfm?id=1868472

And here's what I believe is the minimum it takes to obscure the location of someone in the absence of side channels: https://tools.ietf.org/html/draft-thomson-geopriv-location-obscuring  The proposed method does not do that.

I'm sensitive to potential need for a fixed geolocation for some use cases (TOR); that's a separate issue.

Before you try to land code, I think that it's important to articulate what you think the threat model is and what you intend to achieve.

Comment 8

[Dave Huseby [:huseby]]

Attachment: gecko-ala-v2.1.patch

updated and rebased v2.1 patch

Comment 9

[Dave Huseby [:huseby]]

(In reply to Doug Turner (:dougt) from comment #6)
> I have made it very clear that mozilla will not fuzz geolocation.

But you also said that if it is not enabled by default, and only enabled by a compile-time switch, that you'd be OK with that.

Comment 10

[Dave Huseby [:huseby]]

(In reply to Martin Thomson [:mt] from comment #7)
> Sections 13.2 and 13.3 of RFC 6772 describe the sorts of trivial attacks
> that are possible against this method:
> https://tools.ietf.org/html/rfc6772#section-13.2   This document also
> describes how a motivated attacker can use side channel information to
> recover geolocation.

The operative phrase in that section is this: "Returning location shapes that are randomly computed will over time reveal more and more information about the Target."  The grid method contained in this patch does not do a location shape that is randomly computed.

I can't read the first paper you link to because it's behind the ACM paywall.  The second one I'm reading right now.

Comment 11

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

updated and rebased.

Comment 12

[Dave Huseby [:huseby]]

(In reply to Martin Thomson [:mt] from comment #7)
> Comment on attachment 8499890 [details] [diff] [review]
> Before you try to land code, I think that it's important to articulate what
> you think the threat model is and what you intend to achieve.

I believe the method implemented in this patch is sound.  It calculates a regular grid over the entire planet and only returns the geographical center of the grid square that the device is currently in.  The only attack I can think of against this is a time-based one where an observer can see the time when the grid square changes from one grid square to an adjacent grid square.  In that limited time window, one of the two axis of uncertainty is collapsed to zero (i.e. we know the device is somewhere along the border between two grid squares).  But that still leaves one axis of uncertainty that is as big as the user-selected "error".  So if the user selects a 100km grid, then the error is still 100km in one axis.

The next obvious step is to cross reference major roads and rail lines that cross the border between grid squares.  If there is only a single transportation corridor then it is plausible that the location of the device can be determined with "good enough" accuracy.  But if there are multiple transportation corridors, then the odds of picking the right one are reduced to 1/<number of corridors>.

Did I miss something?  I'd love to learn of a better attack against a regular grid location obfuscation method.

Comment 13

[Martin Thomson [:mt:]]

(In reply to Dave Huseby [:huseby] from comment #10)
> (In reply to Martin Thomson [:mt] from comment #7)
> > Sections 13.2 and 13.3 of RFC 6772 describe the sorts of trivial attacks
> > that are possible against this method:
> > https://tools.ietf.org/html/rfc6772#section-13.2   This document also
> > describes how a motivated attacker can use side channel information to
> > recover geolocation.
> 
> The operative phrase in that section is this: "Returning location shapes
> that are randomly computed will over time reveal more and more information
> about the Target."  The grid method contained in this patch does not do a
> location shape that is randomly computed.

That's not the only point, that's just the most primitive attack.  Try Section 13.5.  The location-obscuring goes into different mitigations for other attacks.

> I can't read the first paper you link to because it's behind the ACM
> paywall.  The second one I'm reading right now.

The paywall is a trap.  Try searching google scholar.  I've found all of Matt's papers online.

Comment 14

[Martin Thomson [:mt:]]

http://www.geosensor.net/phpws/index.php?module=pagemaster&PAGE_user_op=view_page&PAGE_id=3

e.g., http://www.geosensor.net/papers/duckham06.GISCIENCE.pdf

Comment 15

[Martin Thomson [:mt:]]

I find this particularly helpful as introductory material: http://geosensor.net/papers/duckham10.SPRINGL.pdf

Comment 16

[Doug Turner (:dougt)]

I have reconsidered after talking to Martin.  Let's drop the tiles stuff and implement draft-thomson-geopriv-location-obscuring -- behind a preference (not a run time flag).

We should work together on the text.  I am very concerned that we might over sell this feature.  Please file a separate follow up for a Firefox desktop UX.

Comment 17

[Martin Thomson [:mt:]]

I have code that you can use as a guide (for comparison purposes, etc...) and a demonstration of this in action here:
http://held-location.sourceforge.net/js_geoshape/maptest.html

I did this for another company, and the license terms aren't clear enough, so I will recommend a port of this.

Note that this depends on having a per-origin random seed that shares the fate of cookies.

Comment 18

[Josh Matthews [:jdm]]

Comment on attachment 8501572 [details] [diff] [review]
gecko-ala-master.patch

I'm looking at this.

Comment 19

[Martin Thomson [:mt:]]

Comment on attachment 8501572 [details] [diff] [review]
gecko-ala-master.patch

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

The design around getting per-origin settings is a little odd, and not particular well documented.  Wouldn't it be easier to add an origin field to nsGeolocationRequest?

I would have thought that using PermissionManager to store and manage these permissions was the right thing to be doing here, rather than invent something new.

This needs quite a bit more work.

::: dom/geolocation/nsGeolocation.cpp
@@ +521,5 @@
>  }
>  
> +static nsIDOMGeoPosition*
> +NullGeoPosition() {
> +  return new nsGeoPosition(0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0);

What is the intent of this?  Why can't you return nullptr in cases where things fail?  That doesn't need to surface in the DOM API; it can be turned into an error as necessary.

@@ +525,5 @@
> +  return new nsGeoPosition(0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0);
> +}
> +
> +static nsIDOMGeoPosition*
> +CopyGeoPosition(nsIDOMGeoPosition* aPosition) {

All these methods should be operating with nsCOMPtr, not bare pointers.

@@ +543,5 @@
> +}
> +
> +nsIDOMGeoPosition*
> +nsGeolocationRequest::ChangeLocation(nsIDOMGeoPosition* aPosition) {
> +  if (XRE_GetProcessType() == GeckoProcessType_Content) {

I don't understand the implications of this.  Does fuzzing only occur in the parent process?

@@ +609,5 @@
> +  CalculateGridCoords(aDistance, lat, lon);
> +  GPSLOG("approximate location with delta %ul is %f, %f", aDistance, lat, lon);
> +
> +  // return a location with grid lat/lon
> +  return SynthesizeLocation(aPosition, lat, lon);

This needs to report the "accuracy" as being max(aDistance, originalAccuracy)

@@ +623,5 @@
> +    return NullGeoPosition();
> +  }
> +
> +  DOMTimeStamp ts;
> +  nsresult rv = aPosition->GetTimestamp(&ts);

Copying the timestamp leaks information about the source.  Create a new timestamp for the fixed position; only reuse the timestamp for a fuzzed location.

@@ +633,5 @@
> +
> +void
> +nsGeolocationRequest::CalculateGridCoords(int32_t aDistance,
> +                                          double & aLatitude,
> +                                          double & aLongitude)

1. if the intent of this is to generate a fuzzed location, then make that the name of the function and change its signature accordingly.  Then, we can make algorithm improvements without changing code around too much.

2. The numbers in this function aren't explained.  Please document their origin, or use more conventional methods of derivation (deriving from the WGS84 geoid radius might make this clearer).

@@ +636,5 @@
> +                                          double & aLatitude,
> +                                          double & aLongitude)
> +{
> +  double fi = (aLatitude * 3.14) / 180;
> +  double kmSize = 3600 / (cos(fi) * 111.27);

These are constants.

@@ +829,1 @@
>      NS_ENSURE_SUCCESS(rv, rv);

Leaks callback.

@@ +829,4 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    callback = new GeolocationSettingsCallback();
> +    rv = settingsLock->Get(GEO_ALA_TYPE, callback);
> +    NS_ENSURE_SUCCESS(rv, rv);

Leaks callback.

@@ +955,2 @@
>      }
> +  }

Nit: please make whitespace-only changes in separate patches.

@@ +1655,5 @@
>    NS_DispatchToMainThread(ev);
>    return true;
>  }
>  
> +

Nit: extra line.

::: dom/geolocation/nsGeolocationSettings.cpp
@@ +168,5 @@
> +}
> +
> +
> +void
> +nsGeolocationSettings::HandleGeolocationSettingsError(nsAString const & aName)

None of these are coding errors.  Either report them to the browser console, or just use sensible defaults.  #ifdef DEBUG works too.

@@ +187,5 @@
> +    NS_WARNING("Unable to get value for '" GEO_ALA_ALWAYS_PRECISE "'");
> +  }
> +}
> +
> +void

nsresult

@@ +206,5 @@
> +  GPSLOG("unmapping watch ID %d", aWatchID );
> +  mCurrentWatches.Remove(static_cast<uint32_t>(aWatchID));
> +}
> +
> +void

nsresult

@@ +383,5 @@
> +       NS_ConvertUTF16toUTF8(str).get(),
> +       static_cast<int>(bt));
> +
> +    mType = bt;
> +  } 

space

@@ +385,5 @@
> +
> +    mType = bt;
> +  } 
> +
> +  // based on the new type, we need to clean up the other settings

only if there is a risk that they are going to be used

@@ +443,5 @@
> +  }
> +
> +  // parse the string and store the global lat/lon
> +	int32_t const comma = str.Find(",");
> +  if ( (str.CharAt(0) != '@') || (comma == -1) ) {

What is the significance of an '@' here?

@@ +451,5 @@
> +  nsresult result;
> +  double lat = 0.0, lon = 0.0;
> +
> +  nsString sval = str;
> +  sval.Cut(comma, str.Length() - comma);

Substring is better and uses less code.

::: dom/geolocation/nsGeolocationSettings.h
@@ +134,5 @@
> +  void GetWatchOrigin(int32_t, nsCString &);
> +  inline bool IsAlaEnabled() const { return mAlaEnabled; }
> +
> +  // given a watch ID, retrieve the geolocation settings.  the watch ID is
> +  // mapped to the origin of the listener/request which is then used to 

trailing space

@@ +139,5 @@
> +  // retreive the geolocation settings for the origin.
> +  // if the origin is in the always-precise list, the settings will always be
> +  // 'precise'. if the origin has origin-specific settings, that will be returned
> +  // otherwise the global geolocation settings will be returned.
> +  // NOTE: this returns a copy of the settings to enforce read-only client access

That can't be true if it returns a const-ref

Comment 20

[Dave Huseby [:huseby]]

Martin,

Thanks for the detailed review, I really appreciate it.  Let me incorporate the changes ASAP.

Comment 21

[Josh Matthews [:jdm]]

Comment on attachment 8501572 [details] [diff] [review]
gecko-ala-master.patch

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

I agree with Martin that this seems like an odd way to go about this. At minimum, I would want much more documentation about the sorts of values we're storing, because there is a scary amount of JSAPI in use to deal with all of the settings values.

::: dom/geolocation/nsGeolocation.cpp
@@ +97,5 @@
> +  nsIDOMGeoPosition* ApproximateLocation(nsIDOMGeoPosition*, int32_t);
> +#endif
> +  nsIDOMGeoPosition* SynthesizeLocation(nsIDOMGeoPosition*, double, double);
> +  void CalculateGridCoords(int32_t, double &, double &);
> +  double GridAlgorithm(int32_t, double, double);

I don't think any of these new methods need to be member functions of nsGeolocationRequest except for ChangeLocation.

@@ +542,5 @@
> +  return new nsGeoPosition(coords, ts);
> +}
> +
> +nsIDOMGeoPosition*
> +nsGeolocationRequest::ChangeLocation(nsIDOMGeoPosition* aPosition) {

Let's call this AdjustedLocation

@@ +683,5 @@
> +        coords->GetLatitude(&lat);
> +        coords->GetLongitude(&lon);
> +        GPSLOG("returning coordinates: %f, %f", lat, lon);
> +#endif
> +      if (aPosition) {

This is already inside a null-check.

@@ +841,5 @@
> +    callback = new GeolocationSettingsCallback();
> +    rv = settingsLock->Get(GEO_ALA_APP_SETTINGS, callback);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    callback = new GeolocationSettingsCallback();
> +    rv = settingsLock->Get(GEO_ALA_ALWAYS_PRECISE, callback);

Can we not store these all together in the same setting?

@@ +1655,5 @@
>    NS_DispatchToMainThread(ev);
>    return true;
>  }
>  
> +

nit: extra newline

::: dom/geolocation/nsGeolocationSettings.cpp
@@ +43,5 @@
> +  NS_INTERFACE_MAP_ENTRY(nsIObserver)
> +NS_INTERFACE_MAP_END
> +
> +NS_IMPL_ADDREF(nsGeolocationSettings)
> +NS_IMPL_RELEASE(nsGeolocationSettings)

Let's use NS_IMPL_ISUPPORTS(nsGeolocationSettings, nsIObserver) instead of these manual macros.

@@ +71,5 @@
> +  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> +    return NS_OK;
> +  }
> +
> +  // query for the current settings...

What?

@@ +126,5 @@
> +
> +
> +void
> +nsGeolocationSettings::HandleGeolocationSettingsChange(nsAString const & aKey,
> +                                                       JS::Value const & aVal)

nit: const T&, please.

@@ +189,5 @@
> +}
> +
> +void
> +nsGeolocationSettings::PutWatchOrigin(int32_t aWatchID,
> +                                      nsCString const & aOrigin)

nit: const T&

@@ +280,5 @@
> +    if (!propertyValue.isObject())
> +      continue;
> +    JS::RootedObject settingObj(cx, &propertyValue.toObject());
> +
> +    GeolocationSetting *aSettings = new GeolocationSetting(origin);

nit: the a prefix is only for arguments.

@@ +351,5 @@
> +  }
> +}
> +
> +
> +

nit: extra newlines

@@ +383,5 @@
> +       NS_ConvertUTF16toUTF8(str).get(),
> +       static_cast<int>(bt));
> +
> +    mType = bt;
> +  } 

nit: trailing ws

@@ +442,5 @@
> +    return;
> +  }
> +
> +  // parse the string and store the global lat/lon
> +	int32_t const comma = str.Find(",");

nit: indentation

@@ +454,5 @@
> +  nsString sval = str;
> +  sval.Cut(comma, str.Length() - comma);
> +  sval.Cut(0, 1);
> +  lat = sval.ToDouble(&result);
> +  if (result != NS_OK) {

NS_FAILED

@@ +461,5 @@
> +
> +  sval = str;
> +  sval.Cut(0, comma + 1);
> +  lon = sval.ToDouble(&result);
> +  if (result != NS_OK) {

NS_FAILED

::: dom/geolocation/nsGeolocationSettings.h
@@ +119,5 @@
> +public:
> +  static already_AddRefed<nsGeolocationSettings> GetGeolocationSettings();
> +  static mozilla::StaticRefPtr<nsGeolocationSettings> sSettings;
> +
> +  NS_DECL_THREADSAFE_ISUPPORTS

Why is this threadsafe?

::: dom/ipc/ContentParent.cpp
@@ +3593,5 @@
>      mGeolocationWatchID = AddGeolocationListener(this, aHighAccuracy);
> +
> +    // let the the settings cache know the origin of the new listener
> +    nsAutoCString origin;
> +    static_cast<nsIPrincipal*>(aPrincipal)->GetOrigin(getter_Copies(origin));

This is scary. Please use |nsCOMPtr<nsIPrincipal> principal = aPrincipal| instead.

@@ +3596,5 @@
> +    nsAutoCString origin;
> +    static_cast<nsIPrincipal*>(aPrincipal)->GetOrigin(getter_Copies(origin));
> +    nsRefPtr<nsGeolocationSettings> gs = nsGeolocationSettings::GetGeolocationSettings();
> +    if (gs) {
> +     gs->PutWatchOrigin( mGeolocationWatchID, origin );

nit: no extra spaces

Comment 22

[Josh Matthews [:jdm]]

Comment on attachment 8501572 [details] [diff] [review]
gecko-ala-master.patch

Oh Bugzilla.

Comment 23

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

This is the updated patch that addresses all of the issues in the review.  The only one I didn't understand was the comment about "This needs to report the "accuracy" as being max(aDistance, originalAccuracy)" but I think that is just for the logging.  we can always fix it later.

This patch has the grid algorithm code moved out into a helper class that can be easily replaced with another algorithm when we write it.

Comment 24

[Dave Huseby [:huseby]]

Try push: http://mzl.la/1vfbm3o

Comment 25

[Dave Huseby [:huseby]]

I'm seeing compile failures due to the unused utility enum hacks I added for the ALA enum.

> /builds/slave/try-l64-0000000000000000000000/build/dom/geolocation/nsGeolocation.cpp:577:10: error: enumeration value 'GEO_ALA_TYPE_LAST' not handled in switch [-Werror=switch]
> /builds/slave/try-l64-0000000000000000000000/build/dom/geolocation/nsGeolocation.cpp:577:10: error: enumeration value 'GEO_ALA_TYPE_COUNT' not handled in switch [-Werror=switch]
> /builds/slave/try-l64-0000000000000000000000/build/dom/geolocation/nsGeolocation.cpp:577:10: error: enumeration value 'GEO_ALA_TYPE_INVALID' not handled in switch [-Werror=switch]
> /builds/slave/try-l64-0000000000000000000000/build/dom/geolocation/nsGeolocationSettings.cpp:386:9: error: enumeration value 'GEO_ALA_TYPE_LAST' not handled in switch [-Werror=switch]
> /builds/slave/try-l64-0000000000000000000000/build/dom/geolocation/nsGeolocationSettings.cpp:386:9: error: enumeration value 'GEO_ALA_TYPE_COUNT' not handled in switch [-Werror=switch]
> /builds/slave/try-l64-0000000000000000000000/build/dom/geolocation/nsGeolocationSettings.cpp:386:9: error: enumeration value 'GEO_ALA_TYPE_INVALID' not handled in switch [-Werror=switch]

Is this going to prevent the patch from landing?  I'll fix them real quick if it will block.  They show up as warnings when doing local builds, so I thought it would be OK.

Comment 26

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

This is an updated patch with the enum warnings/errors fixed.  The try push is here: http://mzl.la/1rvTz1w

Comment 27

[Doug Turner (:dougt)]

> Is this going to prevent the patch from landing?
 
This hasn't gotten a code review, and I suspect the review will want this to compile without warnings.

Comment 28

[Martin Thomson [:mt:]]

Well, my half-done review is now gone for some reason, probably by marking the review obsolete, or maybe that's just how splinter works.  Hopefully it's still sitting on my work machine.

Comment 29

[Dave Huseby [:huseby]]

Anybody know why the hazard analysis build Linux x64 opt (H) would be broken the way it is?  http://mzl.la/1rvTz1w  The other builds all seem to work.

Comment 30

[Dave Huseby [:huseby]]

(In reply to Doug Turner (:dougt) from comment #27)
> ...I suspect the review will want this to compile without warnings.

that's what I assumed so I fixed the warnings and submitted a new patch.  the latest try push looks all green except for the 64bit linux "hazard analysis" build mentioned in Comment 29.

Comment 31

[Doug Turner (:dougt)]

dave:

+#ifdef MOZ_GPS_DEBUG
+#ifdef ANDROID
  +#include <android/log.h>
  +#define GPSLOG(fmt, ...) __android_log_print(ANDROID_LOG_WARN, "GPS", "%12s:%-5d " fmt,  __FILE__, __LINE__, ##__VA_ARGS__)
+#endif // ANDROID

GPSLOG() <=========== undefined here

+#else
+#define GPSLOG(...) {;}
+#endif // MOZ_GPS_DEBUG

Comment 32

[Dave Huseby [:huseby]]

(In reply to Doug Turner (:dougt) from comment #31)
> GPSLOG() <=========== undefined here

Aha! yes, thanks.  Let me fix that real quick.

Comment 33

[Martin Thomson [:mt:]]

Comment on attachment 8505781 [details] [diff] [review]
gecko-ala-master.patch

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

This per-origin stuff settings is a little gnarly.  I think that you need to consider a different structure here to make this easier to deal with.

Have a global fuzzer and a per-origin fuzzer rather than settings objects.

The global settings object can add the collection of per-origin fuzzers, as well as routing to them for both settings changes and fuzzing requests.  If a setting comes through for the per-origin object, it's handled in exactly the same.  If a fuzzing request comes in, then it can route based on the origin of the requester.

e.g.,

fuzzedLocation = globalSettings->GetFuzzer(origin)->Fuzz(location);
if (!location) {
  // guess we're not reporting now
}

That last bit means that you will need to attach an origin (or principal...) to a geolocation watch.  That's better than the strange registration logic you have added to support this.

The other thing I think that you should consider is to have a fuzzer instance attached to nsGeolocationSettings.  At both levels.  Rather than store a grab-bag of attributes, the settings can be used to construct a new fuzzer with the desired parameters.

That changes the structure of the settings, maybe something unified:

{ fuzzer: "none" } <- global settings have no origin
{ origin: "https://example.com", fuzzer: "fixed", latitude: 12.6, longitude: -76.263 }
{ origin: "https://google.com", fuzzer: "random", distance: 1000.0 }

All the global object needs to do is a) check if it has an origin, then either consume directly, or route to the right consumer.

Then, each will look at the fuzzer, and pass the appropriate arguments to the constructor of the fuzzer, maybe after checking for equality or something like that (don't want to make a new fuzzer with the same parameters if that means losing state...)

::: dom/geolocation/nsGeoGridFuzzer.cpp
@@ +11,5 @@
> +#ifdef MOZ_APPROX_LOCATION
> +
> +#define SECONDS_PER_DEGREE  (3600)
> +#define KM_PER_DEGREE       (111.27)
> +#define SECONDS_PER_KM      (32.39)

Still don't know where these came from.  WGS84 uses 6378137m radius (and inverse flattening of 1/298.257223563).  Can you derive these values from that instead so we're actually using the same values, just let the compiler run the calculations, it's not expensive.

@@ +28,5 @@
> +  // which grid cell does the position belong to?
> +  double belongsTo = aCoord / gridSize;
> +
> +  // return the center of that grid cell
> +  return (floor(belongsTo) * gridSize + ceil(belongsTo) * gridSize) / 2;

This will return the edge of the grid if aCoord is a whole multiple of the grid size.

This is going to break at the poles; and along the 180th meridian.  Might want to note that.

@@ +37,5 @@
> + * location and calculates which grid cell the coordinates fall within and
> + * then returns the coordinates of the geographical center of the grid square.
> + */
> +static void CalculateGridCoords(int32_t aDistance,
> +                                double & aLatitude,

spacing of qualifiers is ``Type* aIdentifier`` or ``Type& aIdentifier``

::: dom/geolocation/nsGeoGridFuzzer.h
@@ +13,5 @@
> +class nsGeoGridFuzzer MOZ_FINAL {
> +public:
> +
> +  static already_AddRefed<nsIDOMGeoPosition>
> +    FuzzLocation(const GeolocationSetting &, nsIDOMGeoPosition *);

I think that it's customary to provide argument names as a form of documentation.

Check qualifier spacing.

s/nsIDOMGeoPosition*/nsCOMPtr<nsIDOMGeoPosition>&/

@@ +16,5 @@
> +  static already_AddRefed<nsIDOMGeoPosition>
> +    FuzzLocation(const GeolocationSetting &, nsIDOMGeoPosition *);
> +
> +private:
> +  nsGeoGridFuzzer() {} // can't construct

Any reason this can't be a simple exported function?

Actually, I think that I can answer my own question.  See below.

::: dom/geolocation/nsGeolocation.cpp
@@ +515,5 @@
>    }
>  }
>  
> +static already_AddRefed<nsIDOMGeoPosition>
> +CopyGeoPosition(nsIDOMGeoPosition * aPosition)

This function has the dubious characteristic of erasing fields from the nsGeoPosition that might have been present previously (speed, heading, etc...).

More below...

@@ +573,5 @@
> +
> +  // look up the geolocation settings via the watch ID
> +  DOMTimeStamp ts(PR_Now() / PR_USEC_PER_MSEC);
> +  GeolocationSetting setting = gs->LookupGeolocationSetting(mWatchId);
> +  switch (setting.GetType()) {

Why does this not switch into a table of GeolocationFuzzer or GeolocationTransformation instances, each with a different implementation?  Returning a fixed location is one transformation, the copy operation is another, and the grid fuzzer is another.  You can simplify this code considerably.

(That answers the question regarding the nsGeoGridFuzzer interface; it's just one of these.)

Note that a general mechanism is stateful.

::: dom/geolocation/nsGeolocationSettings.cpp
@@ +165,5 @@
> +}
> +
> +
> +void
> +nsGeolocationSettings::HandleGeolocationSettingsError(const nsAString & aName)

Unused, I think.

@@ +306,5 @@
> +  }
> +}
> +
> +void
> +nsGeolocationSettings::HandleGeolocationAlwaysPreciseChange(const JS::Value & aVal)

Why have always precise for apps, why not a new, different fuzzer?

@@ +447,5 @@
> +
> +  nsresult rv;
> +  double lat = 0.0, lon = 0.0;
> +  //comma++;
> +  //nsString sval(Substring(str, comma, str.Length() - comma));

?

what's wrong with:

nsString slat = Substring(str, 1, comma);
nsString slon = Substring(str, comma + 1);
double lat = slat.ToDouble(&rv); NS_ENSURE_SUCCESS(rv,);
double lon = slong.ToDouble(&rv); NS_ENSURE_SUCCESS(rv,);
mLatitude = lat;
mLongitude = lon;

And avoid all this cutting around and copying.  Or avoid it entirely (as in the first comment).

Comment 34

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

This updated patch fixes the GPSLOG issue.

Comment 35

[Dave Huseby [:huseby]]

Thanks for the feedback, I'll incorporate this right away.

Comment 36

[Dave Huseby [:huseby]]

just so I don't forget this...here's the try for the GPSLOG fix: http://mzl.la/1vFVCbd

Comment 37

[Dave Huseby [:huseby]]

(In reply to Martin Thomson [:mt] from comment #33)
> Comment on attachment 8505781 [details] [diff] [review]
> gecko-ala-master.patch
> 
> Review of attachment 8505781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This per-origin stuff settings is a little gnarly.  I think that you need to
> consider a different structure here to make this easier to deal with.
> 
> Have a global fuzzer and a per-origin fuzzer rather than settings objects.
> 
> That last bit means that you will need to attach an origin (or principal...)
> to a geolocation watch.  That's better than the strange registration logic
> you have added to support this.

I originally had it structured this way but I had to re-write it when I discovered that the only place I had access to the origin was from the IPC::Principal in the ContentParent::RecvAddGeolocationListener.  This seems fine, except that I cannot/will not change any interfaces because of where we are in the release cycle.  Inside of RecvAddGeolocationListener, there's only three possible vectors for transferring the principal/origin to the geolocation watch:

1. The principal/origin could be added to PositionOptions, but that's a dictionary defined in dom/webidl/Geolocation.webidl.  Since I can't/won't change any interfaces, that's out.  The PositionOptions is passed to the Geolocation::WatchPosition function that creates the geolocation watch.

2. A new parameter could be added to Geolocation::WatchPosition, but that API (nsIDOMGeoGeolocation) is defined in dom/interfaces/geolocation/nsIDOMGeoGeolocation.idl.  Since I can't/won't change any interfaces, that's out.

3. The only thing left is to use the geolocation watch ID to associate an origin with the geolocation watch.  So that's what I did.

If you can think of any other way to get the origin to the geolocation watch without changing any of the API's, please let me know.  I tried looking up the principal in the geolocation watch but because of the way the IPC is organized, the geolocation watch objects that receive the position callbacks are in the parent process and you can't get the right principal because that's in the content process.  The only place in the parent process where we know the origin requesting the geolocation watch is in the ContentParent::RecvAddGeolocationListener.

Comment 38

[Dave Huseby [:huseby]]

(In reply to Martin Thomson [:mt] from comment #33)
> Comment on attachment 8505781 [details] [diff] [review]
> gecko-ala-master.patch
> 
> Review of attachment 8505781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This per-origin stuff settings is a little gnarly.  I think that you need to
> consider a different structure here to make this easier to deal with.

Because of what I said in Comment 37, I think the best thing to do is to keep the mapping of watch ID's to origins, that's the only way that can be done.  But I like the idea of making a unified setting dictionary that translates into a fuzzer object instead of a settings object.  When a geolocation watch needs to fuzz a location, they will look up the fuzzer by watch ID.  The global settings object will map the watch ID to the origin, then try to get the fuzzer object by origin.  If there is a per-origin fuzzer, that will be returned, if there isn't a per-origin fuzzer, the global one will be returned.

e.g.

fuzzedLocation = globalSettings->GetFuzzer(watchID)->Fuzz(location);

already_AddRefed<nsIGeoFuzzer>
GetFuzzer(int32_t aWatchID) {
  const nsCString* const pOrigin = mCurrentWatches.Get(aWatchID);
  if (!pOrigin) {
    return mpGlobalFuzzer;
  }
  nsCOMPtr<nsIGeoFuzzer> fuzzer = mPerOriginFuzzers.Get(NS_ConvertUTF8toUTF16(*pOrigin));
  return (fuzzer ? fuzzer : mpGlobalFuzzer);
}

This will allow us to have an nsIGeoFuzzer interface behind which we can have different fuzzing implementations.  The fuzzing settings dictionary can be defined as a FuzzerOptions dictionary in the same interface file.  The specific fuzzer object created will depend on the fuzzer objects object passed to the static factory function.

Then it would be just a matter of changing the ::HandleMozsettingsChange() code to first try looking up a fuzzer associated with the origin.  If one exists, then there must be a way to tell if the fuzzer is the same type as the one the fuzzer options specify.  If they are different, then the old one needs to be destroyed and replaced with a newly created one.  If I understand nsCOMPtr correctly, this should be easy to do now that we have move symantics in nsCOMPtr.  I think a simple assignment should release the old one and replace it with the new one.

What do you think?

Comment 39

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

I addressed all of the cleanup issues and switched to using the WGS84 geoid model for the grid calculations.  But since I haven't heard back on how we want to do the refactor, that's all that is in the this patch.

If we're willing to accept the structure as is, I think I've met all of the nits and cleanup fixes.

I'm also most of the way through implementing the fuzzer in the Thomson draft as well.

Comment 40

[Dave Huseby [:huseby]]

(In reply to Martin Thomson [:mt] from comment #33)
> Comment on attachment 8505781 [details] [diff] [review]
> gecko-ala-master.patch
> 
> Review of attachment 8505781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> s/nsIDOMGeoPosition*/nsCOMPtr<nsIDOMGeoPosition>&/
> 
> @@ +16,5 @@
> > +  static already_AddRefed<nsIDOMGeoPosition>
> > +    FuzzLocation(const GeolocationSetting &, nsIDOMGeoPosition *);
> > +
> > +private:
> > +  nsGeoGridFuzzer() {} // can't construct

Are you trying to tell me that the naked nsIDOMGeoPosition* parameters should instead be nsCOMPtr<nsIDOMGeoPosition>& ?

Comment 41

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

Whoops, cleaned up some trailing spaces.

Comment 42

[Dave Huseby [:huseby]]

Try push of the latest patch: http://mzl.la/1DmFZXa

Comment 43

[Josh Matthews [:jdm]]

I just realized that the design as explained in comment 37 relies on a number of factors in order to work correctly:
* we only ever use a single instance of Geolocation in the parent process, so the watch ids stored happen to be unique
* we only use watchPosition, never getCurrentPosition, so we always have non-zero watch ids
* we never use the geolocation APIs from the parent process for any other content

This feels really brittle to me; there are a lot of assumptions being made.

Comment 44

[Josh Matthews [:jdm]]

Sorry, accidental needinfo.

Comment 45

[Josh Matthews [:jdm]]

(In reply to Josh Matthews [:jdm] from comment #43)
> I just realized that the design as explained in comment 37 relies on a
> number of factors in order to work correctly:
> * we only ever use a single instance of Geolocation in the parent process,
> so the watch ids stored happen to be unique
> * we only use watchPosition, never getCurrentPosition, so we always have
> non-zero watch ids
> * we never use the geolocation APIs from the parent process for any other
> content
> 
> This feels really brittle to me; there are a lot of assumptions being made.

My mistake, I forgot that we're only storing the origin/watch association from the parent process IPC geolocation code, so those assumptions are just the facts of life. We'll still have overlapping settings if any parent process code decides to use watchPosition for any other reason than cross-process geolocation.

Comment 46

[Josh Matthews [:jdm]]

Comment on attachment 8507155 [details] [diff] [review]
gecko-ala-master.patch

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

Given the constraints of the origin stuff, I don't have a better idea for storing this. It's certainly not my favourite, however.

::: configure.in
@@ +7005,5 @@
> +dnl ========================================================
> +dnl moz_gps_debug
> +dnl ========================================================
> +MOZ_ARG_ENABLE_BOOL(gps_debug,
> +[ --enable-gps-debug    Enable gps specific debug messages ],

Is this configure flag really necessary, or can we just rely on people to add the define themselves?

::: dom/geolocation/nsGeoGridFuzzer.h
@@ +9,5 @@
> +#include "nsCOMPtr.h"
> +#include "nsIDOMGeoPosition.h"
> +#include "nsGeolocationSettings.h"
> +
> +class nsGeoGridFuzzer MOZ_FINAL {

nit: { on the next line.

::: dom/geolocation/nsGeolocation.cpp
@@ +515,5 @@
>    }
>  }
>  
> +static already_AddRefed<nsIDOMGeoPosition>
> +CopyGeoPosition(nsIDOMGeoPosition * aPosition)

This function feels unnecessary.

@@ +534,5 @@
> +  return pos.forget();
> +}
> +
> +static already_AddRefed<nsIDOMGeoPosition>
> +SynthesizeLocation(nsIDOMGeoPosition * aPosition,

I don't see the point of passing aPosition here.

@@ +555,5 @@
> +nsGeolocationRequest::AdjustedLocation(nsIDOMGeoPosition * aPosition)
> +{
> +  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> +    GPSLOG("child process just copying position");
> +    return CopyGeoPosition(aPosition);

Just store a copy of aPosition in a refptr and return that instead of calling CopyGeoPosition everywhere.

@@ +854,5 @@
>  
>  void
>  nsGeolocationService::HandleMozsettingChanged(nsISupports* aSubject)
>  {
> +  // The string that we're interested in will be a JSON string that looks like:

nit: please don't mix unrelated whitespace changes in this patch.

@@ +881,5 @@
>  
>  void
>  nsGeolocationService::HandleMozsettingValue(const bool aValue)
>  {
> +  if (!aValue) {

nit: Please don't mix unrelated whitespace changes in this patch.

@@ +1591,5 @@
>  Geolocation::RegisterRequestWithPrompt(nsGeolocationRequest* request)
>  {
>    if (Preferences::GetBool("geo.prompt.testing", false)) {
>      bool allow = Preferences::GetBool("geo.prompt.testing.allow", false);
> +    nsCOMPtr<nsIRunnable> ev = new RequestAllowEvent(allow, 

nit: trailing space

@@ +1592,5 @@
>  {
>    if (Preferences::GetBool("geo.prompt.testing", false)) {
>      bool allow = Preferences::GetBool("geo.prompt.testing.allow", false);
> +    nsCOMPtr<nsIRunnable> ev = new RequestAllowEvent(allow, 
> +                                                    request);

nit: indentation.

@@ +1602,5 @@
>    NS_DispatchToMainThread(ev);
>    return true;
>  }
>  
> +

nit: extra newline

::: dom/geolocation/nsGeolocationSettings.cpp
@@ +64,5 @@
> +nsresult nsGeolocationSettings::Init()
> +{
> +  // this singleton is only needed in the parent process so skip init in
> +  // a content process.
> +  if (XRE_GetProcessType() == GeckoProcessType_Content) {

Why not return null from GetGeolocationSettings instead?

@@ +107,5 @@
> +
> +GeolocationSetting
> +nsGeolocationSettings::LookupGeolocationSetting(int32_t aWatchID)
> +{
> +  nsCString const * const pOrigin = mCurrentWatches.Get(aWatchID);

nit: please no hungarian notiation.

@@ +117,5 @@
> +  // set gb == nullptr
> +  GeolocationSetting const * const gb = mPerOriginSettings.Get(NS_ConvertUTF8toUTF16(*pOrigin));
> +
> +  // return a copy of the per-app or global settings
> +  return (gb ? *gb : mGlobalSetting);

No need for surrounding ().

@@ +147,5 @@
> +nsGeolocationSettings::HandleMozsettingsChanged(nsISupports* aSubject)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  // The string that we're interested in will be a JSON string that looks like:
> +  //  {"key":"gelocation.enabled","value":true}

I don't see how this comment makes sense here.

@@ +193,5 @@
> +    return;
> +  }
> +
> +  GPSLOG("mapping watch ID %d to origin %s", aWatchID, aOrigin.get());
> +  mCurrentWatches.Put(static_cast<uint32_t>(aWatchID), new nsCString(aOrigin));

Why is this dynamically allocated?

@@ +199,5 @@
> +
> +void
> +nsGeolocationSettings::RemoveWatchOrigin(int32_t aWatchID)
> +{
> +  GPSLOG("unmapping watch ID %d", aWatchID );

nit: no space before )

@@ +200,5 @@
> +void
> +nsGeolocationSettings::RemoveWatchOrigin(int32_t aWatchID)
> +{
> +  GPSLOG("unmapping watch ID %d", aWatchID );
> +  mCurrentWatches.Remove(static_cast<uint32_t>(aWatchID));

This leaks memory.

@@ +206,5 @@
> +
> +void
> +nsGeolocationSettings::GetWatchOrigin(int32_t aWatchID, nsCString & aOrigin)
> +{
> +  nsACString * pStr = mCurrentWatches.Get(aWatchID);

nit: please no hungarian notation.

@@ +232,5 @@
> +    return;
> +  }
> +
> +  // clear the hash table
> +  mPerOriginSettings.Clear();

This leaks memory.

@@ +243,5 @@
> +  // if we get no ids then the exception list is empty and we can return here.
> +  if (!ids)
> +      return;
> +
> +  JS::RootedId id(cx);

Move this into the loop, please.

@@ +244,5 @@
> +  if (!ids)
> +      return;
> +
> +  JS::RootedId id(cx);
> +  JS::RootedValue propertyValue(cx);

Why so far away from its use?

@@ +254,5 @@
> +    JS::RootedValue v(cx);
> +    if (!JS_IdToValue(cx, id, &v))
> +      continue;
> +
> +    if (!v.isString())

This can be combined with the previous check.

@@ +257,5 @@
> +
> +    if (!v.isString())
> +      continue;
> +
> +    JS::RootedString str(cx, JS::ToString(cx, v));

v.toString()

@@ +273,5 @@
> +
> +    // get the app setting object
> +    if (!JS_GetPropertyById(cx, obj, id, &propertyValue))
> +      continue;
> +    if (!propertyValue.isObject())

This can be combined with the previous check.

@@ +277,5 @@
> +    if (!propertyValue.isObject())
> +      continue;
> +    JS::RootedObject settingObj(cx, &propertyValue.toObject());
> +
> +    GeolocationSetting *settings = new GeolocationSetting(origin);

Why is this dynamically allocated?

@@ +358,5 @@
> +  }
> +
> +  nsString str;
> +  AutoSafeJSContext cx;
> +  AssignJSString(cx, str, aVal.toString());

This needs to be checked for failure.

@@ +373,5 @@
> +  } else if (str.EqualsASCII(GEO_ALA_TYPE_VALUE_NONE)) {
> +    bt = GEO_ALA_TYPE_NONE;
> +  }
> +
> +  if (IS_VALID_GEO_ALA_TYPE(bt)) {

I don't think this needs to be a macro.

@@ +382,5 @@
> +    mType = bt;
> +  }
> +
> +  // based on the new type, we need to clean up the other settings
> +  switch(bt) {

mType?

@@ +432,5 @@
> +  }
> +
> +  nsString str;
> +  AutoSafeJSContext cx;
> +  AssignJSString(cx, str, aVal.toString());

This needs to be checked for failure.

@@ +447,5 @@
> +
> +  nsresult rv;
> +  double lat = 0.0, lon = 0.0;
> +  //comma++;
> +  //nsString sval(Substring(str, comma, str.Length() - comma));

Why is this left here? What about Martin's suggestions?

::: dom/geolocation/nsGeolocationSettings.h
@@ +37,5 @@
> +enum GeolocationType {
> +  GEO_ALA_TYPE_PRECISE, // default, GPS/AGPS location
> +#ifdef MOZ_APPROX_LOCATION
> +  GEO_ALA_TYPE_APPROX,  // approximate, grid-based location
> +#endif

This will change the meaning if you use a profile from a non-enabled build with a build with approximate fuzzing enabled, right?

@@ +116,5 @@
> +public:
> +  static already_AddRefed<nsGeolocationSettings> GetGeolocationSettings();
> +  static mozilla::StaticRefPtr<nsGeolocationSettings> sSettings;
> +
> +  NS_DECL_THREADSAFE_ISUPPORTS

Why threadsafe?

@@ +142,5 @@
> +
> +private:
> +  ~nsGeolocationSettings() {}
> +  nsGeolocationSettings(const nsGeolocationSettings &) :
> +    mGlobalSetting(NullString()) {} // can't copy obj

What does this mean?

::: dom/ipc/ContentParent.cpp
@@ +3595,5 @@
> +    // let the the settings cache know the origin of the new listener
> +    nsAutoCString origin;
> +    // hint to the compiler to use the conversion operator to nsIPrincipal*
> +    nsCOMPtr<nsIPrincipal> principal = static_cast<nsIPrincipal*>(aPrincipal);
> +    principal->GetOrigin(getter_Copies(origin));

Why the getter_Copies? I think & should work fine.

@@ +3598,5 @@
> +    nsCOMPtr<nsIPrincipal> principal = static_cast<nsIPrincipal*>(aPrincipal);
> +    principal->GetOrigin(getter_Copies(origin));
> +    nsRefPtr<nsGeolocationSettings> gs = nsGeolocationSettings::GetGeolocationSettings();
> +    if (gs) {
> +     gs->PutWatchOrigin( mGeolocationWatchID, origin );

nit: no spaces after ( or before )

Comment 47

[Josh Matthews [:jdm]]

I don't know how I ended up reviewing the old patch, but I believe all of my comments are still valid.

Comment 48

[Dave Huseby [:huseby]]

(In reply to Josh Matthews [:jdm] from comment #43)
> I just realized that the design as explained in comment 37 relies on a
> number of factors in order to work correctly:
> * we only ever use a single instance of Geolocation in the parent process,
> so the watch ids stored happen to be unique

This is always true.

> * we only use watchPosition, never getCurrentPosition, so we always have
> non-zero watch ids

getCurrentPosition gets implemented in terms of watchPosition on the parent thread.  so there is a watch ID that gets associated with the origin and everything works as expected.  getCurrentPosition creates an nsGeolocationRequest with the aWatchPositionRequest set to false which makes it a one-shot request.

> * we never use the geolocation APIs from the parent process for any other
> content

AFAIK, no we don't.  But even if we did, as long as it goes through the geolocation service, everything should work just fine.

Comment 49

[Dave Huseby [:huseby]]

(In reply to Josh Matthews [:jdm] from comment #46)
> Comment on attachment 8507155 [details] [diff] [review]
> gecko-ala-master.patch
> 
> Review of attachment 8507155 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Given the constraints of the origin stuff, I don't have a better idea for
> storing this. It's certainly not my favourite, however.
> 
> ::: configure.in
> @@ +7005,5 @@
> > +dnl ========================================================
> > +dnl moz_gps_debug
> > +dnl ========================================================
> > +MOZ_ARG_ENABLE_BOOL(gps_debug,
> > +[ --enable-gps-debug    Enable gps specific debug messages ],
> 
> Is this configure flag really necessary, or can we just rely on people to
> add the define themselves?

Why not?  It's a convenience and not a high priority.
 
> ::: dom/geolocation/nsGeoGridFuzzer.h
> @@ +9,5 @@
> > +#include "nsCOMPtr.h"
> > +#include "nsIDOMGeoPosition.h"
> > +#include "nsGeolocationSettings.h"
> > +
> > +class nsGeoGridFuzzer MOZ_FINAL {
> 
> nit: { on the next line.

Fixed.

> ::: dom/geolocation/nsGeolocation.cpp
> @@ +515,5 @@
> >    }
> >  }
> >  
> > +static already_AddRefed<nsIDOMGeoPosition>
> > +CopyGeoPosition(nsIDOMGeoPosition * aPosition)
> 
> This function feels unnecessary.

Removed.

> @@ +534,5 @@
> > +  return pos.forget();
> > +}
> > +
> > +static already_AddRefed<nsIDOMGeoPosition>
> > +SynthesizeLocation(nsIDOMGeoPosition * aPosition,
> 
> I don't see the point of passing aPosition here.

Good catch.  It used to be necessary but the last refactor made it unused.  Fixed.
 
> @@ +555,5 @@
> > +nsGeolocationRequest::AdjustedLocation(nsIDOMGeoPosition * aPosition)
> > +{
> > +  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> > +    GPSLOG("child process just copying position");
> > +    return CopyGeoPosition(aPosition);
> 
> Just store a copy of aPosition in a refptr and return that instead of
> calling CopyGeoPosition everywhere.

Fixed.

> @@ +854,5 @@
> >  
> >  void
> >  nsGeolocationService::HandleMozsettingChanged(nsISupports* aSubject)
> >  {
> > +  // The string that we're interested in will be a JSON string that looks like:
> 
> nit: please don't mix unrelated whitespace changes in this patch.

Fixed.
 
> @@ +881,5 @@
> >  
> >  void
> >  nsGeolocationService::HandleMozsettingValue(const bool aValue)
> >  {
> > +  if (!aValue) {
> 
> nit: Please don't mix unrelated whitespace changes in this patch.

Fixed.

> @@ +1591,5 @@
> >  Geolocation::RegisterRequestWithPrompt(nsGeolocationRequest* request)
> >  {
> >    if (Preferences::GetBool("geo.prompt.testing", false)) {
> >      bool allow = Preferences::GetBool("geo.prompt.testing.allow", false);
> > +    nsCOMPtr<nsIRunnable> ev = new RequestAllowEvent(allow, 
> 
> nit: trailing space

Fixed.

> @@ +1592,5 @@
> >  {
> >    if (Preferences::GetBool("geo.prompt.testing", false)) {
> >      bool allow = Preferences::GetBool("geo.prompt.testing.allow", false);
> > +    nsCOMPtr<nsIRunnable> ev = new RequestAllowEvent(allow, 
> > +                                                    request);
> 
> nit: indentation.

Fixed.

> @@ +1602,5 @@
> >    NS_DispatchToMainThread(ev);
> >    return true;
> >  }
> >  
> > +
> 
> nit: extra newline

Fixed.

> ::: dom/geolocation/nsGeolocationSettings.cpp
> @@ +64,5 @@
> > +nsresult nsGeolocationSettings::Init()
> > +{
> > +  // this singleton is only needed in the parent process so skip init in
> > +  // a content process.
> > +  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> 
> Why not return null from GetGeolocationSettings instead?

Fixed.

> @@ +107,5 @@
> > +
> > +GeolocationSetting
> > +nsGeolocationSettings::LookupGeolocationSetting(int32_t aWatchID)
> > +{
> > +  nsCString const * const pOrigin = mCurrentWatches.Get(aWatchID);
> 
> nit: please no hungarian notiation.

Oops, old habit.  Fixed.
 
> @@ +117,5 @@
> > +  // set gb == nullptr
> > +  GeolocationSetting const * const gb = mPerOriginSettings.Get(NS_ConvertUTF8toUTF16(*pOrigin));
> > +
> > +  // return a copy of the per-app or global settings
> > +  return (gb ? *gb : mGlobalSetting);
> 
> No need for surrounding ().

This is also an old habit.  Fixed.
 
> @@ +147,5 @@
> > +nsGeolocationSettings::HandleMozsettingsChanged(nsISupports* aSubject)
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  // The string that we're interested in will be a JSON string that looks like:
> > +  //  {"key":"gelocation.enabled","value":true}
> 
> I don't see how this comment makes sense here.

Fixed.

> @@ +193,5 @@
> > +    return;
> > +  }
> > +
> > +  GPSLOG("mapping watch ID %d to origin %s", aWatchID, aOrigin.get());
> > +  mCurrentWatches.Put(static_cast<uint32_t>(aWatchID), new nsCString(aOrigin));
> 
> Why is this dynamically allocated?

It's because the nsClassHashtable can only store pointers to objects rather than concrete objects.  There is an example of an nsClassHashtable storing nsCStrings in dom/camera/GonkCameraParameters.cpp.  The hashtable is called mIsoModeMap.  The code calls ::Put(new nsCString) and it also has a call to ::Clear as well.  If my code is incorrect, then that code is incorrect as well.

> @@ +199,5 @@
> > +
> > +void
> > +nsGeolocationSettings::RemoveWatchOrigin(int32_t aWatchID)
> > +{
> > +  GPSLOG("unmapping watch ID %d", aWatchID );
> 
> nit: no space before )

Another old habit.  Fixed.

> @@ +200,5 @@
> > +void
> > +nsGeolocationSettings::RemoveWatchOrigin(int32_t aWatchID)
> > +{
> > +  GPSLOG("unmapping watch ID %d", aWatchID );
> > +  mCurrentWatches.Remove(static_cast<uint32_t>(aWatchID));
> 
> This leaks memory.

How?  If I understand documentation, nsClassHashtable behaves correctly: http://mzl.la/1DyWVcZ and http://mzl.la/1vL37MJ  The docs say that nsClassHashtable deletes the value objects when they are removed.
 
> @@ +206,5 @@
> > +
> > +void
> > +nsGeolocationSettings::GetWatchOrigin(int32_t aWatchID, nsCString & aOrigin)
> > +{
> > +  nsACString * pStr = mCurrentWatches.Get(aWatchID);
> 
> nit: please no hungarian notation.

Fixed.
 
> @@ +232,5 @@
> > +    return;
> > +  }
> > +
> > +  // clear the hash table
> > +  mPerOriginSettings.Clear();
> 
> This leaks memory.

Hrm...So I don't see how clearing the hashtable leaks memory, the documentation in the code (http://mzl.la/1vL37MJ) suggests that the contained objects are deleted when removed and ::Clear is implemented in terms of ::Remove.  If you look at workers/ServiceWorkerManager.cpp there is an nsClassHashtable that stores dynamically allocated PendingReadyPromise instances exactly the same way I'm handling GeolocationSettings objects.  If my code leaks memory, then I think that does too.

> @@ +243,5 @@
> > +  // if we get no ids then the exception list is empty and we can return here.
> > +  if (!ids)
> > +      return;
> > +
> > +  JS::RootedId id(cx);
> 
> Move this into the loop, please.

I'm not sure why, but I fixed it.

> @@ +244,5 @@
> > +  if (!ids)
> > +      return;
> > +
> > +  JS::RootedId id(cx);
> > +  JS::RootedValue propertyValue(cx);
> 
> Why so far away from its use?

Fixed.

> @@ +254,5 @@
> > +    JS::RootedValue v(cx);
> > +    if (!JS_IdToValue(cx, id, &v))
> > +      continue;
> > +
> > +    if (!v.isString())
> 
> This can be combined with the previous check.

Fixed.

> @@ +257,5 @@
> > +
> > +    if (!v.isString())
> > +      continue;
> > +
> > +    JS::RootedString str(cx, JS::ToString(cx, v));
> 
> v.toString()

Fixed.

> @@ +273,5 @@
> > +
> > +    // get the app setting object
> > +    if (!JS_GetPropertyById(cx, obj, id, &propertyValue))
> > +      continue;
> > +    if (!propertyValue.isObject())
> 
> This can be combined with the previous check.

Fixed.

> @@ +277,5 @@
> > +    if (!propertyValue.isObject())
> > +      continue;
> > +    JS::RootedObject settingObj(cx, &propertyValue.toObject());
> > +
> > +    GeolocationSetting *settings = new GeolocationSetting(origin);
> 
> Why is this dynamically allocated?

Fixed.

> @@ +358,5 @@
> > +  }
> > +
> > +  nsString str;
> > +  AutoSafeJSContext cx;
> > +  AssignJSString(cx, str, aVal.toString());
> 
> This needs to be checked for failure.

Fixed.

> @@ +373,5 @@
> > +  } else if (str.EqualsASCII(GEO_ALA_TYPE_VALUE_NONE)) {
> > +    bt = GEO_ALA_TYPE_NONE;
> > +  }
> > +
> > +  if (IS_VALID_GEO_ALA_TYPE(bt)) {
> 
> I don't think this needs to be a macro.

I disagree, but I fixed it anyway.  We compile with a flag that errors out if switch/case blocks don't have clauses for every value in an enum.  That prevents us from using some useful enum hacks like automating range checking like so:

> enum blah {
>   FOO,
>   BAR,
>   BAZ,
>   LAST,
>   FIRST = FOO,
>   COUNT = LAST - FIRST
> };
> IS_VALID_BLAH(x) (((x) >= FIRST) && ((x) < LAST))

I was using code like this but the try server was failing builds because I didn't have case clauses for FIRST, LAST, and COUNT.  Anyway, I removed the macro and did the test inline.  It was the only one, so it makes sense I guess.  I always use a macro if I'm going to do range checks all over the place.

> @@ +382,5 @@
> > +    mType = bt;
> > +  }
> > +
> > +  // based on the new type, we need to clean up the other settings
> > +  switch(bt) {
> 
> mType?

Changed this to mFuzzMethod.  That's probably more clear.
 
> @@ +432,5 @@
> > +  }
> > +
> > +  nsString str;
> > +  AutoSafeJSContext cx;
> > +  AssignJSString(cx, str, aVal.toString());
> 
> This needs to be checked for failure.

Fixed.

> @@ +447,5 @@
> > +
> > +  nsresult rv;
> > +  double lat = 0.0, lon = 0.0;
> > +  //comma++;
> > +  //nsString sval(Substring(str, comma, str.Length() - comma));
> 
> Why is this left here? What about Martin's suggestions?

Fixed in latest version.

> ::: dom/geolocation/nsGeolocationSettings.h
> @@ +37,5 @@
> > +enum GeolocationType {
> > +  GEO_ALA_TYPE_PRECISE, // default, GPS/AGPS location
> > +#ifdef MOZ_APPROX_LOCATION
> > +  GEO_ALA_TYPE_APPROX,  // approximate, grid-based location
> > +#endif
> 
> This will change the meaning if you use a profile from a non-enabled build
> with a build with approximate fuzzing enabled, right?

I moved it to the end.  I also changed GeolocationSetting::HandleTypeChange() so that it always matches against GEO_ALA_TYPE_VALUE_APPROX but on non-MOZ_APPROX_LOCATION builds, it will set the fuzz method to be NONE.
 
> @@ +116,5 @@
> > +public:
> > +  static already_AddRefed<nsGeolocationSettings> GetGeolocationSettings();
> > +  static mozilla::StaticRefPtr<nsGeolocationSettings> sSettings;
> > +
> > +  NS_DECL_THREADSAFE_ISUPPORTS
> 
> Why threadsafe?

Already fixed.
 
> @@ +142,5 @@
> > +
> > +private:
> > +  ~nsGeolocationSettings() {}
> > +  nsGeolocationSettings(const nsGeolocationSettings &) :
> > +    mGlobalSetting(NullString()) {} // can't copy obj
> 
> What does this mean?

Part of making a singleton is disabling the ability to make a copy of it.  By making the copy constructor private, I can prevent a copy.

> ::: dom/ipc/ContentParent.cpp
> @@ +3595,5 @@
> > +    // let the the settings cache know the origin of the new listener
> > +    nsAutoCString origin;
> > +    // hint to the compiler to use the conversion operator to nsIPrincipal*
> > +    nsCOMPtr<nsIPrincipal> principal = static_cast<nsIPrincipal*>(aPrincipal);
> > +    principal->GetOrigin(getter_Copies(origin));
> 
> Why the getter_Copies? I think & should work fine.

Because the origin attribute is declared as a "string" in the interface.  That creates a getter function of type GetOrigin(char **origin);  There is an example of this already in dom/quota/QuotaManager.cpp around line 2161.  In the function QuotaManager::GetInfoFromPrincipal() it gets the origin from an nsIPrincipal the exact same way.  If my code is incorrect, then that code is incorrect as well.`1

> @@ +3598,5 @@
> > +    nsCOMPtr<nsIPrincipal> principal = static_cast<nsIPrincipal*>(aPrincipal);
> > +    principal->GetOrigin(getter_Copies(origin));
> > +    nsRefPtr<nsGeolocationSettings> gs = nsGeolocationSettings::GetGeolocationSettings();
> > +    if (gs) {
> > +     gs->PutWatchOrigin( mGeolocationWatchID, origin );
> 
> nit: no spaces after ( or before )

Fixed.

Patch is inbound.

Comment 50

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

This patch incorporates all of :jdm's feedback as well as all of the feedback from prior passes.

Comment 51

[Dave Huseby [:huseby]]

Push try: http://mzl.la/1wtpiap

Comment 52

[Dave Huseby [:huseby]]

Attachment: gaia-geoloc.patch

This patch adds getCurrentLocation capabilities to the geoloc dev_app.  I'm putting it here so I don't lose it.

Comment 53

[Josh Matthews [:jdm]]

You're right about the class table usage; I didn't realize that it's built on nsAutoPtr under the hood.

Comment 54

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

Whoops, I didn't do a full rebuild before submitting the last patch.  There was one invalid constant I just fixed.

Comment 55

[Dave Huseby [:huseby]]

New try push: http://mzl.la/1sPHSa2

Comment 56

[Josh Matthews [:jdm]]

Comment on attachment 8509645 [details] [diff] [review]
gecko-ala-master.patch

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

::: dom/geolocation/nsGeolocation.cpp
@@ +528,5 @@
> +
> +already_AddRefed<nsIDOMGeoPosition>
> +nsGeolocationRequest::AdjustedLocation(nsIDOMGeoPosition *aPosition)
> +{
> +  nsRefPtr<nsIDOMGeoPosition> pos = aPosition;

nsCOMPtr

@@ +531,5 @@
> +{
> +  nsRefPtr<nsIDOMGeoPosition> pos = aPosition;
> +  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> +    GPSLOG("child process just copying position");
> +    return nsRefPtr<nsIDOMGeoPosition>(pos).forget();

return pos.forget();
Same for all subsequent ones.

@@ +561,5 @@
> +#endif
> +    case GEO_ALA_TYPE_FIXED:
> +      GPSLOG("returning fixed location for watch ID:: %d", mWatchId);
> +      // use "now" as time stamp
> +      return SynthesizeLocation(ts, setting.GetFixedLatitude(),

Now that SynthesizeLocation doesn't null check, this method needs a null check instead.

::: dom/geolocation/nsGeolocationSettings.cpp
@@ +382,5 @@
> +    mFuzzMethod = fm;
> +  }
> +
> +  // based on the new type, we need to clean up the other settings
> +  switch(fm) {

This should switch on mFuzzMethod. Also, space before (.

::: dom/ipc/ContentParent.cpp
@@ +3635,5 @@
> +    // let the the settings cache know the origin of the new listener
> +    nsAutoCString origin;
> +    // hint to the compiler to use the conversion operator to nsIPrincipal*
> +    nsCOMPtr<nsIPrincipal> principal = static_cast<nsIPrincipal*>(aPrincipal);
> +    principal->GetOrigin(getter_Copies(origin));

We have no principal in xpcshell tests; that's what's causing the crash on try.

@@ +3638,5 @@
> +    nsCOMPtr<nsIPrincipal> principal = static_cast<nsIPrincipal*>(aPrincipal);
> +    principal->GetOrigin(getter_Copies(origin));
> +    nsRefPtr<nsGeolocationSettings> gs = nsGeolocationSettings::GetGeolocationSettings();
> +    if (gs) {
> +     gs->PutWatchOrigin(mGeolocationWatchID, origin);

nit: indentation

Comment 57

[Dave Huseby [:huseby]]

(In reply to Josh Matthews [:jdm] from comment #56)
> Comment on attachment 8509645 [details] [diff] [review]
> gecko-ala-master.patch
> 
> Review of attachment 8509645 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/geolocation/nsGeolocation.cpp
> @@ +528,5 @@
> > +
> > +already_AddRefed<nsIDOMGeoPosition>
> > +nsGeolocationRequest::AdjustedLocation(nsIDOMGeoPosition *aPosition)
> > +{
> > +  nsRefPtr<nsIDOMGeoPosition> pos = aPosition;
> 
> nsCOMPtr

Ah, yes.  It's a pointer to a COM interface.
 
> @@ +531,5 @@
> > +{
> > +  nsRefPtr<nsIDOMGeoPosition> pos = aPosition;
> > +  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> > +    GPSLOG("child process just copying position");
> > +    return nsRefPtr<nsIDOMGeoPosition>(pos).forget();
> 
> return pos.forget();
> Same for all subsequent ones.

Ooops!  Fixed.  I hate it when I make a change, get distracted, and then comeback and can't remember where I was.  Then I wind up with two different fixes at the same time.  I had intended to return pos.forget() everywhere :)

> @@ +561,5 @@
> > +#endif
> > +    case GEO_ALA_TYPE_FIXED:
> > +      GPSLOG("returning fixed location for watch ID:: %d", mWatchId);
> > +      // use "now" as time stamp
> > +      return SynthesizeLocation(ts, setting.GetFixedLatitude(),
> 
> Now that SynthesizeLocation doesn't null check, this method needs a null
> check instead.

The fixed location isn't based on the actual location.  Even if aPosition is null, we should still return the fixed location.  Did you mean the call to FuzzLocation?

> ::: dom/geolocation/nsGeolocationSettings.cpp
> @@ +382,5 @@
> > +    mFuzzMethod = fm;
> > +  }
> > +
> > +  // based on the new type, we need to clean up the other settings
> > +  switch(fm) {
> 
> This should switch on mFuzzMethod. Also, space before (.

Fixed and Fixed.

> ::: dom/ipc/ContentParent.cpp
> @@ +3635,5 @@
> > +    // let the the settings cache know the origin of the new listener
> > +    nsAutoCString origin;
> > +    // hint to the compiler to use the conversion operator to nsIPrincipal*
> > +    nsCOMPtr<nsIPrincipal> principal = static_cast<nsIPrincipal*>(aPrincipal);
> > +    principal->GetOrigin(getter_Copies(origin));
> 
> We have no principal in xpcshell tests; that's what's causing the crash on
> try.

Fixed.  I added a null check on principal before dereferencing it.  The end result is, in the xpcshell when we don't have a principal, there will be no mapping between the watch ID and the origin.  Then in nsGeolocationRequest::AdjustedLocation, when it calls nsGeolocationSettings::LookupGeolocationSetting, the watch ID won't be in the hashtable so the global settings will be returned always.  That means with the current set of xpcshell tests, we can't test the fuzzing, fixed location, or no location.  But the tests won't crash anymore either.  I'll put fixing this on the TODO list.

> @@ +3638,5 @@
> > +    nsCOMPtr<nsIPrincipal> principal = static_cast<nsIPrincipal*>(aPrincipal);
> > +    principal->GetOrigin(getter_Copies(origin));
> > +    nsRefPtr<nsGeolocationSettings> gs = nsGeolocationSettings::GetGeolocationSettings();
> > +    if (gs) {
> > +     gs->PutWatchOrigin(mGeolocationWatchID, origin);
> 
> nit: indentation

Fixed.

Comment 58

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

I fixed it again.  See comment 57 for details.

Comment 59

[Dave Huseby [:huseby]]

Try push: http://mzl.la/1z1OzLh

One of the things I fixed in this latest round should prevent the crashes in the Windows XP opt builds that we saw in the last try push.

Comment 60

[Josh Matthews [:jdm]]

Comment on attachment 8510537 [details] [diff] [review]
gecko-ala-master.patch

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

It's not just good, it's good enough!

Comment 61

[Dave Huseby [:huseby]]

Thanks!

Comment 62

[Martin Thomson [:mt:]]

Comment on attachment 8510537 [details] [diff] [review]
gecko-ala-master.patch

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

::: dom/geolocation/nsGeoGridFuzzer.cpp
@@ +61,5 @@
> +  // which grid cell does the position belong to?
> +  double belongsTo = coord / gridSizeDeg;
> +
> +  // return the center of that grid cell
> +  return (floor(belongsTo) * gridSizeDeg + ceil(belongsTo) * gridSizeDeg) / 2;

This reports an edge in some cases, not the center.

Comment 63

[Josh Matthews [:jdm]]

Oops, I didn't make clear that I totally skimmed the actual fuzzing code; someone should definitely look closely at it. I am a bear of very little brain.

Comment 64

[Dave Huseby [:huseby]]

(In reply to Martin Thomson [:mt] from comment #62)
> Comment on attachment 8510537 [details] [diff] [review]
> gecko-ala-master.patch
> 
> Review of attachment 8510537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/geolocation/nsGeoGridFuzzer.cpp
> @@ +61,5 @@
> > +  // which grid cell does the position belong to?
> > +  double belongsTo = coord / gridSizeDeg;
> > +
> > +  // return the center of that grid cell
> > +  return (floor(belongsTo) * gridSizeDeg + ceil(belongsTo) * gridSizeDeg) / 2;
> 
> This reports an edge in some cases, not the center.

So I don't believe the error you pointed out before is still valid.  Look at all of the code in the function and the callers of it:

> static double GridAlgorithm(int32_t aDistMeters,
>                             double aMetersPerDegree,
>                             double aCoordDeg)
> {
>   // convert meters to degrees
>   double gridSizeDeg = aDistMeters / aMetersPerDegree;

aDistMeters can never be zero because the function that calls GridAlgorithm checks for zero and prevents zero from every being passed as aDistMeters.

>   // check for zero aCoordDeg and make sure it is at least an epsilon away
>   // to prevent the grid algorithm from collapsing
>   double coord = aCoordDeg;
>   if (fabs(aCoordDeg) < WGS84_EPSILON) {
>     coord = sign(aCoordDeg) * WGS84_EPSILON;
>   }
> 
>   // which grid cell does the position belong to?
>   double belongsTo = coord / gridSizeDeg;
> 
>   // return the center of that grid cell
>   return (floor(belongsTo) * gridSizeDeg + ceil(belongsTo) * gridSizeDeg) / 2;
> }

You'll notice that I'm checking aCoordDeg for very small values less than WGS84_EPSILON and if it is small, it forces it to be at least the epsilon.  That means the division of coord / gridSizeDeg can never be ∞ (gridSizeDeg is never 0 or ∞) or 0 (coord is always at least +/- WGS84_EPSILON).  This avoids the previously stated problem of the grid cell center calculation from breaking down.

> static void CalculateGridCoords(int32_t aDistKm, double&  aLatDeg, double& aLonDeg)
> {
>   // a grid size of 0 is the same as precise
>   if (aDistKm == 0) {
>     return;
>   }

Check for aDistKM being zero so that we never pass 0 as aDistMeters to GridAlgorithm function.

>   double phi = (aLatDeg * M_PI) / 180;
>   double metersPerLonDegree = (2 * M_PI * LON_RADIUS(phi)) / 360;
>   aLonDeg = GridAlgorithm(aDistKm * 1000, metersPerLonDegree, aLonDeg);
>   aLatDeg = GridAlgorithm(aDistKm * 1000, METERS_PER_LAT_DEG, aLatDeg);
> }

That's it, I don't think the error still exists.

Comment 65

[Martin Thomson [:mt:]]

(In reply to Dave Huseby [:huseby] from comment #64)
> (In reply to Martin Thomson [:mt] from comment #62)
> > Comment on attachment 8510537 [details] [diff] [review]
> > gecko-ala-master.patch
> > 
> > Review of attachment 8510537 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/geolocation/nsGeoGridFuzzer.cpp
> > @@ +61,5 @@
> > > +  // which grid cell does the position belong to?
> > > +  double belongsTo = coord / gridSizeDeg;
> > > +
> > > +  // return the center of that grid cell
> > > +  return (floor(belongsTo) * gridSizeDeg + ceil(belongsTo) * gridSizeDeg) / 2;
> > 
> > This reports an edge in some cases, not the center.
> 
> So I don't believe the error you pointed out before is still valid.  Look at
> all of the code in the function and the callers of it:

Don't overthink it.  What happens when belongsTo is a discrete integer?

  return (2 * std::floor(belongsTo) + 1) * gridSizeDeg / 2;

Comment 66

[Dave Huseby [:huseby]]

Oh yeah, you're right.  Let me fix that.

Comment 67

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

I fixed the problem with the grid algorithm.  I added proper wrapping and proper edge case handling.

Comment 68

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

whoops, trailing spaces.

Comment 69

[Dave Huseby [:huseby]]

Try push: http://mzl.la/1FMuHNY

Comment 70

[Josh Matthews [:jdm]]

Comment on attachment 8511350 [details] [diff] [review]
gecko-ala-master.patch

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

No changes to the parts I previously reviewed.

Comment 71

[Martin Thomson [:mt:]]

Comment on attachment 8511350 [details] [diff] [review]
gecko-ala-master.patch

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

I think that this can go.  There are several things that I think need to be fixed up:

1. I was briefly concerned about the threading here.  It's not always clear where code is running, just by looking.  A few more MOZ_ASSERT(NS_IsMainThread()) lines would be really helpful.

2. The origin stuff is unnecessarily complex.  You have the right principal at the point that the fuzzer runs.  You can interrogate that for an origin.

3. The settings handling needs a refactor.  As it stands, it's complicated and likely to be error prone.  I'd prefer to see nsGeolocationSettings turned into a container for a single global fuzzer and any per-origin fuzzers, each of which is created based on the value of the settings and each of which is created using the same basic logic.  As it stands, per-origin fuzzers are configured differently to the main one.

Please fix the wrapping/clamping code and the error printing before landing.

::: dom/geolocation/nsGeoGridFuzzer.cpp
@@ +44,5 @@
> +  /* switch to radians */
> +  double phi = (aLatDeg * M_PI) / 180;
> +
> +  /* properly wrap the latitude */
> +  phi = atan(sin(phi) / fabs(cos(phi)));

Why use something so expensive to do something so simple:
phi %= M_PI; if (fabs(phi) > M_PI/2) phi = sign(phi) * (M_PI - fabs(phi);

@@ +54,5 @@
> +   * grid cell to find the center latitude in radians */
> +  double gridCenterPhi = gridSizeRad * floor(phi / gridSizeRad) + gridSizeRad / 2;
> +
> +  /* properly wrap it and return it in degrees */
> +  return atan(sin(gridCenterPhi) / fabs(cos(gridCenterPhi))) * (180.0 / M_PI);

As above, except here you can clamp rather than wrap:
if (fabs(gridCenterPhi) > M_PI/2) gridCenterPhi = sign(gridCenterPhi) * M_PI / 2;

@@ +69,5 @@
> +  double theta = (aLonDeg * M_PI) / 180;
> +
> +  /* properly wrap the lat/lon */
> +  phi = atan(sin(phi) / fabs(cos(phi)));
> +  theta = atan2(sin(theta), cos(theta));

See above for lat; for longitude, I find that ((theta % 2*M_PI) + M_PI) % 2*M_PI - M_PI works well enough and if you aren't concerned about values that are stupidly large, you can drop the inner modulus op.

::: dom/geolocation/nsGeolocationSettings.cpp
@@ +179,5 @@
> +  } else if (aName.EqualsASCII(GEO_ALA_APP_SETTINGS)) {
> +    GPSLOG("Unable to get value for '" GEO_ALA_APP_SETTINGS "'");
> +  } else if (aName.EqualsASCII(GEO_ALA_ALWAYS_PRECISE)) {
> +    GPSLOG("Unable to get value for '" GEO_ALA_ALWAYS_PRECISE "'");
> +  }

Hang on a minute: this could be replaced with a single line statement (maybe two, since you have to use PromiseFlatCString()).  All of these just print the value of aName.

See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings#Raw_Character_Pointers

::: dom/ipc/ContentParent.cpp
@@ +3656,5 @@
> +    }
> +    principal->GetOrigin(getter_Copies(origin));
> +    nsRefPtr<nsGeolocationSettings> gs = nsGeolocationSettings::GetGeolocationSettings();
> +    if (gs) {
> +      gs->PutWatchOrigin(mGeolocationWatchID, origin);

OK, I've looked into this.  And this isn't needed.  You have the origin on nsGeolocationRequest.

@@ +3673,5 @@
>          geo->ClearWatch(mGeolocationWatchID);
> +
> +        nsRefPtr<nsGeolocationSettings> gs = nsGeolocationSettings::GetGeolocationSettings();
> +        if (gs) {
> +          gs->RemoveWatchOrigin(mGeolocationWatchID);

Then this goes away.

@@ +3699,5 @@
>          mGeolocationWatchID = AddGeolocationListener(this, aEnable);
> +
> +        // map the new watch ID to the origin
> +        if (gs) {
> +          gs->PutWatchOrigin(mGeolocationWatchID, origin);

Ditto

Comment 72

[Josh Matthews [:jdm]]

(In reply to Martin Thomson [:mt] from comment #71)
> Comment on attachment 8511350 [details] [diff] [review]
> gecko-ala-master.patch
> 
> Review of attachment 8511350 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think that this can go.  There are several things that I think need to be
> fixed up:
> 
> 1. I was briefly concerned about the threading here.  It's not always clear
> where code is running, just by looking.  A few more
> MOZ_ASSERT(NS_IsMainThread()) lines would be really helpful.

The default assumption is main-thread only, in my experience, and none of these changes involve other threads. I'm not sure I see the value here.

> 2. The origin stuff is unnecessarily complex.  You have the right principal
> at the point that the fuzzer runs.  You can interrogate that for an origin. 
>  
> ::: dom/ipc/ContentParent.cpp
> @@ +3656,5 @@
> > +    }
> > +    principal->GetOrigin(getter_Copies(origin));
> > +    nsRefPtr<nsGeolocationSettings> gs = nsGeolocationSettings::GetGeolocationSettings();
> > +    if (gs) {
> > +      gs->PutWatchOrigin(mGeolocationWatchID, origin);
> 
> OK, I've looked into this.  And this isn't needed.  You have the origin on
> nsGeolocationRequest.
> 

That's only for content Geolocation types; when we use it from the parent process we don't have a window and principal.

Comment 73

[Josh Matthews [:jdm]]

Granted, we could modify nsIDOMGeoGeolocation to include an explicit method to override the default mPrincipal with one that's passed in, but one of the original restrictions Dave specified was no interface changes given the timeframe for these changes.

Comment 74

[Dave Huseby [:huseby]]

(In reply to Martin Thomson [:mt] from comment #71)
> Comment on attachment 8511350 [details] [diff] [review]
> gecko-ala-master.patch
> 
> Review of attachment 8511350 [details] [diff] [review]:
> -----------------------------------------------------------------
> Please fix the wrapping/clamping code and the error printing before landing.
> 
> ::: dom/geolocation/nsGeoGridFuzzer.cpp
> @@ +44,5 @@
> > +  /* switch to radians */
> > +  double phi = (aLatDeg * M_PI) / 180;
> > +
> > +  /* properly wrap the latitude */
> > +  phi = atan(sin(phi) / fabs(cos(phi)));
> 
> Why use something so expensive to do something so simple:
> phi %= M_PI; if (fabs(phi) > M_PI/2) phi = sign(phi) * (M_PI - fabs(phi);

This doesn't calculate the correct value.  See the following test code:

> #include <stdio.h>
> #include <math.h>
> 
> #define sign(f)  (((f) < 0) ? -1 : 1)
> 
> double wrap1(double d) {
>   double t = d * (M_PI / 180);
>   t = fmod(t, M_PI);
>   if (fabs(t) > M_PI/2) {
>     t = sign(t) * (M_PI - fabs(t));
>   }
>   return t * (180.0 / M_PI);
> }
> 
> double wrap2(double d) {
>   double t = d * (M_PI / 180);
>   t = atan(sin(t) / fabs(cos(t)));
>   return t * (180.0 / M_PI);
> }
> 
> int main(int argc, char** argv) {
>   int i;
>   for (i = -720; i < 720; i += 45) {
>     printf("%f:  %f  %f\n", (double)i, wrap1((double)i), wrap2((double)i));
>   }
>   return 1;
> }

This prints out:

> -720.000000:  -0.000000  0.000000
> -675.000000:  -45.000000  45.000000
> -630.000000:  -90.000000  90.000000
> -585.000000:  -45.000000  45.000000
> -540.000000:  -0.000000  -0.000000
> -495.000000:  -45.000000  -45.000000
> -450.000000:  -90.000000  -90.000000
> -405.000000:  -45.000000  -45.000000
> -360.000000:  -0.000000  0.000000
> -315.000000:  -45.000000  45.000000
> -270.000000:  -90.000000  90.000000
> -225.000000:  -45.000000  45.000000
> -180.000000:  -0.000000  -0.000000
> -135.000000:  -45.000000  -45.000000
> -90.000000:  -90.000000  -90.000000
> -45.000000:  -45.000000  -45.000000
> 0.000000:  0.000000  0.000000
> 45.000000:  45.000000  45.000000
> 90.000000:  90.000000  90.000000
> 135.000000:  45.000000  45.000000
> 180.000000:  0.000000  0.000000
> 225.000000:  45.000000  -45.000000
> 270.000000:  90.000000  -90.000000
> 315.000000:  45.000000  -45.000000
> 360.000000:  0.000000  -0.000000
> 405.000000:  45.000000  45.000000
> 450.000000:  90.000000  90.000000
> 495.000000:  45.000000  45.000000
> 540.000000:  0.000000  0.000000
> 585.000000:  45.000000  -45.000000
> 630.000000:  90.000000  -90.000000
> 675.000000:  45.000000  -45.000000

You'll see that your wrapping code gets the sign wrong in some quadrants.  The numbers from your method are the middle column and the numbers from my method are in the right column.  I know my numbers are correct.

You're longitude code is correct, so I'll make that change.

Comment 75

[Dave Huseby [:huseby]]

(In reply to Martin Thomson [:mt] from comment #71)
> Comment on attachment 8511350 [details] [diff] [review]
> gecko-ala-master.patch
> 
> Review of attachment 8511350 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/geolocation/nsGeolocationSettings.cpp
> @@ +179,5 @@
> > +  } else if (aName.EqualsASCII(GEO_ALA_APP_SETTINGS)) {
> > +    GPSLOG("Unable to get value for '" GEO_ALA_APP_SETTINGS "'");
> > +  } else if (aName.EqualsASCII(GEO_ALA_ALWAYS_PRECISE)) {
> > +    GPSLOG("Unable to get value for '" GEO_ALA_ALWAYS_PRECISE "'");
> > +  }
> 
> Hang on a minute: this could be replaced with a single line statement (maybe
> two, since you have to use PromiseFlatCString()).  All of these just print
> the value of aName.
> 
> See
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/
> Internal_strings#Raw_Character_Pointers

I'm not going to make this change because those are GPSLOG statements which are empty statements {;} when the GPS logging isn't enabled--which is the default--so the compiler will optimize all of this code away in that case.

Comment 76

[Dave Huseby [:huseby]]

(In reply to Josh Matthews [:jdm] from comment #73)
> Granted, we could modify nsIDOMGeoGeolocation to include an explicit method
> to override the default mPrincipal with one that's passed in, but one of the
> original restrictions Dave specified was no interface changes given the
> timeframe for these changes.

+1 I'm pretty sure that is the way I will go.  But not in this patch.

Comment 77

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

I fixed the longitude wrapping to use the faster method.

Comment 78

[Dave Huseby [:huseby]]

try push: http://mzl.la/1z5ORRm

Comment 79

[Dave Huseby [:huseby]]

You know what scratch that.  I'm going to use my original method for doing the longitude wrapping.  The method Martin proposes calculates numbers that are technically correct (-270 instead of 90), it doesn't limit the values to between -180 and 180 which is what we need for producing values for GPS coords.  I wrote a test program to verify what I calculated in my head:

> #include <stdio.h>
> #include <math.h>
> 
> #define sign(f)  (((f) < 0) ? -1 : 1)
> 
> double wrap1(double d) {
>   double t = d * (M_PI / 180);
>   t = fmod((fmod(t, 2*M_PI) + M_PI), 2*M_PI) - M_PI;
>   return t * (180.0 / M_PI);
> }
> 
> double wrap2(double d) {
>   double t = d * (M_PI / 180);
>   t = atan2(sin(t), cos(t));
>   return t * (180.0 / M_PI);
> }
> 
> int main(int argc, char** argv) {
>   int i;
>   for (i = -720; i < 720; i += 45) {
>     printf("%f:  %f  %f\n", (double)i, wrap1((double)i), wrap2((double)i));
>   }
>   return 1;
> }

This verifies that I am correct.  The output looks like:

> -720.000000:  0.000000  0.000000
> -675.000000:  -315.000000  45.000000
> -630.000000:  -270.000000  90.000000
> -585.000000:  -225.000000  135.000000
> -540.000000:  -180.000000  -180.000000
> -495.000000:  -135.000000  -135.000000
> -450.000000:  -90.000000  -90.000000
> -405.000000:  -45.000000  -45.000000
> -360.000000:  0.000000  0.000000
> -315.000000:  -315.000000  45.000000
> -270.000000:  -270.000000  90.000000
> -225.000000:  -225.000000  135.000000
> -180.000000:  -180.000000  -180.000000
> -135.000000:  -135.000000  -135.000000
> -90.000000:  -90.000000  -90.000000
> -45.000000:  -45.000000  -45.000000
> 0.000000:  0.000000  0.000000
> 45.000000:  45.000000  45.000000
> 90.000000:  90.000000  90.000000
> 135.000000:  135.000000  135.000000
> 180.000000:  -180.000000  180.000000
> 225.000000:  -135.000000  -135.000000
> 270.000000:  -90.000000  -90.000000
> 315.000000:  -45.000000  -45.000000
> 360.000000:  0.000000  -0.000000
> 405.000000:  45.000000  45.000000
> 450.000000:  90.000000  90.000000
> 495.000000:  135.000000  135.000000
> 540.000000:  -180.000000  180.000000
> 585.000000:  -135.000000  -135.000000
> 630.000000:  -90.000000  -90.000000
> 675.000000:  -45.000000  -45.000000

As you can see, if the longitude measurement is -225, Martin's method calculates -225 instead of 135.  The goal of the wrapping code is to give the correct value between -180 and 180.

And keeping this in perspective, this code only gets executed when we get updates from the GPS/AGPS provider.  That only happens once or twice per second.  I think trying to squeeze every last ounce of speed out of this code is unwarranted.  I want this to be accurate and normalized.

I am removing the longitude change now and will resubmit in a few minutes as is.  There are no more changes.  Considering the circumstances, I think this code is done.  Please r+ it.

Comment 80

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

Sorry for the spam.  I reverted the change to the longitude calculation back to the way I had it.  I want the numbers to be between -180 and 180.

Comment 81

[Dave Huseby [:huseby]]

try push: http://mzl.la/1uSpSKo

Comment 82

[Josh Matthews [:jdm]]

Comment on attachment 8511424 [details] [diff] [review]
gecko-ala-master.patch

My last r+ still stands.

Comment 83

[Martin Thomson [:mt:]]

Comment on attachment 8511424 [details] [diff] [review]
gecko-ala-master.patch

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

I just got a few things wrong.  Sorry, I didn't test my code.  I was just noting that atan/tan isn't necessary.

Comment 84

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9742b31c634a

Comment 85

[Carsten Book [:Tomcat]]

backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/c0e72ece8901 for bustage like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=710195&repo=b2g-inbound

Comment 86

[Wilfred Mathanaraj [:WDM]]

paul - do you know what this means? is there more to do before checking in?

Comment 87

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Wilfred Mathanaraj [:WDM] from comment #86)
> paul - do you know what this means? is there more to do before checking in?

Looks like the code broke on inbound, on debug build maybe? I'm not sure why that would show up there only.  The error looks reasonable enough, looks like this code missing is missing the following line:

using namespace mozilla::dom;

That's a guess though, and I don't understand why it worked on Try.

Comment 88

[Dave Huseby [:huseby]]

I'll fix this up and re-sumbit it.

Comment 89

[Dave Huseby [:huseby]]

Attachment: gecko-ala-master.patch

fixes the bustage.  try: http://mzl.la/1rXGakt

Comment 90

[Dave Huseby [:huseby]]

Attachment: gecko-ala-v2.1.patch

the master patch, refactored for v2.1

Comment 91

[Dave Huseby [:huseby]]

So, this landed and then was backed out for bustage.  I fixed the bustage, re-ran a try pass.  What else do I need to do to get this landed again?

Comment 92

[Ryan VanderMeulen [:RyanVM]]

Me or one of my compatriots having the time to do it. You requested checkin late in the day on Friday and not even one full business day later you're complaining?

Comment 93

[Dave Huseby [:huseby]]

I'm sorry for giving you the wrong impression.  I didn't think I was complaining.  I was asking if there was anything else I needed to do to get this landed again.  I've never dealt with a backed out patch before and didn't know if there was any other hoops I needed to jump through before it could get re-landed.

Comment 94

[Ryan VanderMeulen [:RyanVM]]

Obviously it was a bit of a misunderstanding. No worries :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b1f0a4afb91

Comment 95

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0b1f0a4afb91

Comment 96

[Dave Huseby [:huseby]]

Attachment: gecko-ala-v2.1.patch

an updated v2.1 patch that includes the fixes from master.

Comment 97

[:garvan]

1) What is the use-case for this feature? How would we describe the user who this feature is targeted at? Are they a user who is concerned for their safety/security if their location is revealed? 

2) What claims being made about personal security/safety by landing this feature?

Comment 98

[Doug Turner (:dougt)]

Garvan's concerns were not addressed.  I think he's right and implementing this was the wrong thing to do.

Comment 99

[Dave Huseby [:huseby]]

(In reply to Garvan Keeley [:garvank] from comment #97)
> 1) What is the use-case for this feature? How would we describe the user who
> this feature is targeted at? Are they a user who is concerned for their
> safety/security if their location is revealed? 
> 
> 2) What claims being made about personal security/safety by landing this
> feature?

The use case for this is to give the user the ability to control how geolocation data is reported to apps.  Users can choose from "no location", "fixed location", "accurate location", and "approximate location".  They can set a global default and per-app exceptions.

IIRC, Doug's only objection is to the "approximate location" feature and not with the "no location", "fixed location", or "accurate location" features.  You can read more about "approximate location" here: http://www.ieee-security.org/TC/SP2014/posters/PIEKA.pdf or talk to Martin Thomson about it.

For v3 I am going to refactor all of this and remove the fuzzing.

Comment 100

[:garvan]

Thanks Dave, I am (somewhat/passingly) familiar with the background, but I can't recall reading that doc before. Anyway, it seems like approx. location is out for now. If the thread is to continue at some point, the understanding I was looking to reach is that, as a user-facing feature, what is the user-facing use case? 

i.e. "Approx. location is for a user who is concerned about XXX."

Whether XXX _is_ or _is not_ "safety/security" affects the next stage of discussion. In either case, there is the issue of misuse/misunderstanding of the feature; in the former case, because the security is not absolute, in the latter, because the user assumes the feature does provide security.

Comment 101

[Dave Huseby [:huseby]]

As I see it now, there is a solid case for the "no location", "fixed location", and "accurate location" modes of geolocation operation.  I also think there is a significant case to be made for an API that allows add-ons to offer other modes of operation.  The add-ons would receive the accurate location data, modify it somehow, and then return the modified data that is then passed on to the JS consumer of the data.  If we did that, then an add-on for location "fuzzing" could be made.

Comment 102

[BMO Automation]

Firefox OS is not being worked on