Closed Bug 1034301 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Geolocation, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
2.0 S6 (18july)
blocking-b2g 2.0+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: gwagner, Assigned: aus)

References

Details

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

Attachments

(1 file, 3 obsolete files)

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}]
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
(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
(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)
gregor, do you have a mxr link?  linenumbers doesn't line up.
(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.
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
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)
: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]
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.
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)
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+
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.
What will actually be committed. (r+ carries)
Attachment #8457877 - Attachment is obsolete: true
Attachment #8457914 - Flags: review+
*sigh*, forgot to hg qrefresh before posting.
Attachment #8457938 - Flags: review+
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-
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+
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!
https://hg.mozilla.org/mozilla-central/rev/de58560fbb42
Status: ASSIGNED → RESOLVED
Closed: 5 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.
(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.
Not exactly a great answer. :(

Can we dig in and figure out what's really happening?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 1047484
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.