1009560 - Regression: Sharing an image crashes the browser @ at org.mozilla.gecko.GeckoAppShell.downloadImageForIntent(GeckoAppShell.java:2690)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

W/dalvikvm( 5713): threadid=1: thread exiting with uncaught exception (group=0x41d13da0)
E/GeckoAppShell( 5713): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
E/GeckoAppShell( 5713): android.os.NetworkOnMainThreadException
E/GeckoAppShell( 5713): 	at android.os.StrictMode$AndroidBlockGuardPolicy.onNetwork(StrictMode.java:1166)
E/GeckoAppShell( 5713): 	at java.net.InetAddress.lookupHostByName(InetAddress.java:385)
E/GeckoAppShell( 5713): 	at java.net.InetAddress.getAllByNameImpl(InetAddress.java:236)
E/GeckoAppShell( 5713): 	at java.net.InetAddress.getAllByName(InetAddress.java:214)
E/GeckoAppShell( 5713): 	at com.android.okhttp.internal.Dns$1.getAllByName(Dns.java:28)
E/GeckoAppShell( 5713): 	at com.android.okhttp.internal.http.RouteSelector.resetNextInetSocketAddress(RouteSelector.java:216)
E/GeckoAppShell( 5713): 	at com.android.okhttp.internal.http.RouteSelector.next(RouteSelector.java:122)
E/GeckoAppShell( 5713): 	at com.android.okhttp.internal.http.HttpEngine.connect(HttpEngine.java:390)
E/GeckoAppShell( 5713): 	at com.android.okhttp.internal.http.HttpEngine.sendSocketRequest(HttpEngine.java:343)
E/GeckoAppShell( 5713): 	at com.android.okhttp.internal.http.HttpEngine.sendRequest(HttpEngine.java:289)
E/GeckoAppShell( 5713): 	at com.android.okhttp.internal.http.HttpURLConnectionImpl.execute(HttpURLConnectionImpl.java:345)
E/GeckoAppShell( 5713): 	at com.android.okhttp.internal.http.HttpURLConnectionImpl.getResponse(HttpURLConnectionImpl.java:296)
E/GeckoAppShell( 5713): 	at com.android.okhttp.internal.http.HttpURLConnectionImpl.getInputStream(HttpURLConnectionImpl.java:179)
E/GeckoAppShell( 5713): 	at java.net.URL.openStream(URL.java:470)
E/GeckoAppShell( 5713): 	at org.mozilla.gecko.GeckoAppShell.downloadImageForIntent(GeckoAppShell.java:2690)
E/GeckoAppShell( 5713): 	at org.mozilla.gecko.widget.GeckoActionProvider$Callbacks.chooseActivity(GeckoActionProvider.java:191)
E/GeckoAppShell( 5713): 	at org.mozilla.gecko.widget.GeckoActionProvider$Callbacks.access$100(GeckoActionProvider.java:182)
E/GeckoAppShell( 5713): 	at org.mozilla.gecko.widget.GeckoActionProvider.chooseActivity(GeckoActionProvider.java:176)
E/GeckoAppShell( 5713): 	at org.mozilla.gecko.prompts.PromptListAdapter$1$1.onIntentSelected$7a9ca511(PromptListAdapter.java:201)
E/GeckoAppShell( 5713): 	at org.mozilla.gecko.prompts.IntentChooserPrompt$1.onPromptFinished(IntentChooserPrompt.java:85)
E/GeckoAppShell( 5713): 	at org.mozilla.gecko.prompts.Prompt.notifyClosing(Prompt.java:424)
E/GeckoAppShell( 5713): 	at org.mozilla.gecko.prompts.Prompt.closeDialog(Prompt.java:409)
E/GeckoAppShell( 5713): 	at org.mozilla.gecko.prompts.Prompt.onClick(Prompt.java:200)
E/GeckoAppShell( 5713): 	at com.android.internal.app.AlertController$AlertParams$3.onItemClick(AlertController.java:954)
E/GeckoAppShell( 5713): 	at android.widget.AdapterView.performItemClick(AdapterView.java:308)
E/GeckoAppShell( 5713): 	at android.widget.AbsListView.performItemClick(AbsListView.java:1524)
E/GeckoAppShell( 5713): 	at android.widget.AbsListView$PerformClick.run(AbsListView.java:3531)
E/GeckoAppShell( 5713): 	at android.widget.AbsListView$3.run(AbsListView.java:4898)
E/GeckoAppShell( 5713): 	at android.os.Handler.handleCallback(Handler.java:733)
E/GeckoAppShell( 5713): 	at android.os.Handler.dispatchMessage(Handler.java:95)
E/GeckoAppShell( 5713): 	at android.os.Looper.loop(Looper.java:136)
E/GeckoAppShell( 5713): 	at android.app.ActivityThread.main(ActivityThread.java:5476)
E/GeckoAppShell( 5713): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/GeckoAppShell( 5713): 	at java.lang.reflect.Method.invoke(Method.java:515)
E/GeckoAppShell( 5713): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1268)
E/GeckoAppShell( 5713): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1084)
E/GeckoAppShell( 5713): 	at dalvik.system.NativeStart.main(Native Method)

