1716149 - Opening large data uri based image with "open image in new tab" can be very slow

Status: NEW

(Firefox :: Tabbed Browser, defect)

Description

[Matt]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:89.0) Gecko/20100101 Firefox/89.0

Steps to reproduce:

This is probably a niche use case. I'm using TiddlyWiki, which displays images to the user via data:// URIs. When I right click a large (14MB PNG) image to "open image in new tab" ...

Actual results:

The browser became unresponsive for several seconds. There is also a noticeable delay for the right click menu to appear.

Expected results:

The browser should not become unresponsive, or the operation should be faster.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox::Menus' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Andrei Purice]

Hey Matt,
I tried repdroducing this issue on the latest versions of Firefox Nightly 91.0a1 (2021-06-13), beta 90.0b7 and release 89.0 but it works fine for me.

Can you test the issue while in Safe Mode? You can find helpful info here : https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode .
Also a fresh new profile could help. You can find more about creating a new profile here : https://support.mozilla.org/en-US/kb/troubleshoot-and-diagnose-firefox-problems#w_6-create-a-new-firefox-profile .
If possible, you can test this issue on the nightly build as well. Download the build from : https://www.mozilla.org/en-US/firefox/nightly/all/ .

Comment 3

[Matt]

I can reproduce the issue in Safe Mode and in Nightly.

