1479354 - Null-check this.listener when reporting cached geolocation values

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Osmose [:osmose, :mkelly]]

This bug was automatically filed from Sentry: https://sentry.prod.mozaws.net/operations/nightly-js-errors/issues/2202677/

TypeError: this.listener is null

---

The raw JSON of the event in Sentry shows the error being raised from NetworkGeolocationProvider.js. There's only two occurrences of `this.listener.` in the file, one of which is behind a truth check. It's probably the other one here: https://searchfox.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#374

We're seeing between 300 and 800 of these a day on Nightly, and that's when we're only collecting 0.1% of errors occurring. I suspect this is happening pretty frequently. First occurrence logged in Sentry was April 30th, 2018.

Comment 1

[Josh Matthews [:jdm]]

Yes, we should definitely add a null check to that code.

Comment 2

[Thi Huynh]

Hi, I would like to work on this.

Comment 3

[Josh Matthews [:jdm]]

Please do! I don't know of any way to test that the changes solve the problem, unfortunately, but you should certainly be able to test that regular geolocation use from a webpage continues to work after your changes. https://www.w3schools.com/htmL/html5_geolocation.asp is a page that can do that.

Comment 4

[jwilander]

I added a null check. Is this what you are referring to? 

      if (this.listener) {
          this.listener.update(gCachedRequest.location);
      }

This is my first contribution, so let me know if I have misunderstood the bug. Not sure how I could test a cached location.

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to jwilander from comment #4)
> I added a null check. Is this what you are referring to? 
> 
>       if (this.listener) {
>           this.listener.update(gCachedRequest.location);
>       }
> 
> This is my first contribution, so let me know if I have misunderstood the
> bug. Not sure how I could test a cached location.

I hit a similar bug (and fixed by adding the notifyPositionUnavailable() function to NetworkGeolocationProvider.js [1]) when I was manually testing a geolocation web page. I hit a null this.listener when testing geolocation with Wi-Fi disabled on my laptop. There is a race condition where the WifiGeoPositionProvider gets shut down (settting this.listener to null) but the xhr request is still alive (and the xhr callback later hits the null this.listener).

You might be able to reproduce this bug (and then manually test your fix) by something like like:

1. Request a position with Wi-Fi enabled so we have a gCachedRequest.
2. Disable Wi-Fi on your computer.
3. Request a new position that tries to use the gCachedRequest. You might need to just hack the isCachedRequestMoreAccurateThanServerRequest() function to always return true. :)

[1] https://searchfox.org/mozilla-central/commit/8a194945790336ccd52d849402812d595d849879

Comment 6

[Osmose [:osmose, :mkelly]]

I'm not familiar with the code, redirecting needinfo to the mentor. :D

Comment 7

[Thi Huynh]

I think the problem is because shutdown function could be called (and set this.listener to null) after useCached is set to true at [1]. I will add a delay at [2] to reproduce the error.

[1] https://searchfox.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#367
[2] https://searchfox.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#373

Comment 8

[Thi Huynh]

Attachment: patch_v1.diff

After trying to reproduce, I don't how this.listener at line 374 [1] could be null. Could the methods of WifiGeoPositionProvider run in parallel?

Anyway, I think the null check should still be added.

[1] https://searchfox.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#374

Comment 9

[Josh Matthews [:jdm]]

Yeah, the way that this could occur is also unclear to me. My best guess is a race when shutdown is called but the timer or wifi scanning has already queued an event that will call sendLocationRequest. I don't think it's worth artificially reproducing that situation in a test.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb1e9182ad38
Null-check this.listener when reporting cached geolocation values r=jdm

Comment 11

[Chris Peterson [:cpeterson]]

Thanks for fixing this bug, Thi!

(In reply to Michael Kelly [:mkelly,:Osmose] from comment #0)
> We're seeing between 300 and 800 of these a day on Nightly, and that's when
> we're only collecting 0.1% of errors occurring. I suspect this is happening
> pretty frequently. First occurrence logged in Sentry was April 30th, 2018.

Michael, based on the Nightly error volume you see, do you we should uplift this fix to 62 Beta?

Comment 12

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/fb1e9182ad38

Comment 13

[Osmose [:osmose, :mkelly]]

(In reply to Chris Peterson [:cpeterson] from comment #11)
> Michael, based on the Nightly error volume you see, do you we should uplift
> this fix to 62 Beta?

I think so. It first occurred when 61 was still in Nightly, so it's definitely affecting 62. And the rate has been pretty consistent for at least a month. Given the sample rate and the audience size differences, I'd ballpark this as happening tens of millions of times in release.

Comment 14

[Chris Peterson [:cpeterson]]

Comment on attachment 8997090 [details] [diff] [review]
patch_v1.diff

Approval Request Comment

[Feature/Bug causing the regression]: I don't know. mkelly says the error first occurred when 61 was still in Nightly, so it's definitely affecting 62.

[User impact if declined]:
This bug is an unhandled exception in Firefox's geolocation JavaScript code. The user impact of the error is unknown, but probably not serious. However, mkelly estimates that this error is happening tens of millions of times in the Release channel.

[Is this code covered by automated tests?]: No

[Has the fix been verified in Nightly?]: Not yet. If you like, we could monitor Firefox's JavaScript error reports (nightly-js-errors on Sentry) for a few days to see if the error volume goes down.

[Needs manual test from QE? If yes, steps to reproduce]: No. We don't have manual STR.

[List of other uplifts needed for the feature/fix]: None

[Is the change risky?]: No

[Why is the change risky/not risky?]: The change is a simple null check.

[String changes made/needed]: None

Comment 15

[Chris Peterson [:cpeterson]]

The error volume in Sentry seems to have dropped since this fix landed on 2018-08-11, but it's hard to tell because (AFAIK) Sentry does not allow errors to be filtered by Firefox version.

https://sentry.prod.mozaws.net/operations/nightly-js-errors/?query=is%3Aunresolved++this.listener+is+null+&statsPeriod=14d

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8997090 [details] [diff] [review]
patch_v1.diff

Fix for a high volume geolocation error, sounds like this is good to get into release asap. Let's uplift for beta 18.

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8c9fa6f809a8