Comment 1

[Aaron Train [:aaronmt]]

My crash report:

bp-06a18e34-79ce-4805-9f15-23e122140513

Signature:

[@ android.os.NetworkOnMainThreadException: at android.os.StrictMode$AndroidBlockGuardPolicy.onNetwork(StrictMode.java)]

Comment 2

[Paul [pwd]]

As per bug 990642 comment 25, wesj did you get a chance to look at this?

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch v1

I'm not able to reproduce this crash on my phone, but I do see some strict mode warnings. This fires the intent (and downloads the image) from a background thread.

I purposefully left notifying the target that something was selected on the callers thread. That's used to update the menu UI. It should happen quickly. If the intent firing fails, it will still be called I think that's likely OK, and not much different than the previous behaviour (which also doesn't care if you successfully shared, just cares that you tried to share with an app).

There's a build here:

http://people.mozilla.com/~wjohnston/shareCrash.apk

can QA test it?

Comment 4

[Paul [pwd]]

I can confirm that sharing images is indeed working correctly in the try build and that there's no crashes.

Comment 5

[Aaron Train [:aaronmt]]

Same here, WFM. I just was sharing the main Google image header to Google+ as a test.

Comment 6

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

Comment on attachment 8425628 [details] [diff] [review]
Patch v1

Review of attachment 8425628 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to provide the same functionality, but puts networking on the background thread, so lgtm.

::: mobile/android/base/widget/GeckoActionProvider.java
@@ +189,5 @@
> +                // This may cause a download to happen. Make sure we're on the background thread.
> +                ThreadUtils.postToBackgroundThread(new Runnable() {
> +                    @Override
> +                    public void run() {
> +                        // Share image syncrhonously downloads the image before sharing it.

nit: synchronously

Comment 7

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/501b00809f10

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/501b00809f10

Comment 9

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Device: Samsung Galaxy Nexus (Android 4.2)
Build: Firefox for Android 32.0a1 (2014-05-22)

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 8425628 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 990642
User impact if declined: Crash sharing images on newer devices because of network on the main thread.
Risk to taking this patch (and alternatives if risky): Pretty low risk. Just pushes this onto a background thread like it previously was. Alternative to this is to hold the share patches up I think. We're regressed from 29 at that point, but still have feature parity with other browsers.
String or IDL/UUID changes made by this patch: None.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8425628 [details] [diff] [review]
Patch v1

Since this is verified and we have two mobile betas left (so we can backout if we find we need to) - let's take it and not regress from 29.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8425628 [details] [diff] [review]
Patch v1

Hold the phone (pun intended).  This is marked unaffected on 31 and no status on 30 - can I get someone to confirm this presents on those branches?

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8425628 [details] [diff] [review]
Patch v1

Nevermind. It's Friday.  I see that bug 990642 is approved to land all the way up to beta and this is needed in that case. I will update the flags accordingly.

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking 30/31 affected for once bug 990642 lands to branches, they will be.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/20a719d54b3e
https://hg.mozilla.org/releases/mozilla-beta/rev/ea269bc19468

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ea269bc19468

Comment 17

[Catalin Suciu [:csuciu]]

Sharing images is working correctly on Aurora 31.0a2 (2014-05-28) and Firefox 30 Beta 8