If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

A long data: URI can hang Fennec for a long time

RESOLVED FIXED in Firefox 34

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: Martijn Wargers (dead), Assigned: rnewman)

Tracking

({perf, reproducible, testcase})

Trunk
Firefox 34
ARM
Android
perf, reproducible, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8454471 [details]
testcase

See testcase, when tapping on the "Very long data: URI" link, Fennec seems to hang for a long while, eventually it will load the page.
Tested on a Nexus 5 on trunk, beta and release builds.

I don't see this problem in Google Chrome for Android.
Reproducible on my Galaxy S5 too.
Keywords: reproducible
OS: Mac OS X → Android
Hardware: x86 → ARM
(Assignee)

Comment 2

3 years ago
To clarify for the bug record: is this hanging the browser (Java, before Gecko loads), or Gecko?
(Reporter)

Comment 3

3 years ago
I think it is hanging the browser, the Java side. But I don't know for sure.
(Assignee)

Comment 4

3 years ago
98% of the profile is in ProcessNextNativeEvent::Wait. The rest (< 1%) is basically timed GarbageCollectNow.

Here's the log:

10:19:06.229 D/GeckoToolbar( 5363): onTabChanged: START
10:19:06.229 I/GeckoToolbarDisplayLayout( 5363): zerdatime 19467466 - Throbber start
10:19:06.229 D/GeckoBrowserApp( 5363): BrowserApp.onTabChanged: 6: START
10:19:06.239 D/GeckoTabs( 5363): handleMessage: SessionHistory:New
10:19:06.249 D/GeckoTabs( 5363): handleMessage: Content:LocationChange
10:19:06.249 D/GeckoTabs( 5363): handleMessage: Content:SecurityChange
10:19:06.269 D/GeckoToolbar( 5363): onTabChanged: TITLE
10:19:06.279 D/GeckoTabs( 5363): handleMessage: DOMTitleChanged
10:19:06.299 D/GeckoTabs( 5363): handleMessage: DOMContentLoaded
10:19:06.350 D/GeckoTabs( 5363): handleMessage: Content:PageShow
10:19:06.420 D/GeckoTabs( 5363): handleMessage: Tab:ViewportMetadata
10:19:06.430 D/GeckoTabs( 5363): handleMessage: Content:StateChange
10:19:06.940 D/GeckoThumbnailHelper( 5363): Using new thumbnail size: 526080 (width 480)
10:19:06.940 D/GeckoThumbnailHelper( 5363): Sending thumbnail event: 480, 274
10:19:07.070 D/GeckoThumbnailHelper( 5363): handleThumbnailData: 526080
10:19:07.781 D/GeckoBrowserApp( 5363): BrowserApp.onTabChanged: 6: TITLE
10:19:07.781 D/GeckoToolbar( 5363): onTabChanged: MENU_UPDATED
10:19:07.781 D/GeckoBrowserApp( 5363): BrowserApp.onTabChanged: 6: MENU_UPDATED
10:19:07.781 D/GeckoToolbar( 5363): onTabChanged: LOCATION_CHANGE
10:19:07.781 D/GeckoToolbarDisplayLayout( 5363): updateFavicon(null)
10:19:07.791 I/GeckoToolbarDisplayLayout( 5363): zerdatime 19469023 - Throbber stop
10:19:07.791 D/GeckoBrowserApp( 5363): BrowserApp.onTabChanged: 6: LOCATION_CHANGE
10:19:07.791 D/GeckoToolbar( 5363): onTabChanged: SECURITY_CHANGE
10:19:07.791 D/GeckoBrowserApp( 5363): BrowserApp.onTabChanged: 6: SECURITY_CHANGE
10:19:07.791 I/InputMethodManager( 5363): org.mozilla.fennec called startInputInner(Do not restart input for non input field, ic=null) from android.view.inputmethod.InputMethodManager.restartInput(InputMethodManager.java:1135) <- org.mozilla.gecko.GeckoInputConnection.restartInput(GeckoInputConnection.java:418)
10:19:07.791 D/GeckoToolbar( 5363): onTabChanged: MENU_UPDATED
10:19:07.791 D/GeckoBrowserApp( 5363): BrowserApp.onTabChanged: 6: MENU_UPDATED
10:19:25.670 D/GeckoToolbar( 5363): onTabChanged: LOADED
10:19:25.670 D/GeckoBrowserApp( 5363): BrowserApp.onTabChanged: 6: LOADED
10:19:25.680 D/GeckoToolbar( 5363): onTabChanged: PAGE_SHOW
10:19:25.680 D/GeckoBrowserApp( 5363): BrowserApp.onTabChanged: 6: PAGE_SHOW
10:19:25.700 D/GeckoToolbar( 5363): onTabChanged: VIEWPORT_CHANGE
10:19:25.700 D/GeckoBrowserApp( 5363): BrowserApp.onTabChanged: 6: VIEWPORT_CHANGE
10:19:25.700 D/GeckoToolbar( 5363): onTabChanged: STOP
10:19:27.152 D/GeckoBrowserApp( 5363): BrowserApp.onTabChanged: 6: STOP
10:19:27.172 D/GeckoToolbar( 5363): onTabChanged: THUMBNAIL
10:19:27.172 D/GeckoBrowserApp( 5363): BrowserApp.onTabChanged: 6: THUMBNAIL
10:19:27.172 I/Choreographer( 5363): Skipped 90 frames!  The application may be doing too much work on its main thread.
10:19:44.971 D/GeckoToolbar( 5363): onTabChanged: FAVICON
10:19:44.971 D/GeckoToolbarDisplayLayout( 5363): updateFavicon(android.gra



Note that it took 20 seconds before we got the THUMBNAIL event, 20 more before we got the FAVICON event.

During this whole time Gecko's profile reported that it's just idle.
(Assignee)

Updated

3 years ago
Summary: A Long data: uri can hang Fennec for a long time → A long data: URI can hang Fennec for a long time
(Assignee)

Comment 5

3 years ago
I think at least half of the problem here is that we don't truncate the page title when we stuff it into Android UI elements. It takes forever to load the tab switcher with this page open. Fortunately we filter it out of history!
(Assignee)

Comment 6

3 years ago
I'm going to take a quick pass at this, see if it's just title/URI manipulation.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
(Assignee)

Comment 7

3 years ago
Created attachment 8454601 [details] [diff] [review]
Hacky patch. v1

This solves the problem for me.

It's necessary to truncate the URI itself, not just the title (or even the use of the URI as a title), because you can do fun stuff like edit the URI.

Of course, this patch breaks the editing of very long URIs, and breaks accurate storage of long URIs. We might consider using a higher (but still sane) URI limit. That's a more complex patch, of course.
(Assignee)

Comment 8

3 years ago
https://hg.mozilla.org/try/rev/2b2672de1946
(Assignee)

Comment 9

3 years ago
Created attachment 8455698 [details] [diff] [review]
Proposed patch. v2

I think this'll do.

Verified that truncation doesn't affect stored history (which goes via the nsAndroidHistory interface).

Verified that truncation to 2000 (URI) and 255 (title) is sufficient to prevent stalling in the UI. (The test URI is 16,000 chars, which balloons to 32,000 due to %20s.)

Did not yet verify favicon loading collision for real URIs longer than 2000 chars. Will do that next and file a follow-up if necessary -- I don't consider correct favicon display for super-long URLs to be a high priority.
Attachment #8455698 - Flags: review?(mark.finkle)
(Assignee)

Updated

3 years ago
Attachment #8454601 - Attachment is obsolete: true
Comment on attachment 8455698 [details] [diff] [review]
Proposed patch. v2

I have a worry that some URLs, like Loop URLs (2048), or viewing images, which might convert to a data: URI, will be truncated and passed to Java. If the user tries to tap on the URL bar and press 'go', it will fail.

Is truncating the URL required to fix this bug?
We can always truncate the URL when-used-as-a-title in Java and leave the raw URL alone.
(Assignee)

Comment 12

3 years ago
I found that half the hang was title. The other half was putting the huge string in the edit box. Android text fields have a hard time with 16kchar strings. 

There is probably some overhead from sending 32KB of URL in a message, too. I didn't assess that individually. 

The only thing broken by this patch is editing or copying the url of an already loaded page. External loads, pasted links, history, all remain unchanged. And 2000 chars is long, but if you have a specific size that lots of people are needing to edit, we can bump up to that. 

The likely next step from this patch is to send the full url and try to figure out an editing mode for urls that big, rather than editing them in a text field. Or we can decide to suffer the hang on edit. 

Another minor mitigation would be custom handling for copy/share that asks Gecko for the huge string.
(In reply to Richard Newman [:rnewman] from comment #12)

> The only thing broken by this patch is editing or copying the url of an
> already loaded page. External loads, pasted links, history, all remain
> unchanged. And 2000 chars is long, but if you have a specific size that lots
> of people are needing to edit, we can bump up to that. 

I wonder about Session Restore, but that might be fine too since it's handled via SessionStore.js and not Java. I was going to say "let's just use 4K" but it's still a potential gotcha.

> The likely next step from this patch is to send the full url and try to
> figure out an editing mode for urls that big, rather than editing them in a
> text field. Or we can decide to suffer the hang on edit. 

Sending the full URL would be my preference for now, and suffering the hang on edit. That fixes the pageload issue (right?) which seems more critical to me. We could do truncating in Java too, as a followup even.

Yes, there is overhead in the JSON messaging, but I'd rather treat that as a followup as well after we do some measurements.

> Another minor mitigation would be custom handling for copy/share that asks
> Gecko for the huge string.

Yes, because the Tab class doesn't have the raw URL anymore, right?

I notice that Martijn says Chrome for Android doesn't have the problem. Are they doing something different (truncating) or is their UI such that it just doesn't happen?
(Assignee)

Comment 14

3 years ago
Created attachment 8463730 [details] [diff] [review]
A long data: URI can hang Fennec for a long time. v3

This version keeps the truncation call, but uses 25,000 characters as the upper limit for URIs. This has zero runtime cost (just a function call, no string ops), but if we change our minds we can just adjust that constant. And it will save our bacon if someone does something stupid like use a 1MB URI.
Attachment #8463730 - Flags: review?(mark.finkle)
(Assignee)

Updated

3 years ago
Attachment #8455698 - Attachment is obsolete: true
Attachment #8455698 - Flags: review?(mark.finkle)
Comment on attachment 8463730 [details] [diff] [review]
A long data: URI can hang Fennec for a long time. v3

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+function truncate(title, max=MAX_TITLE_LENGTH) {

This started out as a truncation for titles,. but you use it as much for URIs now too.
* title -> text
* let's drop the implicit MAX_TITLE_LENGTH. It seems better to be explicit for all cases.

>           sendMessageToJava({
>             type: "Reader:Added",
>             result: result,
>-            title: article.title,
>+            title: truncate(article.title),
>             url: url,

I assume we don't need to do the truncation on the URL here?

What about the "Reader:Added" message in aboutReader.js? Do we need to cover the title truncation there as well?
Attachment #8463730 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 16

3 years ago
(In reply to Mark Finkle (:mfinkle) from comment #15)

> * title -> text
> * let's drop the implicit MAX_TITLE_LENGTH. It seems better to be explicit
> for all cases.

Done.

> I assume we don't need to do the truncation on the URL here?

Added.
 
> What about the "Reader:Added" message in aboutReader.js? Do we need to cover
> the title truncation there as well?

Odds of a super-long URL being loaded as a reader page are slim, I suspect… and it would involve duplicating truncation logic there. I suggest leaving that until there's some evidence that we need it.
(Assignee)

Comment 17

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/0d834e0bcc29
https://hg.mozilla.org/mozilla-central/rev/0d834e0bcc29
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
You need to log in before you can comment on or make changes to this bug.