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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

Attachment: 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.

Comment 1

[Aaron Train [:aaronmt]]

Reproducible on my Galaxy S5 too.

Comment 2

[Richard Newman [:rnewman]]

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

Comment 3

[Martijn Wargers (dead)]

I think it is hanging the browser, the Java side. But I don't know for sure.

Comment 4

[Richard Newman [:rnewman]]

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.

Comment 5

[Richard Newman [:rnewman]]

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!

Comment 6

[Richard Newman [:rnewman]]

I'm going to take a quick pass at this, see if it's just title/URI manipulation.

Comment 7

[Richard Newman [:rnewman]]

Attachment: 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.

Comment 8

[Richard Newman [:rnewman]]

https://hg.mozilla.org/try/rev/2b2672de1946

Comment 9

[Richard Newman [:rnewman]]

Attachment: 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.

Comment 10

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

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?

Comment 11

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

We can always truncate the URL when-used-as-a-title in Java and leave the raw URL alone.

Comment 12

[Richard Newman [:rnewman]]

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.

Comment 13

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

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

Comment 14

[Richard Newman [:rnewman]]

Attachment: 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.

Comment 15

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

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?

Comment 16

[Richard Newman [:rnewman]]

(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.

Comment 17

[Richard Newman [:rnewman]]

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

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0d834e0bcc29