crash in java.lang.NullPointerException: at org.mozilla.mozstumbler.service.uploadthread.UploadAlarmReceiver$UploadAlarmService.onHandleIntent(UploadAlarmReceiver.java)

RESOLVED FIXED in Firefox 35

Status

()

Firefox for Android
General
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aaronmt, Assigned: garvan)

Tracking

37 Branch
Firefox 37
All
Android
Points:
---

Firefox Tracking Flags

(firefox34 unaffected, firefox35+ fixed, firefox36+ fixed, firefox37+ fixed, fennec35+)

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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)
(Assignee)

Comment 1

3 years ago
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
        }
(Assignee)

Comment 2

3 years ago
Created attachment 8533895 [details] [diff] [review]
Part 1: guard against a null intent
Attachment #8533895 - Flags: review?(vng)
(Assignee)

Comment 3

3 years ago
Created attachment 8533899 [details] [diff] [review]
Part 2: additional safety for Prefs

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 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+
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)
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 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+
(Assignee)

Comment 9

3 years ago
> 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

3 years ago
Created attachment 8535054 [details] [diff] [review]
Part 2: additional safety for Prefs

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+
[Tracking Requested - why for this release]:
tracking-fennec: ? → 35+
status-firefox34: --- → unaffected
status-firefox35: --- → affected
tracking-firefox35: --- → ?
tracking-firefox36: --- → ?
(Assignee)

Comment 12

3 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).
(Assignee)

Updated

3 years ago
Keywords: crash → checkin-needed
(Assignee)

Comment 13

3 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

3 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?
https://hg.mozilla.org/mozilla-central/rev/774b930d3c30
https://hg.mozilla.org/mozilla-central/rev/75b1795062d9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
tracking-firefox35: ? → +
tracking-firefox36: ? → +
tracking-firefox37: --- → +
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 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

3 years ago
Thanks Lukas, this is my first direct-to-Beta uplift, I forgot about marking it for Aurora
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 → ---
Garvan - Can you mark this one FIXED and open a new bug for comment 21?
Flags: needinfo?(gkeeley)
(Assignee)

Updated

3 years ago
See Also: → bug 1130052
(Assignee)

Comment 23

3 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)
(Assignee)

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.