If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Can't open smart collection: JavaScript Error: "listener is null" NetworkGeolocationProvider.js" line: 279

RESOLVED FIXED in Firefox 32, Firefox OS v2.0

Status

()

Core
Geolocation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gwagner, Assigned: aus)

Tracking

(Blocks: 1 bug)

unspecified
2.0 S6 (18july)
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [systemsfe][p=5])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
STR:
Set memory on Flame to 256mb
open a smart collection.
open another app so the homescreen and smart collection gets killed
go back to homescreen and try to enter a smart collection again.

JavaScript Error: "listener is null" {file: "jar:file:///system/b2g/omni.ja!/components/NetworkGeolocationProvider.js" line: 279}]
(Reporter)

Updated

3 years ago
blocking-b2g: --- → 2.0+
Summary: JavaScript Error: "listener is null" NetworkGeolocationProvider.js" line: 279 → Can't open smart collection: JavaScript Error: "listener is null" NetworkGeolocationProvider.js" line: 279
Gregor: do you know if this JavaScript error is a recent regression in 2.0? Garvan has landed a flurry of geolocation patches recently.. :)
Flags: needinfo?(anygregor)
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
Blocks: 1033669
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → ?
(Reporter)

Comment 2

3 years ago
(In reply to Chris Peterson (:cpeterson) from comment #1)
> Gregor: do you know if this JavaScript error is a recent regression in 2.0?
> Garvan has landed a flurry of geolocation patches recently.. :)

I don't know if it's a recent regression. I started testing with a memory constrained device today.
Flags: needinfo?(anygregor)
QA Wanted to branch check under 273 MB Flame.
Keywords: qawanted
Blocks: 1036153

Comment 4

