crash in android.os.TransactionTooLargeException: at android.os.BinderProxy.transactNative(Native Method)

VERIFIED FIXED in Firefox 49

Status

()

Firefox for Android
General
--
critical
VERIFIED FIXED
2 years ago
6 months ago

People

(Reporter: Ninu, Assigned: mcomella)

Tracking

({crash})

Trunk
Firefox 49
ARM
Android
crash
Points:
---

Firefox Tracking Flags

(firefox44 affected, firefox45 affected, firefox46 affected, firefox47 affected, firefox49 fixed, fennec47+, firefox54 verified, firefox55 verified)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-ffcff69b-6da0-49bd-84ba-ea9f22160127.
=============================================================

Device: Nexus 7 (Android 5.1.1)
Build : Aurora(46.0a2)

Steps to reproduce:
1. Go to mobile.twitter.com
2. Write a tweet and attach an immage
3. Tap multiple times on the loaded image/long tap the image in order to trigger context menu

Result:
Firefox crashes

https://twitter.com/fcassia/status/688774323268628480
(Reporter)

Updated

2 years ago
tracking-fennec: --- → ?
About 350 crashes/week on 43.
Wat.

In the absence of wesj, let's throw this at mcomella and bnicholson.


android.os.TransactionTooLargeException
	at android.os.BinderProxy.transactNative(Native Method)
	at android.os.BinderProxy.transact(Binder.java:496)
	at android.content.pm.IPackageManager$Stub$Proxy.queryIntentActivities(IPackageManager.java:2489)
	at android.app.ApplicationPackageManager.queryIntentActivitiesAsUser(ApplicationPackageManager.java:565)
	at android.app.ApplicationPackageManager.queryIntentActivities(ApplicationPackageManager.java:557)
	at org.mozilla.gecko.widget.ActivityChooserModel.ensureConsistentState(ActivityChooserModel.java:722)
	at org.mozilla.gecko.widget.ActivityChooserModel.setIntent(ActivityChooserModel.java:413)
	at org.mozilla.gecko.widget.GeckoActionProvider.setIntent(GeckoActionProvider.java:189)
	at org.mozilla.gecko.prompts.PromptListAdapter.getView(PromptListAdapter.java:233)
	at android.widget.AbsListView.obtainView(AbsListView.java:2347)
	at android.widget.ListView.measureHeightOfChildren(ListView.java:1270)
	at android.widget.ListView.onMeasure(ListView.java:1182)
	at android.view.View.measure(View.java:17547)
	at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:5535)
	at android.widget.FrameLayout.onMeasure(FrameLayout.java:436)
	at android.view.View.measure(View.java:17547)
	at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:5535)
	at android.widget.LinearLayout.measureChildBeforeLayout(LinearLayout.java:1436)
	at android.widget.LinearLayout.measureVertical(LinearLayout.java:722)
	at android.widget.LinearLayout.onMeasure(LinearLayout.java:613)
	at android.view.View.measure(View.java:17547)
	at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:5535)
	at android.widget.FrameLayout.onMeasure(FrameLayout.java:436)
	at android.view.View.measure(View.java:17547)
	at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:5535)
	at android.widget.FrameLayout.onMeasure(FrameLayout.java:436)
	at android.view.View.measure(View.java:17547)
	at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:5535)
	at android.widget.FrameLayout.onMeasure(FrameLayout.java:436)
	at com.android.internal.policy.impl.PhoneWindow$DecorView.onMeasure(PhoneWindow.java:2615)
	at android.view.View.measure(View.java:17547)
	at android.view.ViewRootImpl.performMeasure(ViewRootImpl.java:2015)
	at android.view.ViewRootImpl.measureHierarchy(ViewRootImpl.java:1148)
	at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:1379)
	at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:1061)
	at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:5885)
	at android.view.Choreographer$CallbackRecord.run(Choreographer.java:767)
	at android.view.Choreographer.doCallbacks(Choreographer.java:580)
	at android.view.Choreographer.doFrame(Choreographer.java:550)
	at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:753)
	at android.os.Handler.handleCallback(Handler.java:739)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:135)
	at android.app.ActivityThread.main(ActivityThread.java:5254)
	at java.lang.reflect.Method.invoke(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:372)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:903)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:698)
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(bnicholson)

