Closed
Bug 1106584
Opened 9 years ago
Closed 9 years ago
crash in java.lang.NullPointerException: at org.mozilla.mozstumbler.service.uploadthread.UploadAlarmReceiver$UploadAlarmService.onHandleIntent(UploadAlarmReceiver.java)
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox34 unaffected, firefox35+ fixed, firefox36+ fixed, firefox37+ fixed, fennec35+)
RESOLVED
FIXED
Firefox 37
People
(Reporter: aaronmt, Assigned: garvan)
References
Details
Crash Data
Attachments
(2 files, 1 obsolete file)
1.34 KB,
patch
|
vng
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
18.51 KB,
patch
|
garvan
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-a89d8fe9-37fa-422b-9edf-72e602141129. ============================================================= java.lang.NullPointerException at org.mozilla.mozstumbler.service.uploadthread.UploadAlarmReceiver$UploadAlarmService.onHandleIntent(UploadAlarmReceiver.java:55) at android.app.IntentService$ServiceHandler.handleMessage(IntentService.java:65) at android.os.Handler.dispatchMessage(Handler.java:102) at android.os.Looper.loop(Looper.java:136) at android.os.HandlerThread.run(HandlerThread.java:61)
About to board a flight, but this is the offending code to add some additional NP checks to: @Override protected void onHandleIntent(Intent intent) { boolean isRepeating = intent.getBooleanExtra(EXTRA_IS_REPEATING, true); if (DataStorageManager.getInstance() == null) { DataStorageManager.createGlobalInstance(this, null); } upload(isRepeating); <-- line 55, not likely the actual crash, more likely a null intent }
Attachment #8533895 -
Flags: review?(vng)
When analyzing this crash, I identified a risk that we could hit Prefs.getInstance() before Prefs.createGlobalInstance(Context) has been called. The stumbler code may be entered through the upload alarm, as opposed to starting/stopping the stumbler service, so the alarm may not have created the Prefs as the internals of the stumbler expects. This patch adds an additional level of safety around the Prefs access.
Attachment #8533899 -
Flags: review?(vng)
Comment 4•9 years ago
|
||
Comment on attachment 8533895 [details] [diff] [review] Part 1: guard against a null intent Review of attachment 8533895 [details] [diff] [review]: ----------------------------------------------------------------- This looks ok to me. My only nit is that it looks like the Socorro crash report shows line 55 causing the error when in fact it's line 51. I think this is probably because of ProGuard mangling the LineNumberTable. Aside from that, I don't see any other way that a NullPointerException could have been triggered within this block of code.
Attachment #8533895 -
Flags: review?(vng) → review+
Comment 5•9 years ago
|
||
This crash was reported on line 55, but the patch corrects line 51. That said, I don't see anyway that line 55 could have actually caused a NullPointer exception - I think socorro is incorrect because of ProGuard mangling the line number table. :rnewman - can you confirm that fennec mangles the line number table? I really don't see any other way a NPE could have been triggered in those 10 lines of code.
Flags: needinfo?(rnewman)
Comment 6•9 years ago
|
||
ProGuard might eliminate whole methods (and thus remove stack frames), but I don't think it changes line numbers. Those are tracked during compilation into bytecode, IIRC. Preprocessing will make line numbers incorrect. But see the crash report metadata: sp-processor08_phx1_mozilla_com.6113:2012; MozillaProcessorAlgorithm2015; JavaSignatureTool: stack trace line 1 is not in the expected format; skunk_classifier: reject - not a plugin hang so perhaps we can't trust this.
Flags: needinfo?(rnewman)
Comment 7•9 years ago
|
||
Comment on attachment 8533899 [details] [diff] [review] Part 2: additional safety for Prefs Review of attachment 8533899 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/uploadthread/AsyncUploader.java @@ -111,5 @@ > > private class Submitter extends AbstractCommunicator { > private static final String SUBMIT_URL = "https://location.services.mozilla.com/v1/submit"; > > - public Submitter() { We should probably set some user-agent string. Some spambot filters reject empty string user-agents right away. Not sure what our ops people do, but we might as well set something like "fennec-mozstumbler" if the Prefs objects doesn't exist.
Attachment #8533899 -
Flags: review?(vng) → review+
Comment 8•9 years ago
|
||
Here's the pattern we follow for UAs in background services: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/FxAccountConstants.java.in#26
> We should probably set some user-agent string. Some spambot filters reject
> empty string user-agents right away. Not sure what our ops people do, but
> we might as well set something like "fennec-mozstumbler" if the Prefs
> objects doesn't exist.
If the UA is not set, this is an unknown, erroneous, code path. As long as it is set to something that it easy to filter on the server side. How about "fennec-stumbler-unset-user-agent", and we can watch for that.
Assignee | ||
Comment 10•9 years ago
|
||
Carry over r+ from vng. Updated with a default user agent in case Prefs is not initialized.
Attachment #8533899 -
Attachment is obsolete: true
Attachment #8535054 -
Flags: review+
Comment 11•9 years ago
|
||
[Tracking Requested - why for this release]:
tracking-fennec: ? → 35+
status-firefox34:
--- → unaffected
status-firefox35:
--- → affected
tracking-firefox35:
--- → ?
tracking-firefox36:
--- → ?
Assignee | ||
Comment 12•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0dc4847399de Try is green (Some non-relevant red tests from try servers being shut down).
Keywords: crash → checkin-needed
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8535054 [details] [diff] [review] Part 2: additional safety for Prefs Approval Request Comment [Feature/regressing bug #]: Geo stumbler for Fennec [User impact if declined]: Potential crash when users have opted-in to stumbling (well, in this isolated non-reproducible case) [Describe test coverage new/current, TBPL]: None [Risks and why]: No risk. Just added safety checks to handle unexpected null states. [String/UUID change made/needed]: None.
Attachment #8535054 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8533895 [details] [diff] [review] Part 1: guard against a null intent Approval Request Comment [Feature/regressing bug #]: Geo stumbler for Fennec [User impact if declined]: Potential crash when users have opted-in to stumbling (well, in this isolated non-reproducible case) [Describe test coverage new/current, TBPL]: None [Risks and why]: No risk. Just added one additional null pointer check for the area this crash was occurring. [String/UUID change made/needed]: None.
Attachment #8533895 -
Flags: approval-mozilla-beta?
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/774b930d3c30 https://hg.mozilla.org/integration/fx-team/rev/75b1795062d9
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/774b930d3c30 https://hg.mozilla.org/mozilla-central/rev/75b1795062d9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
Updated•9 years ago
|
Comment 17•9 years ago
|
||
Comment on attachment 8533895 [details] [diff] [review] Part 1: guard against a null intent [Triage Comment] looks like this is needed on aurora too, approving for uplift.
Attachment #8533895 -
Flags: approval-mozilla-beta?
Attachment #8533895 -
Flags: approval-mozilla-beta+
Attachment #8533895 -
Flags: approval-mozilla-aurora+
Comment 18•9 years ago
|
||
Comment on attachment 8535054 [details] [diff] [review] Part 2: additional safety for Prefs [Triage Comment] same here.
Attachment #8535054 -
Flags: approval-mozilla-beta?
Attachment #8535054 -
Flags: approval-mozilla-beta+
Attachment #8535054 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 19•9 years ago
|
||
Thanks Lukas, this is my first direct-to-Beta uplift, I forgot about marking it for Aurora
Comment 20•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/586519c20ccd https://hg.mozilla.org/releases/mozilla-aurora/rev/a4222a91e3bb https://hg.mozilla.org/releases/mozilla-beta/rev/79b650cd0944 https://hg.mozilla.org/releases/mozilla-beta/rev/6117947187de
I see an instance of this crash present on Beta (36). From the Play Store console: java.lang.NullPointerException at org.mozilla.mozstumbler.service.uploadthread.UploadAlarmReceiver$UploadAlarmService.onHandleIntent(UploadAlarmReceiver.java:56) at android.app.IntentService$ServiceHandler.handleMessage(IntentService.java:65) at android.os.Handler.dispatchMessage(Handler.java:102) at android.os.Looper.loop(Looper.java:212) at android.os.HandlerThread.run(HandlerThread.java:61)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 22•9 years ago
|
||
Garvan - Can you mark this one FIXED and open a new bug for comment 21?
Flags: needinfo?(gkeeley)
Assignee | ||
Comment 23•9 years ago
|
||
Sorry, need to add REOPENED to my personal bug filters, I didn't see this one. Created: https://bugzilla.mozilla.org/show_bug.cgi?id=1130052 Addressing it now
Flags: needinfo?(gkeeley)
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•