3 years ago
(In reply to Jason Smith [:jsmith] from comment #3)
> QA Wanted to branch check under 273 MB Flame.

Hi, Brian, is it something your team can help? Or, it goes to Tony's team? Thanks!
Flags: needinfo?(brhuang)
Unable to reproduce the original issue on either 273mb or 256mb.

I can't even launch smart collections in 256mb. When I tap on a collection it does nothing. In 273mb I was able to launch a collection once, but it force closed itself before fully loaded, and all subsequent attempts failed (tapping on a collection does nothing).

Said JavaScript Error "listener is null" was NOT seen in logcat.

Tested on:
Device: Flame (256mb & 273mb)
Build ID: 20140703000246
Gaia: 7ad00b0bd84d5d97e0801e3b3ceaac33fcd90e05
Gecko: e87f7b398fce
Version: 32.0a2 (2.0)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
(In reply to Kevin Hu [:khu] from comment #4)
> (In reply to Jason Smith [:jsmith] from comment #3)
> > QA Wanted to branch check under 273 MB Flame.
> 
> Hi, Brian, is it something your team can help? Or, it goes to Tony's team?
> Thanks!

this will be processed under QAnalyst-Triage and then QA will check it
Flags: needinfo?(brhuang)

Comment 7

3 years ago
gregor, do you have a mxr link?  linenumbers doesn't line up.
(Assignee)

Comment 8

3 years ago
(In reply to Doug Turner (:dougt) from comment #7)
> gregor, do you have a mxr link?  linenumbers doesn't line up.

https://mxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#475

I was going to look into this but would need some guidance as I don't know that code. I'm assuming the issue is related to us killing the application that's requesting geolocation and somehow not setting our new listener as expected when it is relaunched.

Comment 9

3 years ago
I don't see 2.0 in MXR (I see 1.3, 1.4, no 2.0). I was going to check to see if 
https://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsIGeolocationProvider.idl
is as expected for NetGeoProv.js.
Assigning to Ghislain for now because the B2G release driver want all 2.0+ blockers to have an assignee. <:)
Assignee: nobody → aus
(Assignee)

Comment 11

3 years ago
Guess I'm definitely looking into this now ;)
Status: NEW → ASSIGNED
Whiteboard: [systemsfe]
I found if you kill the homescreen, wait for re-launching and try to open a smart collection. It does not work.

The STR:

1. Start the device
2. Manually kills the Homescreen from adb shell (eqivalent to wait for Homescreen to be killed by the OOM killer).
3. Wait for the Homescreen to be relaunched.
4. Try to open a smart collection.

Expected:
The smart collections opens.

Actual:
Nothing happens.

In the logcat, it is appearing:
I/GeckoDump( 2334): XXX FIXME : Got a mozContentEvent: activity-choice
I found the failure reason for my STR in Gaia. Gregor, do yo mind to test my STR? If it works, do you think we should file another bug?
Flags: needinfo?(anygregor)
Keywords: qawanted
Whiteboard: [systemsfe] → [systemsfe][ETA=7/20]
Target Milestone: --- → 2.0 S6 (18july)
(Assignee)

Comment 14

3 years ago
:salva, let's go ahead and file a separate bug for that and feel free to attach the patch and assign review to me.
Flags: needinfo?(anygregor) → needinfo?(salva)
Whiteboard: [systemsfe][ETA=7/20] → [systemsfe][p=5]
(Assignee)

Comment 15

3 years ago
After looking into this issue, I think we should probably just add a try/catch block to protect against our listener throwing. This should fix this particular bug and we should guard against bad listener behavior anyway.
(Assignee)

Comment 16

3 years ago
Created attachment 8457877 [details] [diff] [review]
Patch - v1 - Protect against bad listener behavior.

This patch is to protect against overall bad listener behavior. There is a gaia side fix for what's going on so that we don't set a null listener (see bug 1039984)
Attachment #8457877 - Flags: review?(kchen)
(Assignee)

Updated

3 years ago
Attachment #8457877 - Attachment description: bug1034301-v1.patch → Patch - v1 - Protect against bad listener behavior.
(In reply to Ghislain Aus Lacroix [:aus] from comment #14)
> :salva, let's go ahead and file a separate bug for that and feel free to
> attach the patch and assign review to me.

Ok, providing bug 1039984 for that end. I asked for your feedback and alive for review as he the recommended reviewer here.
Flags: needinfo?(salva)
Comment on attachment 8457877 [details] [diff] [review]
Patch - v1 - Protect against bad listener behavior.

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

I think this is OK. The call sites get some repetition. Could we use this.listener directly in notifyListiner and remove the first argument? Or change notifyListener to a more generic function name.

::: dom/system/NetworkGeolocationProvider.js
@@ +461,5 @@
>      if (useCached) {
>        gCachedRequest.location.timestamp = Date.now();
> +      this.notifyListener(this.listener, 
> +                          this.listener.update, 
> +                          [gCachedRequest.location]);

nit: trailing white spaces
Attachment #8457877 - Flags: review?(kchen) → review+
(Assignee)

Comment 19

3 years ago
As per IRC convo, will update notifyListener method to use 'this.listener' and we will only pass in the function we want called. I was going to rename the method to 'notify' but that is already used in this object, other naming options for this aren't very great. For example: advise, apprise, inform, etc. They all seem a little silly and unfortunately aren't as clear as 'notifyListener'. I will therefor leave the name as is.
(Assignee)

Comment 20

3 years ago
Created attachment 8457914 [details] [diff] [review]
Patch - v2 - Guard against bad listener behavior.

What will actually be committed. (r+ carries)
Attachment #8457877 - Attachment is obsolete: true
Attachment #8457914 - Flags: review+
(Assignee)

Comment 21

3 years ago
Created attachment 8457938 [details] [diff] [review]
Patch - v3 - Guard against bad listener behavior.

*sigh*, forgot to hg qrefresh before posting.
Attachment #8457938 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8457914 - Attachment is obsolete: true
Comment on attachment 8457914 [details] [diff] [review]
Patch - v2 - Guard against bad listener behavior.

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

Sorry, on a second thought, this is not right. If this.listener is null then you will get error when you try to access this.listener.update. So this patch wouldn't work.
Attachment #8457914 - Attachment is obsolete: false
Attachment #8457914 - Flags: review+ → review-
(Assignee)

Comment 23

3 years ago
Created attachment 8457947 [details] [diff] [review]
Patch - v4 - Guard against bad listener behavior.

Addressed concerns we discussed over IRC.
Attachment #8457914 - Attachment is obsolete: true
Attachment #8457938 - Attachment is obsolete: true
Attachment #8457947 - Flags: review?(kchen)
Attachment #8457947 - Flags: review?(kchen) → review+
(Assignee)

Comment 24

3 years ago
Commit (b2g-inbound): https://hg.mozilla.org/integration/b2g-inbound/rev/de58560fbb42

Uplift and all that good stuff, as usual, will be taken care of by our awesome sheriffs!
status-b2g-v2.1: ? → affected
https://hg.mozilla.org/mozilla-central/rev/de58560fbb42
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
glacroix, why is listener becoming null in the first place?
Removing qawanted tag due to bug being marked as fixed.
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
(In reply to Salvador de la Puente González [:salva] from comment #12)
Not repro on today's 2.0 build.
(Assignee)

Comment 29

3 years ago
(In reply to Doug Turner (:dougt) from comment #26)
> glacroix, why is listener becoming null in the first place?

dougt, I wasn't able to track down where this was happening and given the time constraints I just used the largest hammer available to beat this issue into submission.
https://hg.mozilla.org/releases/mozilla-aurora/rev/0f29e8a22f67
status-b2g-v2.0: affected → fixed
status-b2g-v2.1: affected → fixed
status-firefox31: --- → wontfix
status-firefox32: --- → fixed
status-firefox33: --- → fixed
Not exactly a great answer. :(

Can we dig in and figure out what's really happening?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Updated

3 years ago
Blocks: 1047484
(Reporter)

Updated

3 years ago
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.