Closed Bug 1282289 Opened 3 years ago Closed 3 years ago

Crash when using Android add-on/HelperApps.jsm and receiving malformed Activity result

Categories

(Firefox for Android :: General, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: dragoslav.mlakar, Assigned: dragoslav.mlakar, Mentored)

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.49 Safari/537.36

Steps to reproduce:

Trying to start android application with response. Intent:OpenForResult
If response doesn't have proper syntax it crashed process. 
There should be more error handling not just JSONException. Some responses doesn't have data.getData() set.

https://dxr.mozilla.org/mozilla-beta/source/mobile/android/base/java/org/mozilla/gecko/IntentHelper.java#594

https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/HelperApps.jsm

needinfo?(nalexander@mozilla.com)

tested on beta and nightly.



Actual results:

06-26 14:12:52.754 27235 27235 E GeckoCrashHandler: >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler: java.lang.NullPointerException: Attempt to invoke virtual method 'java.lang.String android.net.Uri.toString()' on a nu
ll object reference
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at org.mozilla.gecko.IntentHelper$ResultHandler.onActivityResult(IntentHelper.java:269)
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at org.mozilla.gecko.ActivityHandlerHelper.handleActivityResult(ActivityHandlerHelper.java:56)
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at org.mozilla.gecko.GeckoApp.onActivityResult(GeckoApp.java:2470)
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at org.mozilla.gecko.BrowserApp.onActivityResult(BrowserApp.java:2658)
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at android.app.Activity.dispatchActivityResult(Activity.java:6490)
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at android.app.ActivityThread.deliverResults(ActivityThread.java:3794)
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at android.app.ActivityThread.handleSendResult(ActivityThread.java:3841)
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at android.app.ActivityThread.access$1400(ActivityThread.java:154)
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1440)
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at android.os.Handler.dispatchMessage(Handler.java:102)
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at android.os.Looper.loop(Looper.java:224)
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at android.app.ActivityThread.main(ActivityThread.java:5526)
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at java.lang.reflect.Method.invoke(Native Method)
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
06-26 14:12:52.754 27235 27235 E GeckoCrashHandler:     at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)


Expected results:

Callback function should be called even if data.getData() is missing.
Component: Untriaged → General
Product: Firefox → Firefox for Android
Dragoslav, can you explain what your add-on was doing to invoke this code?

In general, however, I concur that this should be hardened.  There's a big assumption that the Activity that processes the request will return a result in a given form which is not sufficiently defensive.

This could be a [good next bug].
Mentor: nalexander, michael.l.comella
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [lang=java][good next bug]
Summary: android addon executing HelperApps.jsm → Crash when using Android add-on/HelperApps.jsm and receiving malformed Activity result
Trying to call zxing. After scan doesn't fill intent data just extras.
Probably also extras can be null in some cases so error handling must be also about that.

https://dxr.mozilla.org/mozilla-release/source/mobile/android/modules/HelperApps.jsm#198
https://dxr.mozilla.org/mozilla-release/source/mobile/android/modules/Messaging.jsm#89

 function appCallback(result){
     try{
       console.log("appCallback result is: " + result );
     } catch(e){
     }
 }
 
 function getMessage (type, uri, options = {} ) {
    let mimeType = options.mimeType;
    if (uri && mimeType == undefined) {
      mimeType = '';
    }
    return {
      type: type,
      mime: mimeType,
      action: options.action || '',
      url: uri ? uri.spec : '',
      packageName: options.packageName || '',
      className: options.className || ''
    };
 }

 let msg = getMessage('Intent:OpenForResult', uri, 
                            { packageName: "com.google.zxing.client.android", 
                              className: "com.google.zxing.client.android.CaptureActivity",
                              action: "com.google.zxing.client.android.SCAN",
                            });
 Messaging.sendRequestForResult(msg).then(appCallback);
