Closed
Bug 1396292
Opened 7 years ago
Closed 7 years ago
Crash in android.os.NetworkOnMainThreadException: at android.os.StrictMode$AndroidBlockGuardPolicy.onNetwork(StrictMode.java)
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(fennec+, firefox55 unaffected, firefox56 fixed, firefox57 fixed)
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)
59 bytes,
text/x-review-board-request
|
JanH
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
sebastian
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
sebastian
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
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.
Assignee | ||
Updated•7 years ago
|
User Story: (updated)
Assignee | ||
Updated•7 years ago
|
status-firefox57:
--- → affected
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8903947 [details] Bug 1396292 - Part 0 - Clean up imports. https://reviewboard.mozilla.org/r/175692/#review180744
Attachment #8903947 -
Flags: review+
Comment 5•7 years ago
|
||
mozreview-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 6•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
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...
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
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
Assignee | ||
Comment 12•7 years ago
|
||
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?
Assignee | ||
Comment 13•7 years ago
|
||
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?
Assignee | ||
Comment 14•7 years ago
|
||
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 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9d0a3883066e https://hg.mozilla.org/mozilla-central/rev/6949a7ee45ee https://hg.mozilla.org/mozilla-central/rev/1320c22e2179
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•7 years ago
|
status-firefox55:
--- → unaffected
Updated•7 years ago
|
Keywords: regression
Comment 16•7 years ago
|
||
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+
Comment 17•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/64b7ee7060ed https://hg.mozilla.org/releases/mozilla-beta/rev/4bc7d657fca2
Flags: in-testsuite+
Updated•7 years ago
|
Attachment #8903949 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
tracking-fennec: ? → +
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•