Closed Bug 1396292 Opened 3 years ago Closed 3 years ago

Crash in android.os.NetworkOnMainThreadException: at android.os.StrictMode$AndroidBlockGuardPolicy.onNetwork(StrictMode.java)

Categories

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

56 Branch
All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
fennec + ---
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: JanH, Assigned: JanH)

References

Details

(Keywords: crash, regression)

Crash Data

User Story

tried to save a picture of Mario and toad. maybe the file was in another castle?

Attachments

(3 files)

This bug was filed from the Socorro interface and is 
report bp-1fc9ebbf-c7e7-4604-9a65-1ef4d0170901.
=============================================================
android.os.NetworkOnMainThreadException
> at android.os.StrictMode$AndroidBlockGuardPolicy.onNetwork(StrictMode.java:1303)
> at java.net.Inet6AddressImpl.lookupHostByName(Inet6AddressImpl.java:86)
> at java.net.Inet6AddressImpl.lookupAllHostAddr(Inet6AddressImpl.java:74)
> at java.net.InetAddress.getAllByName(InetAddress.java:752)
> at com.android.okhttp.internal.Network$1.resolveInetAddresses(Network.java:29)
> [...]
> at org.mozilla.gecko.GeckoApp.downloadImageForSetImage(GeckoApp.java:953)
> at org.mozilla.gecko.GeckoApp.access$000(GeckoApp.java:126)
> at org.mozilla.gecko.GeckoApp$9.run(GeckoApp.java:926)
> at org.mozilla.gecko.permissions.PermissionBlock.executeRunnable(PermissionBlock.java:122)
> at org.mozilla.gecko.permissions.PermissionBlock.onPermissionsGranted(PermissionBlock.java:107)
> at org.mozilla.gecko.permissions.Permissions.processGrantResults(Permissions.java:132)
> at org.mozilla.gecko.permissions.Permissions.onRequestPermissionsResult(Permissions.java:111)
> at org.mozilla.gecko.GeckoApp.onRequestPermissionsResult(GeckoApp.java:2470)
> at android.app.Activity.dispatchRequestPermissionsResult(Activity.java:7087)
> at android.app.Activity.dispatchActivityResult(Activity.java:6939)
> [...]