Any more info needed to resolve issue?
Flags: needinfo?(nalexander)
(In reply to Dragoslav Mlakar from comment #3)
> Any more info needed to resolve issue?

Not right now, no.  I won't have time to fix this myself, but it's a good mentor ticket and I think the next steps are clear.  Let's see if a community member (you?) wants to pick it up.  Thanks for your help!
Flags: needinfo?(nalexander)
Well, I could fix this one, but probably must first establish some mozilla build system on computer.
Can you give some links how to start with this?
Flags: needinfo?(nalexander)
(In reply to Dragoslav Mlakar from comment #5)
> Well, I could fix this one, but probably must first establish some mozilla
> build system on computer.
> Can you give some links how to start with this?

Dragoslav -- this would be great!  We have build docs on getting started at https://developer.mozilla.org/en-US/docs/Simple_Firefox_for_Android_build.  This is all Java and JavaScript, so you can use an Artifact Build, which is much faster to get started with.  Plus you can use Android Studio, etc.  Try getting a build up and running and see how that goes.  See also https://wiki.mozilla.org/Mobile/Get_Involved for links to the IRC channel, etc.

Good luck!
Flags: needinfo?(nalexander)
https://reviewboard.mozilla.org/r/66720/#review63636

dragoslav: this is looking good!  I have one question, which you might know more about than me.

It appears to me that this doesn't really change behaviour for existing consumers, since it would have been possible for a producer to give `null` in just the right places already.  That's good, since I can't follow all the existing consumers.

One nit -- in the commit line, fix the space after the period and drop the "@mozilla.com", so:

Bug 1282289 - Crash when using Android add-on/HelperApps. r=nalexander

Thanks for your patch.  First patch?

::: mobile/android/base/java/org/mozilla/gecko/IntentHelper.java:595
(Diff revision 1)
>  
>              try {
>                  if (data != null) {
> +                    if (data.getExtras() != null) {
> -                    response.put("extras", JSONUtils.bundleToJSON(data.getExtras()));
> +                        response.put("extras", JSONUtils.bundleToJSON(data.getExtras()));
> +                    } else {

What is the reason to include `null` in the result bundle?  I see that https://developer.android.com/reference/android/os/Bundle.html more or less handle missing as equivalent to `null`, but I'd prefer to not include these keys at all.

Am I missing something?
https://reviewboard.mozilla.org/r/66720/#review63636

This is my first patch so I struggling with process flow :)

> What is the reason to include `null` in the result bundle?  I see that https://developer.android.com/reference/android/os/Bundle.html more or less handle missing as equivalent to `null`, but I'd prefer to not include these keys at all.
> 
> Am I missing something?

I was not sure what all mozilla flow expects in result collection so I put it with null rather than be missing. I test it with zxing scan intent.
Should I change it?
Flags: needinfo?(nalexander)
(In reply to Dragoslav Mlakar from comment #10)
> Should I change it?

Heh, we're both unsure.  Let's remove the `null` value for now (so just nothing under that key if it's missing).  Firefox can't assume anything right now, since it'll be missing in the new places you're trying to support so we should be safe.

Thanks, Dragoslav!
Flags: needinfo?(nalexander)
Comment on attachment 8774118 [details]
Bug 1282289 - Crash when using Android add-on/HelperApps .@mozilla.com

https://reviewboard.mozilla.org/r/66720/#review64490

Just marking this as not ready to land quite yet; re-flag me when it's updated.  Thanks!
Attachment #8774118 - Flags: review?(nalexander) → review-
https://reviewboard.mozilla.org/r/66720/#review64490

Changed it that there is no null setter and test it with zxing intents.
https://reviewboard.mozilla.org/r/66720/#review64490

Added also xpi to test intents. Very hardcoded and messy.
Comment on attachment 8775427 [details]
Bug 1282289 - Crash when using Android add-on/HelperApps.

https://reviewboard.mozilla.org/r/67606/#review65474

This looks good.  Fold the two commits into one and we should be good to land.  Thanks, Dragoslav!
Attachment #8775427 - Flags: review?(nalexander) → review+
I hope that i did it ok.
Flags: needinfo?(nalexander)
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/f8fc21bb2320
Avoid crash when using Android add-on/HelperApps. r=nalexander
(In reply to Dragoslav Mlakar from comment #18)
> I hope that i did it ok.

Looks like the fold didn't take -- I've done it and pushed it locally.  Thanks, Dragoslav!

Are you interested in another ticket?  I don't have anything in particular right now, but I know that mcomella usually has a supply of [good next bug] tickets on hand...  Mike?
Assignee: nobody → dragoslav.mlakar
Status: NEW → ASSIGNED
Flags: needinfo?(nalexander) → needinfo?(michael.l.comella)
https://hg.mozilla.org/mozilla-central/rev/f8fc21bb2320
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
(In reply to Nick Alexander :nalexander (Away Aug 9 - Aug 27) from comment #20)
> Are you interested in another ticket?  I don't have anything in particular
> right now, but I know that mcomella usually has a supply of [good next bug]
> tickets on hand...  Mike?

Maybe bug 1123040 or bug 1119401?
Flags: needinfo?(michael.l.comella)
Sorry, quite busy so will not have time for new ones. 
Maybe will do some FF addon that makes using android intents from web pages more friendly.
Thank you for help and accepting changes.
You need to log in before you can comment on or make changes to this bug.