Closed
Bug 1497526
Opened 6 years ago
Closed 6 years ago
Crash in android.os.TransactionTooLargeException: at android.os.BinderProxy.transactNative(Native Method)
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(firefox62 wontfix, firefox63+ verified, firefox64+ verified, firefox65+ verified)
VERIFIED
FIXED
Firefox 65
People
(Reporter: marcia, Assigned: petru)
References
Details
(Keywords: crash, Whiteboard: [priority:high])
Crash Data
Attachments
(1 file)
46 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
pascalc
:
approval-mozilla-release+
|
Details | Review |
This bug was filed from the Socorro interface and is report bp-d400ba89-e809-4864-8bc1-5673c0181009. ============================================================= This crash is trending up again in 63b11 after being addressed in Bug 1243305: https://bit.ly/2PoutX4. Currently the crash count in beta is larger than release. APIs range from 28-24, and a fairly good variety of devices. One comment on beta: "Firefox crashed when I was closing a website Marlboro.com" Java stack trace: android.os.TransactionTooLargeException at android.os.BinderProxy.transactNative(Native Method) at android.os.BinderProxy.transact(Binder.java:761) at android.app.IActivityManager$Stub$Proxy.activityStopped(IActivityManager.java:5155) at android.app.ActivityThread$StopInfo.run(ActivityThread.java:4146) at android.os.Handler.handleCallback(Handler.java:789) at android.os.Handler.dispatchMessage(Handler.java:98) at android.os.Looper.loop(Looper.java:164) at android.app.ActivityThread.main(ActivityThread.java:6938) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:327) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1374)
Reporter | ||
Comment 1•6 years ago
|
||
philipp mentioned in IRC this increase could have also been due to Bug 1496599.
Comment 2•6 years ago
|
||
I am able to reproduce this on Pixel 2 - Android 9 constantly with these steps: - Go to https://myoctocat.com/build-your-octocat/ - Add some elements to your octocat - Press next and choose to download it ( check the box with agree terms before) Reports: https://crash-stats.mozilla.com/report/index/82015af5-7ee5-4b56-ab0f-63d240181020 https://crash-stats.mozilla.com/report/index/c95db94f-cc0a-42df-8cc8-143ce0181020 https://crash-stats.mozilla.com/report/index/9b6e975f-95ea-437f-9ea6-b66020181022 Video: https://youtu.be/XSZsF4NirY4
Flags: needinfo?(sdaswani)
Comment 3•6 years ago
|
||
Tracking as the numbers of crashes are very high on RCs as per Marcia. We definitely would take a patch in a potential dot release if we have one. Susheel can we get somebody on it asap?
tracking-firefox63:
--- → +
Reporter | ||
Comment 4•6 years ago
|
||
(In reply to Pascal Chevrel:pascalc from comment #3) > Tracking as the numbers of crashes are very high on RCs as per Marcia. We > definitely would take a patch in a potential dot release if we have one. > Susheel can we get somebody on it asap? It appears we have 416 crashes on the 2nd RC build - here is the query that shows the crashes: https://bit.ly/2yRFdGo. Combined total between both RC is over 1000 crashes.
Updated•6 years ago
|
Petru, another fix needed immediately. Pascal, is there is a priortized list of what bugs you need fixed for dot releases?
Flags: needinfo?(sdaswani)
Flags: needinfo?(petru.lingurar)
Flags: needinfo?(pascalc)
Whiteboard: --do_not_change--[priority:high]
Reporter | ||
Comment 6•6 years ago
|
||
Removing ni for pascal since this question was answered in email.
Flags: needinfo?(pascalc)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
Flags: needinfo?(petru.lingurar)
Assignee | ||
Comment 7•6 years ago
|
||
Based on the STR from Ioana this is an old issue that stems from putting download's source url as `cookie` [1] in the download notification [2]. This cookie then functions like a tag, allowing to identify a particular download. The problem is that that download's url can be a data url for which Mozilla imposes no size limit. [3] When an Android Intent with that payload is used it counts against Binder's 1MB limit [4] which can result in the TransactionTooLargeException. In the case of downloads from https://myoctocat.com/build-your-octocat/ the data url would be for a 2000x2000 png image. From what I can tell there is no need to put the entire url in the `cookie` and would be enough to have maybe just the last 100 characters of it. The cookie is composed of - download.target.path - download.source.url (can this be truncated?) - download.startTime If we are to truncate `download.source.url` to an arbitrary length of 100? we should still get a unique `cookie` for that download. [1] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/mobile/android/modules/DownloadNotifications.jsm#184 [2] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java#367 [3] https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs [4] https://developer.android.com/reference/android/os/TransactionTooLargeException
Reporter | ||
Comment 8•6 years ago
|
||
Crash stats is showing 1902 crashes so far on 63 release, and that includes the rc builds. Build ID 20181018182531 has 1215 crashes so far, and we are currently throttled.
Assignee | ||
Comment 9•6 years ago
|
||
The issue stems from putting download's source url as `cookie` in the pending intent for when the download notification is clicked. This cookie functions like a tag, allowing to identify a particular download. The problem is that a download's url can be a Data URL for which Mozilla imposes no size limit. When an Android Intent with that payload is used it counts against Binder's 1MB limit which can result in the TransactionTooLargeException. If we are to truncate download's source url to an arbitrary length of 100 we should still get a unique `cookie` for that download and at the same time avoid cluttering Notification's contentIntent.
Assignee | ||
Comment 10•6 years ago
|
||
While the above patch fixes the `TransactionTooLargeException` for when downloading from Data URLs there might still be other cases where this could happen. This because before recently targeting API 26 any such exceptions would be treated as an internal warning, and only now they actually crash the app. So this is still an issue that we must keep a close eye on. Once we have STR for a particular case the solution should be easy but without knowing where such an exception is thrown searching for it is like looking for a needle in the haystack.
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 11•6 years ago
|
||
Pushed by nerli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8d38810fdc73 TransactionTooLargeException when downloading from Data URLs; r=jchen
Keywords: checkin-needed
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8d38810fdc73
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Assignee | ||
Updated•6 years ago
|
Flags: qe-verify+
Assignee | ||
Comment 13•6 years ago
|
||
Ioana, can you please confirm that the crash for the STR in comment 2 is not an issue anymore?
Flags: needinfo?(ioana.chiorean)
Hi Petru, should we uplift this fix to Beta65? This will give us more confidence when we want to include it in a Fennec 63.x dot release.
Flags: needinfo?(petru.lingurar)
Assignee | ||
Comment 15•6 years ago
|
||
Comment on attachment 9019732 [details] Bug 1497526 - TransactionTooLargeException when downloading from Data URLs; r?sdaswani [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: None User impact if declined: Crash when downloading from a large Data URL Is this code covered by automated tests?: No Has the fix been verified in Nightly?: No Needs manual test from QE?: Yes If yes, steps to reproduce: Try download from https://jsfiddle.net/0wn2f8Lq/show List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): Small fix for a specific crash. String changes made/needed:
Flags: needinfo?(petru.lingurar)
Attachment #9019732 -
Flags: approval-mozilla-beta?
Reporter | ||
Comment 16•6 years ago
|
||
Petru: I see crashes after the fix landed in nightly - based on your Comment 10 should I file a new bug for those?
Flags: needinfo?(petru.lingurar)
Assignee | ||
Comment 17•6 years ago
|
||
(In reply to Marcia Knous [:marcia - needinfo? me] from comment #16) > Petru: I see crashes after the fix landed in nightly - based on your Comment > 10 should I file a new bug for those? I was indeed fearful of this. I think a meta to gather all past and new such issue would be best. Then work to identify places in which this error occur and solve them one by one.
Flags: needinfo?(petru.lingurar)
Comment 18•6 years ago
|
||
(In reply to Petru-Mugurel Lingurar[:petru] from comment #13) > Ioana, can you please confirm that the crash for the STR in comment 2 is not > an issue anymore? I am not able to reproduce the crash with my previous mentioned steps. Leaving the NI to check it on beta after uplift too.
Comment 19•6 years ago
|
||
Comment on attachment 9019732 [details] Bug 1497526 - TransactionTooLargeException when downloading from Data URLs; r?sdaswani [Triage Comment] Doesn't appear to have made much of an impact on the Nightly crash rate, but this is verified to fix one instance of the TransactionTooLargeException crashes we're getting at the moment. Approved for 64.0b7 (or b6 if there ends up being one).
Attachment #9019732 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 20•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f9da620994ae
Updated•6 years ago
|
Priority: -- → P1
Comment 21•6 years ago
|
||
Petru, given that after this patch there is a noticeable drop of crashes on the last 64 beta 6 for this signature compared to previous betas, I'd like to have it uplifted to release as well, could you request an uplift to mozilla-release please? Thanks
Flags: needinfo?(petru.lingurar)
Assignee | ||
Comment 22•6 years ago
|
||
Comment on attachment 9019732 [details] Bug 1497526 - TransactionTooLargeException when downloading from Data URLs; r?sdaswani [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: None User impact if declined: Crash when downloading from a large Data URL Is this code covered by automated tests?: No Has the fix been verified in Nightly?: Yes Needs manual test from QE?: No If yes, steps to reproduce: List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): Small fix for a specific crash. Already tested in Nightly and Beta. String changes made/needed:
Flags: needinfo?(petru.lingurar)
Attachment #9019732 -
Flags: approval-mozilla-release?
Comment 23•6 years ago
|
||
Comment on attachment 9019732 [details] Bug 1497526 - TransactionTooLargeException when downloading from Data URLs; r?sdaswani Crash fix, uplift approved for release, thanks.
Attachment #9019732 -
Flags: approval-mozilla-release? → approval-mozilla-release+
Comment 24•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/ea5bceb1d4ac
Comment 25•6 years ago
|
||
I was able to reproduce this issue following the steps from comment 2 with Google Pixel (Android 9) and I can confirm the fix on latest Beta build (64.0b7) and RC (63.0.2).
Flags: qe-verify+
Flags: needinfo?(ioana.chiorean)
Updated•5 years ago
|
Status: RESOLVED → VERIFIED
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
•