Closed Bug 1106584 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect, critical)

37 Branch
All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 37
Tracking Status
firefox34 --- unaffected
firefox35 + fixed
firefox36 + fixed
firefox37 + fixed
fennec 35+ ---

People

(Reporter: aaronmt, Assigned: garvan)

References

Details

Crash Data

Attachments

(2 files, 1 obsolete file)

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 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+
> 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.
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+
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: crashcheckin-needed
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?
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
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
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+
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)
See Also: → 1130052
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: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.