What's weird is that as far as I can tell
- Strict Mode should only be enabled on local builds (https://dxr.mozilla.org/mozilla-central/rev/a3585c77e2b1bc5f5fea907e97762f7b47a12033/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1074), unless our MOZ_UPDATE_CHANNEL-based check there is somehow messed up
- even when enabled, the penalty for violations is only logging
- downloadImageForSetImage is only being called from this permissions check (https://dxr.mozilla.org/mozilla-central/rev/a3585c77e2b1bc5f5fea907e97762f7b47a12033/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#926) in setImageAs, which in turn is invoked via a "Image:SetAs" message, which is done via a *Background*ThreadListener
- PermissionBlock can optionally redispatch the callback runnable to the UiThread, but we don't invoke that option and the stack trace also clearly shows that we're just calling the Runnable's run method and not switching threads.

Unfortunately I cannot provide an answer for the first two questions, but I've tracked down the reason why we're on the main thread all of a sudden:
If we don't already have the storage permission (which we can determine synchronously), we have to obtain it and do so asynchronously. Eventually Android provides the result here (https://dxr.mozilla.org/mozilla-central/rev/a3585c77e2b1bc5f5fea907e97762f7b47a12033/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#2470) and it does so on the main thread.
User Story: (updated)
Comment on attachment 8903947 [details]
Bug 1396292 - Part 0 - Clean up imports.

https://reviewboard.mozilla.org/r/175692/#review180744
Attachment #8903947 - Flags: review+
Comment on attachment 8903949 [details]
Bug 1396292 - Part 2 - Explicitly run permissions callback on background thread where applicable.

https://reviewboard.mozilla.org/r/175696/#review180958

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:923
(Diff revision 1)
>                      }
>                  })
>                  .run(new Runnable() {
>                      @Override
>                      public void run() {
>                          downloadImageForSetImage(aSrc);

Why don't we just execute this method always on a background thread. Like showSetImageResult always runs on the UI thread.
Attachment #8903949 - Flags: review?(s.kaspari) → review+
Comment on attachment 8903948 [details]
Bug 1396292 - Part 1 - Provide facilities to explicitly run permissions check callbacks on the background thread.

https://reviewboard.mozilla.org/r/175694/#review180956

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/permissions/PermissionBlock.java:55
(Diff revision 1)
> +     * Execute all callbacks on the background thread.
> +     */
> +    public PermissionBlock onBackgroundThread() {
> +        this.onBackgroundThread = true;
> +        return this;
> +    }

This code is covered by unit tests. Would it be possible to add one for this new method too?
Attachment #8903948 - Flags: review?(s.kaspari) → review+
Comment on attachment 8903949 [details]
Bug 1396292 - Part 2 - Explicitly run permissions callback on background thread where applicable.

https://reviewboard.mozilla.org/r/175696/#review180958

> Why don't we just execute this method always on a background thread. Like showSetImageResult always runs on the UI thread.

I guess to avoid the detour via the thread's message queue, because the event listener this method is being called from is a _BackgroundThreadListener_ (and now we have also closed the PermissionsCheck loophole), so we should be on the correct thread already. With showSetImageResult on the other hand, we know we're coming in on the wrong thread and have to redispatch accordingly.

Having said that, this does indeed merit at least a corresponding assertion, so local builds will crash as well if we get this wrong in the future.
Comment on attachment 8903948 [details]
Bug 1396292 - Part 1 - Provide facilities to explicitly run permissions check callbacks on the background thread.

https://reviewboard.mozilla.org/r/175694/#review180956

> This code is covered by unit tests. Would it be possible to add one for this new method too?

I've added a test for the error when requesting both threads, but unless you have a good suggestion, I think I'll punt on testing onBackgroundThread/onUiThread alone:

* We already don't have a test for onUiThread at the moment
* Unless I'm mistaken, for testing the thread options I'd have to mock ThreadUtils, which isn't possible with Mockito as it's a static class
* I've tried using PowerMock, but that in turn causes problems with existing tests, e.g.
* in TestDownloadAction, mocking a base class method that *isn't* overridden in a child class doesn't work any more
* When then switching from Mockito's _TestRunner_ to the _PowerMockRunner_ in an attempt to see if this solves (or allows a workaround to) the above problem, the automatic mocking of various framework classes doesn't work any more
* Mocking more than one static class at a time to handle the above problem is itself problematic
* and then I gave up...
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/9d0a3883066e
Part 0 - Clean up imports. r=JanH
https://hg.mozilla.org/integration/autoland/rev/6949a7ee45ee
Part 1 - Provide facilities to explicitly run permissions check callbacks on the background thread. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/1320c22e2179
Part 2 - Explicitly run permissions callback on background thread where applicable. r=sebastian
Comment on attachment 8903948 [details]
Bug 1396292 - Part 1 - Provide facilities to explicitly run permissions check callbacks on the background thread.

Approval Request Comment
[Feature/Bug causing the regression]: Runtime permissions check, uncovered by bug 
1378253
[User impact if declined]: Using "Set Image As" will crash Firefox if we have to ask for the storage permission.
[Is this code covered by automated tests?]: A little.
[Has the fix been verified in Nightly?]: Verified locally.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Small change to allow explicitly running the permissions callback on the background thread, too.
[String changes made/needed]: none
Attachment #8903948 - Flags: approval-mozilla-beta?
Comment on attachment 8903949 [details]
Bug 1396292 - Part 2 - Explicitly run permissions callback on background thread where applicable.

See Part 1
Attachment #8903949 - Flags: approval-mozilla-beta?
It seems like crashing on main thread network access has been Android's default StrictMode policy for a long time (all apps targeting Honeycomb and up).
Comment on attachment 8903948 [details]
Bug 1396292 - Part 1 - Provide facilities to explicitly run permissions check callbacks on the background thread.

Permissions issue, should fix a new crash in 56. Let's uplift for beta 10.
Attachment #8903948 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8903949 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
tracking-fennec: ? → +
You need to log in before you can comment on or make changes to this bug.