Closed
Bug 1034301
Opened 10 years ago
Closed 10 years ago
Can't open smart collection: JavaScript Error: "listener is null" NetworkGeolocationProvider.js" line: 279
Categories
(Core :: DOM: Geolocation, defect)
Tracking
()
People
(Reporter: gwagner, Assigned: aus)
References
Details
(Whiteboard: [systemsfe][p=5])
Attachments
(1 file, 3 obsolete files)
3.38 KB,
patch
|
kanru
:
review+
|
Details | Diff | Splinter Review |
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•10 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
Comment 1•10 years ago
|
||
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
Updated•10 years ago
|
Reporter | ||
Comment 2•10 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)
Comment 4•10 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)
Comment 5•10 years ago
|
||
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
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Comment 6•10 years ago
|
||
(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•10 years ago
|
||
gregor, do you have a mxr link? linenumbers doesn't line up.
Assignee | ||
Comment 8•10 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.
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.
Comment 10•10 years ago
|
||
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•10 years ago
|
||
Guess I'm definitely looking into this now ;)
Status: NEW → ASSIGNED
Whiteboard: [systemsfe]
Comment 12•10 years ago
|
||
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
Comment 13•10 years ago
|
||
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
Updated•10 years ago
|
Whiteboard: [systemsfe] → [systemsfe][ETA=7/20]
Target Milestone: --- → 2.0 S6 (18july)
Assignee | ||
Comment 14•10 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•10 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•10 years ago
|
||
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•10 years ago
|
Attachment #8457877 -
Attachment description: bug1034301-v1.patch → Patch - v1 - Protect against bad listener behavior.
Comment 17•10 years ago
|
||
(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 18•10 years ago
|
||
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•10 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•10 years ago
|
||
What will actually be committed. (r+ carries)
Attachment #8457877 -
Attachment is obsolete: true
Attachment #8457914 -
Flags: review+
Assignee | ||
Comment 21•10 years ago
|
||
*sigh*, forgot to hg qrefresh before posting.
Attachment #8457938 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8457914 -
Attachment is obsolete: true
Comment 22•10 years ago
|
||
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•10 years ago
|
||
Addressed concerns we discussed over IRC.
Attachment #8457914 -
Attachment is obsolete: true
Attachment #8457938 -
Attachment is obsolete: true
Attachment #8457947 -
Flags: review?(kchen)
Updated•10 years ago
|
Attachment #8457947 -
Flags: review?(kchen) → review+
Assignee | ||
Comment 24•10 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!
Comment 25•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/de58560fbb42
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 26•10 years ago
|
||
glacroix, why is listener becoming null in the first place?
Comment 27•10 years ago
|
||
Removing qawanted tag due to bug being marked as fixed.
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Comment 28•10 years ago
|
||
(In reply to Salvador de la Puente González [:salva] from comment #12) Not repro on today's 2.0 build.
Assignee | ||
Comment 29•10 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.
Comment 30•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/0f29e8a22f67
Comment 31•10 years ago
|
||
Not exactly a great answer. :( Can we dig in and figure out what's really happening?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Updated•10 years ago
|
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•