"Open Image in New Tab" on very large data URL (11MB) is very slow or crashes browser
Categories
(Core :: Layout: Images, Video, and HTML Frames, defect)
Tracking
()
| Performance Impact | medium |
People
(Reporter: danweiss, Unassigned)
References
(Depends on 1 open bug, )
Details
(5 keywords)
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:106.0) Gecko/20100101 Firefox/106.0
Steps to reproduce:
Some websites or web applications will use a data URL with an embedded PNG image. For small images, this is fine. But Firefox is struggling badly on very large images (where the data URL is over 11MB long).
One such example is available at https://niutech.github.io/jxl.js/
When the page is loaded, a JPEG-XL image (379KB) is converted into a 8.06MB large PNG image, then that is injected into the DOM as a data URL, which is 10.7MB large.
If you right click on the image, then select "Open Image in New Tab", things go badly.
Actual results:
On my main profile, after opening the 10.7MB long data URL in a new tab, Firefox was crashing at this point. So I also tested this on a clean profile to see if it would crash there too.
Firefox became unresponsive for 7 seconds before the new tab appeared in the tabs bar. After the tab appears, the text becomes "(PNG Image, 2048 x 2560 pixels)", but before that, the text on the tab bar shows the data URL of the image. Perhaps it is trying to process all 11M text characters of that URL.
After switching to that tab, it takes 7 seconds for it to switch to that tab, and the Address Bar now contains the full 11M text characters of the URL.
Clicking on the address bar can sometimes cause the browser to fail to draw anything properly at all. The main user interface is missing all text and UI elements, and the image on the page failed to render.
Expected results:
Should not have bad performance when opening large data URLs in new tabs.
|
||
Comment 1•2 years ago
|
||
The Bugbug bot thinks this bug should belong to the 'Firefox::Address Bar' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.
The URL mentioned in the bug report no longer works, so I have mirrored the 11MB Data URL here: https://www.dwedit.org/files/.data-url-bug/huge-data-url-example.html
|
||
Comment 3•1 year ago
|
||
Reproducible on Windows 10 and MacOS 11.6 as well.
I'm setting a component in order to get this checked out by the development team.
If this does not seem like the correct one, feel free to set a more suitable one.
- Additional note - Chrome has "Open image in a New Tab" greyed out in the context menu.
Thank you for reporting!
|
||
Comment 4•1 year ago
•
|
||
The crash part of the problem here seems to be that the very long uri is not handled very well when sending it to webrender: we send the entire url to render even though only a few hundred characters are visible. I first ran into this problem at https://bugzilla.mozilla.org/show_bug.cgi?id=1772994#c7
|
|
||
Comment 5•1 year ago
|
||
Interestingly Chrome greys out the "open image in new tab" option for that page. And if I use the inspector to get the full data uri and try to paste that in a new tab it looks like it gets cut off, perhaps they have a max limit on pasted uris.
Safari does better, opening the image in a new tab works okay, but laggy. Pasting the uri in an empty new tab takes quite a while but works.
|
|
||
Comment 6•1 year ago
•
|
||
The long pause in just opening the new tab in the background is a long reflow of a text frame. I assume this is reflowing the text of the very long uri. But it's not actually displayed anywhere on screen, so perhaps we can avoid this? If not, another way to avoid that long pause would maybe be to have long uri handling in the front end, so that it doesn't put the entire url into the dom and displays an ellipsis to indicate the url is shortened.
I do see all text on the window turn to red boxes temporarily while just opening the tab in the background, so something bad is happening, but the browser seems to recover.
The long pause in switching to the tab is also a long reflow of a text frame.
The developers of the Jpeg XL WASM decoder (seen in the first URL posted) switched out the long data URL for a canvas tag. If you open that in a new tab, you get a Blob URL. (such as "blob:null/76cc4569-f1ac-4f56-9bfa-363f3bc4fc8c")
I wonder if long data URLs could be replaced with a blob object like that? Maybe anything over 32K could be considered long.
Otherwise, maybe truncate the text that displays in the tab bar to 1000 bytes. It's way more than would ever normally be displayed in a tab.
|
||
Comment 8•1 year ago
|
||
(In reply to Dan Weiss from comment #7)
The developers of the Jpeg XL WASM decoder (seen in the first URL posted) switched out the long data URL for a canvas tag. If you open that in a new tab, you get a Blob URL. (such as "blob:null/76cc4569-f1ac-4f56-9bfa-363f3bc4fc8c")
I wonder if long data URLs could be replaced with a blob object like that? Maybe anything over 32K could be considered long.
Otherwise, maybe truncate the text that displays in the tab bar to 1000 bytes. It's way more than would ever normally be displayed in a tab.
We already do: https://searchfox.org/mozilla-central/rev/a156a65ced2dae5913ae35a68e9445b8ee7ca457/browser/base/content/tabbrowser.js#1653-1660 .
The URL bar has an even more severe limit: https://searchfox.org/mozilla-central/rev/a156a65ced2dae5913ae35a68e9445b8ee7ca457/browser/components/urlbar/UrlbarUtils.sys.mjs#149
So the question is why that isn't being hit here and/or if we're trying to display the full URL somewhere else.
|
|
||
Updated•1 year ago
|
|
|
||
Comment 9•1 year ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #4)
The crash part of the problem here seems to be that the very long uri is not handled very well when sending it to webrender: we send the entire url to render even though only a few hundred characters are visible. I first ran into this problem at https://bugzilla.mozilla.org/show_bug.cgi?id=1772994#c7
I see this same crash. :tnikkel, reading your comments I'm not 100% sure I fully understand - is this still related to a text run, or to trying to send the URL for (I'm guessing) bookkeeping purposes? I guess I'm not sure why this would result in a textrun in webrender if the URL is not displayed (which it shouldn't be, in the content area, even if there's some other bit in the parent process that is causing the hang described in comment 6 with a text frame reflow). Or are we hanging doing the reflow and then crashing sending the URL to webrender for something in the browser UI?
If the latter, could we limit the size of text runs supported to something "reasonable" to avoid the IPC crash with webrender?
|
|
||
Comment 10•1 year ago
|
||
(In reply to :Gijs (he/him) from comment #9)
I see this same crash. :tnikkel, reading your comments I'm not 100% sure I fully understand - is this still related to a text run, or to trying to send the URL for (I'm guessing) bookkeeping purposes? I guess I'm not sure why this would result in a textrun in webrender if the URL is not displayed (which it shouldn't be, in the content area, even if there's some other bit in the parent process that is causing the hang described in comment 6 with a text frame reflow). Or are we hanging doing the reflow and then crashing sending the URL to webrender for something in the browser UI?
Hmm, I forget exactly what I determined now when I was looking at this a couple of months ago. Given the new to me information that we do limit the display of long urls in the chrome I'll have to take another look to see where the problem was coming from. The fact that I saw a very long reflow of a text frame (comment 6) would suggest there is a text frame somewhere with the whole url in it (or something else really long).
If the latter, could we limit the size of text runs supported to something "reasonable" to avoid the IPC crash with webrender?
Hopefully something like that is possible/easy.
|
|
||
Comment 11•1 year ago
|
||
There is a testframe with the whole data uri in it. In a content dump it looks like
XUL* vbox@0x126fa93a0 class="tab-label-container" onoverflow="this.setAttribute('textoverflow', 'true');" onunderflow="this.removeAttribute('textoverflow');" align="start" pack="center" flex="1" labeldirection="ltr" textoverflow="true" state=[1000000] flags=[00008006] primaryframe=0x14db12188 refcount=9<
XUL* label@0x126fa9790 class="tab-text tab-label" role="presentation" fadein="true" state=[1040000] flags=[00008002] primaryframe=0x14db12640 refcount=4<
Text@0x126fa2880 flags=[00300000] primaryframe=0x14db12708 refcount=2<data:image/png;base64,iVBORw0KGgoAA....
seems like this might be the parent
https://searchfox.org/mozilla-central/rev/fb9a504ca73529fa550efe488db2a012a4bf5169/browser/base/content/tabbrowser-tab.js#31
|
|
||
Comment 12•1 year ago
|
||
Thanks :tnikkel, and sorry for not digging into this more first before bouncing the needinfo.
It would appear that the tabstrip's code reduces the text run if setTabTitle is called. But in the case in this bugreport, setInitialTabTitle is called because we create the tab with the URI ready, and that still passes the 1 million char URI to the tab. Once the page loads it gets reduced to the much smaller title that the image document generates (the size of the image + zoom percentage).
I have a local fix for this and have confirmed that the tab's label becomes shorter and isn't ever so long. I'm still seeing a hang (on Windows) in this case though. I tried using the profiler to investigate this but that causes the tab to crash and/or the parent process to hang permanently (instead of "just" for a few seconds) so either way I don't get useful data in the profiler.
I'll file a separate follow-up bug for the tabstrip fix, but I'm curious if there is another long text run there, perhaps from the URL bar? Marco, do you know if there's a bug tracking reducing the URL there somehow? I found code linked in comment #8 but I'm suspecting that isn't applying to the URLbar value itself when editable. I'm not sure what we can really do about a focused URL bar though, as if we start messing with the URLs it's hard to see how users would still copy-paste them or manipulate them...
|
|
||
Comment 13•1 year ago
|
||
(In reply to :Gijs (he/him) from comment #12)
I have a local fix for this and have confirmed that the tab's label becomes shorter and isn't ever so long. I'm still seeing a hang (on Windows) in this case though. I tried using the profiler to investigate this but that causes the tab to crash and/or the parent process to hang permanently (instead of "just" for a few seconds) so either way I don't get useful data in the profiler.
Adding :florian for good measure, in case this is of interest to him. The patch for tabbrowser is now in bug 1810025, the hang [still] happens when trying to select the tab in question.
|
|
||
Comment 14•1 year ago
|
||
The patch in bug 1810055 fixes the crashes with the profiler running when switching to the new tab for me.
|
|
||
Comment 15•1 year ago
|
||
After applying the patch for bug 1810025, when actually switching to the new tab with the big data uri (but not focusing the url bar) I still see a text frame with what looks like the whole data uri in it. The text frame is inside
html:input@0x139919980 id="urlbar-input" anonid="input" aria-controls="urlbar-results" aria-autocomplete="both" inputmode="mozAwesomebar" data-l10n-id="urlbar-placeholder-with-name" data-l10n-attrs="placeholder" placeholder="Search with Google or enter address" data-l10n-args="{"name":"Google"}" aria-expanded="false" style="--urlbar-scheme-size: 0px;" state=[402061408] flags=[00008002] primaryframe=0x144992c88 refcount=19<>
|
||
Comment 16•1 year ago
|
||
(In reply to :Gijs (he/him) from comment #12)
Marco, do you know if there's a bug tracking reducing the URL there somehow? I found code linked in comment #8 but I'm suspecting that isn't applying to the URLbar value itself when editable. I'm not sure what we can really do about a focused URL bar though, as if we start messing with the URLs it's hard to see how users would still copy-paste them or manipulate them...
We don't have a specific bug about avoiding long urls in the input field, we have a few bugs about better handling of long strings like
https://bugzilla.mozilla.org/show_bug.cgi?id=1589602 and various bugs have been fixed in the past.
I agree that the focused case is more complicate, though over a certain amount of characters the valid use cases to edit or copy a part of this string far from the beginning are likely non-existent. You'd have to scroll the string or move the cursor for a long time. It seems not the best way to handle such content, so it should be ok to not support it. Overall I think visually cutting the textrun in both focused and unfocused states would be ok, provided all the APIs keep returning the whole string on copy or when value/textValue are accessed.
|
|
||
Comment 17•1 year ago
|
||
Looking at an unrelated bug, I'm seeing us just not render ends of long text runs on macOS, when in the URL bar. I'll try and get a testcase for that - but based on that, truncating the text we render at some point before that is probably not that bad a thing, as it's not like it works well today...
|
||
Comment 18•1 year ago
|
||
With both the patches from bug 1810055 and bug 1810025 applied, the content process still crashes for me when I select the tab with the large data URL if the profiler is running.
I can get profiles of the time it takes to open the new tab from the "Open Image in New Tab" context menu, and it looks like we are calling nsIIOService.newURI 8 times: https://share.firefox.dev/3QDNwgu with each call taking more than 40ms on my machine.
I can't profile selecting the tab as it crashes, but one slow thing I can profile is hovering the tab bar while the tab with the large image is selected. It takes more than a second for the tab hover effect to update. https://share.firefox.dev/3IPDYgq
|
|
||
Comment 19•1 year ago
|
||
(In reply to Florian Quèze [:florian] from comment #18)
I can get profiles of the time it takes to open the new tab from the "Open Image in New Tab" context menu, and it looks like we are calling
nsIIOService.newURI8 times: https://share.firefox.dev/3QDNwgu with each call taking more than 40ms on my machine.
This doesn't seem as bad as the other issues here as 8x 40ms is still "only" 300-odd ms, but I filed bug 1810141 for it.
|
||
Comment 20•1 year ago
|
||
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
×3 [x] macOS
×3 [x] Linux
×1 [x] Android
×1.5
Impact on browser: Renders browser effectively unusable
+10
[x] Able to reproduce locally
×2
[x] Bug affects multiple sites
×2
|
|
||
Comment 21•1 year ago
|
||
The severity field for this bug is set to S3. However, the Performance Impact field flags this bug as having a high impact on the performance.
:emilio, could you consider increasing the severity of this performance-impacting bug? Alternatively, if you think the performance impact is lower than previously assessed, could you request a re-triage from the performance team by setting the Performance Impact flag to ??
For more information, please visit auto_nag documentation.
|
||
Comment 22•1 year ago
|
||
Seems like S3 is appropriate as it's a rather uncommon thing to do and other browsers don't deal well with it either, though it is being actively worked on.
|
|
||
Comment 23•1 year ago
|
||
(In reply to :Gijs (he/him) from comment #12)
I tried using the profiler to investigate this but that causes the tab to crash and/or the parent process to hang permanently (instead of "just" for a few seconds) so either way I don't get useful data in the profiler.
With bug 1812368 fixed, I can now profile this bug correctly without crashing the content process, here's a profile: https://share.firefox.dev/40HS8H3
|
|
||
Comment 24•1 year ago
|
||
That still shows a long text run being the prime culprit (5.1 second hang on mousedown of the opened tab). There's also a 500ms hang due to all the newURI calls - I've started chipping away at that in bug 1810141 (will land after soft freeze) though rearchitecting more of the infrastructure there will be a bit of a slog. I'll file more follow-up bugs around that after 1810141 lands.
For the main remaining hang, although the URL bar could try to work around this, either specifically for data: URIs or for long text runs generally, I wonder if there's some way in CSS or otherwise to avoid gecko trying to render all this text? After all, it's all in the overflown part of the input field so it won't actually make it to the screen. This would be more reliable because it could be done without affecting the .value getters for the html:input field that underpins the URL bar, and would also benefit web content for variations on bug 1589602. I'm not familiar with font rendering internals so I'm wondering if either of you can comment, :jfkthame, :tnikkel?
|
||
Comment 25•1 year ago
|
||
It would be nice, but.... I think it's difficult to short-circuit much of the work of shaping a really long line of text in the general case (even if we know that only a limited width of it will be visible) for a few reasons. E.g. with bidi content, we have to do a substantial amount of the work in order to know which part of the text will be visible (it may not be the "first" few hundred characters that we care about...) And in many cases if there's overflowing content, we need to know how big it is in order to render a scrollbar correctly.
In the image-as-data-URL case, we know the content is plain left-to-right ASCII, so the bidi complication doesn't arise; and the URL bar doesn't show a horizontal scrollbar when the text overflows, so perhaps we don't need to know the true width either. But neither of those simplifications necessarily hold for the more general HTML rendering case.
Even for the URL bar, the question of what happens when the user moves the cursor through the string -- potentially beyond the part we've rendered, perhaps all the way to the end (e.g. Cmd-RightArrow or Select-All followed by RightArrow) -- may be tricky to handle.
|
|
||
Comment 26•1 year ago
|
||
I'll defer to jfkthame on the text reflow parts. As for the part about sending a giant string of text (of which a few hundred chars are visible) to the gpu process, hopefully it's not too hard to avoid that, I don't currently have deep knowledge of the text frame display list code but I do hope to look into it soon.
|
|
||
Comment 27•1 year ago
|
||
New profile with bug 1810141 fixed, we still call NewURI several times: https://share.firefox.dev/3Yw6poF I'm not sure if this patch set was meant to reduce the calls to NewURI significantly or if it was only refactoring in preparation for future perf fixes.
|
|
||
Comment 28•1 year ago
|
||
(In reply to Florian Quèze [:florian] from comment #27)
New profile with bug 1810141 fixed, we still call NewURI several times: https://share.firefox.dev/3Yw6poF I'm not sure if this patch set was meant to reduce the calls to NewURI significantly or if it was only refactoring in preparation for future perf fixes.
Yeah, sorry, I morphed that bug after filing when it became clear that just the refactoring would touch >100 files and be hard to reason about on its own. I tried to indicate this in comment 24 but was probably not detailed enough.
The most common codepaths were only refactored, not optimized. I did initially try to optimize them but it was felt that was risky and doing it in a separate bug would be better, as it'd be easier to figure out regressions. Only some of the more trivial cases (loadURI(uri.spec) and such) were actually updated to not reparse URIs. So some of the optimization will happen in bug 1815509, and some of it will need to happen in further bugs I still need to file. I'll do that once searchfox has been updated to include the work from 1810141 because then it becomes a lot easier to audit (can just look for calls to fixupAndLoadURIString - 0 hits right now).
In your profile, some of the callers are in openLinkIn and the context menu code. There's no way right now for openLinkIn to take and pass a URI object, nor does gBrowser.addTab take a URI object either. So all of that will take yet more work. Other things I noticed when working on 1810141 were that E10sUtils determines remote types by taking a string URI and then parsing it, etc. etc.
(There are also some callers in the URL bar code and tabswitch code, ie in this hang block that seem completely orthogonal to the actual loading of the URL so they would need their own bugs - you may wish to investigate and file some. It may be lower-hanging fruit, or not - I'm not sure off-hand.)
|
|
||
Comment 29•1 year ago
|
||
The proposed patch in bug 1818654 should help with interaction here (drag-selecting within the URI) once it is displayed (though not with the initial reflow).
|
|
||
Updated•3 months ago
|
|
|
||
Comment 30•3 months ago
|
||
Gonna ask for re-triage here. Several patches have landed to improve this.
Chrome doesn't even allow this (open in new tab is greyed out).
Safari feels about the same as us, maybe a little better.
Given that Chrome doesn't even support this, it's hard to think that should have high impact.
|
|
||
Comment 31•3 months ago
|
||
Profile on a recent build: https://share.firefox.dev/47YFINp
|
||
Comment 32•3 months ago
|
||
The Performance Impact Calculator has determined this bug's performance impact to be medium. 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 [x] Android
Impact on browser: Causes noticeable jank
Impact on site: Causes noticeable jank
Websites affected: Rare
[x] Able to reproduce locally
[x] Bug affects multiple sites
| Comment hidden (spam) |
Description
•