Closed Bug 1243305 Opened 4 years ago Closed 4 years ago

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

Categories

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

ARM
Android
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 49
Tracking Status
firefox44 --- affected
firefox45 --- affected
firefox46 --- affected
firefox47 --- affected
firefox49 --- fixed
fennec 47+ ---
firefox54 --- verified
firefox55 --- verified

People

(Reporter: u549602, Assigned: mcomella)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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
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)
Assignee: nobody → michael.l.comella
tracking-fennec: ? → 47+
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)
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.
Furthermore, there is a crash when I upload a large image (> 4MB) and not a small one (< 12KB).
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)
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
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.
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
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)
Margaret, any ideas how to test the nsIImageLoadingContent section?
Flags: needinfo?(margaret.leibovic)
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+
(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)
(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`.
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/
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)
https://hg.mozilla.org/mozilla-central/rev/77fe72f30c92
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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
You need to log in before you can comment on or make changes to this bug.