956716 - Browser becomes unresponsive on attempt to invoke a context-menu on a base64 encoded PNG

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Currently when one attempts to invoke a context-menu on a base-64 encoded PNG, the browser becomes unresponsive.

Visit http://people.mozilla.org/~atrain/mobile/tests/base64png.html → long tap on the image

This is reproducible on all shipping channels.

--
LG Nexus 5 (Android 4.4.2)
Nightly (01/06)

Comment 1

[Glenn Randers-Pehrson]

Does the behavior change if you fold that one long line
(e.g., fold -72 base64png.html > base64pngf72.png)?

Comment 2

[Glenn Randers-Pehrson]

Argh.  Wrong target.  I should have written
(e.g., fold -72 base64png.html > base64pngf72.html)?

Comment 3

[Aaron Train [:aaronmt]]

There is still a hefty large unresponsive delay with the folded target, but yes the context-menu does appear. After dismissing the context-menu, the browser becomes responsive.

http://people.mozilla.org/~atrain/mobile/tests/base64pngf72.html

Chrome handles both cases fine, it looks like Gecko is churning on something here. Wonder if this is a core ImageLib issue or something.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch

In my logging, all of the JS processing and message passing, even calling builder.show() goes really fast. The only places this url matters/appears then is in the title, so I tried just slipping the dialog title at 256 characters. Seems to make things much happier.

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 8395952 [details] [diff] [review]
Patch

Review of attachment 8395952 [details] [diff] [review]:
-----------------------------------------------------------------

256 still seems huge for a title; does that fit on the screen? I wonder if we could we use something smaller such as 32 or 64.

::: mobile/android/base/prompts/Prompt.java
@@ +91,5 @@
>  
>          AlertDialog.Builder builder = new AlertDialog.Builder(mContext);
>          if (!TextUtils.isEmpty(title)) {
> +            // Long strings can delay showing the dialog, so we cap the number of characters shown to 256.
> +            builder.setTitle(title.substring(0, Math.min(title.length(), 256)-1));

Nit: spaces around "-"

Comment 6

[Wesley Johnston (:wesj)]

I'm not sure what the cap should be either. These can wrap to two lines, so 256 doesn't seem TOO crazy:

https://hg.mozilla.org/integration/fx-team/rev/6ad1bb2854cc

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

Brian pointed out String.substring actually selects to character end-1. i.e. we don't need the -1 here. Removed it.

Comment 8

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/b64373b02d2d

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6ad1bb2854cc
https://hg.mozilla.org/mozilla-central/rev/b64373b02d2d

Comment 10

[Aaron Train [:aaronmt]]

lol at the cause

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes - Would it make sense to cap the string on the JS side too? Why bother sending a long string across the JSON?

See also: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3900
(We cap the title for DOMTitleChanged too)

Comment 12

[Wesley Johnston (:wesj)]

I thought about it, mostly because I initially suspected it was the slowness here. Compared to the issues we are seeing here JS-Java bridge is extremely fast. A bunch of logging showed that happening on ms timescales. The string layout in Java that was taking 5-10seconds.

But there's probably a memory hit here building some longer strings that we throw away. I'll file a separate bug for that.

Comment 13

[Wesley Johnston (:wesj)]

Filed bug 989191 for that.