Updated

2 years ago
Assignee: nobody → michael.l.comella
tracking-fennec: ? → 47+
(Assignee)

Comment 3

2 years ago
Initial guess: we're starting an IPC call for ApplicationPackageManager.queryIntentActivities, which gets passed the Intent as an argument. The Binder transaction buffer, which stores the arguments and return values for IPC calls, is 1MB for all active IPC calls in the process. We're probably stuffing the whole image in the Intent and trying to send it over the IPC wire, which forces the framework to kill us.
Flags: needinfo?(michael.l.comella)
(Assignee)

Comment 4

2 years ago
Theory confirmed: Was able to repro and stopped the crash in the debugger – we appear have a base64 dump of the image in the `mExtras` toString.
(Assignee)

Comment 5

2 years ago
Furthermore, there is a crash when I upload a large image (> 4MB) and not a small one (< 12KB).
(Assignee)

Comment 6

2 years ago
The Intent with the image is being created from an entry in a JSONObject being passed from Gecko in:
 PromptService.handleMessage
  Prompt.show (we get the JSONArray with the key "listitems" and pass it into)
   PromptListItem.getArray
    PromptListItem (constructor)

These PromptListItems are accessed in PromptListAdapter.getView before throwing in:
 PromptListAdapter.getActionView
  GeckoActionProvider.setIntent (which eventually makes the IPC call)

We appear to include a URL when we normally long-press images so it's unclear to me why we're including the base64 image content here.

It looks like we add the items to the JSONArray in Prompt.jsm, _setListItems:
 https://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Prompt.jsm?rev=380817d573cd#219
Flags: needinfo?(bnicholson)
(Assignee)

Comment 7

2 years ago
It looks like the icon comes from the event target in contextmenus.show in browser.js [1]:
 this._target = BrowserEventHandler._highlightElement || event.target;

This gets passed into contextmenus._innerShow which reformats the target with contextmenus._reformatList before passing it into:
 Prompt.setSingleChoiceItems
  Prompt._setListItems

Where it attaches an icon to an object [2]:
 obj.icon = item.icon;

That is pushed onto the JSONArray [3]:
 this.msg.listitems.push(obj);

which is sent to the code in comment 6.

Next step would be to figure out why a full image is being passed in, as opposed to urls in other cases.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2808
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Prompt.jsm?rev=380817d573cd#192
[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Prompt.jsm?rev=380817d573cd#219
Comment hidden (typo)
(Assignee)

Comment 9

2 years ago
It appears the crash is caused because the src attribute of the img tag is passed over the IPC call to when retrieving the Intent. For large images, we run out of buffer space and we crash. i.e. Twitter adds this source to the html content when we upload an image on their mobile page.
 <img src="data:image/jpeg;base64, ..."/>

I'm able to reproduce on a test site I made:
 https://people.mozilla.org/~mcomella/test/base64.html

Some solutions:
 1) Disable the ability to long-press on a base64 image
 2) Identify large base64 images and either a) scale them down or b) remove their src tags (like (1))
 3) Maybe just remove the image src attribute when sending it over the IPC wire – we have the mime type so this might just work.

3) seems ideal, but we may have other OOM issues when dealing with excessively large base64 images so 1 or 2 might be safer.
(Assignee)

Comment 10

2 years ago
In trying to implement 3, I learned in NativeWindow.contextmenus._innerShow, we call `this._reformatList(target)` [1] which takes the src attribute off of the `target` argument and uses a crazy method reference to call the function defined in NativeWindow.contextmenus.add, specifically the showAsActions attr [2]. The items is passed to Java...

where they are extracted in Prompt.show(JSONObject) via the "listitems" attribute [3].

