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

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
10 months ago
4 months ago

People

(Reporter: florian, Assigned: nikkis, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 67
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: [fxperf:p3])

Attachments

(2 attachments)

Reporter

Description

10 months ago
Posted 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.
Reporter

Comment 1

10 months ago
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]
Priority: -- → P3

Comment 3

9 months ago
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)

Comment 6

8 months ago
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]

Comment 8

7 months ago
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

Comment 10

4 months ago

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!

Comment 11

4 months ago

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

Assignee

Comment 14

4 months ago

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

Assignee

Comment 16

4 months ago

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
Assignee

Comment 17

4 months ago

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

Comment 18

4 months ago
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?

Comment 21

4 months ago
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
Assignee

Comment 22

4 months ago

Thanks for your help, Felipe! :)

Flags: needinfo?(nikkisharpley)

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

Comment 24

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.