Closed Bug 956716 Opened 6 years ago Closed 6 years ago

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


(Firefox for Android :: General, defect)

Not set



Firefox 31
Tracking Status
firefox26 --- affected
firefox27 --- affected
firefox28 --- affected
firefox29 --- affected
fennec + ---


(Reporter: aaronmt, Assigned: wesj)




(Keywords: reproducible, testcase)


(2 files)

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)
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)?
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.
Attached patch PatchSplinter 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+
I'm not sure what the cap should be either. These can wrap to two lines, so 256 doesn't seem TOO crazy:
Attached patch Patch 2/2Splinter Review
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+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
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)
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)
Filed bug 989191 for that.
You need to log in before you can comment on or make changes to this bug.