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

Status: RESOLVED FIXED

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

Description

[Florian Quèze [:florian]]

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

Comment 1

[Florian Quèze [:florian]]

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.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 3

[Grant Wong]

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.

Comment 4

[Mike Conley (: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!

Comment 5

[:Felipe Gomes (needinfo for replies!)]

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)

Comment 6

[Grant Wong]

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.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Hi Grant, are you still working on this?

Comment 8

[Grant Wong]

Hi Mike, I won't be contributing a fix to this bug.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Hi Grant, okay, thanks for letting us know!

Comment 10

[Joanna Jo [:jjo]]

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

[PhoenixAbhishek]

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?

Comment 12

[Erik Carrillo [:E_Carr]]

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

Comment 13

[:Felipe Gomes (needinfo for replies!)]

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

Comment 14

[Nikki Sharpley [:nikkis]]

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!

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Yep, let's use Phabricator

Comment 16

[Nikki Sharpley [:nikkis]]

Attachment: Crop large data URLs before displaying them in the status bar

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.

Comment 17

[Nikki Sharpley [:nikkis]]

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6890499eb5d1
Crop large data URLs before displaying them in the status bar r=Felipe

Comment 19

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 6890499eb5d1 (Bug 1484071) for ESlint failure at tabbrowser.js.

Backout: https://hg.mozilla.org/integration/autoland/rev/bd0449584cba8f18c4c9c5666ba4b7a459d573b7

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=6890499eb5d12fe8bce5ce1289ecb052ceb89343&selectedJob=232012381

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=232012381&repo=autoland&lineNumber=283

Comment 20

[:Felipe Gomes (needinfo for replies!)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f25fac04788d
Crop large data URLs before displaying them in the status bar r=Felipe

Comment 22

[Nikki Sharpley [:nikkis]]

Thanks for your help, Felipe! :)

Comment 23

[:Felipe Gomes (needinfo for replies!)]

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

Comment 24

[Cristina Coroiu [:ccoroiu]]

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