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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dragoslav Mlakar]

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?([e-mail])

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.

Comment 1

[Nick Alexander :nalexander [he/him]]

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].

Comment 2

[Dragoslav Mlakar]

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);

Comment 3

[Dragoslav Mlakar]

Any more info needed to resolve issue?

Comment 4

[Nick Alexander :nalexander [he/him]]

(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!

Comment 5

[Dragoslav Mlakar]

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?

Comment 6

[Nick Alexander :nalexander [he/him]]

(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!

Comment 7

[Dragoslav Mlakar]

Attachment: Bug 1282289 - Crash when using Android add-on/HelperApps [e-mail]

Review commit: https://reviewboard.mozilla.org/r/66720/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66720/

Comment 8

[Nick Alexander :nalexander [he/him]]

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?

Comment 9

[Dragoslav Mlakar]

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.

Comment 10

[Dragoslav Mlakar]

Should I change it?

Comment 11

[Nick Alexander :nalexander [he/him]]

(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!

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8774118 [details]
Bug 1282289 - Crash when using Android add-on/HelperApps [e-mail]

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!

Comment 13

[Dragoslav Mlakar]

Attachment: Bug 1282289 - Crash when using Android add-on/HelperApps.

Review commit: https://reviewboard.mozilla.org/r/67606/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67606/

Comment 14

[Dragoslav Mlakar]

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

Changed it that there is no null setter and test it with zxing intents.

Comment 15

[Dragoslav Mlakar]

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

Added also xpi to test intents. Very hardcoded and messy.

Comment 16

[Nick Alexander :nalexander [he/him]]

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!

Comment 17

[Dragoslav Mlakar]

https://reviewboard.mozilla.org/r/66718/#review65572

Comment 18

[Dragoslav Mlakar]

I hope that i did it ok.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/f8fc21bb2320
Avoid crash when using Android add-on/HelperApps. r=nalexander

Comment 20

[Nick Alexander :nalexander [he/him]]

(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?

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f8fc21bb2320

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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?

Comment 23

[Dragoslav Mlakar]

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.