1253399 - Crash when connecting with DevTools over WiFi and barcode scanner not installed

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Had a report over IRC from dx that Fennec can crash if you try to connect with DevTools via WiFi and the barcode app is not installed.

I believe they were testing with 44 at the time.

Comment 1

[dx]

My crash report:

https://crash-stats.mozilla.com/report/index/d966bc1f-3d03-4335-821b-a37a82160303

Other reports: (since the same signature covers different, unrelated intents)

https://crash-stats.mozilla.com/signature/?java_stack_trace=com.google.zxing.client.android.SCAN&signature=android.content.ActivityNotFoundException%3A+at+android.app.Instrumentation.checkStartActivityResult%28Instrumentation.java%29#reports

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The WiFi debugging option on the device is disabled if the barcode scanner app is not installed.  However, that does not protect against removing the scanner app later on.

So, possible STR:

1. Install Barcode Scanner app
2. Enable WiFi debugging in Fennec
3. Uninstall Barcode Scanner app
4. Connect to device via WiFi
5. Press Scan on device auth prompt

This will crash Fennec since the scanner app is no longer there.  So, we should catch that case and probably provide some error message.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I have a fix for this locally, but I am at a work week, and it is hard to reproduce the issue on this WiFi network.  Will continue when I get back home next week.

Comment 4

[:Margaret Leibovic]

Have you been able to make any progress on this bug?

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: WIP: Attempt to stop crashing

(In reply to :Margaret Leibovic from comment #4)
> Have you been able to make any progress on this bug?

I think I had some build trouble at the time and then it fell off my radar.

I gave it another try just now, and I've managed to prevent the crash by catching the ActivityNotFoundException, but then Fennec seems to hang after that.  I'm not sure why that would happen or how to debug it...  I don't have a rooted phone at the moment, so a lot of the debug tools don't work for me.

Do you know why it would hang or how to investigate it?  Is there something I needed to do with the Messaging API when it fails?

Comment 6

[:Margaret Leibovic]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) (at work week until June 21) from comment #5)
> Created attachment 8762176 [details] [diff] [review]
> WIP: Attempt to stop crashing
> 
> (In reply to :Margaret Leibovic from comment #4)
> > Have you been able to make any progress on this bug?
> 
> I think I had some build trouble at the time and then it fell off my radar.
> 
> I gave it another try just now, and I've managed to prevent the crash by
> catching the ActivityNotFoundException, but then Fennec seems to hang after
> that.  I'm not sure why that would happen or how to debug it...  I don't
> have a rooted phone at the moment, so a lot of the debug tools don't work
> for me.

You shouldn't need a rooted phone to debug an app crash. Are you using Android Studio? I believe the Java debugger should Just Work if you have that set up.

> Do you know why it would hang or how to investigate it?  Is there something
> I needed to do with the Messaging API when it fails?

I'm not sure why Fennec would hang. Are there any errors in the log? I don't think this should have to do with the Messaging API.

Maybe Sebastian has some ideas here?

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

I can reproduce this with the STR and applied the patch.

It seems like the only problem is the sendError() call from the catch block: onActivityResult() is called and the callback is executed - after that we catch the exception and try to call the callback again (exception / crash).

It's weird that onActivityResult() gets called even though we were not able to start an activity. According to Stackoverflow our launchMode might be triggering this:
http://stackoverflow.com/questions/3354955/onactivityresult-called-prematurely

You could:
- Not call the callback yourself and rely on onActivityResult. But this is a bit scary and we need to make sure that this calls onError() and not onSuccess() in this case.
- OR: Query the PackageManager upfront to see if there's an activity that can handle the Intent. If there's none then do not do anything and just invoke onError().

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1253399 - Avoid crashes by checking for QR scanner app.

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

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a77122ade343

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8763982 [details]
Bug 1253399 - Avoid crashes by checking for QR scanner app.

https://reviewboard.mozilla.org/r/60056/#review57226

::: mobile/android/base/java/org/mozilla/gecko/DevToolsAuthHelper.java:51
(Diff revision 1)
> +        } catch (final SecurityException e) {
> +            Log.w(LOGTAG, "Forbidden to launch activity.", e);
> +            callback.sendError("Forbidden to launch activity.");
> +        }

Did you test this specific case? I wonder if this will result in the same error because the callback is executed twice (I'm not sure if onActivityResult() will be called here).

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/60056/#review57226

> Did you test this specific case? I wonder if this will result in the same error because the callback is executed twice (I'm not sure if onActivityResult() will be called here).

No, I was just cargo-culting from `ActivityHandlerHelper.startIntentAndCatch`.  Looks like Android docs say it shouldn't throw a SecurityException anyway, and I don't have a test case, so I'll just remove this part for now.

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8763982 [details]
Bug 1253399 - Avoid crashes by checking for QR scanner app.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60056/diff/1-2/

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Marking 48 and 49 fix-optional, up to Android team if they'd like to uplift.  This is not a new issue, it's been around for since WiFi debugging originally appeared.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/7e6896f7bed5
Avoid crashes by checking for QR scanner app. r=sebastian

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/7e6896f7bed5