Closed Bug 1484071 Opened 3 years ago Closed 2 years ago

Having a large data url in the Highlights causes terrible reflow performance in the parent process

Categories

(Firefox :: Toolbars and Customization, enhancement, P3)

Product:
Firefox
Component:
Toolbars and Customization
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: florian, Assigned: nikkis, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [fxperf:p3])

Attachments

(2 files)

Screencast
2.69 MB, video/quicktime
Details
Crop large data URLs before displaying them in the status bar
47 bytes, text/x-phabricator-request
Details | Review
Attached video Screencast — 
After using the devtools' "Take a screenshot of the entire page" feature, I ended up with a data: url of the screenshot in the Highlights in about:newtab. The screenshot I had weights 1.1MB.

When hovering that box of the highlights section, it takes more than a second for the mouse cursor to change back and forth between the arrow and the hand.

Here is a profile of it: https://perfht.ml/2L4BLwk and I'm attaching a screencast.

It's not clear to me why the jank is on the parent process rather than on the content process where about:newtab is loaded, but I think the fix is straight forward: when a base64 encoded data url is long, we should only put the beginning of it in the DOM. See bug 1408854 for a similar bug we already fixed.
I forgot to say this is on a current nightly.

On release, the behavior I'm seeing matches the screenshot in bug 1459948, and it's the activity stream content process that gets blocked for 2s during the DisplayList step of the refresh driver tick.
Version: 54 Branch → Trunk
According to the profile, the time is spent in the _showDelayed function, which is related to the status panel that displays the hovered link. As seen on the screencast, the problem is that the status panel is taking a long time to render the giant data URL.

The data URL is cropped in the middle, so it should be fairly straightforward to just take the first and last N characters to pass to display instead of trying to display the whole URL and ending up with it cropped anyway.
Mentor: felipc
Component: Activity Streams: Newtab → Toolbars and Customization
Keywords: good-first-bug
Whiteboard: [fxperf:p2]
Hi Mike, this bug fix seems straight forward and I believe is narrow enough in scope to take on as a newbie. It also seems well defined from comment 1 and an idea of a possible solution is given in comment 2. A fix would make the user experience feel more responsive by eliminating unnecessary lag. Would you say this is a GFB to take on? Thanks for guiding me away from my previous bug I wanted to fix that was too complex, not well defined, and too far reaching for my skill level.
Flags: needinfo?(mconley)
Hi Grant,

I think putting a hard limit on the URL going into the status bar makes sense. You're right - this bug seems much better as a good first bug than the last one. Felipe has signed himself up as a mentor on this, so I'll let him guide you through it.

Thanks!
Flags: needinfo?(mconley) → needinfo?(felipc)
Hello Grant,

I'll guide you on this bug. Have you already figured out things like pulling the source code with Mercurial and building Firefox? If not yet, I recommend that you take a look at something called Artifact builds (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds) which will make building Firefox very fast (since for this bug, and most of the front-end, we don't need to make C++ changes)

If you already have gotten all that ready, here's where to start fixing this bug:

according to comment 2, the starting point of the problem is the _showDelayed function, from here:
https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/browser/base/content/browser.js#5161

that function calls StatusPanel.update, which is implemented here:
https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/browser/base/content/tabbrowser.js#5141

and it's likely the place where you'll need to make the fix.


(btw I highly recommend using https://searchfox.org/ to search and navigate the source code)
Assignee: nobody → grant.hj.wong
Status: NEW → ASSIGNED
Flags: needinfo?(felipc)
Hi Felipe,

I've pulled and built FireFox so I can readily begin implementing the fix. Thank you for taking on the task of guiding me through this bug and I'll update you when I have a patch ready for review.
Hi Grant, are you still working on this?
Flags: needinfo?(grant.hj.wong)
Whiteboard: [fxperf:p2] → [fxperf:p3]
Hi Mike, I won't be contributing a fix to this bug.
Flags: needinfo?(grant.hj.wong)
Hi Grant, okay, thanks for letting us know!
Assignee: grant.hj.wong → nobody
Status: ASSIGNED → NEW

Hi Felipe and Mike, is it possible if this bug could be assigned to me? I'm hoping that I can make this my first contribution to the Firefox frontend!

Hi Mike, I am Phoenix, a participant for the current Outreachy round.
I would like to work on this, do we need a complete build for this, or will an artefact build suffice?

Hello, my name is Erik and I'm an Outreachy candidate. Can I please be assigned this bug for my initial contribution? Thanks.

Hello everyone! Since there have been a number of people asking to be assigned to this bug, I don't want to just pick one at random.. So, for folks who are willing to give it a try, please take a look at the comment 5 here in this bug, which has detailed instructions.

The most important first step is to get a local Firefox build up and going.. If you have that already, take a look at the rest of the instructions in comment 5 and see if you get something +/- working (doesn't need to be perfect, just to have a good perspective that the bug will be fixed). Once someone has that, I'll be happy to mentor it to the end (fixing rough edges, providing feedback and review..)

Hi Felipe,
Just throwing my name in that hat! I haven't commented yet, but have been looking into this the past few days.
I have a local Firefox build and have been able to reproduce the bug. I think I know where I need to implement the fix (thanks for the helpful hints!) so will have an initial rough cut up shortly for review. Would you like this submitted via Phabricator?
Thanks!

Yep, let's use Phabricator

This commit provides a simple fix to reduce the time spent
rendering larger data URLs (such as large screenshots) in
the status panel when the Highlights tab is hovered over.
By limiting the number of characters displayed, this
decreases the time spent in the _showDelayed function.

Assignee: nobody → nikkisharpley
Status: NEW → ASSIGNED

Hi Felipe. Hope you had a good weekend! Just wondering if there was anything I could improve on with my patch? Thank you again!

Attachment #9047504 - Attachment description: status panel: decrease time to render larger data urls (bug 1484071) → Crop large data URLs before displaying them in the status bar
Pushed by fgomes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6890499eb5d1
Crop large data URLs before displaying them in the status bar r=Felipe

Nikki, can you run mach eslint browser/base/content/tabbrowser.js to check that there are no other eslint complaints with the patch?

Pushed by fgomes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f25fac04788d
Crop large data URLs before displaying them in the status bar r=Felipe

Thanks for your help, Felipe! :)

Flags: needinfo?(nikkisharpley)

You're welcome! Thanks for your contribution to Firefox :)

https://hg.mozilla.org/mozilla-central/rev/f25fac04788d

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.