1154435 - FxOS Geo Stumbling for Mozilla Location Service

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[:garvan]

Here is the planning doc we can collaborate on:
https://etherpad.mozilla.org/W0PvfCsh3I

We can add info there or in this bug.

Comment 2

[Alphan Chen [:alchen]]

Here is current plan.
I will divide the work into 2 phase.

Phase 1:
1. GPS location arrives (1 stumble observation per min, 50KB per day, limited to 250 KB on disk)
   - WiFi Scan & get Cell info.
   - Convert [location, CellTowever, WiFiAp] into JSON
2. An key for enabling and disabling
3. Date : 5/5 ~ 5/22 (3 weeks)
4. Checkpoint : 5/25


Phase 2:
1. Upload
   - when the JSON file reaches (~50KB) // up to 250KB
   - when the JSON file is (~1day) old, // up to 5 days
        - Delete any data older than 5 days
        - convert to gzip
        - Delete uploaded file
2. Date : 5/26 ~ 6/12 (3 weeks)
3. Checkpoint : 6/15

Comment 3

[Alphan Chen [:alchen]]

Update the progress of phase 1.

- Phase 1: Working on dump stumb information to JSON
  = Refine the OpenTempFile in nsDumpUtils.cpp to let the function more general and powerful.(Done)
  = Design the policy and flow of dumping and manage json file.
  = Implementation Progress -60%

Comment 4

[Alphan Chen [:alchen]]

Hi Garvan,
I'm implementing this feature.
I have some questions about some entries.


In geosubmit-upload-format as below:
http://mozilla-ichnaea.readthedocs.org/en/latest/api/index.html#geosubmit-upload-format

1. How many cell towers do we need?
   Now I only dump the cell we current connect.
2. In this cell tower information, 
   - What is age?
   - What is asu? (calculate from SignalStrength?)
   - What is timingAdvance? (We only have this attribute in ltecellinfo)

Comment 5

[Alphan Chen [:alchen]]

Attachment: Phase 1- WIP (0515)

Update current status.

[File Path]
The json files will be located at /data/local/tmp/stumbler.


[Json files management]
illustration : http://slides.com/alphan/deck-2-3#/1/1

I limit the size of json file less than 12.5 KB.
The maximum number of json file is 4. (feeding-1.json ~ feeding-4.json)
When feeding-1.json is more than 12.5 KB. We will use feeding-2.json to dump the information.(feeding-2.json is over 12.5 KB -> use feeding-3.json, etc.)

If 4 json files are all more than 12.5KB, we will remove the first one and rename the other json files. In the end, use the new created json file (feeding-4.json)



The following is what I achieve now.
{"items": [ 
  {
    "latitude":25.032658,
    "longitude":121.566307,
    "accuracy":100.000000,
    "altitude":0.000000,
    "altitudeAccuracy":0.000000,
    "timestamp":1431670003815,,
    "heading":NaN,
    "speed":NaN,
    "cellTowers": [
      "radioType":gsm,
      "cellId":9111196,
      "locationAreaCode":11115,
      "mobileCountryCode":466,
      "mobileNetworkCode":97,
      "signalStrength":0,
    ],
    "wifiAccessPoints": [
    ]
  }
]}

Comment 6

[Alphan Chen [:alchen]]

In order to easy testing, current patch is hacking in "GonkGPSGeolocationProvider::NetworkLocationUpdate::Update".

I will trigger the runnable in GonkGPSGeolocationProvider::LocationCallback in the final patch.

(In reply to Alphan Chen [:alchen] from comment #5)
> Created attachment 8606129 [details] [diff] [review]
> Phase 1- WIP (0515)
> 
> Update current status.
> 
> [File Path]
> The json files will be located at /data/local/tmp/stumbler.
> 
> 
> [Json files management]
> illustration : http://slides.com/alphan/deck-2-3#/1/1
> 
> I limit the size of json file less than 12.5 KB.
> The maximum number of json file is 4. (feeding-1.json ~ feeding-4.json)
> When feeding-1.json is more than 12.5 KB. We will use feeding-2.json to dump
> the information.(feeding-2.json is over 12.5 KB -> use feeding-3.json, etc.)
> 
> If 4 json files are all more than 12.5KB, we will remove the first one and
> rename the other json files. In the end, use the new created json file
> (feeding-4.json)
> 
> 
> 
> The following is what I achieve now.
> {"items": [ 
>   {
>     "latitude":25.032658,
>     "longitude":121.566307,
>     "accuracy":100.000000,
>     "altitude":0.000000,
>     "altitudeAccuracy":0.000000,
>     "timestamp":1431670003815,,
>     "heading":NaN,
>     "speed":NaN,
>     "cellTowers": [
>       "radioType":gsm,
>       "cellId":9111196,
>       "locationAreaCode":11115,
>       "mobileCountryCode":466,
>       "mobileNetworkCode":97,
>       "signalStrength":0,
>     ],
>     "wifiAccessPoints": [
>     ]
>   }
> ]}

Comment 7

[Hanno Schlichting]

Hanno from the service side here.

(In reply to Alphan Chen [:alchen] from comment #4)
> In geosubmit-upload-format as below:
> http://mozilla-ichnaea.readthedocs.org/en/latest/api/index.html#geosubmit-
> upload-format
> 
> 1. How many cell towers do we need?
>    Now I only dump the cell we current connect.

We'd really like to get the neighboring cell networks at the same time. The geolocation code in FxOS doesn't use those yet, though it should (open bug #1032865). The API to access all cells was implemented in bug #1032858 and is called "getCellInfoList". There is also an earlier less complete API called "getNeighboringCellIds" (bug #1010356), ignore that one.

getCellInfoList returns CellInfo objects depending on the radio type of the connection. So for each radio type the cell info fields need to be mapped to the API fields:

nsIGsmCellInfo

radioType -> "gsm"
mcc -> mobileCountryCode
mnc -> mobileNetworkCode
lac -> locationAreaCode
cid -> cellId
signalStrength -> asu (cellinfo API returns 0-31, not negative dBm numbers)

nsIWcdmaCellInfo
radioType -> "wcdma"
mcc -> mobileCountryCode
mnc -> mobileNetworkCode
lac -> locationAreaCode
cid -> cellId
psc -> psc
signalStrength -> asu (same as GSM)

nsILteCellInfo
radioType -> "lte"
mcc -> mobileCountryCode
mnc -> mobileNetworkCode
tac -> locationAreaCode (cellinfo calls this tac and not lac)
cid -> cellId
pcid -> psc (cellinfo calls this pcid instead of psc here)
timingAdvance -> timingAdvance
signalStrength -> asu (same as GSM)
rsrp * -1 -> signalStrength

For signalStrength we want negative dBm numbers, so -44 to -150, the cellinfo API gives a positive integer.

nsICdmaCellInfo
radioType -> "cdma"
(no mobileCountryCode field)
systemId -> mobileNetworkCode
networkId -> locationAreaCode
baseStationId -> cellId
(evdoDbm or cdmaDbm) * -1 -> signalStrength

For signalStrength we want the actual RSSI value, typically a value of -75 to -100. nsICdmaCellInfo has both a cdmaDbm and evdoDbm field, which both are the RSSI value multiplied by -1.

So we'd need to check if evdoDbm is set, if so use it, otherwise see if cdmaDbm is set, if so use that or if neither is set, ignore it. Then if we have a value send the value multiplied by -1 in the signalStrength field.

> 2. In this cell tower information, 
>    - What is age?
>    - What is asu? (calculate from SignalStrength?)
>    - What is timingAdvance? (We only have this attribute in ltecellinfo)

Ignore age, in theory it could express a time difference between the GPS, cell and wifi scans. Say one of them was done five seconds ago, vs. another one from "now", but we don't use that anywhere.

I hope I clarified where to use ASU and signalStrength in the above mappings. ASU is a positive integer, typically 0-31, for signalStrength we always want negative dBm numbers. We have both fields in the API, so depending on what the client has it can sent either one or the other (or both). But I wouldn't do any calculation on the client side to convert from one unit to the other.

timingAdvance is in theory also defined for GSM, but we can't access it there.

In general, all fields are optional. If some field is not available, just omit it. The same is true for any sort of "unknown" special values. If those are well defined in the API, please just omit sending the field, instead of forwarding the special value to the service. Almost all of the API's have some kind of unknown value like INT_MAX, NaN or 99.

Comment 8

[Hanno Schlichting]

Regarding getCellInfoList, one thing I'm not sure about, is whether or not that API returns information from the currently active "serving" cell, or if that still has to come from the API you use now.

If you can distinguish between the active/serving cell and neighboring/additional cells, I'd also appreciate it, if you could set an additional field for each cell called "serving" and set it to 1 for the active/serving cell and to 0 for neighboring cells. As usual if we can't get that information, just don't send the field.

Comment 9

[:garvan]

> In geosubmit-upload-format as below:
> http://mozilla-ichnaea.readthedocs.org/en/latest/api/index.html#geosubmit-
> upload-format
> 
> 1. How many cell towers do we need?
> 2. In this cell tower information, 

Hi Alphan, I'll clear the ni for these as Hanno is addressing them, and I'll look at your slide deck now and see if I have questions.

Comment 10

[:garvan]

> [Json files management]
> illustration : http://slides.com/alphan/deck-2-3#/1/1
> 
> I limit the size of json file less than 12.5 KB.
> The maximum number of json file is 4. (feeding-1.json ~ feeding-4.json)
> When feeding-1.json is more than 12.5 KB. We will use feeding-2.json to dump
> the information.(feeding-2.json is over 12.5 KB -> use feeding-3.json, etc.)
> 
> If 4 json files are all more than 12.5KB, we will remove the first one and
> rename the other json files. In the end, use the new created json file
> (feeding-4.json)

Sounds reasonable. On the java stumbler library, the approach is different because we had different considerations. On Android, app kills are frequent (for instance user swipes away app from the recent app list), and file corruption on disk writes is a consideration. We write to many small files (and if any are corrupted, they are thrown away) instead of appending to a larger file. Regardless, FxOS is system-level stumbling so I think there is no problem here.

Comment 11

[:garvan]

Regarding the disk writes, this writes to disk on every GPS (potentially 1 Hz). 
Is there a power consumption concern with this? Or possibly I/O contention that might affect performance?
Would an in-memory buffer that gets periodically flushed to disk be appropriate here?
In the java stumbler lib it holds stumbles in a memory buffer then flushes the memory buffer to disk after ~3 mins. The java lib has lots of other buffer management that goes on to make sure data isn't lost (again due to the kill freq of android apps). If you decide on adding a memory buffer, the occasional loss of some in-memory stumbling data when Gecko is shut down or killed isn't a concern.

Comment 12

[:garvan]

Comment on attachment 8606129 [details] [diff] [review]
Phase 1- WIP (0515)

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +819,5 @@
> +              }
> +              number = MAXFILENUM;
> +            }
> +          } else { // fileSize <= MAXFILESIZE
> +            nsCOMPtr<nsIFileOutputStream> ostream =

I would have thought file I/O on the main event loop thread is to be avoided.

@@ +1136,5 @@
>      }
>    }
>  
> +  //NS_DispatchToMainThread(new DumpStumblerFeedingEvent(lat, lon, acc));
> +  NS_DispatchToMainThread(new DumpStumblerFeedingEvent(position));

I didn't expect to see NS_DispatchToMainThread being used (as it would block main thread to run this), I may be missing a detail as to why, could you explain it briefly?

Comment 13

[:garvan]

Comment on attachment 8606129 [details] [diff] [review]
Phase 1- WIP (0515)

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +1136,5 @@
>      }
>    }
>  
> +  //NS_DispatchToMainThread(new DumpStumblerFeedingEvent(lat, lon, acc));
> +  NS_DispatchToMainThread(new DumpStumblerFeedingEvent(position));

I shouldn't nit on a WIP :) but I think this passes a zero-refcount xpcom object, which is to be avoided AFAIK. Assigning it to nsRefPtr would ensure it has a refcount of one. (Hmm .... just checked some more code, I see zero-refcount passing in other places, I am sure I was told to avoid this though)

Comment 14

[Alphan Chen [:alchen]]

Comment on attachment 8606129 [details] [diff] [review]
Phase 1- WIP (0515)

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

Hi Garvan,
thanks for your feedback.
I addressed my reply in-line.

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +1137,5 @@
>    }
>  
> +  //NS_DispatchToMainThread(new DumpStumblerFeedingEvent(lat, lon, acc));
> +  NS_DispatchToMainThread(new DumpStumblerFeedingEvent(position));
> +

Since there are some permission restriction for RIL API, that's why I dispatch the runnable to main thread to avoid some annoyances in this WIP patch.
I will put the runnable with file I/O to IO thread in next patch.


For zero-refount xpcom object, I just follow the other code in this file to dispatch a runnable.
e.g. https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp#132
But I think you are right, I will rewrite this part. :)

Comment 15

[Alphan Chen [:alchen]]

Hi Hanno,
Very well explained.
Thanks.

