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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[u549602]

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

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

About 350 crashes/week on 43.

Comment 2

[Richard Newman [:rnewman]]

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)

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 (currently, the only option is "Share Image" so it's already missing functionality, as opposed to url-based images)
 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.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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/

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Margaret, any ideas how to test the nsIImageLoadingContent section?

Comment 13

[: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).

Comment 14

[:Margaret Leibovic]

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

No, but maybe Seth can help?

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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/

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/77fe72f30c92

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> 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

Comment 21

[Bogdan Surd, QA [:bsurd, NI]]

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.