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

VERIFIED FIXED in Firefox 31

Status

()

Firefox for Android
General
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: aaronmt, Assigned: wesj)

Tracking

({reproducible, testcase})

Trunk
Firefox 31
ARM
Android
reproducible, testcase
Points:
---

Firefox Tracking Flags

(firefox26 affected, firefox27 affected, firefox28 affected, firefox29 affected, fennec+)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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)
(Reporter)

Updated

5 years ago
tracking-fennec: --- → ?
Assignee: nobody → wjohnston
tracking-fennec: ? → +
Does the behavior change if you fold that one long line
(e.g., fold -72 base64png.html > base64pngf72.png)?
Argh.  Wrong target.  I should have written
(e.g., fold -72 base64png.html > base64pngf72.html)?
(Reporter)

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
Created attachment 8395952 [details] [diff] [review]
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.
Attachment #8395952 - Flags: review?(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 "-"
Attachment #8395952 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 6

4 years ago
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
(Assignee)

Comment 7

4 years ago
Created attachment 8396758 [details] [diff] [review]
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.
Attachment #8396758 - Flags: review?(bnicholson)
Attachment #8396758 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/6ad1bb2854cc
https://hg.mozilla.org/mozilla-central/rev/b64373b02d2d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
(Reporter)

Comment 10

4 years ago
lol at the cause
Status: RESOLVED → VERIFIED
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)
Flags: needinfo?(wjohnston)
(Assignee)

Comment 12

4 years ago
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.
Flags: needinfo?(wjohnston)
(Assignee)

Comment 13

4 years ago
Filed bug 989191 for that.
You need to log in before you can comment on or make changes to this bug.