Closed Bug 1479354 Opened Last year Closed Last year

Null-check this.listener when reporting cached geolocation values

Categories

(Core :: DOM: Geolocation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: osmose, Assigned: so61pi.re, Mentored)

References

()

Details

(Keywords: good-first-bug, Whiteboard: [nightly-js-sentry:2202677][lang=js])

Attachments

(1 file)

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.
Yes, we should definitely add a null check to that code.
Mentor: josh
Keywords: good-first-bug
Whiteboard: [nightly-js-sentry:2202677] → [nightly-js-sentry:2202677][lang=js]
Summary: TypeError: this.listener is null → Null-check this.listener when reporting cached geolocation values
Hi, I would like to work on this.
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.
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.
Flags: needinfo?(mkelly)
(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
I'm not familiar with the code, redirecting needinfo to the mentor. :D
Flags: needinfo?(mkelly) → needinfo?(josh)
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
Attached patch patch_v1.diffSplinter Review
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
Attachment #8997090 - Flags: review?(josh)
Attachment #8997090 - Flags: review?(josh) → review+
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.
Flags: needinfo?(josh)
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb1e9182ad38
Null-check this.listener when reporting cached geolocation values r=jdm
Keywords: checkin-needed
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?
Assignee: nobody → so61pi.re
Flags: needinfo?(mkelly)
https://hg.mozilla.org/mozilla-central/rev/fb1e9182ad38
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(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.
Flags: needinfo?(mkelly)
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
Attachment #8997090 - Flags: approval-mozilla-beta?
See Also: → 1482555
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 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.
Attachment #8997090 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.