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

VERIFIED FIXED in Firefox 31



Firefox for Android
5 years ago
4 years ago


(Reporter: aaronmt, Assigned: wesj)


({reproducible, testcase})

Firefox 31
reproducible, testcase

Firefox Tracking Flags

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




(2 attachments)



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

Visit → long tap on the image

This is reproducible on all shipping channels.

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


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)?

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.

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

4 years ago
Created attachment 8395952 [details] [diff] [review]

In my logging, all of the JS processing and message passing, even calling 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]

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/
@@ +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+

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:

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+
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31

Comment 10

4 years ago
lol at the cause
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:
(We cap the title for DOMTitleChanged too)
Flags: needinfo?(wjohnston)

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)

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.