Experimenting with it some more, I notice that switching to the tab containing the data-uri-sourced (data://) image also incurs the delay and unresponsiveness. The same image opened as a file-uri (file://) has no problems.

Smaller PNG images loaded via data URI perform well.

One other thing I notice is that for the larger data URI, I can't see anything in the address bar. The smaller image displays the URI appropriately.

Comment 4

[Matt]

Attachment: image of two PNGs loaded via data URI, the one in the background is larger

Both are data URI PNGs. Foreground window works fine. Background causes ff to be unresponsive and displays no URI.

Comment 5

[Matt]

https://www.dropbox.com/s/jjzyx5rq8lmb7ut/Large-Data-URI-Image-utf8.html?dl=0

HTML document with the large data-uri image to help with reproducing the issue.

Comment 6

[:Gijs (he/him)]

Thanks for the report and testcase, they were very helpful.

Performance profile: https://share.firefox.dev/3zHYEA6

AFAICT from a quick skim of the profile, the issue is that we try to stick the 14mb data URI in the tab label, and then graphics code tries to reflow that text and it has a Very Bad Time. Of course this is all kinda daft because once the image loads the tab title will say something more descriptive and less madly long.

I could have sworn we've fixed either this or something very similar in the past, but I can't find it. This is annoying because it'd be useful to know if we fixed this for the address bar and then haven't for the tabstrip, or if we fixed this in the previous tabstrip implementation and regressed it with proton or something. Marco or Jonathan, do you recall a similar issue?

A naive fix would be limiting the amount of text that we try to display on the tab; a complication with that is that we reuse that label for the window title when the text is selected (where there is more space, so we'll need a higher limit for the window). Probably something like 32k chars should be enough for anybody (famous last words). I don't know if we'll need to do extra work for RTL or other 'fun' things - Jonathan/Marco, comments on that would be appreciated, too.

Once the main issue is fixed, it'd probably be worth digging into the fact that we re-parse the URL a ton of times, and why the context menu is slow to come up (which I suspect is to do with sending this giant URI over IPC or something).

Comment 7

[Jonathan Kew [:jfkthame]]

We've certainly seen similar things before, but I don't recall that we've "fixed" it.... the most recent report I can find offhand would be bug 1694420, which is still open/unfixed.

Truncating the text we ask it to display would be the obvious fix, but truncating bidi is a bit tricky as the "correct" display of the entire URL might involve displaying the (logically) last characters near the start-edge of the available space, in which case we oughtn't really to just truncate; we should elide. But doing that correctly will also be expensive in the general case.

If we're willing to accept a naïve truncation that has a (tiny) chance of altering the part of a bidi URL that actually appears within the available width of the tab, then we could do something like limit it to 1K chars, or even less, then check if it is narrower than the available width and if so, grab some more (keep doubling the truncation limit until we overflow the space or run out of URI, maybe). In practice the only case where we'd end up needing to grow the truncation limit would be a pathological URL full of non-spacing diacritics or other zero-width characters. (Would that even be possible, or would they be %-encoded and therefore occupy plenty of width anyhow? I suppose if they're not in the domain name but some other part of the URL, there's a greater chance they'd be literally present.)

Comment 8

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #6)

I could have sworn we've fixed either this or something very similar in the past, but I can't find it. This is annoying because it'd be useful to know if we fixed this for the address bar and then haven't for the tabstrip, or if we fixed this in the previous tabstrip implementation and regressed it with proton or something. Marco or Jonathan, do you recall a similar issue?

We fixed similar issues in the urlbar panel and code, for example in bug 1682434 and bug 1684667.
The urlbar field is a bit more problematic because the user can scroll through the url and put the cursor into a specific point of it. I think we're not limiting the field contents atm and bug 1694420 is likely a consequence.
I don't remember tabbar fixes, but I also didn't go through all the tabbar bugs recently.

A naive fix would be limiting the amount of text that we try to display on the tab; a complication with that is that we reuse that label for the window title when the text is selected (where there is more space, so we'll need a higher limit for the window). Probably something like 32k chars should be enough for anybody (famous last words).

Limiting to a meaningful amount of characters is usually fine, for example in the urlbar we made a pessimistic guess, and so far we didn't see problems reported with it.

I don't know if we'll need to do extra work for RTL or other 'fun' things - Jonathan/Marco, comments on that would be appreciated, too.

Jonathan already commented about this, I'd like just to add that potentially an initial fix could target data: uris that are pathological problematic by their nature.

Comment 9

[:Gijs (he/him)]

The Performance Impact Calculator has determined this bug's performance impact to be high. If you'd like to request re-triage, you can reset the Performance Impact flag to "?" or needinfo the triage sheriff.

Platforms: [x] Windows [x] macOS [x] Linux
Impact on browser: Causes noticeable jank
Impact on site: Causes noticeable jank
Websites affected: Rare
[x] Affects animation smoothness
[x] Able to reproduce locally
[x] Bug affects multiple sites
[x] Multiple reporters

I'm not convinced "high" is really the right answer here. But I think I'm using the calculator correctly...

(multiple reporters because bug 1800596 is a more recent instance of the same problem - on a different site, even though "Websites affected: rare" is imho accurate)

We've tried to reduce the impact of large data URIs in various places. There is definitely more we can do here. (there's more recent analysis in that other bug)

However, the other thing that comes to mind is that when an image is a data URI, we could somewhat easily transform the data URI into a blob one (when the user chooses "open image in new tab" or similar). That would break restoring sessions with such tabs (as the in-memory blobs would be gone), but it would vastly improve performance because the string, in-memory and graphical representation of the URL as such is separated from the data that loading the URL produces...

I don't know if that (breaking session restore) is a trade-off we're willing to make.

Comment 10

[Florian Quèze [:florian]]

(In reply to :Gijs (he/him) from comment #9)

The Performance Impact Calculator has determined this bug's performance impact to be high. If you'd like to request re-triage, you can reset the Performance Impact flag to "?" or needinfo the triage sheriff.

Platforms: [x] Windows [x] macOS [x] Linux
Impact on browser: Causes noticeable jank
Impact on site: Causes noticeable jank
Websites affected: Rare
[x] Affects animation smoothness
[x] Able to reproduce locally
[x] Bug affects multiple sites
[x] Multiple reporters

I'm not convinced "high" is really the right answer here. But I think I'm using the calculator correctly...

(multiple reporters because bug 1800596 is a more recent instance of the same problem - on a different site, even though "Websites affected: rare" is imho accurate)

Dave, do you have thoughts on whether some change is needed in the calculator? I agree 'high' doesn't feel right here, but I don't see a way to get another result from the calculator.

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

We have discussed limiting the calculator to either browser or site impact. I see that wouldn't help here, but it does bring the score down a little. The only other thought I have is to change the configuration value to rare, but that doesn't seem right in this case. I suspect this could be an edge case, where we just want to ignore the score and go with our gut. If you think we should change any of the weighting in the calculator then please feel free to make suggestions.

Comment 12

[:Gijs (he/him)]

I'll downgrade to medium based on the discussion, in spite of what the calculator says. :-)