[1]: https://dxr.mozilla.org/mozilla-central/rev/c4449eab07d39e20ea315603f1b1863eeed7dcfe/mobile/android/chrome/content/browser.js#2874/
[2]: https://dxr.mozilla.org/mozilla-central/rev/c4449eab07d39e20ea315603f1b1863eeed7dcfe/mobile/android/chrome/content/browser.js#864
[3]: https://dxr.mozilla.org/mozilla-central/rev/c4449eab07d39e20ea315603f1b1863eeed7dcfe/mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java#96
(Assignee)

Comment 11

2 years ago
Created attachment 8755016 [details]
MozReview Request: Bug 1243305 - Only share images whose src length is not too long. r=margaret

If the src is too long, we crash!

To test this, you can use my test page:
  https://people.mozilla.org/~mcomella/test/base64.html

An alternative approach would be to prevent users from sharing base64
images altogether because the experience is slightly jarring (since the
base64 string is copied to the application rather than the image).

Review commit: https://reviewboard.mozilla.org/r/54330/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54330/
Attachment #8755016 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 12

2 years ago
Margaret, any ideas how to test the nsIImageLoadingContent section?
Flags: needinfo?(margaret.leibovic)

Comment 13

2 years ago
Comment on attachment 8755016 [details]
MozReview Request: Bug 1243305 - Only share images whose src length is not too long. r=margaret

https://reviewboard.mozilla.org/r/54330/#review51648

Seems reasonable to me.

::: mobile/android/chrome/content/browser.js:2463
(Diff revision 1)
> +        // The transaction limit is 1MB and we arbitrarily choose to cap this transaction at 1/4 of that = 250,000 bytes.
> +        // In Java, a UTF-8 character is 1-4 bytes so, 250,000 bytes / 4 bytes/char = 62,500 char
> +        let MAX_IMG_SRC_LEN = 62500;
> +        let isTooLong = imgSrc.length >= MAX_IMG_SRC_LEN;
> +        return !isTooLong && NativeWindow.contextmenus.imageSaveableContext.matches(aElement);
> +      }.bind(this)

I don't see you using `this` inside this function, so you don't need the bind call (or if you did, you could just use a fat arrow function instead).
Attachment #8755016 - Flags: review?(margaret.leibovic) → review+

Comment 14

2 years ago
(In reply to Michael Comella (:mcomella) from comment #12)
> Margaret, any ideas how to test the nsIImageLoadingContent section?

No, but maybe Seth can help?
Flags: needinfo?(margaret.leibovic) → needinfo?(seth)
(Assignee)

Comment 15

2 years ago
(In reply to :Margaret Leibovic from comment #13)
> I don't see you using `this` inside this function, so you don't need the
> bind call (or if you did, you could just use a fat arrow function instead).

Binding `this` is required to reference `NativeWindow.contextmenus...`. I'll explicitly reference it by prepending `this`.
(Assignee)

Comment 16

2 years ago
Comment on attachment 8755016 [details]
MozReview Request: Bug 1243305 - Only share images whose src length is not too long. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54330/diff/1-2/
(Assignee)

Comment 17

2 years ago
seth helped via irc: nsIImageLoadingContent is a super class of nsIDomHTMLImageElement, so I was able to comment out the first if case to test that this worked correctly.
Flags: needinfo?(seth)
(Assignee)

Comment 18

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/77fe72f30c9211b72235b099fb131bdbf98e7900
Bug 1243305 - Only share images whose src length is not too long. r=margaret

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/77fe72f30c92
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
(Assignee)

Comment 20

6 months ago
> To test this, you can use my test page:
>  https://people.mozilla.org/~mcomella/test/base64.html

My test page has been moved:
  https://mcomella.github.io/test/base64.html
Hello,

Verified this using the test page provided by Michael Comella in comment 20 in the latest Nightly (55.0a1), Beta(54.0b14) and Release(54.0 - build2) builds. The page loaded without any errors and the browser did not crash. Marking as verified.
Status: RESOLVED → VERIFIED
status-firefox54: --- → verified
status-firefox55: --- → verified
You need to log in before you can comment on or make changes to this bug.