(In reply to Hanno Schlichting [:hannosch] from comment #7)
> Hanno from the service side here.
> 
> (In reply to Alphan Chen [:alchen] from comment #4)
> > In geosubmit-upload-format as below:
> > http://mozilla-ichnaea.readthedocs.org/en/latest/api/index.html#geosubmit-
> > upload-format
> > 
> > 1. How many cell towers do we need?
> >    Now I only dump the cell we current connect.
> In general, all fields are optional. If some field is not available, just
> omit it. The same is true for any sort of "unknown" special values. If those
> are well defined in the API, please just omit sending the field, instead of
> forwarding the special value to the service. Almost all of the API's have
> some kind of unknown value like INT_MAX, NaN or 99.

Comment 16

[Alphan Chen [:alchen]]

Attachment: feeding-4.json

Update current status.

1. Move file I/O into IO thread.

2.
I use "GetCellInfoList" to get neighboring cell tower info.
This attach is current output file.
In cellTowers info, I also add 'serving' entry for comment 8 request.

    "cellTowers": [
      {
      "radioType":"gsm" ,
      "serving":1,
      "cellId":11032,
      "locationAreaCode":11701,
      "mobileCountryCode":466,
      "mobileNetworkCode":97,
      "asu":22,
      },
      {
      "radioType":"gsm" ,
      "serving":0,
      "cellId":11033,
      "locationAreaCode":11701,
      "mobileCountryCode":466,
      "mobileNetworkCode":97,
      "asu":17,
      }

Comment 17

[Alphan Chen [:alchen]]

By the way, the functionality of "GetCellInfoList" of latest codebase is broken.
Bug 1166174 is talking about this.
I got patch from RIL team to make it work.

Comment 18

[Alphan Chen [:alchen]]

However, I still found some problem when using GetCellInfoList.
I will track the problem with RIL team.

Comment 19

[Alphan Chen [:alchen]]

Hi Garvan,
If I get the same location(or nearby location) shortly(e.g. 2 min), do we still need to produce a stumble?
Do we need a location change delta?
What is the policy in Firefox browser on Android?

Comment 20

[Hanno Schlichting]

(In reply to Alphan Chen [:alchen] from comment #16)
> 2.
> I use "GetCellInfoList" to get neighboring cell tower info.
> This attach is current output file.
> In cellTowers info, I also add 'serving' entry for comment 8 request.

Great! Looks good to me.

Comment 21

[:garvan]

(In reply to Alphan Chen [:alchen] from comment #19)
> Hi Garvan,
> If I get the same location(or nearby location) shortly(e.g. 2 min), do we
> still need to produce a stumble?

Thanks for thinking of this, critical info that I forgot to put in the doc!

Consecutive GPS locations must be 30 meters and 3 minutes apart:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/scanners/GPSScanner.java#207

Comment 22

[Alphan Chen [:alchen]]

Hi Garvan and Hanno,
Here is the questions for Wifi AP part.

1. Do we need serving entry here?
2. How many Wifi AP info we would like to have in one item?

   In my experiment,
   1. 60 Wifi AP from scanning
   2. dump entries : serving, macAddress, signalStrength
   Two items would cost more than 10 KB.
   I think it is too big, isn't it?

Comment 23

[Alphan Chen [:alchen]]

Attachment: (0526) stumble with wifi info

Here is the stumble with wifi info.
The json is more than 10KB with only two items.

Comment 24

[Alphan Chen [:alchen]]

Attachment: (0526) stumble with wifi info

Update latest version.

Comment 25

[Hanno Schlichting]

(In reply to Alphan Chen [:alchen] from comment #22)
> 1. Do we need serving entry here?

For wifi we don't need the serving bit, that's only really useful for cell entries. Just remove it from the wifi part.

> 2. How many Wifi AP info we would like to have in one item?
> 
>    In my experiment,
>    1. 60 Wifi AP from scanning
>    2. dump entries : serving, macAddress, signalStrength
>    Two items would cost more than 10 KB.
>    I think it is too big, isn't it?

I think we limit the number of wifis in the Fennec stumbler to some number, but Garvan would know what that is. I think we had used either 50 or 100 in the past, but I can be wrong. If we want to limit it, we could gather all of them, sort them by signal strength (-40 is better than -60) and only use the Top-N after the sorting.

Another small trick is to remove the separators characters from the mac address, so strip out either '-' or ':', so you just end up with "macAddress": "abcdef123456".

But the real space saver is using gzip on this data. The geosubmit endpoint accepts all of this data in gzip, if you set the HTTP header to "Content-Encoding: gzip". Depending on when and how things get stored to the local disk, you might be able to store data as gzip locally as well, and at transmission time, just stream the local file into the HTTPS connection.

Garvan knows more about this :)

Comment 26

[Victor Ng [:vng] [:crankycoder]]

Maximum wifi per location is currently capped at 200 BSSIDs.  

Looking at the JSON data - it looks like you need to add quotes around value portion of 'macAddress'.

I wouldn't worry about the byte size - as Hanno has pointed out - the big saving here is to use gzip.

Comment 27

[:garvan]

Yes the android lib gzips the saved data. In this case 10KB gzips to .85KB.
The android lib doesn't use file append, but gzip is an appendable compression format IIRC, so your existing method should work.

https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/stumblerthread/datahandling/DataStorageManager.java#304

Comment 28

[Alphan Chen [:alchen]]

Attachment: (0527) stumble

Hi all,
I refine the output again. (including the suggestion in comment 25 and the correction in comment 26)
This content can be parsed by normal json parser.
Please review the output format again.

Comment 29

[Alphan Chen [:alchen]]

Attachment: (0527) stumble content parsed by json parser

Here is the parsing result of attachment 8610983 [details].

Comment 30

[Alphan Chen [:alchen]]

One more question.
If the radiotype is cdma, what information do we need in stunble?

Here is what we can get.
https://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/interfaces/nsICellInfo.idl#136

Comment 31

[Hanno Schlichting]

The new JSON sample attachments look good to me.

(In reply to Alphan Chen [:alchen] from comment #30)
> One more question.
> If the radiotype is cdma, what information do we need in stunble?
> 
> Here is what we can get.
> https://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/
> interfaces/nsICellInfo.idl#136

I hid that information in comment #7 already, the relevant part was:

> nsICdmaCellInfo
> radioType -> "cdma"
> (no mobileCountryCode field)
> systemId -> mobileNetworkCode
> networkId -> locationAreaCode
> baseStationId -> cellId
> (evdoDbm or cdmaDbm) * -1 -> signalStrength
> 
> For signalStrength we want the actual RSSI value, typically a value of -75 to -100. nsICdmaCellInfo has both a cdmaDbm and evdoDbm field, which both are the RSSI value multiplied by -1.
> 
> So we'd need to check if evdoDbm is set, if so use it, otherwise see if cdmaDbm is set, if so use that or if neither is set, ignore it. Then if we have a value send the value multiplied by -1 in the signalStrength field.

So it would be something like:

"cellTowers": [    
    {
     "radioType": "cdma",
     "mobileNetworkCode": <systemId>,
     "locationAreaCode": <networkId>,
     "cellId": <baseStationId>,
     "signalStrength": <(evdoDbm or cdmaDbm) * -1>
    }
]

The signalStrength part is the messy bit. If evdoDbm is set (neither null nor MAX_INT) use it, otherwise use cdmaDbm if it isn't null or MAX_INT. If this results in a value, multiply by -1 and send it as signalStrength. If it doesn't result in a value, skip the signalStrength field.

Comment 32

[:garvan]

I'll clear the ni as I think hanno covered the question about json structure.

Comment 33

[Alphan Chen [:alchen]]

Attachment: part 1. WIP (0528)

Finish:
1. Dump stumble in IO thread
2. Stumble includes LocationInfo, 
                                CellTowerInfo (by getCellInfoList),
                                WifiAPInfo(by GetWifiScanResults)
3. Dump policy :
        - MaxFile Num : 4, MaxFileSize : 12.5KB
        - 1 minute 1 stumble (now set to 3 sec for testing)

TODO:
1. Add a key for on/off
2. compression the feeding files into gz format
3. Upload & Upload policy
   PS: If we want to upload the latest file(less than 12.5KB),
       we need to add "]}" in the latest file.

Comment 34

[Alphan Chen [:alchen]]

(In reply to Garvan Keeley [:garvank] from comment #13)
> Comment on attachment 8606129 [details] [diff] [review]
> Phase 1- WIP (0515)
> 
> Review of attachment 8606129 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
> @@ +1136,5 @@
> >      }
> >    }
> >  
> > +  //NS_DispatchToMainThread(new DumpStumblerFeedingEvent(lat, lon, acc));
> > +  NS_DispatchToMainThread(new DumpStumblerFeedingEvent(position));
> 
> I shouldn't nit on a WIP :) but I think this passes a zero-refcount xpcom
> object, which is to be avoided AFAIK. Assigning it to nsRefPtr would ensure
> it has a refcount of one. (Hmm .... just checked some more code, I see
> zero-refcount passing in other places, I am sure I was told to avoid this
> though)

For this topic, we don't need to do the reference counting by ourself.
The API will delete the object after the runnable finish.
Here is the similar case when doing task in IO thread.
https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/message_loop.cc#362

Comment 35

[Alphan Chen [:alchen]]

Attachment: Refine nsDumpUtils::OpenTempFile to make this function more flexible

Hi Dave,
could you review this patch?
It is created by me when doing "status reporter".

In this patch, I add one more mode into this function.
Originally, the function will open a unique file even when the file already exists. (try variations on the leaf name. e.g. 1.txt exists, use 1-1.txt)

I add one more parameter to switch two mode.
0. create : don't create a new file if it already exists.
1. createUnique (default option) : as before.

Comment 36

[Dave Hylands [:dhylands]]

Comment on attachment 8612174 [details] [diff] [review]
Refine nsDumpUtils::OpenTempFile to make this function more flexible

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

Is there ever a situation that don't want a unique file? Why not just make it always create unique files?

::: xpcom/base/nsDumpUtils.h
@@ +193,5 @@
>     */
>    static nsresult OpenTempFile(const nsACString& aFilename,
>                                 nsIFile** aFile,
> +                               const nsACString& aFoldername = EmptyCString(),
> +                               bool CreateUnique = 1);

nit: 1 isn't a bool

I also generally prefer to use an enum in these types of situations rather than a bool.

The bool looks great when looking at the function, but when looking at the calling code, you can't tell what: OpenTempFile(filename, &myFile, folderName, true) means. If you use an enumeration, then knowing what: OpenTempFile(filename, &myFile, folderName, CREATE_UNIQUE) it's much more obvious what's going on.

Comment 37

[Alphan Chen [:alchen]]

(In reply to Dave Hylands [:dhylands] from comment #36)
> Comment on attachment 8612174 [details] [diff] [review]
> Refine nsDumpUtils::OpenTempFile to make this function more flexible
> 
> Review of attachment 8612174 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there ever a situation that don't want a unique file? Why not just make
> it always create unique files?
> 

In this case, I want to create a new file if it isn't exist.
However, if the file exists, I will append the data in this file without creating a new one.

> ::: xpcom/base/nsDumpUtils.h
> @@ +193,5 @@
> >     */
> >    static nsresult OpenTempFile(const nsACString& aFilename,
> >                                 nsIFile** aFile,
> > +                               const nsACString& aFoldername = EmptyCString(),
> > +                               bool CreateUnique = 1);
> 
> nit: 1 isn't a bool
> 
> I also generally prefer to use an enum in these types of situations rather
> than a bool.
> 
> The bool looks great when looking at the function, but when looking at the
> calling code, you can't tell what: OpenTempFile(filename, &myFile,
> folderName, true) means. If you use an enumeration, then knowing what:
> OpenTempFile(filename, &myFile, folderName, CREATE_UNIQUE) it's much more
> obvious what's going on.

Agree. Will do that.

Comment 38

[Victor Ng [:vng] [:crankycoder]]

Comment on attachment 8610983 [details]
(0527) stumble

I'm clearing the needinfo flag - hannosch has already answered this.

Comment 39

[Alphan Chen [:alchen]]

Attachment: Refine nsDumpUtils::OpenTempFile to make this function more flexible. r=dhylands

The patch is updated with suggestion in comment 36.

Comment 40

[Alphan Chen [:alchen]]

Attachment: Add one more mode for writing a GZFile (CREATE_NEW_ONE/APPEND_IF_EXIST)

Hi Nicholas,
could you review this patch?
I add one more mode for writing a GZFile. 
1. CREATE_NEW_ONE (default) : behavior as before
2. APPEND_IF_EXIST : append the new data to existing data.

Comment 41

[Alphan Chen [:alchen]]

Attachment: [Stumbler] part.1 Dump stumble into specific folder

Hi Garvan,
This patch is based on attachment 8613884 [details] [diff] [review] and attachment 8613885 [details] [diff] [review].
The functionality of Phase 1 is complete.


Finish:
1. Dump stumble (file I/O behavior) in IO thread
2. Stumble includes LocationInfo, 
                    CellTowerInfo (by getCellInfoList),
                    WifiAPInfo(by GetWifiScanResults)
3. Dump policy :
        - MaxFile Num : 4, MaxFileSize : 12.5KB
        - 1 minute 1 stumble (now set to 3 sec for testing)
4. compression the feeding files into gz format

TODO:
1. Add a key for on/off
2. Upload & Upload policy
   PS: If we want to upload the latest file(less than 12.5KB),
       we need to add "]}" in the latest file.

Comment 42

[Nicholas Nethercote [inactive]]

Comment on attachment 8613885 [details] [diff] [review]
Add one more mode for writing a GZFile (CREATE_NEW_ONE/APPEND_IF_EXIST)

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

Looks ok to me if the nits below are fixed, but I'll defer to froydnj because he's the XPCOM module owner.

::: xpcom/base/nsGZFileWriter.cpp
@@ +48,5 @@
> +  if (mMode == CREATE_NEW_ONE) {
> +    rv = aFile->OpenANSIFileDesc("wb", &file);
> +  } else {
> +    rv = aFile->OpenANSIFileDesc("ab", &file);
> +  }

I'd write this like so:

> nsresult rv = aFile->OpenANSIFileDesc(mMode == CREATE_NEW_ONE ? "wb" : "ab", &file);

@@ +62,5 @@
> +  if (mMode == CREATE_NEW_ONE) {
> +    mGZFile = gzdopen(dup(fileno(aFile)), "wb");
> +  } else {
> +    mGZFile = gzdopen(dup(fileno(aFile)), "ab");
> +  }

Likewise here:

> mGZFile = gzdopen(dup(fileno(aFile)), mMode == CREATE_NEW_ONE ? "wb" : "ab");

::: xpcom/base/nsGZFileWriter.h
@@ +12,5 @@
>  
> +enum {
> +APPEND_IF_EXIST,
> +CREATE_NEW_ONE
> +};

Nit: indent the constants by two spaces please.

Also, the names feel strange. I'd use APPEND and CREATE, or maybe MODE_WB and MODE_AB to mirror the mode strings above. Maybe froydnj has opinions about this.

Finally, it would probably be best to move this enum inside nsGZFileWriter to avoid unnecessary pollution of the global namespace.

Comment 43

[Nathan Froyd [:froydnj]]

Comment on attachment 8613885 [details] [diff] [review]
Add one more mode for writing a GZFile (CREATE_NEW_ONE/APPEND_IF_EXIST)

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

Looks good, would like to see the patch with Nicholas's code changes and the enum change below.

::: xpcom/base/nsGZFileWriter.h
@@ +12,5 @@
>  
> +enum {
> +APPEND_IF_EXIST,
> +CREATE_NEW_ONE
> +};

I agree with Nicholas's comments; we should have:

enum Operation {
  Append,
  Create
};

and this enum should be inside nsGZFileWriter.  This also lets us give a descriptive type to nsGZFileWriter's argument.

@@ +23,5 @@
>  {
>    virtual ~nsGZFileWriter();
>  
>  public:
> +  nsGZFileWriter(int aMode = CREATE_NEW_ONE);

I think this constructor needs |explicit| so as to not fail our static analysis builds.

Comment 44

[:garvan]

Comment on attachment 8613900 [details] [diff] [review]
[Stumbler] part.1 Dump stumble into specific folder

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +144,5 @@
> +
> +      // Get Cell Info
> +      nsCOMPtr<nsIMobileConnectionService> service =
> +        do_GetService(NS_MOBILE_CONNECTION_SERVICE_CONTRACTID);
> +      if (service) {

if (!service) {
  //return error
}
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Check_for_errors_early_and_often

@@ +146,5 @@
> +      nsCOMPtr<nsIMobileConnectionService> service =
> +        do_GetService(NS_MOBILE_CONNECTION_SERVICE_CONTRACTID);
> +      if (service) {
> +        nsCOMPtr<nsIMobileConnection> connection;
> +        service->GetItemByServiceId(0 /* Client Id */, getter_AddRefs(connection));

Is it possible to get other interface ids?

@@ +147,5 @@
> +        do_GetService(NS_MOBILE_CONNECTION_SERVICE_CONTRACTID);
> +      if (service) {
> +        nsCOMPtr<nsIMobileConnection> connection;
> +        service->GetItemByServiceId(0 /* Client Id */, getter_AddRefs(connection));
> +        if (connection) {

if (!connection) error return

@@ +158,5 @@
> +      }
> +
> +      // Get Wifi AP Info
> +      nsCOMPtr<nsIInterfaceRequestor> ir = do_GetService("@mozilla.org/telephony/system-worker-manager;1");
> +      if (ir) {

if (!ir) error return

@@ +175,5 @@
> +  private:
> +    nsRefPtr<StumblerInfo> mRequestCallback;
> +  };
> +
> +  static int64_t LastTime_ms = 0;

capital L on the var name?

@@ +182,5 @@
> +  if (LastTime_ms == 0 || timediff >= 3000) {
> +    LastTime_ms = (PR_Now() / PR_USEC_PER_MSEC);
> +
> +    nsRefPtr<StumblerInfo> sRequestCallback = new StumblerInfo(somewhere);
> +    NS_DispatchToMainThread(new  RequestCellInfoEvent(sRequestCallback));

I prefer nsCOMPtr<nsIRunnable> request ...; NS_DispatchToMainThread(request)
to have refcount=1 object passed in

@@ +185,5 @@
> +    nsRefPtr<StumblerInfo> sRequestCallback = new StumblerInfo(somewhere);
> +    NS_DispatchToMainThread(new  RequestCellInfoEvent(sRequestCallback));
> +  } else {
> +    // if we can two continuous location update in the same place. ignore once.
> +    STUMBLER_LOG("stumbler - less than 1 minute. ignore once.\n");

'1 minute' should be 3, or better, make the 3000 a constant and the log can have'%d ms'

@@ +839,5 @@
>  }
>  #endif // MOZ_B2G_RIL
>  
>  
> +#define DUMP(o, s) \

DUMP is too generic. DUMP where?

@@ +844,5 @@
> +  do { \
> +    nsresult rv = (o)->Write(s); \
> +    if (NS_WARN_IF(NS_FAILED(rv))) \
> +      return; \
> +  } while (0)

Can this be static inline instead? Or a private member function, the compiler will inline as necessary.
/me hates macros unless there is no other option :)

@@ +847,5 @@
> +      return; \
> +  } while (0)
> +
> +#define MAXFILENUM 4
> +#define MAXFILESIZE 12.5 * 1024

MAXFILESIZE_KB

@@ +850,5 @@
> +#define MAXFILENUM 4
> +#define MAXFILESIZE 12.5 * 1024
> +
> +nsresult
> +DumpStumblerFeedingEvent::RemoveOldestFile(int FileNum)

aFileNum, aFileCount would be better,'aFileNum' sounds like a single specific file number is being operated on, rather than the entire count of files.

@@ +855,5 @@
> +{
> +  // remove oldest file
> +  nsCOMPtr<nsIFile> tmpFile;
> +  nsresult rv;
> +  rv = nsDumpUtils::OpenTempFile(nsCString("feeding-1.json.gz"), getter_AddRefs(tmpFile),

- nsresult rv = ...
- nsCString -> NS_LITERAL_CSTRING

@@ +863,5 @@
> +    return rv;
> +  }
> +
> +  // Rename the feeding file. (keep the oldest file as 'feeding-1.json.gz')
> +  for (int idx = 1 ; idx < FileNum ; idx++) {

no spaces before ;

note to self: if FileNum = 2, then feeding-1 deleted, and idx=1 is run, and feeding-2 becomes feeding-1, ok that works :)

@@ +864,5 @@
> +  }
> +
> +  // Rename the feeding file. (keep the oldest file as 'feeding-1.json.gz')
> +  for (int idx = 1 ; idx < FileNum ; idx++) {
> +    nsCOMPtr<nsIFile> srFinalFile;

- why 'sr' prefix? why not 'file'
- move to line 872, immediately next to the OpenTempFile

@@ +865,5 @@
> +
> +  // Rename the feeding file. (keep the oldest file as 'feeding-1.json.gz')
> +  for (int idx = 1 ; idx < FileNum ; idx++) {
> +    nsCOMPtr<nsIFile> srFinalFile;
> +    nsCString filename("feeding-");

nsCString DumpStumblerFeedingEvent::Filename(int filenum)
{
  return nsPrintfCString("feeding-%d.json.gz", filenum)
}

@@ +871,5 @@
> +    filename.AppendLiteral(".json.gz");
> +    nsDumpUtils::OpenTempFile(filename, getter_AddRefs(srFinalFile),
> +                              NS_LITERAL_CSTRING("stumble"), 0);
> +    nsAutoString srActualFinalFilename;
> +    rv = srFinalFile->GetLeafName(srActualFinalFilename);

Why do you have to re-get the name you already specified?
Can RemoveOldestFile be reduced to:

nsAutoCString dirname = NS_LITERAL_CSTRING("stumble");
for(int i = 1; i < aFileCount; i++) {
 OpenTempFile(Filename(i + 1), g_A(file), dirname, 0);
 file->MoveTo(nullptr, Filename(i));
}

@@ +876,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +
> +    nsCString mFilename("feeding-");

m for class members only

@@ +892,5 @@
> +}
> +
> +
> +void DumpStumblerFeedingEvent::Run() {
> +

formatting:
void
DumpStumblerFeedingEvent::Run()
{

@@ +893,5 @@
> +
> +
> +void DumpStumblerFeedingEvent::Run() {
> +
> +  STUMBLER_DBG("stunbler - In DumpStumblerFeedingEvent\n");

spelling

@@ +895,5 @@
> +void DumpStumblerFeedingEvent::Run() {
> +
> +  STUMBLER_DBG("stunbler - In DumpStumblerFeedingEvent\n");
> +
> +  int number = 1;

fileNumber

@@ +905,5 @@
> +    filename.AppendLiteral(".json.gz");
> +    nsCOMPtr<nsIFile> tmpFile;
> +    nsresult rv;
> +    rv = nsDumpUtils::OpenTempFile(filename, getter_AddRefs(tmpFile),
> +                                                                NS_LITERAL_CSTRING("stumble"), 0);

- nsACString getDir() { return NS_LITERAL_CSTRING("stumble"); }
- too many spaces here

@@ +919,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return;
> +    }
> +
> +    if (exists) {

if (!exists) {
  continue;
}

@@ +927,5 @@
> +      }
> +        //nsAutoCString dirPath;
> +        //rv = tmpFile->GetNativePath(dirPath);
> +        //nsContentUtils::LogMessageToConsole("stumbler -DumpStumblerFeedingEvent. Exist:%d, name:%s, size:%lld",
> +         //                                                                 exists, dirPath.get(), fileSize);

ok for WIP code, but remember to remove commented out lines

@@ +948,5 @@
> +          return;
> +        }
> +
> +        if (fileSize > 0) {
> +          DUMP(gzWriter, ",\n  {");

perhaps DUMP can be named WriteJSON

@@ +953,5 @@
> +        } else {
> +          DUMP(gzWriter, "{\"items\": [ \n  {");
> +        }
> +        DUMP(gzWriter, mDesc.get());
> +        DUMP(gzWriter, "\n  }");

Can there be only 1 DUMP in this function? Build a formatted jsonOutput string, and write it once.

@@ +974,5 @@
> +      } // fileSize <= MAXFILESIZE
> +    } // if (exists)
> +  } // while (number <= 4 && !done)
> +
> +  return;

Could I suggest something for this function, because I find it tricky to follow and looping every time to find the file to write to is confusing to me.

How about (pseudocode of course):

class member static int sCurrentFileNumber = -1; 

Run() {

if (sCurrentFileNumber < 0) {
  rv = SetCurrentFile(); 
  // above func iterates files calling OpenTempFile(Filename(i)) until
  // current file to write is found, and sets sCurrentFileNumber to i
  // also, handles if sCurrentFile > MAXFILENUM, and removes oldest file
  return if rv error
}

if (sCurrentFileNumber < 0) {
  report error and return
}

File file;
OpenTempFile(Filename(sCurrentFileNumber), file)
if (file too large) {
  rv = SetCurrentFile();
  return if rv error
  OpenTempFile(Filename(sCurrentFileNumber), file)
}

write json to file

} // end of run

@@ +979,5 @@
> +}
> +
> +NS_IMPL_ISUPPORTS(StumblerInfo, nsICellInfoListCallback, nsIWifiScanResultsReady)
> +
> +nsresult StumblerInfo::DumpLocationInfo(nsCString &LocDesc)

'Dump' was used previously to mean disk writes.
Perhaps use 
nsresult
LocationInfoToString(nsCString& locDesc)

@@ +991,5 @@
> +  double lat, lon, alt, acc, altacc, head, spd;
> +  coords->GetLatitude(&lat);
> +  if (!IsNaN(lat)) {
> +    LocDesc += NS_LITERAL_CSTRING("\n    \"latitude\":");
> +    LocDesc += nsPrintfCString("%f,", lat).get();

I would prefer "latitude:%f," as the format string on one line, and without newlines and spaces. I don't see the need to pretty-print this info.

@@ +1016,5 @@
> +  coords->GetAltitudeAccuracy(&altacc);
> +  if (!IsNaN(altacc)) {
> +    LocDesc += NS_LITERAL_CSTRING("\n    \"altitudeAccuracy\":");
> +    LocDesc += nsPrintfCString("%f,", altacc).get();
> +  }

Consider this instead of repeating code:

  nsCString lat = NS_LITERAL_CSTRING("latitude");
  nsCString lon = NS_LITERAL_CSTRING("longitude");
  ....

  std::map<nsCString, double> info; // i don't think this is needed {{lat, 0}, {lng, 0}, ....};
  coords->GetLat(&info[lat]);
  coords->GetLong(&info[lon]);

  .....

  for (auto it = x.begin(); it != x.end(); ++it) {
    double val = it->second;
    if (!IsNaN(val)){
      desc += nsPrintfCString("\"%s\":%f,", it->first.get(), it->second);
    }
  }

@@ +1019,5 @@
> +    LocDesc += nsPrintfCString("%f,", altacc).get();
> +  }
> +
> +  LocDesc += NS_LITERAL_CSTRING("\n    \"timestamp\":");
> +  LocDesc += nsPrintfCString("%lld,", PR_Now() / PR_USEC_PER_MSEC).get();

move this to the end, because it is non conditional, and the last item doesn't end with a comma.

@@ +1034,5 @@
> +    LocDesc += nsPrintfCString("%f,", spd).get();
> +  }
> +  return NS_OK;
> +}
> +

Stopping feedback here for now.
I'll try to continue as soon as I can.

::: dom/system/gonk/GonkGPSGeolocationProvider.h
@@ +30,5 @@
> +#include "nsIMobileConnectionService.h"
> +#include "nsGeoPosition.h"
> +#include "nsIFileStreams.h"
> +#include "nsIWifi.h"
> +#include <android/log.h>

Can't some of these be forward declared?

@@ +160,5 @@
> +                      "%s: " msg, __FUNCTION__, ##__VA_ARGS__)
> +
> +#define STUMBLER_ERR(msg, ...)                                            \
> +  __android_log_print(ANDROID_LOG_ERROR, "Stumbler",                   \
> +                      "%s: " msg, __FUNCTION__, ##__VA_ARGS__)

- Move to cpp.
- Why can't PRLOG be used? That way no define is needed, and logging can be turned on and off at runtime.

@@ +163,5 @@
> +  __android_log_print(ANDROID_LOG_ERROR, "Stumbler",                   \
> +                      "%s: " msg, __FUNCTION__, ##__VA_ARGS__)
> +
> +class StumblerInfo final : public nsICellInfoListCallback,
> +                                          public nsIWifiScanResultsReady

spacing is odd here

@@ +170,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSICELLINFOLISTCALLBACK
> +  NS_DECL_NSIWIFISCANRESULTSREADY
> +
> +  StumblerInfo(nsGeoPosition *position)

cuddle */& with type: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Declarations

Since this apples to the entire patch, I won't note it in each location.

Single-arg constructors are required to be explicit.

@@ +183,5 @@
> +  ~StumblerInfo() {}
> +  nsTArray<nsRefPtr<nsICellInfo>> mCellInfo;
> +  nsCString mWifiDesc;
> +  nsRefPtr<nsGeoPosition> mPosition;
> +  bool CellInfoReady;

mCellInfoReady;

@@ +184,5 @@
> +  nsTArray<nsRefPtr<nsICellInfo>> mCellInfo;
> +  nsCString mWifiDesc;
> +  nsRefPtr<nsGeoPosition> mPosition;
> +  bool CellInfoReady;
> +  bool WifiInfoReady;

mWifiInfoReady

@@ +187,5 @@
> +  bool CellInfoReady;
> +  bool WifiInfoReady;
> +};
> +
> +class DumpStumblerFeedingEvent : public Task{

{ on next line

@@ +194,5 @@
> +    DumpStumblerFeedingEvent(const nsCString &aDesc)
> +      : mDesc(aDesc)
> +    {}
> +
> +    void Run();

void Run() override;

@@ +200,5 @@
> +
> +  private:
> +    ~DumpStumblerFeedingEvent() {}
> +    nsCString mDesc;
> +  };

spacing

Comment 45

[:garvan]

I got part way through the cpp review, I made a note where I stopped (comment 44)

Comment 46

[:garvan]

Can I also suggest that this not go into GonkGPSGeoProvider.h/.cpp and into a file of its own?

Comment 47

[Alphan Chen [:alchen]]

Attachment: Refine nsDumpUtils::OpenTempFile to make this function more flexible. r=dhylands

Move the enum inside nsDumpUtils.

Comment 48

[Alphan Chen [:alchen]]

Attachment: Add one more mode for writing a GZFile (Create/Append)

Update the patch.
Please review it. Thanks.

Comment 49

[Alphan Chen [:alchen]]

Comment on attachment 8613900 [details] [diff] [review]
[Stumbler] part.1 Dump stumble into specific folder

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

Will revise the patch in later patch.

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +1019,5 @@
> +    LocDesc += nsPrintfCString("%f,", altacc).get();
> +  }
> +
> +  LocDesc += NS_LITERAL_CSTRING("\n    \"timestamp\":");
> +  LocDesc += nsPrintfCString("%lld,", PR_Now() / PR_USEC_PER_MSEC).get();

Do you mean move this after "celltower info" and "wifi ap info"?
If timestamp is before "celltower info" or "wifi ap info", we still need a comma.

The format should be like :
{"items": [
   {
    "timestamp": 1405602028568,
    "heading": 45.0,
    "speed": 3.6,
    "cellTowers": [
       {},
       {}
     ],
    "wifiAccessPoints": [
       {},
       {}
     ]
   },
   {
    ...
   },
   {
    ...
   }
]}

Comment 50

[Nathan Froyd [:froydnj]]

Comment on attachment 8615153 [details] [diff] [review]
Add one more mode for writing a GZFile (Create/Append)

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

r=me with the changes below.

::: xpcom/base/nsGZFileWriter.h
@@ +24,5 @@
> +    Create
> +  };
> +
> +
> +  explicit nsGZFileWriter(int aMode = Create);

Please make this:

explicit nsGZFileWriter(Operation aMode = Create);

with the corresponding change to the definition.

@@ +45,5 @@
>      return nsIGZFileWriter::Write(aStr, aLen);
>    }
>  
>  private:
> +  int  mMode;

Likewise:

Operation mMode;

Comment 51

[:garvan]

> Do you mean move this after "celltower info" and "wifi ap info"?
> If timestamp is before "celltower info" or "wifi ap info", we still need a
> comma.

Sorry, I should say to move it to the end because the other items are all doubles and can be easily stuffed into a dictionary and iterated. If that was the last item in the JSON it could also be the one with no comma at the end, but it isn't -I stopped reviewing at that point, continuing now.

Comment 52

[:garvan]

Comment on attachment 8613900 [details] [diff] [review]
[Stumbler] part.1 Dump stumble into specific folder

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +178,5 @@
> +
> +  static int64_t LastTime_ms = 0;
> +  int64_t timediff = (PR_Now() / PR_USEC_PER_MSEC) - LastTime_ms;
> +  STUMBLER_DBG("stumbler - Location. [%f , %f] time_diff:%lld\n", location->longitude, location->latitude, timediff);
> +  if (LastTime_ms == 0 || timediff >= 3000) {

What about also checking the minimum distance threshold? 30 meters is what we found to work well

@@ +1032,5 @@
> +  if (!IsNaN(spd)) {
> +    LocDesc += NS_LITERAL_CSTRING("\n    \"speed\":");
> +    LocDesc += nsPrintfCString("%f,", spd).get();
> +  }
> +  return NS_OK;

if there is not other error than OK from these conversion to string functions, make them void

@@ +1035,5 @@
> +  }
> +  return NS_OK;
> +}
> +
> +nsresult StumblerInfo::DumpCellTowerInfo(nsCString &CellDesc)

nsresult on its own line, and CellDesc -> aCellDesc

@@ +1036,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult StumblerInfo::DumpCellTowerInfo(nsCString &CellDesc)
> +{

DumpCellTowerInfo -> CellTowerInfoToString

@@ +1052,5 @@
> +    }
> +
> +    STUMBLER_DBG("stumbler - type=%d\n", type);
> +
> +    if(type ==1 || type == 4) {

easier to read:
enum { GSM, WCDMA, ...};
if (type == GSM) ...

I think there are constants for these already in the codebase IIRC, I doubt an enum is needed for them here

@@ +1054,5 @@
> +    STUMBLER_DBG("stumbler - type=%d\n", type);
> +
> +    if(type ==1 || type == 4) {
> +      int32_t mcc, mnc, lac, cid, sig;
> +      int32_t psc;

can put this on the previous line

@@ +1104,5 @@
> +          CellDesc += NS_LITERAL_CSTRING("\n      \"psc\":");
> +          CellDesc += nsPrintfCString("%d,", mnc).get();
> +        }
> +        CellDesc += NS_LITERAL_CSTRING("\n      \"asu\":");
> +        CellDesc += nsPrintfCString("%d", sig).get();

Consider this instead (using compiler string concat to split up the long string for reading):
aCellDesc += nsPrintfCString("\"serving\":%d,"
                "\"cellId\":%d," 
                .... the rest of the strings ...,
                registered, cid, ....)

@@ +1113,5 @@
> +        nsCOMPtr<nsICdmaCellInfo> cdmaCellInfo = do_QueryInterface(mCellInfo[idx]);
> +        if (cdmaCellInfo) {
> +          int32_t mnc, lac, cid, sig;
> +          CellDesc += NS_LITERAL_CSTRING("\n      \"radioType\":\"cdma\",");
> +          CellDesc += NS_LITERAL_CSTRING("\n      \"mobileNetworkCod\":");

as above, a single nsPrintfCString for this entire if-block

@@ +1157,5 @@
> +          CellDesc += nsPrintfCString("%d,", cid).get();
> +          CellDesc += NS_LITERAL_CSTRING("\n      \"asu\":");
> +          CellDesc += nsPrintfCString("%d,", sig).get();
> +          CellDesc += NS_LITERAL_CSTRING("\n      \"timingAdvance\":");
> +          CellDesc += nsPrintfCString("%d", timingadvance).get();

a single nsPrintfCString as above

@@ +1185,5 @@
> +{
> +
> +  STUMBLER_DBG("stumbler - There are %d cellinfo in the result\n",count);
> +
> +  for (uint32_t i =0 ; i < count ; i++) {

spacing

@@ +1188,5 @@
> +
> +  for (uint32_t i =0 ; i < count ; i++) {
> +    int32_t type;
> +    aCellInfos[i]->GetType(&type);
> +    if(type ==1) {

look for constants that can be used here

@@ +1232,5 @@
> +          }
> +        }
> +    } else if(type == 4) {
> +        nsCOMPtr<nsIWcdmaCellInfo> wcdmaCellInfo = do_QueryInterface(aCellInfos[i]);
> +        if (!wcdmaCellInfo) {

1,3,4 appear the same.
how about something like the following?

if (type == GSM || type === LTE || type == WCDMA) {
  nsCOMPtr<nsICellInfo> x = ...
  x->GetCid(..)

@@ +1289,5 @@
> +    // mac address
> +    nsString temp;
> +    results[i]->GetBssid(temp);
> +    //   00:00:00:00:00:00 --> 000000000000
> +    for (int32_t x=0; x<6; x++) {

spacing x = 0; x < 6;, no loop needed though

@@ +1290,5 @@
> +    nsString temp;
> +    results[i]->GetBssid(temp);
> +    //   00:00:00:00:00:00 --> 000000000000
> +    for (int32_t x=0; x<6; x++) {
> +      temp.ReplaceSubstring(NS_LITERAL_STRING(":"), NS_LITERAL_STRING(""));

StripChars(":")

@@ +1293,5 @@
> +    for (int32_t x=0; x<6; x++) {
> +      temp.ReplaceSubstring(NS_LITERAL_STRING(":"), NS_LITERAL_STRING(""));
> +    }
> +    nsCString mac;
> +    mac.AssignWithConversion(temp);

AssignWithConversion -> I don't know this function, and I tried to look it up quickly and still don't understand :), can you explain why it is needed here?

Comment 53

[Alphan Chen [:alchen]]

Comment on attachment 8613900 [details] [diff] [review]
[Stumbler] part.1 Dump stumble into specific folder

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +1293,5 @@
> +    for (int32_t x=0; x<6; x++) {
> +      temp.ReplaceSubstring(NS_LITERAL_STRING(":"), NS_LITERAL_STRING(""));
> +    }
> +    nsCString mac;
> +    mac.AssignWithConversion(temp);

It is for nsString to nsCString conversion.
nsString : wide character，16-bit，
nsCString : narrow character，8-bit

Comment 54

[Alphan Chen [:alchen]]

Attachment: Refine nsDumpUtils::OpenTempFile to make this function more flexible. r=dhylands

Refine the parameter part.

Comment 55

[Alphan Chen [:alchen]]

Attachment: Add one more mode for writing a GZFile (Create/Append). r=froydnj

Comment 56

[:garvan]

> > +    mac.AssignWithConversion(temp);
> 
> It is for nsString to nsCString conversion.
> nsString : wide character，16-bit，
> nsCString : narrow character，8-bit

It is defined in a file called nsStringObsolete.cpp which suggests no new code should be written with it. I think there are NS_Convert* functions that are to be used instead.

Comment 57

[Alphan Chen [:alchen]]

Comment on attachment 8613900 [details] [diff] [review]
[Stumbler] part.1 Dump stumble into specific folder

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +871,5 @@
> +    filename.AppendLiteral(".json.gz");
> +    nsDumpUtils::OpenTempFile(filename, getter_AddRefs(srFinalFile),
> +                              NS_LITERAL_CSTRING("stumble"), 0);
> +    nsAutoString srActualFinalFilename;
> +    rv = srFinalFile->GetLeafName(srActualFinalFilename);

The LeafName is not feeding-%d.json.gz.
It should be "/data/local/tmp/stumble/feeding-%d.json.gz" here.
However, the directory depends on different platforms. (different temp directory are being used)

Maybe I can save the current leaf name for next loop usage.

Comment 58

[Alphan Chen [:alchen]]

Comment on attachment 8613900 [details] [diff] [review]
[Stumbler] part.1 Dump stumble into specific folder

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +1232,5 @@
> +          }
> +        }
> +    } else if(type == 4) {
> +        nsCOMPtr<nsIWcdmaCellInfo> wcdmaCellInfo = do_QueryInterface(aCellInfos[i]);
> +        if (!wcdmaCellInfo) {

There is no "GetCid" in nsICellInfo.

Comment 59

[Alphan Chen [:alchen]]

Comment on attachment 8613900 [details] [diff] [review]
[Stumbler] part.1 Dump stumble into specific folder

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

Hi Garvan,
I almost finish the new patch.
Could you answer this two questions?
Thanks.

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +178,5 @@
> +
> +  static int64_t LastTime_ms = 0;
> +  int64_t timediff = (PR_Now() / PR_USEC_PER_MSEC) - LastTime_ms;
> +  STUMBLER_DBG("stumbler - Location. [%f , %f] time_diff:%lld\n", location->longitude, location->latitude, timediff);
> +  if (LastTime_ms == 0 || timediff >= 3000) {

So we need to skip this dump when both two threshold don't meet. Or either one?

@@ +185,5 @@
> +    nsRefPtr<StumblerInfo> sRequestCallback = new StumblerInfo(somewhere);
> +    NS_DispatchToMainThread(new  RequestCellInfoEvent(sRequestCallback));
> +  } else {
> +    // if we can two continuous location update in the same place. ignore once.
> +    STUMBLER_LOG("stumbler - less than 1 minute. ignore once.\n");

3 minutes?

Comment 60

[:garvan]

> So we need to skip this dump when both two threshold don't meet. Or either
> one?

Both thresholds must be met for the stumble to occur. At least, that seems to work well on Android. So, both 3 seconds AND 30 meters must be exceeded in order to stumble. This keeps the data volumes down.
 
> @@ +185,5 @@
> > +    nsRefPtr<StumblerInfo> sRequestCallback = new StumblerInfo(somewhere);
> > +    NS_DispatchToMainThread(new  RequestCellInfoEvent(sRequestCallback));
> > +  } else {
> > +    // if we can two continuous location update in the same place. ignore once.
> > +    STUMBLER_LOG("stumbler - less than 1 minute. ignore once.\n");
> 
> 3 minutes?

Sorry! 3 seconds.

Comment 61

[Alphan Chen [:alchen]]

Attachment: (0611) [Stumbler] part.1 Dump stumble into specific folder

Here is the latest version.
Please review it.
Thanks.

Comment 62

[Hanno Schlichting]

Replying here for the "how / where to upload" part. We already have an API key for MLS set. This gets set as part of the configure process, via a --with-mozilla-api-keyfile argument (https://dxr.mozilla.org/mozilla-central/source/configure.in#3984). There's a default fallback value specified in there, but real builds will have different keys based on this.

As the next step for the locate side, we have a geo.wifi.uri preference, which on the B2G side is set via (https://dxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#203):

pref("geo.wifi.uri", "https://location.services.mozilla.com/v1/geolocate?key=%MOZILLA_API_KEY%");

On the locate side this is later used (https://dxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#489): 

let url = Services.urlFormatter.formatURLPref("geo.wifi.uri");

The Services.urlFormatter.formatURLPref takes care of replacing %MOZILLA_API_KEY% with the real string set during configure.

For the stumbler side, I think we need a new preference like "geo.stumbler.uri". Then set it in b2g.js as "https://location.services.mozilla.com/v1/geosubmit?key=%MOZILLA_API_KEY%". Having this as a preference makes sure someone could customize it and send their data to a different service, and also makes testing easier, as the endpoint can be replaced and data sent do a mock test service.

When this preference is actually used, it needs to interpolate / replace the %MOZILLA_API_KEY% variable with the real one. Unfortunately I don't really know how these pieces work, just the general idea. So I'm not sure how to do this on the C level.

Comment 63

[:garvan]

Comment on attachment 8620877 [details] [diff] [review]
(0611) [Stumbler] part.1 Dump stumble into specific folder

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +91,5 @@
> +double CalculateDeltaInMeter(double aLat, double aLon, double aLastLat, double aLastLon)
> +{
> +    // Use spherical law of cosines to calculate difference
> +    // Not quite as correct as the Haversine but simpler and cheaper
> +    // Should the following be a utility function? Others might need this calc.

You can remove this

@@ +208,5 @@
> +  if (0 != sLastLon || 0 != sLastLat) {
> +    delta = CalculateDeltaInMeter(location->latitude, location->longitude, sLastLat, sLastLon);
> +  }
> +  sLastLat = location->latitude;
> +  sLastLon = location->longitude;

don't assign to these if delta < kMinChangeInMeters, my thinking is that if the user continuously moves less than kMinChangeInMeters, and sLast keeps getting updated, then scanning won't trigger

::: dom/system/gonk/MozStumbler.cpp
@@ +322,5 @@
> +  aCellDesc += NS_LITERAL_CSTRING("]");
> +  return NS_OK;
> +}
> +
> +void StumblerInfo::DumpStumblerInfo()

void \n

@@ +340,5 @@
> +  return;
> +}
> +
> +/* void notifyGetCellInfoList (in uint32_t count, [array, size_is (count)] in nsICellInfo result); */
> +NS_IMETHODIMP StumblerInfo::NotifyGetCellInfoList(uint32_t count, nsICellInfo** aCellInfos)

Can I suggest this to reduce repeated lines and a single error message formatter:

#define MACROSTR(k) #k

/* void notifyGetCellInfoList (in uint32_t count, [array, size_is (count)] in nsICellInfo result); */
NS_IMETHODIMP
NotifyGetCellInfoList(uint32_t count, nsICellInfo** aCellInfos)
{
  STUMBLER_DBG("There are %d cellinfo in the result\n",count);

  for (uint32_t i = 0; i < count; i++) {
    int32_t type;
    aCellInfos[i]->GetType(&type);
    const char* failedTypeCast = 0;
    int32_t cid;
    if (type == nsICellInfo::CELL_INFO_TYPE_GSM) {
      nsCOMPtr<nsIGsmCellInfo> gsmCellInfo = do_QueryInterface(aCellInfos[i]);
      if (!gsmCellInfo) {
        failedTypeCast = MACROSTR(nsICellInfo::CELL_INFO_TYPE_GSM);
      } else {
        gsmCellInfo->GetCid(&cid);
      }
    } else if(type == nsICellInfo::CELL_INFO_TYPE_CDMA) {
      nsCOMPtr<nsICdmaCellInfo> cdmaCellInfo = do_QueryInterface(aCellInfos[i]);
      if (!cdmaCellInfo) {
        failedTypeCast = MACROSTR(nsICellInfo::CELL_INFO_TYPE_CDMA);
      } else {
        cdmaCellInfo->GetBaseStationId(&cid);
      }
    }
    /// other else conditions ///

    if (failedTypeCast) {
      STUMBLER_ERR("count %d isnot type %s => drop\n", i + 1, failedTypeCast);
    } else {
      if (cid < 0 || cid == 0x7fffffff) {
        STUMBLER_ERR("cid:%d, not valid cell info\n", cid);
      } else {
        STUMBLER_DBG("Valid Element [%d], Added! \n",i);
        mCellInfo.AppendElement(aCellInfos[i]);
      }
    }

@@ +419,5 @@
> +  return  NS_OK;
> +}
> +
> +/* void notifyGetCellInfoListFailed (in DOMString error); */
> +NS_IMETHODIMP StumblerInfo::NotifyGetCellInfoListFailed(const nsAString& error)

NS_IMETHODIMP \n

@@ +453,5 @@
> +    temp.StripChars(":");
> +    mWifiDesc += NS_LITERAL_CSTRING("\"macAddress\":\"");
> +    mWifiDesc += NS_ConvertUTF16toUTF8(temp);
> +
> +

remove extra newline

::: dom/system/gonk/MozStumbler.h
@@ +6,5 @@
> +
> +#ifndef mozilla_system_mozstumbler_h__
> +#define mozilla_system_mozstumbler_h__
> +
> +#include "nsICellInfo.h"

forward declare I think

@@ +8,5 @@
> +#define mozilla_system_mozstumbler_h__
> +
> +#include "nsICellInfo.h"
> +#include "nsIMobileConnectionService.h"
> +#include "nsGeoPosition.h"

forward decl

@@ +17,5 @@
> +#define STUMBLE_INTERVAL_MS 3000
> +
> +#if USE_STUMBLER_DEBUG
> +#define STUMBLER_DBG(msg, ...)	\
> +  nsContentUtils::LogMessageToConsole("Stumbler-DBG %s: "	\

Why is it that PRLOG functions aren't used here?

@@ +45,5 @@
> +    : mPosition(position), mCellInfoReady(0), mWifiInfoReady(0)
> +  {}
> +
> +  void DumpStumblerInfo();
> +  nsresult LocationInfoToString(nsCString& LocDesc);

aLocDesc

@@ +65,5 @@
> +  public:
> +    NS_INLINE_DECL_REFCOUNTING(DumpStumblerFeedingEvent)
> +
> +    explicit DumpStumblerFeedingEvent(const nsCString& aDesc)
> +      : mDesc(aDesc), sCurrentFileNumber(1)

sCurrentFileNumber should be static? and thus not initialized here?

@@ +72,5 @@
> +    void Run() override;
> +    nsresult RemoveOldestFile(int aFileNum);
> +    nsCString Filename(int aFileNum)
> +    {
> +      return nsPrintfCString("feeding-%d.json.gz", aFileNum);

move to cpp

@@ +76,5 @@
> +      return nsPrintfCString("feeding-%d.json.gz", aFileNum);
> +    }
> +    nsCString GetDir()
> +    {
> +      return NS_LITERAL_CSTRING("stumble");

move to cpp

@@ +86,5 @@
> +    End,
> +    Unknown
> +  };
> +
> +

remove newline

@@ +93,5 @@
> +    Partition SetCurrentFile();
> +    void WriteJSON(Partition aPart, int aFileNum);
> +
> +    nsCString mDesc;
> +    int sCurrentFileNumber;

should this be static?

Comment 64

[Alphan Chen [:alchen]]

(In reply to Garvan Keeley [:garvank] from comment #63)
> Comment on attachment 8620877 [details] [diff] [review]
> (0611) [Stumbler] part.1 Dump stumble into specific folder
> 
> Review of attachment 8620877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +208,5 @@
> > +  if (0 != sLastLon || 0 != sLastLat) {
> > +    delta = CalculateDeltaInMeter(location->latitude, location->longitude, sLastLat, sLastLon);
> > +  }
> > +  sLastLat = location->latitude;
> > +  sLastLon = location->longitude;
> 
> don't assign to these if delta < kMinChangeInMeters, my thinking is that if
> the user continuously moves less than kMinChangeInMeters, and sLast keeps
> getting updated, then scanning won't trigger
> 
Yes, my bad.


> @@ +17,5 @@
> > +#define STUMBLE_INTERVAL_MS 3000
> > +
> > +#if USE_STUMBLER_DEBUG
> > +#define STUMBLER_DBG(msg, ...)	\
> > +  nsContentUtils::LogMessageToConsole("Stumbler-DBG %s: "	\
> 
> Why is it that PRLOG functions aren't used here?

My thinking is use the same way as GonkGPSGeolocationProvider.

Comment 65

[Alphan Chen [:alchen]]

Attachment: (0615) [Stumbler] part.1 Dump stumble into specific folder

Patch updated with two comments.

1.
Leave "nsICellInfo.h" and "nsIWifi.h" in MozStumbler.h for the parent of class StumblerInfo. (nsICellInfoListCallback and nsIWifiScanResultsReady)

2.
Prefer to use "nsContentUtils::LogMessageToConsole" for coherence with GonkGPSGeolocationProvider.cpp

Comment 66

[Alphan Chen [:alchen]]

(In reply to Hanno Schlichting [:hannosch] from comment #62)
> 
> When this preference is actually used, it needs to interpolate / replace the
> %MOZILLA_API_KEY% variable with the real one. Unfortunately I don't really
> know how these pieces work, just the general idea. So I'm not sure how to do
> this on the C level.

Hi Garvan,
could I get some feedbacks for this part?

Comment 67

[Alphan Chen [:alchen]]

I check the output folder(build for flame) and find the following.


./toolkit/components/urlformatter/mozilla_api_key

[content]
#define MOZ_MOZILLA_API_KEY no-mozilla-api-key


Do we need similar patch as bug 1009761(dolphin) for flame?

Comment 68

[Alphan Chen [:alchen]]

Comment on attachment 8622314 [details] [diff] [review]
(0615) [Stumbler] part.1 Dump stumble into specific folder

Will revise the patch again.

Comment 69

[:garvan]

(In reply to Alphan Chen [:alchen] from comment #66)
> (In reply to Hanno Schlichting [:hannosch] from comment #62)
> > 
> > When this preference is actually used, it needs to interpolate / replace the
> > %MOZILLA_API_KEY% variable with the real one. Unfortunately I don't really
> > know how these pieces work, just the general idea. So I'm not sure how to do
> > this on the C level.
> 
> Hi Garvan,
> could I get some feedbacks for this part?

No feedback in particular, other than the steps of adding the geo.stumbler.url to b2g.js, and do_CreateInstance("@mozilla.org/toolkit/URLFormatterService;1") and call FormatUrl (and any changes to url formatter to pickup the geo.stumbler.url).

Any issues with the value of MOZILLA_API_KEY we can file as separate bugs.

(In reply to Alphan Chen [:alchen] from comment #67)
> I check the output folder(build for flame) and find the following.
> ./toolkit/components/urlformatter/mozilla_api_key
> [content]
> #define MOZ_MOZILLA_API_KEY no-mozilla-api-key
> Do we need similar patch as bug 1009761(dolphin) for flame?

no-mozilla-api-key is ok for now. The submission will work with this key, or no key argument on the URL at all, it is not restrictive. The API key is being used for internal reporting only ATM.

As for filing a bug, we'll defer that for now. Adding API keys for every device as a private build config is not working, the API key is not getting reliably set and expecting this to be set reliably for every new device is unrealistic. Hanno and I will get a bug filed for this when we have a better idea of what to do.

Comment 70

[:garvan]

Alphan + Hanno, I am trying to simplify the cell network code.

Hanno I added a note on line 94 where I am confused. The code would be cleaner if we could just use 'asu', but if not, I can special case this line to use 'signalStrength'

https://gist.github.com/garvankeeley/903ec1b81a23037fd21d

Comment 71

[:garvan]

Updated https://gist.github.com/garvankeeley/903ec1b81a23037fd21d

This is the cell network formatting code. Handles signal strength for both 'asu' (unitless) and 'signalStrength' (dbm). Also has a check for UNKNOWN_VALUE, and leaves the JSON keys unset if the value is not valid.

Comment 72

[:garvan]

Comment on attachment 8622314 [details] [diff] [review]
(0615) [Stumbler] part.1 Dump stumble into specific folder

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

::: dom/system/gonk/MozStumbler.cpp
@@ +193,5 @@
> +  if (!coords) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCString lat = NS_LITERAL_CSTRING("latitude");

change to NS_NAMED_LITERAL_CSTRING (just discovered this macro!)

@@ +201,5 @@
> +  nsCString altacc = NS_LITERAL_CSTRING("altitudeAccuracy");
> +  nsCString head = NS_LITERAL_CSTRING("heading");
> +  nsCString spd = NS_LITERAL_CSTRING("speed");
> +
> +  std::map<nsCString, double> info;

nsCString -> nsLiteralCString

@@ +221,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +StumblerInfo::CellTowerInfoToString(nsCString& aCellDesc)

Please see my gist replacement code for this function.
Also, function name should s/Tower/Network/, as network is the correct term (a single tower has many cell networks).

Comment 73

[Alphan Chen [:alchen]]

Attachment: (0616) [Stumbler] part.1 Dump stumble into specific folder

Update patch.

Comment 74

[Alphan Chen [:alchen]]

Attachment: (0616) [Stumbler] part.1 Dump stumble into specific folder

Remove unnecessary space.

Comment 75

[Alphan Chen [:alchen]]

Comment on attachment 8622932 [details] [diff] [review]
(0616) [Stumbler] part.1 Dump stumble into specific folder

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

Request review.

Comment 76

[Hanno Schlichting]

Comment on attachment 8622932 [details] [diff] [review]
(0616) [Stumbler] part.1 Dump stumble into specific folder

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

I only commented on the parts related to the resulting JSON data send to the service.

::: dom/system/gonk/MozStumbler.cpp
@@ +334,5 @@
> +        lteCellInfo->GetRsrp(&rsrp);
> +        info[keyLac] = lac;
> +        info[keyTimingAdvance] = timingAdvance;
> +        info[keyPsc] = pcid;
> +        info[keyStrengthDbm] = rsrp;

Rsrp is defined as "The current Reference Signal Receive Power in dBm multipled by -1. Range: 44 to 140 dBm, UNKNOWN_VALUE if unknown."

We need to check whether the value is UNKNOWN_VALUE and if it is omit it. If it is a real value, we need to multiply it by -1 to get the negative dBm value as expected by the service (which wants something like -44 or -100).

@@ +344,5 @@
> +    } else {
> +      aCellDesc += nsPrintfCString("\"%s\":\"%s\"", keyRadioType.get(), radioType);
> +              STUMBLER_DBG("356\n");
> +      for (auto iter = info.begin(); iter != info.end(); ++iter) {
> +                STUMBLER_DBG("358\n");

I'm guessing this debug line and the one two lines above weren't meant to be in the patch?

@@ +346,5 @@
> +              STUMBLER_DBG("356\n");
> +      for (auto iter = info.begin(); iter != info.end(); ++iter) {
> +                STUMBLER_DBG("358\n");
> +        int32_t value = iter->second;
> +        if (value != 0x7fffffff) {

Should this use nsICellInfo::UNKNOWN_VALUE instead of the literal 0x7fffffff value?

@@ +424,5 @@
> +    if (failedTypeCast) {
> +      STUMBLER_ERR("count %d isnot type %s => drop\n", i + 1, failedTypeCast);
> +    } else {
> +      if (cid < 0 || cid == 0x7fffffff) {
> +        STUMBLER_ERR("cid:%d, not valid cell info\n", cid);

Using cid as a way to check for valid cell entries doesn't quite work. There can be valid values which have all the other fields (like radioType, country code and network code) but have no known / valid cell id field.

For example on GSM/2G connections we generally only get a complete cell entry with all values for the currently registered / serving cell. But all other neighboring cells have incomplete entries, especially missing the location area code and cell id. We still want to capture those and send them to the service.

Basically if a cell entry contains any valid field, we want to consider it as good data and send it to the service. Or rather if we can figure out the radioType of a cell entry, then we want to capture its data. I think we can just drop the cid check here and all the code above that gets it from the cellinfo objects.

@@ +468,5 @@
> +      mWifiDesc += NS_LITERAL_CSTRING(",{");
> +    } else {
> +      mWifiDesc += NS_LITERAL_CSTRING("{");
> +    }
> +

We need to add two checks here that both filter out certain wifi entries from data collection. Both look at the ssid (the clear text name, not the bssid/mac address).

1. If the ssid is empty, null or otherwise not set, skip the wifi entry. This should filter out hidden wifi networks.
2. If the ssid is a non-empty string, check that it does not end with "_nomap". If it ends with _nomap skip the entry.

Both of those checks need to happen on the client as the ssid is not and must not be send to the service.

Comment 77

[:garvan]

> > Why is it that PRLOG functions aren't used here?
> 
> My thinking is use the same way as GonkGPSGeolocationProvider.

I wasn't involved in that code, however current standards are to follow MOZ_LOG:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Gecko_Logging

Comment 78

[:garvan]

(In reply to Hanno Schlichting [:hannosch] from comment #76)
> Comment on attachment 8622932 [details] [diff] [review]
> (0616) [Stumbler] part.1 Dump stumble into specific folder
> 
> 
> ::: dom/system/gonk/MozStumbler.cpp
> @@ +334,5 @@
> > +        lteCellInfo->GetRsrp(&rsrp);
> > +        info[keyLac] = lac;
> > +        info[keyTimingAdvance] = timingAdvance;
> > +        info[keyPsc] = pcid;
> > +        info[keyStrengthDbm] = rsrp;
> 
> Rsrp is defined as "The current Reference Signal Receive Power in dBm
> multipled by -1. Range: 44 to 140 dBm, UNKNOWN_VALUE if unknown."
> 
> We need to check whether the value is UNKNOWN_VALUE and if it is omit it. If
> it is a real value, we need to multiply it by -1 to get the negative dBm
> value as expected by the service (which wants something like -44 or -100).

All values are checked for UNKNOWN_VALUE (at the end of the function), so just doing
info[keyStrengthDbm] = -rsrp;
should be ok.

 
> @@ +344,5 @@
> > +    } else {
> > +      aCellDesc += nsPrintfCString("\"%s\":\"%s\"", keyRadioType.get(), radioType);
> > +              STUMBLER_DBG("356\n");
> > +      for (auto iter = info.begin(); iter != info.end(); ++iter) {
> > +                STUMBLER_DBG("358\n");
> 
> I'm guessing this debug line and the one two lines above weren't meant to be
> in the patch?
> 
> @@ +346,5 @@
> > +              STUMBLER_DBG("356\n");
> > +      for (auto iter = info.begin(); iter != info.end(); ++iter) {
> > +                STUMBLER_DBG("358\n");
> > +        int32_t value = iter->second;
> > +        if (value != 0x7fffffff) {
> 
> Should this use nsICellInfo::UNKNOWN_VALUE instead of the literal 0x7fffffff
> value?
> 
> @@ +424,5 @@
> > +    if (failedTypeCast) {
> > +      STUMBLER_ERR("count %d isnot type %s => drop\n", i + 1, failedTypeCast);
> > +    } else {
> > +      if (cid < 0 || cid == 0x7fffffff) {
> > +        STUMBLER_ERR("cid:%d, not valid cell info\n", cid);
> 
> Using cid as a way to check for valid cell entries doesn't quite work. There
> can be valid values which have all the other fields (like radioType, country
> code and network code) but have no known / valid cell id field.
> 
> For example on GSM/2G connections we generally only get a complete cell
> entry with all values for the currently registered / serving cell. But all
> other neighboring cells have incomplete entries, especially missing the
> location area code and cell id. We still want to capture those and send them
> to the service.
> 
> Basically if a cell entry contains any valid field, we want to consider it
> as good data and send it to the service. Or rather if we can figure out the
> radioType of a cell entry, then we want to capture its data. I think we can
> just drop the cid check here and all the code above that gets it from the
> cellinfo objects.
> 
> @@ +468,5 @@
> > +      mWifiDesc += NS_LITERAL_CSTRING(",{");
> > +    } else {
> > +      mWifiDesc += NS_LITERAL_CSTRING("{");
> > +    }
> > +
> 
> We need to add two checks here that both filter out certain wifi entries
> from data collection. Both look at the ssid (the clear text name, not the
> bssid/mac address).
> 
> 1. If the ssid is empty, null or otherwise not set, skip the wifi entry.
> This should filter out hidden wifi networks.
> 2. If the ssid is a non-empty string, check that it does not end with
> "_nomap". If it ends with _nomap skip the entry.
> 
> Both of those checks need to happen on the client as the ssid is not and
> must not be send to the service.

Comment 79

[:garvan]

(In reply to Garvan Keeley [:garvank] from comment #78)

> > We need to check whether the value is UNKNOWN_VALUE and if it is omit it. If
> > it is a real value, we need to multiply it by -1 to get the negative dBm
> > value as expected by the service (which wants something like -44 or -100).
> 
> All values are checked for UNKNOWN_VALUE (at the end of the function), so
> just doing
> info[keyStrengthDbm] = -rsrp;
> should be ok.
> 
Oops, ignore that. That will make the unknown value check break. Only negate when != to the unknown val (Alphan was doing this correctly, just my this comment is wrong)

Comment 80

[:garvan]

Comment on attachment 8622932 [details] [diff] [review]
(0616) [Stumbler] part.1 Dump stumble into specific folder

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

Almost there :). Thanks to Hanno for catching the '_nomap' case, I should have noticed that earlier.

::: dom/system/gonk/MozStumbler.cpp
@@ +15,5 @@
> +
> +#define MAXFILENUM 4
> +#define MAXFILESIZE_KB 12.5 * 1024
> +
> +/* static */ int DumpStumblerFeedingEvent::sCurrentFileNumber = 1;

remove comment

@@ +68,5 @@
> +  }
> +  return NS_OK;
> +}
> +
> +

remove extra newline

@@ +171,5 @@
> +    return Middle;
> +  }
> +}
> +
> +

remove extra newline

@@ +185,5 @@
> +    return;
> +  }
> +}
> +
> +

remove extra newline

@@ +247,5 @@
> +  info[keyMcc] = mcc;
> +  info[keyMnc] = mnc;
> +  info[keyCid] = cid;
> +  info[keyStrengthAsu] = sig;
> +};

remove ;

@@ +256,5 @@
> +  aCellDesc += NS_LITERAL_CSTRING("\"cellTowers\": [");
> +
> +  for (uint32_t idx = 0; idx < mCellInfo.Length() ; idx++) {
> +    const char* radioType = 0;
> +    bool hasTypeError = false;

Remove this, as mCellInfo has already validated the types

@@ +275,5 @@
> +
> +    if(type == nsICellInfo::CELL_INFO_TYPE_GSM) {
> +      radioType = "gsm";
> +      nsCOMPtr<nsIGsmCellInfo> gsmCellInfo = do_QueryInterface(mCellInfo[idx]);
> +      if (!gsmCellInfo) {

remove

@@ +286,5 @@
> +      }
> +    } else if (type == nsICellInfo::CELL_INFO_TYPE_WCDMA) {
> +      radioType = "wcdma";
> +      nsCOMPtr<nsIWcdmaCellInfo> wcdmaCellInfo = do_QueryInterface(mCellInfo[idx]);
> +      if (!wcdmaCellInfo) {

remove

@@ +299,5 @@
> +      }
> +    } else if (type == nsICellInfo::CELL_INFO_TYPE_CDMA) {
> +      radioType = "cdma";
> +      nsCOMPtr<nsICdmaCellInfo> cdmaCellInfo = do_QueryInterface(mCellInfo[idx]);
> +      if (!cdmaCellInfo) {

remove

@@ +334,5 @@
> +        lteCellInfo->GetRsrp(&rsrp);
> +        info[keyLac] = lac;
> +        info[keyTimingAdvance] = timingAdvance;
> +        info[keyPsc] = pcid;
> +        info[keyStrengthDbm] = rsrp;

as per Hanno's comment
if (rsrp != unknown val) {
info[keyStrengthDbm] = -rsrp
}

@@ +338,5 @@
> +        info[keyStrengthDbm] = rsrp;
> +      }
> +    }
> + 
> +    if (hasTypeError) {

remove

@@ +379,5 @@
> +#define MACROSTR(k) #k
> +
> +/* void notifyGetCellInfoList (in uint32_t count, [array, size_is (count)] in nsICellInfo result); */
> +NS_IMETHODIMP
> +StumblerInfo::NotifyGetCellInfoList(uint32_t count, nsICellInfo** aCellInfos)

Following Hanno's comment, this function should only validate the cell infos are castable, and place them in mCellInfo

@@ +381,5 @@
> +/* void notifyGetCellInfoList (in uint32_t count, [array, size_is (count)] in nsICellInfo result); */
> +NS_IMETHODIMP
> +StumblerInfo::NotifyGetCellInfoList(uint32_t count, nsICellInfo** aCellInfos)
> +{
> +

remove newline

::: dom/system/gonk/MozStumbler.h
@@ +9,5 @@
> +
> +#include "nsICellInfo.h"
> +#include "nsIWifi.h"
> +
> +#define USE_STUMBLER_DEBUG 0

Don't need this, MOZ_LOG will handle turning on and off debugging.
The way I am aware of is to add NSPR_LOG_MODULES=<name>:<level> to /system/bin/b2g.sh

@@ +14,5 @@
> +#define STUMBLE_INTERVAL_MS 3000
> +
> +#if USE_STUMBLER_DEBUG
> +#define STUMBLER_DBG(msg, ...)	\
> +  nsContentUtils::LogMessageToConsole("Stumbler-DBG %s: "	\

Remove all the logging macros in this header, MOZ_LOG distinguishes between Debug vs. Error vs. Info/Verbose.

Comment 81

[Alphan Chen [:alchen]]

Comment on attachment 8622932 [details] [diff] [review]
(0616) [Stumbler] part.1 Dump stumble into specific folder

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

::: dom/system/gonk/MozStumbler.cpp
@@ +424,5 @@
> +    if (failedTypeCast) {
> +      STUMBLER_ERR("count %d isnot type %s => drop\n", i + 1, failedTypeCast);
> +    } else {
> +      if (cid < 0 || cid == 0x7fffffff) {
> +        STUMBLER_ERR("cid:%d, not valid cell info\n", cid);

OK, I got your point.

The checking is added in developing stage.
We got some garbage data at that time.
I have some discussion with RIL team in bug 1168064.
After discussion, RIL team filter some garbage in API level.
As a result, I will drop this check.

Comment 82

[Alphan Chen [:alchen]]

> 
> ::: dom/system/gonk/MozStumbler.h
> @@ +9,5 @@
> > +
> > +#include "nsICellInfo.h"
> > +#include "nsIWifi.h"
> > +
> > +#define USE_STUMBLER_DEBUG 0
> 
> Don't need this, MOZ_LOG will handle turning on and off debugging.
> The way I am aware of is to add NSPR_LOG_MODULES=<name>:<level> to
> /system/bin/b2g.sh
> 
> @@ +14,5 @@
> > +#define STUMBLE_INTERVAL_MS 3000
> > +
> > +#if USE_STUMBLER_DEBUG
> > +#define STUMBLER_DBG(msg, ...)	\
> > +  nsContentUtils::LogMessageToConsole("Stumbler-DBG %s: "	\
> 
> Remove all the logging macros in this header, MOZ_LOG distinguishes between
> Debug vs. Error vs. Info/Verbose.

I try to use the following code to replace the original implementation.
But I cannot see the log in logcat by this.
Anything I missed?


static PRLogModuleInfo *gStumblerLog = PR_NewLogModule("STUMBLER");
#define STUMBLER_DBG(arg, ...)     MOZ_LOG(gStumblerLog, mozilla::LogLevel::Debug, ("STUMBLER - %s: " arg, __func__, ##__VA_ARGS__))
#define STUMBLER_LOG(arg, ...)     MOZ_LOG(gStumblerLog, mozilla::LogLevel::Info, ("STUMBLER - %s: " arg, __func__, ##__VA_ARGS__))
#define STUMBLER_ERR(arg, ...)     MOZ_LOG(gStumblerLog, mozilla::LogLevel::Error, ("STUMBLER -%s: " arg, __func__, ##__VA_ARGS__))

Comment 83

[Alphan Chen [:alchen]]

Sorry I missed the MDN document.

[quote from https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Gecko_Logging
The log level for a module is controlled by setting an environment variable prior to launching the application
By default all logging output is disabled.


> 
> I try to use the following code to replace the original implementation.
> But I cannot see the log in logcat by this.
> Anything I missed?
> 
> 
> static PRLogModuleInfo *gStumblerLog = PR_NewLogModule("STUMBLER");
> #define STUMBLER_DBG(arg, ...)     MOZ_LOG(gStumblerLog,
> mozilla::LogLevel::Debug, ("STUMBLER - %s: " arg, __func__, ##__VA_ARGS__))
> #define STUMBLER_LOG(arg, ...)     MOZ_LOG(gStumblerLog,
> mozilla::LogLevel::Info, ("STUMBLER - %s: " arg, __func__, ##__VA_ARGS__))
> #define STUMBLER_ERR(arg, ...)     MOZ_LOG(gStumblerLog,
> mozilla::LogLevel::Error, ("STUMBLER -%s: " arg, __func__, ##__VA_ARGS__))

Comment 84

[Alphan Chen [:alchen]]

I use adb command to set the environment variable.
(adb shell export NSPR_LOG_MODULES="STUMBLER:4")
However, I still cannot find the log.
Any suggestion to verify the MOZ_LOG?

Comment 85

[Hanno Schlichting]

(In reply to Alphan Chen [:alchen] from comment #81)
> Comment on attachment 8622932 [details] [diff] [review]
> (0616) [Stumbler] part.1 Dump stumble into specific folder
> 
> Review of attachment 8622932 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/MozStumbler.cpp
> @@ +424,5 @@
> > +    if (failedTypeCast) {
> > +      STUMBLER_ERR("count %d isnot type %s => drop\n", i + 1, failedTypeCast);
> > +    } else {
> > +      if (cid < 0 || cid == 0x7fffffff) {
> > +        STUMBLER_ERR("cid:%d, not valid cell info\n", cid);
> 
> OK, I got your point.
> 
> The checking is added in developing stage.
> We got some garbage data at that time.
> I have some discussion with RIL team in bug 1168064.
> After discussion, RIL team filter some garbage in API level.
> As a result, I will drop this check.

Ah ok, I was wondering where all those weird and inconsistent "unknown" special values had gone. Nice work on the underlying API. That does make our live here a lot easier.

Comment 86

[Alphan Chen [:alchen]]

Attachment: 0617-[Stumbler] part.1 Dump stumble into specific folder

Update the patch.

Comment 87

[:garvan]

(In reply to Alphan Chen [:alchen] from comment #84)
> I use adb command to set the environment variable.
> (adb shell export NSPR_LOG_MODULES="STUMBLER:4")
> However, I still cannot find the log.
> Any suggestion to verify the MOZ_LOG?

Not sure -check on #b2g maybe? I assumed the env var has to be set too early in the boot process to use adb shell. 
If NSPR_LOG_FILE is set to non-empty it will write to that file instead of logcat, although I doubt yours is set. Did you try this: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Firefox_OS/Customizing_the_b2g.sh_script

Comment 88

[:garvan]

Comment on attachment 8623567 [details] [diff] [review]
0617-[Stumbler] part.1 Dump stumble into specific folder

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

r+, just a few minor items to address

::: dom/system/gonk/MozStumbler.cpp
@@ +13,5 @@
> +
> +using mozilla::LogLevel;
> +static PRLogModuleInfo* GetLog()
> +{
> +  static PRLogModuleInfo* log = PR_NewLogModule("example_logger");

"mozstumbler"

@@ +260,5 @@
> +
> +nsresult
> +StumblerInfo::CellNetworkInfoToString(nsCString& aCellDesc)
> +{
> +  aCellDesc += NS_LITERAL_CSTRING("\"cellTowers\": [");

NS_LITERAL_CSTRING can be removed.
Forgive me, this is the most gecko string-handling code I have had to deal with :).
For c-strings not passed to functions expecting the abstract base type, there is no advantage to wrapping.
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings

StumblerInfo::Onready() can also remove them, I assume the code compiles fine without those there also.

@@ +306,5 @@
> +      info[keyMnc] = mnc;
> +      info[keyLac] = lac;
> +      info[keyCid] = cid;
> +
> +        cdmaCellInfo->GetEvdoDbm(&sig);

310-317 have bad indents.

@@ +326,5 @@
> +      lteCellInfo->GetRsrp(&rsrp);
> +      info[keyLac] = lac;
> +      info[keyTimingAdvance] = timingAdvance;
> +      info[keyPsc] = pcid;
> +      if  (rsrp != nsICellInfo::UNKNOWN_VALUE) {

extra space

@@ +362,5 @@
> +  XRE_GetIOMessageLoop()->PostTask(FROM_HERE, new DumpStumblerFeedingEvent(desc));
> +  return;
> +}
> +
> +#define MACROSTR(k) #k

remove

@@ +405,5 @@
> +
> +  mWifiDesc += NS_LITERAL_CSTRING(",\"wifiAccessPoints\": [");
> +  bool firstItem = true;
> +  for (uint32_t i = 0 ; i < count ; i++) {
> +    nsString temp;

temp -> call it ssid

@@ +412,5 @@
> +      STUMBLER_DBG("no ssid, skip this AP\n");
> +      continue;
> +    }
> +
> +    if (temp.Find("_nomap", 0, temp.Length()-6, 6) != -1) {

does Find() safely handle the case where length is < 6?
Is the second arg a boolean or a start position? If a boolean, use false

Comment 89

[Alphan Chen [:alchen]]

(In reply to Garvan Keeley [:garvank] from comment #88)
> Comment on attachment 8623567 [details] [diff] [review]
> 0617-[Stumbler] part.1 Dump stumble into specific folder
> 
> > +    }
> > +
> > +    if (temp.Find("_nomap", 0, temp.Length()-6, 6) != -1) {
> 
> does Find() safely handle the case where length is < 6?
> Is the second arg a boolean or a start position? If a boolean, use false
Yes, it is safe.
But I will add a check before calling Find() to reduce this call in some cases.

Comment 90

[Alphan Chen [:alchen]]

Attachment: [Stumbler] part.1 Dump stumble into specific folder. r=garvank

Here is the latest patch.

Comment 91

[:garvan]

Comment on attachment 8624080 [details] [diff] [review]
[Stumbler] part.1 Dump stumble into specific folder. r=garvank

Looks good.
Alphan, did you get the logging to work?

Comment 92

[:garvan]

Comment on attachment 8624080 [details] [diff] [review]
[Stumbler] part.1 Dump stumble into specific folder. r=garvank

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +166,5 @@
> +      if (!service) {
> +        nsContentUtils::LogMessageToConsole("Stumbler-can not get nsIMobileConnectionService \n");
> +      } else {
> +        nsCOMPtr<nsIMobileConnection> connection;
> +        service->GetItemByServiceId(0 /* Client Id */, getter_AddRefs(connection));

This should iterate the SIMs like this does:
https://dxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#421

Comment 94

[Alphan Chen [:alchen]]

(In reply to Garvan Keeley [:garvank] from comment #92)
> 
> ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
> @@ +166,5 @@
> > +      if (!service) {
> > +        nsContentUtils::LogMessageToConsole("Stumbler-can not get nsIMobileConnectionService \n");
> > +      } else {
> > +        nsCOMPtr<nsIMobileConnection> connection;
> > +        service->GetItemByServiceId(0 /* Client Id */, getter_AddRefs(connection));
> 
> This should iterate the SIMs like this does:
> https://dxr.mozilla.org/mozilla-central/source/dom/system/
> NetworkGeolocationProvider.js#421

Do we need to iterate the SIMs?
Or do "service->GetItemByServiceId(mRilDataServiceId, getter_AddRefs(connection));" as below:
https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp#1058

Comment 95

[Alphan Chen [:alchen]]

There is some lacks in the patch of phase 1.

1. Should set StumblerInfo::mCellInfoReady as 1, 
     if we don't call "connection->GetCellInfoList(callback)".
2. Should set StumblerInfo::mWifiInfoReady as 1, 
     if we don't call "wifi->GetWifiScanResults(mRequestCallback)".
3. Need to consider about multiple SIMs cases.

Comment 96

[Alphan Chen [:alchen]]

Question?

If there are two SIMs in this phone, do we need the CellNetworkInfos of both SIMs?

Comment 97

[Alphan Chen [:alchen]]

Attachment: (WIP) Bug-1154435 [Stumbler] part.2 Upload Stumble

Hi Garvan,
could you take a look about this wip patch?

Comment 98

[Hanno Schlichting]

(In reply to Alphan Chen [:alchen] from comment #96)
> If there are two SIMs in this phone, do we need the CellNetworkInfos of both
> SIMs?

Yes, that would be best. The unique ids we get from those two cards are different, and we want them all. There are some 4 SIM card slot phones we had used in the past to do stumbling, which are particularly effective. But that depends on us actually querying all SIM cards.

The cell infos from all sim cards can be combined into a single entry for upload, as the lat/lon metadata will still be the same.

Comment 99

[:garvan]

Comment on attachment 8628688 [details] [diff] [review]
(WIP) Bug-1154435 [Stumbler] part.2 Upload Stumble

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

Thanks, looks good.

::: dom/system/gonk/MozStumbler.cpp
@@ +45,5 @@
>  }
>  
> +void
> +DumpStumblerFeedingEvent::TryToUploadFile()
> +{

I like to see MOZ_ASSERT(!NS_IsMainThread()) as a self-documentation method on code. May be helpful as the code grows to see quickly that the sync xhr is safe because it isn't on the main thread.

@@ +47,5 @@
> +void
> +DumpStumblerFeedingEvent::TryToUploadFile()
> +{
> +  nsCOMPtr<nsIFile> tmpFile;
> +  nsresult rv = nsDumpUtils::OpenTempFile(Filename(1), getter_AddRefs(tmpFile),

#define OLDEST_FILE_NUMBER 1 
would help readability, and maybe it is used elsewhere in this code - i had to to think too hard reading this if 1 was oldest or newest :).

@@ +59,5 @@
> +    return;
> +  }
> +  STUMBLER_LOG("size : %d", fileSize);
> +
> +  if (fileSize > 0) {

if (fileSize < 1) { 
  return;
}

@@ +85,5 @@
> +      mXhr->SlowAbort();
> +      mXhr = nullptr;
> +    }
> +
> +    mXhr = do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv);

83 to 89:
nsCOMPtr<nsIXMLHttpRequest> xhr = do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv);

and check the value of rv

@@ +88,5 @@
> +
> +    mXhr = do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv);
> +
> +    NS_NAMED_LITERAL_CSTRING(getString, "GET");
> +    const nsAString& empty = EmptyString();

i prefer xhr->Open(......, EmptyString(), EmptyString()) and not use this varible

Comment 100

[Hanno Schlichting]

Comment on attachment 8628688 [details] [diff] [review]
(WIP) Bug-1154435 [Stumbler] part.2 Upload Stumble

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

Added a couple comments, especially around network stuff. Generally anything that can go wrong in network requests does go wrong.

::: dom/system/gonk/MozStumbler.cpp
@@ +87,5 @@
> +    }
> +
> +    mXhr = do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv);
> +
> +    NS_NAMED_LITERAL_CSTRING(getString, "GET");

This should be a postString.

@@ +108,5 @@
> +    nsString url;
> +    rv = formatter->FormatURLPref(NS_LITERAL_STRING("geo.stumbler.url"), url);
> +    NS_ENSURE_SUCCESS_VOID(rv);
> +
> +    rv = mXhr->Open(getString, NS_ConvertUTF16toUTF8(url), false, empty, empty);

As mentioned above, the request needs to be a POST request and not a GET.

@@ +112,5 @@
> +    rv = mXhr->Open(getString, NS_ConvertUTF16toUTF8(url), false, empty, empty);
> +    NS_ENSURE_SUCCESS_VOID(rv);
> +    mXhr->SetRequestHeader(NS_LITERAL_CSTRING("Content-Type"), NS_LITERAL_CSTRING("application/json"));
> +    mXhr->SetRequestHeader(NS_LITERAL_CSTRING("Content-Encoding"), NS_LITERAL_CSTRING("gzip"));
> +    mXhr->SetMozBackgroundRequest(true);

Is there some way to set a timeout for the outbound request? We typically set a timeout of 60 seconds, so if we encounter network problems (slow network, connection breaks, captive portals), we don't keep the request and its associated sockets around forever.

In case we get a timeout, we should retry the upload later. In JS this is the ontimeout callback.

@@ +114,5 @@
> +    mXhr->SetRequestHeader(NS_LITERAL_CSTRING("Content-Type"), NS_LITERAL_CSTRING("application/json"));
> +    mXhr->SetRequestHeader(NS_LITERAL_CSTRING("Content-Encoding"), NS_LITERAL_CSTRING("gzip"));
> +    mXhr->SetMozBackgroundRequest(true);
> +    rv = mXhr->Send(variant);
> +    NS_ENSURE_SUCCESS_VOID(rv);

The mXhr request might fail for various reasons. If we get a 500 or any 5xx request (501, 502, etc.) we should not consider the upload to be a success. In that case we should keep the local data and retry the upload the next time around.

Other failure cases might be DNS resolution problems, in JS those are typically caught in an onerror handler.

@@ +227,4 @@
>  DumpStumblerFeedingEvent::SetCurrentFile() {
>    nsresult rv;
>    if (sCurrentFileNumber > MAXFILENUM) {
> +      TryToUploadFile();

This might return a simple success true/false. Depending on that we can either remove the oldest file or keep it around.

But we then need to make sure that we don't keep too much data around if some device has permanent connection problems.

Comment 101

[:garvan]

Comment on attachment 8628688 [details] [diff] [review]
(WIP) Bug-1154435 [Stumbler] part.2 Upload Stumble

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

::: dom/system/gonk/MozStumbler.cpp
@@ +64,5 @@
> +    nsCOMPtr<nsIFileStream> fileStream;
> +    nsresult rv = NS_NewLocalFileStream(getter_AddRefs(fileStream), tmpFile,
> +                               PR_RDWR | PR_CREATE_FILE, 0640);
> +    NS_ENSURE_SUCCESS_VOID(rv);
> +

For lines 64-70, can it be this instead:

  nsCOMPtr<nsIInputStream> inStream;
  NS_NewLocalFileInputStream(getter_AddRefs(inStream), tmpFile);
  NS_ENSURE_TRUE_VOID(inStream);

Comment 102

[:garvan]

Running these patches, the IO_Thread crashes instantiating an XHR request, specifically js::CurrentThreadIsIonCompiling() crashes http://mxr.mozilla.org/mozilla-central/source/js/src/gc/Barrier.cpp#44

Comment 103

[Alphan Chen [:alchen]]

(In reply to Garvan Keeley [:garvank] from comment #102)
> Running these patches, the IO_Thread crashes instantiating an XHR request,
> specifically js::CurrentThreadIsIonCompiling() crashes
> http://mxr.mozilla.org/mozilla-central/source/js/src/gc/Barrier.cpp#44

I've run the patches without phase2 successfully.
What is your case?
For phase2, I will come out a new one later.
Thanks.

Comment 104

[Alphan Chen [:alchen]]

Attachment: 0716-refinement for part.1

Hi Garvan,
here is the patch based on attachment 8628688 [details] [diff] [review].
I add the handling for multi-SIM and fill the lacks mentioned in comment 95.
The change will be merged to a new patch of part 1 once the review is done.
Please take a look.
Thanks.

Comment 105

[:garvan]

Comment on attachment 8634586 [details] [diff] [review]
0716-refinement for part.1

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +180,5 @@
> +          if (!connection) {
> +            nsContentUtils::LogMessageToConsole("Stumbler-can not get nsIMobileConnection \n");
> +          } else {
> +            cellInfoNum++;
> +            connection->GetCellInfoList(mRequestCallback);

Can this return before 187 has happened?

::: dom/system/gonk/MozStumbler.h
@@ +27,2 @@
>    {}
> +  void SetWifiInfoReady();

SetWifiInfoResponseReceived()

@@ +27,3 @@
>    {}
> +  void SetWifiInfoReady();
> +  void SetCellInfoNum(int count);

SetCellInfoResponsesExpected(int count);

@@ +35,5 @@
>    nsresult CellNetworkInfoToString(nsCString& aCellDesc);
>    nsTArray<nsRefPtr<nsICellInfo>> mCellInfo;
>    nsCString mWifiDesc;
>    nsRefPtr<nsGeoPosition> mPosition;
> +  int mCellInfoNum;

mCellInfoResponsesExpected;

@@ +36,5 @@
>    nsTArray<nsRefPtr<nsICellInfo>> mCellInfo;
>    nsCString mWifiDesc;
>    nsRefPtr<nsGeoPosition> mPosition;
> +  int mCellInfoNum;
> +  int mCellInfoReadyNum;

mCellInfoResponsesReceived;

@@ +41,1 @@
>    bool mWifiInfoReady;

mIsWifiInfoResponseReceived

Comment 106

[:garvan]

A few comments:
• The upload part will need to be restructured as the XHR requires a principal and a few other things that are main-thread only.
The upload will have to be split into a thread that reads the file to upload, then return to the main to run an async XHR to perform the upload.

• I am uncertain about the use of XRE_GetIOMessageLoop(). There is little docs on this, but it appears to be mainly used for message passing between parent+child (and chrome+content), not so much as a worker pool for potentially blocking tasks.
If you look for NS_STREAMTRANSPORTSERVICE_CONTRACTID, this is used more clearly as a worker for heavy tasks. (And it is a thread pool, so there is no way stumbler can block).
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIStreamTransportService.idl

This may mean adding a guard so that if the worker runnable is not complete, just return, don't stumble.

• I would like stumbler to be more modular and not require changes to GonkGPS, so the only change in GonkGPSGeolocationProvider.cpp to be something like:
(in GonkGPS LocationCallback)
  MozStumbler::NewLocation(somewhere, timediff); // note i didn't check what info we need for stumbling
Or something equivalently minimal in terms of code changes.
Which means moving RequestCellInfo event out of that class, and the logic as to whether a location is used for stumbling also out of that class.

• To explain the above a bit more, after this patch lands, I need to plan for the next stage of stumbling code which is to move any code changes out of GonkGPS, because that code is replaced by some partners. I'll have to refactor some the nsGeolocation code for this, and that is where we eventually want to call stumbling from.

• I am on vacation next week \o/, so I won't be responsive.

Comment 107

[Alphan Chen [:alchen]]

Comment on attachment 8634586 [details] [diff] [review]
0716-refinement for part.1

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +180,5 @@
> +          if (!connection) {
> +            nsContentUtils::LogMessageToConsole("Stumbler-can not get nsIMobileConnection \n");
> +          } else {
> +            cellInfoNum++;
> +            connection->GetCellInfoList(mRequestCallback);

Originally, I think this is impossible due to scan the tower need time.
However, I will initial mCellInfoResponsesExpected as 0 to prevent surprise in later patch.
Moreover, I will call "DumpStumblerInfo()" in SetWifiInfoResponseReceived/SetCellInfoResponsesExpected if "mIsWifiInfoResponseReceived && (mCellInfoResponsesReceived == mCellInfoResponsesExpected)" it true.

Comment 108

[Alphan Chen [:alchen]]

Attachment: [Stumbler] part.1 Dump stumble into specific folder. r=garvank

Update the patch to the latest one.

Comment 109

[:garvan]

Comment on attachment 8637813 [details] [diff] [review]
[Stumbler] part.1 Dump stumble into specific folder. r=garvank

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +183,5 @@
> +            cellInfoNum++;
> +            connection->GetCellInfoList(mRequestCallback);
> +          }
> +        }
> +        mRequestCallback->SetCellInfoResponsesExpected(cellInfoNum);

Use 2 loops, which avoids the race condition:

for (uint32_t rilNum = 0; rilNum < numberOfRilServices; rilNum++) {
  ...
  if (!connection) {
   ...
  } else {
    cellInfoNum++;
 }
}
mRequestCallback->SetCellInfoResponsesExpected(cellInfoNum);

for (uint32_t rilNum = 0; rilNum < numberOfRilServices; rilNum++) {
  ...
  connection->GetCellInfoList(mRequestCallback);  
}

Comment 110

[Alphan Chen [:alchen]]

Comment on attachment 8637813 [details] [diff] [review]
[Stumbler] part.1 Dump stumble into specific folder. r=garvank

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +183,5 @@
> +            cellInfoNum++;
> +            connection->GetCellInfoList(mRequestCallback);
> +          }
> +        }
> +        mRequestCallback->SetCellInfoResponsesExpected(cellInfoNum);

We run this runnable in mainthread.
Will we meet the race condition you mentioned?

Comment 111

[:garvan]

Comment on attachment 8637813 [details] [diff] [review]
[Stumbler] part.1 Dump stumble into specific folder. r=garvank

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

A few more assertions will clarify to the reader what threads these calls are on.

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +162,5 @@
> +      : mRequestCallback(callback)
> +      {}
> +
> +    NS_IMETHOD Run() {
> +

assert that this is on the main thread

::: dom/system/gonk/MozStumbler.cpp
@@ +389,5 @@
> +
> +/* void notifyGetCellInfoList (in uint32_t count, [array, size_is (count)] in nsICellInfo result); */
> +NS_IMETHODIMP
> +StumblerInfo::NotifyGetCellInfoList(uint32_t count, nsICellInfo** aCellInfos)
> +{

assert this is main thread

@@ +407,5 @@
> +}
> +
> +/* void notifyGetCellInfoListFailed (in DOMString error); */
> +NS_IMETHODIMP StumblerInfo::NotifyGetCellInfoListFailed(const nsAString& error)
> +{

assert this is main thread

Comment 112

[Alphan Chen [:alchen]]

Attachment: [Stumbler] part.1 Dump stumble into specific folder. r=garvank

Add main thread assertion as described in comment 111.

Comment 113

[Alphan Chen [:alchen]]

Attachment: (WIP) Bug-1154435 [Stumbler] part.2 Upload Stumble to MLS database

Hi Garvan,
here is the latest patch of phase2.

Comment 114

[Alphan Chen [:alchen]]

Comment on attachment 8641543 [details] [diff] [review]
(WIP) Bug-1154435 [Stumbler] part.2 Upload Stumble to MLS database

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

::: dom/system/gonk/MozStumbler.cpp
@@ +681,5 @@
> +
> +  if (type.EqualsLiteral("load")) {
> +    STUMBLER_DBG("Got load Event : size %lld", mFileSize);
> +    // I suspect that the file upload is finish when we got load event.
> +    // Will record the amount of upload and the time of upload

I am wondering how can I know the upload is finish.
From log, I got load event just after I add event listener.

07-31 15:04:48.459 25365 25365 I GeckoConsole: In TryToUploadFil : FileSize = 13362
07-31 15:04:48.469 25365 25365 I GeckoConsole: In TryToUploadFil :add event listener
07-31 15:04:48.469 25365 25365 I GeckoConsole: In TryToUploadFil :before send
07-31 15:04:48.469 25365 25365 I GeckoConsole: Got load Event : size 13362
07-31 15:04:48.469 25365 25365 I GeckoConsole: receive l
07-31 15:04:48.479 25365 25365 I GeckoConsole: Send Success

Comment 115

[Alphan Chen [:alchen]]

(In reply to Alphan Chen [:alchen] from comment #114)
> Comment on attachment 8641543 [details] [diff] [review]
> (WIP) Bug-1154435 [Stumbler] part.2 Upload Stumble to MLS database
> 
> Review of attachment 8641543 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/MozStumbler.cpp
> @@ +681,5 @@
> > +
> > +  if (type.EqualsLiteral("load")) {
> > +    STUMBLER_DBG("Got load Event : size %lld", mFileSize);
> > +    // I suspect that the file upload is finish when we got load event.
> > +    // Will record the amount of upload and the time of upload
> 
> I am wondering how can I know the upload is finish.
> From log, I got load event just after I add event listener.
> 
> 07-31 15:04:48.459 25365 25365 I GeckoConsole: In TryToUploadFil : FileSize
> = 13362
> 07-31 15:04:48.469 25365 25365 I GeckoConsole: In TryToUploadFil :add event
> listener
> 07-31 15:04:48.469 25365 25365 I GeckoConsole: In TryToUploadFil :before send
> 07-31 15:04:48.469 25365 25365 I GeckoConsole: Got load Event : size 13362
> 07-31 15:04:48.469 25365 25365 I GeckoConsole: receive l
> 07-31 15:04:48.479 25365 25365 I GeckoConsole: Send Success

I also enlarge the size of upload file.
But the timing is similar.
07-31 15:25:19.499 27271 27271 I GeckoConsole: Alphan-In TryToUploadFil : FileSize = 12525886
07-31 15:25:19.629 27271 27271 I GeckoConsole: Alphan-In TryToUploadFil :add event listener
07-31 15:25:19.629 27271 27271 I GeckoConsole: Alphan-In TryToUploadFil :before send
07-31 15:25:19.629 27271 27271 I GeckoConsole: Alphan-Got load Event : size 12525886
07-31 15:25:19.629 27271 27271 I GeckoConsole: Alphan-receive l
07-31 15:25:19.629 27271 27271 I GeckoConsole: Alphan-Send Success

Comment 116

[Alphan Chen [:alchen]]

Attachment: Testing patch for phase2

You can use this patch to test attachment 8641543 [details] [diff] [review].
The whole process can be triggered when there is network location.

Comment 117

[:garvan]

(In reply to Alphan Chen [:alchen] from comment #116)
> Created attachment 8641601 [details] [diff] [review]
> Testing patch for phase2
> 
> You can use this patch to test attachment 8641543 [details] [diff] [review].
> The whole process can be triggered when there is network location.

How are you running this patch?  The use of nsPrincipal in here seems to conflict with my understanding (see https://dxr.mozilla.org/mozilla-central/source/caps/nsPrincipal.cpp#50) which enforces main-thread-only usage. Maybe it requires building in debug, but I think this code will crash.

Comment 118

[Alphan Chen [:alchen]]

Comment on attachment 8641543 [details] [diff] [review]
(WIP) Bug-1154435 [Stumbler] part.2 Upload Stumble to MLS database

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

::: dom/system/gonk/MozStumbler.cpp
@@ +575,5 @@
>  
> +void
> +UploadStumbleRunnable::TryToUploadFile()
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());

My bad. This should be "MOZ_ASSERT(NS_IsMainThread());"

Comment 119

[Alphan Chen [:alchen]]

Attachment: (0803-WIP) Bug-1154435 [Stumbler] part.2 Upload Stumble to MLS database

Update a new patch of phase 2.
1. Fix some problems I met when building a debug build.
2. Fix the problem mentioned in comment 118

Comment 120

[Alphan Chen [:alchen]]

Attachment: (0803) Testing patch for phase2

Here are the logs based on this testing patch. (Using Debug Build)

08-03 16:16:07.248   389   389 I GeckoConsole: Alphan-Before Dispatch UploadStumbleRunnable
08-03 16:16:07.298   389   389 I GeckoConsole: Alphan-Before AddEventListener
08-03 16:16:07.298   389   389 I GeckoConsole: Alphan-In RequestCellInfoEvent
08-03 16:16:07.348   389   389 I GeckoConsole: Alphan-Got load Event : size 12892
08-03 16:16:07.348   389   389 I GeckoConsole: Alphan-Receive l Event
08-03 16:16:07.348   389   389 I GeckoConsole: Alphan-After xhr->Send

Comment 121

[:garvan]

Comment on attachment 8642277 [details] [diff] [review]
(0803-WIP) Bug-1154435 [Stumbler] part.2 Upload Stumble to MLS database

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

::: dom/system/gonk/MozStumbler.cpp
@@ +31,5 @@
>  using namespace mozilla::dom;
>  
>  #define OLDEST_FILE_NUMBER 1
>  #define MAXFILENUM 4
> +#define MAXUPLOADFILENUM 15

Just 4 files should be enough. If they are all waiting to be uploaded then stop collecting.

@@ +603,5 @@
> +
> +  // prepare json into nsIInputStream
> +  nsCOMPtr<nsIInputStream> inStream;
> +  rv = NS_NewLocalFileInputStream(getter_AddRefs(inStream), tmpFile, -1, -1,
> +                                  nsIFileInputStream::DEFER_OPEN);

File I/O should be off the main thread.

@@ +685,5 @@
> +
> +  if (type.EqualsLiteral("load")) {
> +    STUMBLER_DBG("Got load Event : size %lld", mFileSize);
> +    // I suspect that the file upload is finish when we got load event.
> +    // Will record the amount of upload and the time of upload

I assume this is place to delete the file

@@ +687,5 @@
> +    STUMBLER_DBG("Got load Event : size %lld", mFileSize);
> +    // I suspect that the file upload is finish when we got load event.
> +    // Will record the amount of upload and the time of upload
> +  } else if (type.EqualsLiteral("error")) {
> +    STUMBLER_ERR("Upload Error");

Can you get the HTTP error code? If so, and it is a 400, delete the file.

Comment 122

[Alphan Chen [:alchen]]

(In reply to Garvan Keeley [:garvank] from comment #121)
> Comment on attachment 8642277 [details] [diff] [review]
> (0803-WIP) Bug-1154435 [Stumbler] part.2 Upload Stumble to MLS database
> 
> Review of attachment 8642277 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/MozStumbler.cpp
> @@ +31,5 @@
> >  using namespace mozilla::dom;
> >  
> >  #define OLDEST_FILE_NUMBER 1
> >  #define MAXFILENUM 4
> > +#define MAXUPLOADFILENUM 15
> 
> Just 4 files should be enough. If they are all waiting to be uploaded then
> stop collecting.
> 
Actually, I will keep the latest 4 files in current design.
I will deduce the number to 4 in next patch. 

> @@ +603,5 @@
> > +
> > +  // prepare json into nsIInputStream
> > +  nsCOMPtr<nsIInputStream> inStream;
> > +  rv = NS_NewLocalFileInputStream(getter_AddRefs(inStream), tmpFile, -1, -1,
> > +                                  nsIFileInputStream::DEFER_OPEN);
> 
> File I/O should be off the main thread.
>
OK.
> @@ +685,5 @@
> > +
> > +  if (type.EqualsLiteral("load")) {
> > +    STUMBLER_DBG("Got load Event : size %lld", mFileSize);
> > +    // I suspect that the file upload is finish when we got load event.
> > +    // Will record the amount of upload and the time of upload
> 
> I assume this is place to delete the file
Yes, I will also remove the file.
> 
> @@ +687,5 @@
> > +    STUMBLER_DBG("Got load Event : size %lld", mFileSize);
> > +    // I suspect that the file upload is finish when we got load event.
> > +    // Will record the amount of upload and the time of upload
> > +  } else if (type.EqualsLiteral("error")) {
> > +    STUMBLER_ERR("Upload Error");
> 
> Can you get the HTTP error code? If so, and it is a 400, delete the file.
I will try to find the way.

Comment 123

[Alphan Chen [:alchen]]

Comment on attachment 8642277 [details] [diff] [review]
(0803-WIP) Bug-1154435 [Stumbler] part.2 Upload Stumble to MLS database

Hi Hanno,
could you also take a look about this patch?
Thanks a lot.

Comment 124

[Hanno Schlichting]

Comment on attachment 8642277 [details] [diff] [review]
(0803-WIP) Bug-1154435 [Stumbler] part.2 Upload Stumble to MLS database

Sorry, I didn't respond to this request earlier. Talked to Garvan and
I'm going to wait with feedback until the new patch over at https://github.com/garvankeeley/fxos-stumbler is ready.

Comment 125

[Alphan Chen [:alchen]]

Attachment: [Stumbler] FxOS Geo Stumbling for Mozilla Location Service

Here is the latest patch.

Hi Garvan,
please find a reviewer for it.
Thanks.

Comment 126

[Alphan Chen [:alchen]]

Comment on attachment 8647411 [details] [diff] [review]
[Stumbler] FxOS Geo Stumbling for Mozilla Location Service

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

::: dom/system/gonk/mozstumbler/UploadStumbleRunnable.cpp
@@ +102,5 @@
> +    if (400 == statusCode) {
> +      doDelete = true;
> +    }
> +  } else {
> +    STUMBLER_DBG("Receive %s Event", NS_ConvertUTF16toUTF8(type).get());

For now, we didn't handle the "loadend" event.
Do we need to add the handle for it?

<Debug LOG>
08-13 17:18:51.720 21208 21208 I GeckoConsole: Before adding event listener
08-13 17:18:51.720 21208 21208 I GeckoConsole: Got load Event : size 15636
08-13 17:18:51.720 21208 21208 I GeckoConsole: Receive loadend Event
08-13 17:18:51.720 21208 21208 I GeckoConsole: After xhr->Send

Comment 127

[Alphan Chen [:alchen]]

I also try to add listener for "readystatechange" and get the state by "GetReadyState".
However, the timing of receiving those event is weird.


08-13 18:23:36.640 32225 32225 I GeckoConsole: Add Event Listener
08-13 18:23:36.640 32225 32225 I GeckoConsole: Receive readystatechange Event(ReadyState=4)
08-13 18:23:36.640 32225 32225 I GeckoConsole: Got load Event : size 25799091
08-13 18:23:36.640 32225 32225 I GeckoConsole: Receive loadend Event(ReadyState=4)
08-13 18:23:36.640 32225 32225 I GeckoConsole: After Send

Comment 128

[:garvan]

Comment on attachment 8647411 [details] [diff] [review]
[Stumbler] FxOS Geo Stumbling for Mozilla Location Service

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

::: dom/system/gonk/mozstumbler/UploadStumbleRunnable.cpp
@@ +62,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  // loadend catches abort, load, and error
> +  rv = target->AddEventListener(NS_LITERAL_STRING("load"), listener, false);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = target->AddEventListener(NS_LITERAL_STRING("loadend"), listener, false);

I misunderstood how loadend could be used, so lets remove that, 
and change to handling "error" and "abort". 
That means HandleEvent will get load, timeout, error, abort. That should be all the events.

Comment 129

[:garvan]

Comment on attachment 8647411 [details] [diff] [review]
[Stumbler] FxOS Geo Stumbling for Mozilla Location Service

Alphan and I collaborated on this, so it needs a 3rd party review.

High-level:
- do sparse stumbling, max ~15K a day.

Basic flow:
- get a location, scan cell+wifi (GonkGPS sets up the request and the response goes to MozStumbler class)
- When MozStumbler sees cell and wifi scans are complete, send those to WriteStumbleOnThread
- WriteStumbleOnThread either writes to a file, or sees the file is ready to upload. In the latter case, it calls UploadStumbleRunnable to upload

WriteStumbleOnThread.h has a header comment that explains things a bit more.

Comment 130

[Josh Matthews [:jdm]]

Comment on attachment 8647411 [details] [diff] [review]
[Stumbler] FxOS Geo Stumbling for Mozilla Location Service

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

Ok, my main area of concern here is with the upload - there are lots of cases where we could hit a failure and lock ourselves out of future uploads because the atomic flag is never reset. Additionally, I want to propose creating an XPCOM component to replace most of UploadStumbleRunnable::Run - creating the XHR from JS is _way_ easier to reach, and we could wrap it all in try/catch that would call back into the code that resets the upload flag. Look at http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/test/mochitest/head.js#134 for example and count the possible line reduction :)

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +161,5 @@
>    }
>  
>    NS_DispatchToMainThread(new UpdateLocationEvent(somewhere));
> +
> +  class RequestCellInfoEvent : public nsRunnable {

Prevailing style is to define classes outside of functions.

@@ +174,5 @@
> +      nsCOMPtr<nsIMobileConnectionService> service =
> +        do_GetService(NS_MOBILE_CONNECTION_SERVICE_CONTRACTID);
> +
> +      if (!service) {
> +        nsContentUtils::LogMessageToConsole("Stumbler-can not get nsIMobileConnectionService \n");

nit: return early, lose the else branch.

@@ +183,5 @@
> +        service->GetNumItems(&numberOfRilServices);
> +        for (uint32_t rilNum = 0; rilNum < numberOfRilServices; rilNum++) {
> +          service->GetItemByServiceId(rilNum /* Client Id */, getter_AddRefs(connection));
> +          if (!connection) {
> +            nsContentUtils::LogMessageToConsole("Stumbler-can not get nsIMobileConnection \n");

This error message lies.

@@ +194,5 @@
> +      }
> +
> +      // Get Wifi AP Info
> +      nsCOMPtr<nsIInterfaceRequestor> ir = do_GetService("@mozilla.org/telephony/system-worker-manager;1");
> +      if (!ir) {

I don't think this block is necessary; do_GetInterface handles nulls fine.

@@ +203,5 @@
> +        if (!wifi) {
> +          mRequestCallback->SetWifiInfoResponseReceived();
> +          nsContentUtils::LogMessageToConsole("Stumbler-can not get nsIWifi interface\n");
> +          return NS_OK;
> +        } else {

No need for else after return.

@@ +232,5 @@
> +  if (lastTime_ms == 0 || ((timediff >= STUMBLE_INTERVAL_MS) && (delta > kMinChangeInMeters))){
> +    lastTime_ms = (PR_Now() / PR_USEC_PER_MSEC);
> +    sLastLat = location->latitude;
> +    sLastLon = location->longitude;
> +    nsRefPtr<StumblerInfo> sRequestCallback = new StumblerInfo(somewhere);

nit: s prefix is for static variables.

@@ +235,5 @@
> +    sLastLon = location->longitude;
> +    nsRefPtr<StumblerInfo> sRequestCallback = new StumblerInfo(somewhere);
> +    NS_DispatchToMainThread(new RequestCellInfoEvent(sRequestCallback));
> +  } else {
> +    // if we can two continuous location update in the same place. ignore once.

Please reword this comment.

@@ -886,5 @@
>    static double sLastMLSPosLon = 0;
>  
>    if (0 != sLastMLSPosLon || 0 != sLastMLSPosLat) {
> -    // Use spherical law of cosines to calculate difference
> -    // Not quite as correct as the Haversine but simpler and cheaper

We lost this comment.

::: dom/system/gonk/mozstumbler/MozStumbler.cpp
@@ +19,5 @@
> +
> +void
> +StumblerInfo::SetWifiInfoResponseReceived()
> +{
> +  mIsWifiInfoResponseReceived = 1;

s/true/1/

@@ +21,5 @@
> +StumblerInfo::SetWifiInfoResponseReceived()
> +{
> +  mIsWifiInfoResponseReceived = 1;
> +
> +  if (mIsWifiInfoResponseReceived && (mCellInfoResponsesReceived == mCellInfoResponsesExpected)) {

nit: no need for the second set of () here.

@@ +33,5 @@
> +{
> +  mCellInfoResponsesExpected = count;
> +  STUMBLER_DBG("SetCellInfoNum (%d)\n", count);
> +
> +  if (mIsWifiInfoResponseReceived && (mCellInfoResponsesReceived == mCellInfoResponsesExpected)) {

nit: same about ()

@@ +40,5 @@
> +  }
> +}
> +
> +nsresult
> +StumblerInfo::LocationInfoToString(nsCString& aLocDesc)

nsACString&

@@ +54,5 @@
> +  NS_NAMED_LITERAL_CSTRING(acc, "accuracy");
> +  NS_NAMED_LITERAL_CSTRING(alt, "altitude");
> +  NS_NAMED_LITERAL_CSTRING(altacc, "altitudeAccuracy");
> +  NS_NAMED_LITERAL_CSTRING(head, "heading");
> +  NS_NAMED_LITERAL_CSTRING(spd, "speed");

I personally dislike these macros and prefer `nsAutoCString lat = NS_LITERAL_CSTRING("latitude");`

@@ +56,5 @@
> +  NS_NAMED_LITERAL_CSTRING(altacc, "altitudeAccuracy");
> +  NS_NAMED_LITERAL_CSTRING(head, "heading");
> +  NS_NAMED_LITERAL_CSTRING(spd, "speed");
> +
> +  std::map<nsLiteralCString, double> info;

Please use nsClassHashtable instead (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Hashtables#Which_Hashtable_Should_I_Use.3F)

@@ +67,5 @@
> +  coords->GetSpeed(&info[spd]);
> +
> +  for (auto it = info.begin(); it != info.end(); ++it) {
> +    double val = it->second;
> +    if (!IsNaN(val)){

nit: space before {

@@ +84,5 @@
> +NS_NAMED_LITERAL_CSTRING(keyPsc, "psc");
> +NS_NAMED_LITERAL_CSTRING(keyStrengthAsu, "asu");
> +NS_NAMED_LITERAL_CSTRING(keyStrengthDbm, "signalStrength");
> +NS_NAMED_LITERAL_CSTRING(keyRegistered, "serving");
> +NS_NAMED_LITERAL_CSTRING(keyTimingAdvance, "timingAdvance");

Static XPCOM string values hurt startup performance, I think. Why not use #define or just inline these?

@@ +102,5 @@
> +  info[keyCid] = cid;
> +  info[keyStrengthAsu] = sig;
> +}
> +
> +nsresult

This can be void if it only ever returns NS_OK.

@@ +103,5 @@
> +  info[keyStrengthAsu] = sig;
> +}
> +
> +nsresult
> +StumblerInfo::CellNetworkInfoToString(nsCString& aCellDesc)

nsACString&

@@ +121,5 @@
> +    }
> +
> +    STUMBLER_DBG("type=%d\n", type);
> +
> +    std::map<nsLiteralCString, int32_t> info;

Same request for nsClassHashtable.

@@ +216,5 @@
> +NS_IMETHODIMP
> +StumblerInfo::NotifyGetCellInfoList(uint32_t count, nsICellInfo** aCellInfos)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  STUMBLER_DBG("There are %d cellinfo in the result\n",count);

nit: space after ,

@@ +224,5 @@
> +  }
> +  mCellInfoResponsesReceived++;
> +  STUMBLER_DBG("NotifyGetCellInfoList mCellInfoResponsesReceived=%d,mCellInfoResponsesExpected=%d, mIsWifiInfoResponseReceived=%d\n",
> +                mCellInfoResponsesReceived, mCellInfoResponsesExpected, mIsWifiInfoResponseReceived);
> +  if (mIsWifiInfoResponseReceived && (mCellInfoResponsesReceived == mCellInfoResponsesExpected)) {

nit: no need for ()

@@ +228,5 @@
> +  if (mIsWifiInfoResponseReceived && (mCellInfoResponsesReceived == mCellInfoResponsesExpected)) {
> +    STUMBLER_DBG("Call DumpStumblerInfo from NotifyGetCellInfoList\n");
> +    DumpStumblerInfo();
> +  }
> +  return  NS_OK;

nit: extra space

@@ +238,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  mCellInfoResponsesReceived++;
> +  STUMBLER_ERR("NotifyGetCellInfoListFailedm CellInfoReadyNum=%d, mCellInfoResponsesExpected=%d, mIsWifiInfoResponseReceived=%d",
> +                mCellInfoResponsesReceived, mCellInfoResponsesExpected, mIsWifiInfoResponseReceived);
> +  if (mIsWifiInfoResponseReceived && (mCellInfoResponsesReceived == mCellInfoResponsesExpected)) {

nit: no need for ()

@@ +242,5 @@
> +  if (mIsWifiInfoResponseReceived && (mCellInfoResponsesReceived == mCellInfoResponsesExpected)) {
> +    STUMBLER_DBG("Call DumpStumblerInfo from NotifyGetCellInfoListFailed\n");
> +    DumpStumblerInfo();
> +  }
> +  return  NS_OK;

nit: extra space

@@ +262,5 @@
> +      continue;
> +    }
> +
> +    if (ssid.Length() >= 6) {
> +      if (ssid.Find("_nomap", false, ssid.Length()-6, 6) != -1) {

Use StringEndsWith instead.

@@ +286,5 @@
> +
> +    uint32_t signal;
> +    results[i]->GetSignalStrength(&signal);
> +    mWifiDesc += "\",\"signalStrength\":";
> +    mWifiDesc += nsPrintfCString("%d", signal).get();

Use AppendInt instead.

@@ +292,5 @@
> +    mWifiDesc += "}";
> +  }
> +  mWifiDesc += "]";
> +
> +  if (mCellInfoResponsesReceived == mCellInfoResponsesExpected) {

Is this really what we want to be checking here?

@@ +306,5 @@
> +StumblerInfo::Onfailure()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  STUMBLER_ERR("GetWifiScanResults Onfailure\n");
> +  if (mCellInfoResponsesReceived == mCellInfoResponsesExpected) {

Same as above.

::: dom/system/gonk/mozstumbler/MozStumbler.h
@@ +23,5 @@
> +  NS_DECL_NSICELLINFOLISTCALLBACK
> +  NS_DECL_NSIWIFISCANRESULTSREADY
> +
> +  explicit StumblerInfo(nsGeoPosition* position)
> +    : mPosition(position), mCellInfoResponsesExpected(0), mCellInfoResponsesReceived(0), mIsWifiInfoResponseReceived(0)

mCellInfoResponsesReceived is never reset to 0. Since there is no code included in this patch that uses NotifyCellInfoResponseReceived/NotifyCellInfoResponseError, it's hard for me to determine if that's a problem or not.

@@ +26,5 @@
> +  explicit StumblerInfo(nsGeoPosition* position)
> +    : mPosition(position), mCellInfoResponsesExpected(0), mCellInfoResponsesReceived(0), mIsWifiInfoResponseReceived(0)
> +  {}
> +  void SetWifiInfoResponseReceived();
> +  void SetCellInfoResponsesExpected(int count);

uint?

::: dom/system/gonk/mozstumbler/StumblerLogging.cpp
@@ +1,1 @@
> +#include "StumblerLogging.h"

License header is missing.

::: dom/system/gonk/mozstumbler/StumblerLogging.h
@@ +1,1 @@
> +#ifndef STUMBLERLOGGING_H

License header is missing.

::: dom/system/gonk/mozstumbler/UploadStumbleRunnable.cpp
@@ +26,5 @@
> +
> +  nsCOMPtr<nsIXMLHttpRequest> xhr = do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NS_NAMED_LITERAL_CSTRING(postString, "POST");

I don't think this adds much; just inline NS_LITERAL_CSTRING("POST") instead.

@@ +65,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = target->AddEventListener(NS_LITERAL_STRING("loadend"), listener, false);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = xhr->Send(variant);

If we return early anywhere before this call, UploadEnded will never be called.

@@ +92,5 @@
> +  bool doDelete = false;
> +  if (type.EqualsLiteral("load")) {
> +    STUMBLER_DBG("Got load Event : size %lld", mFileSize);
> +    // I suspect that the file upload is finish when we got load event.
> +    // Will record the amount of upload and the time of upload

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIXMLHttpRequest.idl#303 gives an event target which has separate DOM events specifically for the upload.

::: dom/system/gonk/mozstumbler/UploadStumbleRunnable.h
@@ +23,5 @@
> +
> +class UploadEventListener : public nsIDOMEventListener
> +{
> +public:
> +  UploadEventListener(nsCOMPtr<nsIXMLHttpRequest> aXHR, int64_t aFileSize);

nsIXMLHttpRequest*

@@ +25,5 @@
> +{
> +public:
> +  UploadEventListener(nsCOMPtr<nsIXMLHttpRequest> aXHR, int64_t aFileSize);
> +
> +  /*interfaces for addref and release and queryinterface*/

This comment doesn't add much.

::: dom/system/gonk/mozstumbler/WriteStumbleOnThread.cpp
@@ +16,5 @@
> +WriteStumbleOnThread::UploadFreqGuard WriteStumbleOnThread::sUploadFreqGuard = {0};
> +
> +NS_NAMED_LITERAL_CSTRING(kOutputFileNameInProgress, "stumbles.json.gz");
> +NS_NAMED_LITERAL_CSTRING(kOutputFileNameCompleted, "stumbles.done.json.gz");
> +NS_NAMED_LITERAL_CSTRING(kOutputDirName, "mozstumbler");

I would prefer #defines for these

@@ +26,5 @@
> +    sIsUploading = false;
> +    return;
> +  }
> +
> +  class DeleteRunnable : public nsRunnable

Define this outside of the method, please.

@@ +39,5 @@
> +      nsresult rv = nsDumpUtils::OpenTempFile(kOutputFileNameCompleted,
> +                                              getter_AddRefs(tmpFile),
> +                                              kOutputDirName,
> +                                              nsDumpUtils::CREATE);
> +      if (!NS_FAILED(rv)) {

NS_SUCCEEDED(rv))

@@ +125,5 @@
> +  } else if (aPart == Partition::Middle) {
> +    gzWriter->Write(",{");
> +  }
> +  gzWriter->Write(mDesc.get());
> +  //  one item is end with '}' (e.g. {item})

nit: ended

@@ +284,5 @@
> +  // prepare json into nsIInputStream
> +  nsCOMPtr<nsIInputStream> inStream;
> +  rv = NS_NewLocalFileInputStream(getter_AddRefs(inStream), tmpFile, -1, -1,
> +                                  nsIFileInputStream::DEFER_OPEN);
> +  NS_ENSURE_TRUE_VOID(inStream);

We don't reset sIsUploading here.

@@ +286,5 @@
> +  rv = NS_NewLocalFileInputStream(getter_AddRefs(inStream), tmpFile, -1, -1,
> +                                  nsIFileInputStream::DEFER_OPEN);
> +  NS_ENSURE_TRUE_VOID(inStream);
> +
> +  nsAutoCString bufStr;

nsCString instead of nsAutoCString; I would expect this file to be larger than the default stack buffer.

@@ +288,5 @@
> +  NS_ENSURE_TRUE_VOID(inStream);
> +
> +  nsAutoCString bufStr;
> +  rv = NS_ReadInputStreamToString(inStream, bufStr, fileSize);
> +  NS_ENSURE_SUCCESS_VOID(rv);

We don't reset sIsUploading here.

Comment 131

[Alphan Chen [:alchen]]

Comment on attachment 8647411 [details] [diff] [review]
[Stumbler] FxOS Geo Stumbling for Mozilla Location Service

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

::: dom/system/gonk/mozstumbler/UploadStumbleRunnable.cpp
@@ +55,5 @@
> +  xhr->SetMozBackgroundRequest(true);
> +  // 60s timeout
> +  xhr->SetTimeout(60 * 1000);
> +
> +  nsCOMPtr<EventTarget> target(do_QueryInterface(xhr));

nsCOMPtr<nsIXMLHttpRequestUpload> target;
  rv = xhr->GetUpload(getter_AddRefs(target));

@@ +92,5 @@
> +  bool doDelete = false;
> +  if (type.EqualsLiteral("load")) {
> +    STUMBLER_DBG("Got load Event : size %lld", mFileSize);
> +    // I suspect that the file upload is finish when we got load event.
> +    // Will record the amount of upload and the time of upload

I tried to replace the event target as above. (also listen timeout, load, error, abort event)
However, I cannot receive event any more.
Any idea?

Comment 132

[:garvan]

Comment on attachment 8647411 [details] [diff] [review]
[Stumbler] FxOS Geo Stumbling for Mozilla Location Service

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

::: dom/system/gonk/mozstumbler/MozStumbler.h
@@ +18,5 @@
> +class StumblerInfo final : public nsICellInfoListCallback,
> +                           public nsIWifiScanResultsReady
> +{
> +public:
> +  NS_DECL_ISUPPORTS

When trying this patch, I needed to change this to NS_DECL_THREADSAFE_ISUPPORTS, or there is an assertion failure.

Comment 133

[Alphan Chen [:alchen]]

Attachment: [Stumbler] FxOS Geo Stumbling for Mozilla Location Service

Hi Josh,
here is the latest patch.
I've done the testing with this patch.
Please have a review.
Thanks.

Comment 134

[:garvan]

Comment on attachment 8652795 [details] [diff] [review]
[Stumbler] FxOS Geo Stumbling for Mozilla Location Service

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

::: dom/system/gonk/mozstumbler/WriteStumbleOnThread.cpp
@@ +7,5 @@
> +#include "nsIInputStream.h"
> +#include "nsPrintfCString.h"
> +
> +#define MAXFILESIZE_KB (15 * 1024)
> +#define ONEDAY_IN_MSEC 1000//(24 * 60 * 60 * 1000)

remove debugging code

Comment 135

[Alphan Chen [:alchen]]

Attachment: [Stumbler] FxOS Geo Stumbling for Mozilla Location Service

Fixed the problem mentioned in comment 134.

Besides, I will file a follow-up bug for the following.
1. Refactoring to a high layer. (a function such as nsGeolocationService::geostumble(nsGeoPosition))
2. Upload Stumble as gzip format
3. Creating an XPCOM component to replace most of UploadStumbleRunnable::Run

Comment 136

[Alphan Chen [:alchen]]

Bug 1199093 is follow-up bug.

Comment 137

[:garvan]

Comment on attachment 8653238 [details] [diff] [review]
[Stumbler] FxOS Geo Stumbling for Mozilla Location Service

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

::: dom/system/gonk/mozstumbler/MozStumbler.cpp
@@ +20,5 @@
> +
> +void
> +StumblerInfo::SetWifiInfoResponseReceived()
> +{
> +  mIsWifiInfoResponseReceived = 1;

1 => true

@@ +207,5 @@
> +
> +void
> +StumblerInfo::DumpStumblerInfo()
> +{
> +

nit: newline

@@ +308,5 @@
> +    mWifiDesc += "}";
> +  }
> +  mWifiDesc += "]";
> +
> +  mIsWifiInfoResponseReceived = 1;

1 -> true

@@ +318,5 @@
> +StumblerInfo::Onfailure()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  STUMBLER_ERR("GetWifiScanResults Onfailure\n");
> +  mIsWifiInfoResponseReceived = 1;

1 -> true

::: dom/system/gonk/mozstumbler/UploadStumbleRunnable.cpp
@@ +1,1 @@
> +#include "UploadStumbleRunnable.h"

add licence header

@@ +62,5 @@
> +  // 60s timeout
> +  xhr->SetTimeout(60 * 1000);
> +
> +  nsCOMPtr<EventTarget> target(do_QueryInterface(xhr));
> +  nsCOMPtr<nsIDOMEventListener> listener = new UploadEventListener(xhr, mUploadData.Length());

Josh had asked about using 
  nsCOMPtr<nsIXMLHttpRequestUpload> target;
  xhr->GetUpload(getter_AddRefs(target));

We should mention that both Alphan and I tried this independently, and none of the 4 events arrive to this target, whereas this target does get the events.
Admittedly, this seems counter to the XHR docs.

::: dom/system/gonk/mozstumbler/WriteStumbleOnThread.cpp
@@ +1,1 @@
> +#include "WriteStumbleOnThread.h"

license header needed

Comment 138

[Alphan Chen [:alchen]]

Attachment: [Stumbler] FxOS Geo Stumbling for Mozilla Location Service

Fix the problems mentioned in comment 137.

Comment 139

[Josh Matthews [:jdm]]

Comment on attachment 8653846 [details] [diff] [review]
[Stumbler] FxOS Geo Stumbling for Mozilla Location Service

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

Better! I'm mainly concerned about our memory management now; I realized that we're probably leaking memory each time we create the XHR and add our listener to it...

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +207,5 @@
>                                          location->longitude,
>                                          location->accuracy);
>    }
>  
>    NS_DispatchToMainThread(new UpdateLocationEvent(somewhere));

This will leak memory if NS_DispatchToMainThread fails

@@ +230,5 @@
> +    lastTime_ms = (PR_Now() / PR_USEC_PER_MSEC);
> +    sLastLat = location->latitude;
> +    sLastLon = location->longitude;
> +    nsRefPtr<StumblerInfo> requestCallback = new StumblerInfo(somewhere);
> +    NS_DispatchToMainThread(new RequestCellInfoEvent(requestCallback));

This will leak memory if NS_DispatchToMainThread fails.

::: dom/system/gonk/mozstumbler/MozStumbler.cpp
@@ +58,5 @@
> +  if (!coords) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsDataHashtable<nsCStringHashKey, double> info;

This should be nsClassHashtable according to https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Hashtables#Which_Hashtable_Should_I_Use.3F

@@ +134,5 @@
> +    }
> +
> +    STUMBLER_DBG("type=%d\n", type);
> +
> +    nsDataHashtable<nsCStringHashKey, int32_t> info;

nsClassHashtable

@@ +201,5 @@
> +
> +    aCellDesc += "}";
> +  }
> +  aCellDesc += "]";
> +  return;

This can be removed.

@@ +212,5 @@
> +    STUMBLER_DBG("CellInfoReceived=%d (Expected=%d), WifiInfoResponseReceived=%d\n",
> +                  mCellInfoResponsesReceived, mCellInfoResponsesExpected, mIsWifiInfoResponseReceived);
> +    return;
> +  }
> +  mIsWifiInfoResponseReceived = 0;

s/0/false/

@@ +231,5 @@
> +  MOZ_ASSERT(target);
> +
> +  nsCOMPtr<nsIRunnable> event = new WriteStumbleOnThread(desc);
> +  target->Dispatch(event, NS_DISPATCH_NORMAL);
> +  return;

This can be removed.

@@ +277,5 @@
> +      continue;
> +    }
> +
> +    if (ssid.Length() >= 6) {
> +      if (StringEndsWith(NS_ConvertUTF16toUTF8(ssid), NS_LITERAL_CSTRING("_nomap"))) {

StringEndsWith(ssid, NS_LITERAL_STRING("_nomap"))

::: dom/system/gonk/mozstumbler/UploadStumbleRunnable.cpp
@@ +1,1 @@
> +#include "UploadStumbleRunnable.h"

nit: missing license header

@@ +65,5 @@
> +  nsCOMPtr<EventTarget> target(do_QueryInterface(xhr));
> +  nsCOMPtr<nsIDOMEventListener> listener = new UploadEventListener(xhr, mUploadData.Length());
> +
> +  const char* const sEventStrings[] = {
> +    // nsIXMLHttpRequestEventTarget event types, supported by both XHR and Upload.

But we're only using non-upload listeners right now.

@@ +69,5 @@
> +    // nsIXMLHttpRequestEventTarget event types, supported by both XHR and Upload.
> +    "abort",
> +    "error",
> +    "load",
> +    "timeout"};

nit: } on the next line.

@@ +91,5 @@
> +{
> +}
> +
> +NS_IMETHODIMP
> +UploadEventListener::HandleEvent(nsIDOMEvent* aEvent)

We'll need to remove all of the listeners that we added or we'll leak.

::: dom/system/gonk/mozstumbler/UploadStumbleRunnable.h
@@ +1,1 @@
> +#ifndef UPLOADSTUMBLERUNNABLE_H

nit: missing license header

@@ +31,5 @@
> +  NS_DECL_NSIDOMEVENTLISTENER
> +
> +protected:
> +  virtual ~UploadEventListener() {}
> +  nsCOMPtr<nsIXMLHttpRequest> mXHR;

This makes a cycle that isn't reported to the cycle collector, so I suspect the XHR and this listener will never be freed. If this were written in a JS XPCOM component we would get this for free :) We either need to do that or make this class cycle-collectable.

::: dom/system/gonk/mozstumbler/WriteStumbleOnThread.cpp
@@ +13,5 @@
> +#include "nsIInputStream.h"
> +#include "nsPrintfCString.h"
> +
> +#define MAXFILESIZE_KB (15 * 1024)
> +#define ONEDAY_IN_MSEC 1000//(24 * 60 * 60 * 1000)

That's a pretty short day.

@@ +311,5 @@
> +    sIsUploading = false;
> +    return;
> +  }
> +
> +  NS_ENSURE_SUCCESS_VOID(rv);

This is redundant now.

Comment 140

[Alphan Chen [:alchen]]

Comment on attachment 8653846 [details] [diff] [review]
[Stumbler] FxOS Geo Stumbling for Mozilla Location Service

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

Hi Josh,
thanks for your review.
There are two questions needed your help.
I will try to fix other problem in next patch.

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +207,5 @@
>                                          location->longitude,
>                                          location->accuracy);
>    }
>  
>    NS_DispatchToMainThread(new UpdateLocationEvent(somewhere));

So do we need to check the result and delete this runnable?
Or use a nsRefPtr for this runnable?

By the way, everyone use this way to dispatch a runnable to mainthread in this file.
Is there any difference that I will cause memory leak here?

::: dom/system/gonk/mozstumbler/UploadStumbleRunnable.h
@@ +31,5 @@
> +  NS_DECL_NSIDOMEVENTLISTENER
> +
> +protected:
> +  virtual ~UploadEventListener() {}
> +  nsCOMPtr<nsIXMLHttpRequest> mXHR;

We set mXHR as nullptr in UploadEventListener::HandleEvent.
Does it help?
Besides, I will remove listener before "mXHR = nullptr" to prevent leak in next patch.

WriteStumbleOnThread::UploadEnded(doDelete);
mXHR = nullptr;

Comment 141

[Alphan Chen [:alchen]]

Need your comment for next patch,
Thanks.

Comment 142

[Alphan Chen [:alchen]]

Comment on attachment 8653846 [details] [diff] [review]
[Stumbler] FxOS Geo Stumbling for Mozilla Location Service

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

For these two usages, I think double and int32_t are both simple types.
We can use nsDataHashtable according to https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Hashtables#Which_Hashtable_Should_I_Use.3F.

::: dom/system/gonk/mozstumbler/MozStumbler.cpp
@@ +58,5 @@
> +  if (!coords) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsDataHashtable<nsCStringHashKey, double> info;

I think double is a simple Types (numbers, booleans, etc).
Isn't it?

@@ +134,5 @@
> +    }
> +
> +    STUMBLER_DBG("type=%d\n", type);
> +
> +    nsDataHashtable<nsCStringHashKey, int32_t> info;

I think int32_t is also a simple Types (numbers, booleans, etc).

Comment 143

[Josh Matthews [:jdm]]

(In reply to Alphan Chen [:alchen] from comment #142)
> Comment on attachment 8653846 [details] [diff] [review]
> [Stumbler] FxOS Geo Stumbling for Mozilla Location Service
> 
> Review of attachment 8653846 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For these two usages, I think double and int32_t are both simple types.
> We can use nsDataHashtable according to
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/
> Hashtables#Which_Hashtable_Should_I_Use.3F.
> 
> ::: dom/system/gonk/mozstumbler/MozStumbler.cpp
> @@ +58,5 @@
> > +  if (!coords) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +
> > +  nsDataHashtable<nsCStringHashKey, double> info;
> 
> I think double is a simple Types (numbers, booleans, etc).
> Isn't it?
> 
> @@ +134,5 @@
> > +    }
> > +
> > +    STUMBLER_DBG("type=%d\n", type);
> > +
> > +    nsDataHashtable<nsCStringHashKey, int32_t> info;
> 
> I think int32_t is also a simple Types (numbers, booleans, etc).

You're right; I was focusing on the key type instead of the value. My mistake.

(In reply to Alphan Chen [:alchen] from comment #140)
> Comment on attachment 8653846 [details] [diff] [review]
> [Stumbler] FxOS Geo Stumbling for Mozilla Location Service
> ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
> @@ +207,5 @@
> >                                          location->longitude,
> >                                          location->accuracy);
> >    }
> >  
> >    NS_DispatchToMainThread(new UpdateLocationEvent(somewhere));
> 
> So do we need to check the result and delete this runnable?
> Or use a nsRefPtr for this runnable?

Use nsRefPtr.
 
> By the way, everyone use this way to dispatch a runnable to mainthread in
> this file.
> Is there any difference that I will cause memory leak here?

No difference; let's file another bug to correct the other usages and make it a mentored, good first bug?

> ::: dom/system/gonk/mozstumbler/UploadStumbleRunnable.h
> @@ +31,5 @@
> > +  NS_DECL_NSIDOMEVENTLISTENER
> > +
> > +protected:
> > +  virtual ~UploadEventListener() {}
> > +  nsCOMPtr<nsIXMLHttpRequest> mXHR;
> 
> We set mXHR as nullptr in UploadEventListener::HandleEvent.
> Does it help?
> Besides, I will remove listener before "mXHR = nullptr" to prevent leak in
> next patch.
> 
> WriteStumbleOnThread::UploadEnded(doDelete);
> mXHR = nullptr;

This is true; I did not consider this. I don't think it should leak, in this case.

Comment 144

[Alphan Chen [:alchen]]

Attachment: [Stumbler] FxOS Geo Stumbling for Mozilla Location Service

Hi Josh,
here is updated patch.
Please take a look.
Thanks.

Comment 145

[Alphan Chen [:alchen]]

Attachment: [Stumbler] FxOS Geo Stumbling for Mozilla Location Service

Update the patch again.

Comment 146

[Josh Matthews [:jdm]]

Comment on attachment 8655823 [details] [diff] [review]
[Stumbler] FxOS Geo Stumbling for Mozilla Location Service

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

Looks good!

::: dom/system/gonk/mozstumbler/UploadStumbleRunnable.cpp
@@ +70,5 @@
> +
> +  nsCOMPtr<EventTarget> target(do_QueryInterface(xhr));
> +  nsRefPtr<nsIDOMEventListener> listener = new UploadEventListener(xhr, mUploadData.Length());
> +
> +  const char* const sEventStrings[] = {

Let's move this into a static global and share it between it the two methods.

Comment 147

[Alphan Chen [:alchen]]

Attachment: [Stumbler] FxOS Geo Stumbling for Mozilla Location Service. r=jdm

Carry the reviewer.

Here is the try server result with other two patches.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=628f90865f75
It looks fine.

Comment 148

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7907970ddde9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a07e5bda361f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c60a6aab34a0

Comment 149

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

https://hg.mozilla.org/mozilla-central/rev/7907970ddde9
https://hg.mozilla.org/mozilla-central/rev/a07e5bda361f
https://hg.mozilla.org/mozilla-central/rev/c60a6aab34a0