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)

Firefox 63
Unspecified
Android
defect

Tracking

(firefox62 wontfix, firefox63+ verified, firefox64+ verified, firefox65+ verified)

VERIFIED FIXED
Firefox 65
Tracking Status
firefox62 --- wontfix
firefox63 + verified
firefox64 + verified
firefox65 + verified

People

(Reporter: marcia, Assigned: petru)

References

Details

(Keywords: crash, Whiteboard: [priority:high])

Crash Data

Attachments

(1 file)

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)
philipp mentioned in IRC this increase could have also been due to Bug 1496599.
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)
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?
(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.
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]
Removing ni for pascal since this question was answered in email.
Flags: needinfo?(pascalc)
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
Flags: needinfo?(petru.lingurar)
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
Whiteboard: --do_not_change--[priority:high] → [priority:high]
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.
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.
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.
Keywords: checkin-needed
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d38810fdc73
TransactionTooLargeException when downloading from Data URLs; r=jchen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8d38810fdc73
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Flags: qe-verify+
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)
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?
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)
(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)
Blocks: 1496435
No longer blocks: 1496435
Blocks: 1503255
(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 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+
Priority: -- → P1
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)
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 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+
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)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: