Don't trigger reflow on tab events

RESOLVED FIXED in Firefox 50

Status

P3
major
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: kmag, Assigned: bsilverberg, Mentored)

Tracking

unspecified
mozilla50
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [tabs]triaged)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
I noticed that some of our intermittent failures are triggering a failed assertion when they try to access the tab.linkedBrowser.clientWidth property:

 17:32:07 INFO - [Parent 1989] ###!!! ASSERTION: CalcDifference() returned bigger hint than MaxDifference(): 'NS_IsHintSubset(difference, maxDifference)', file /builds/slave/m-in-lx-d-00000000000000000000/build/src/layout/style/nsStyleContext.cpp, line 930

I'm not sure what's causing the error, but a few bugs have been fixed recently for the same assertion, so ours may be fixed as well.

Either way, we should really try to avoid triggering a reflow here just to get (very likely unused) width and height properties. There's already enough going on in the UI during tab updates without throwing extra reflows into the mix.

I'm not sure what the right solution is, but I can think of a few:

* Probably the easiest is to add a reflow observer to the browser container and use its size for every tab in the window.

* Ideally we could use resize events from the top-level content window and use its innerWidth/innerHeight, but those changes would have to be proxied to the parent process in e10s.

* The TabParent code already has to track browser resizes so it can proxy them to content processes to trigger said resize events. Maybe it could be modified to send an event at that point, or just make the sizes available to us in properties that don't trigger reflows.
Flags: blocking-webextensions?

Updated

3 years ago
Severity: normal → major
Priority: -- → P3
Whiteboard: [tabs][good first bug] → [tabs][good first bug]triaged
(Reporter)

Updated

3 years ago
Assignee: kmaglione+bmo → nobody

Updated

3 years ago
Flags: blocking-webextensions? → blocking-webextensions+
Whiteboard: [tabs][good first bug]triaged → [tabs]triaged

Comment 1

3 years ago
Hi Kris, I'd like to work on this bug. Can you guide me through?
(Reporter)

Comment 2

3 years ago
Gladly. For a start, you'll need to be able to build Firefox. If you haven't done that before, you'll need to install Mercurial and clone our repository:

hg clone https://hg.mozilla.org/mozilla-central

After that, you can follow these instructions to get a basic build working: https://developer.mozilla.org/en-US/docs/Artifact_builds


Once you've done that, we can talk about the implementation.

At the moment, I still think the easiest approach is to create a reflow observer in the browser window, cache the size of the content area when it fires, and then use that for all tabs. If we decide to go with that approach, I can give you more details on how to do it.


I think it might be simpler, though, if we add platform support to get the last known size of the tab content without forcing a reflow. If you'd like, I can handle the platform side of that and leave the rest to you.
(Reporter)

Comment 3

3 years ago
Bill, I'm thinking about adding lazyClientWidth and lazyClientHeight properties to nsIFrameLoader, and updating them from nsFrameLoader::UpdatePositionAndSize. Does this sound like a good approach to you?
Flags: needinfo?(wmccloskey)
Yeah, sounds good to me.
Flags: needinfo?(wmccloskey)

Comment 5

3 years ago
(In reply to Sourabh Shrivastava from comment #1)
> Hi Kris, I'd like to work on this bug. Can you guide me through?

Have you had chance to look at this? If not there's someone else who can take it on.
Flags: needinfo?(simplysourabh_123)

Comment 6

3 years ago
(In reply to Andy McKay [:andym] from comment #5)
> (In reply to Sourabh Shrivastava from comment #1)
> > Hi Kris, I'd like to work on this bug. Can you guide me through?
> 
> Have you had chance to look at this? If not there's someone else who can
> take it on.

I'm a little busy at the moment. Feel free to take it if you want. Also the bug hasn't be assigned yet.
Flags: needinfo?(simplysourabh_123)

Comment 7

3 years ago
Ok thanks, mate. Bob, its all yours.
Assignee: nobody → bob.silverberg
(Assignee)

Comment 8

3 years ago
Hmm, I'm not sure how this got assigned to me. Not that I mind extra bugs, but I don't recall any discussion of this. It doesn't look like the changes to `nsIFrameLoader` discussed in comment #3 have been done, and I'm not in a position to do that. Kris, is that something you plan to do in this iteration?
Flags: needinfo?(kmaglione+bmo)
(Reporter)

Comment 9

3 years ago
Created attachment 8740160 [details] [diff] [review]
Part 1: Add lazyWidth and lazyHeight properties to frame loaders
Attachment #8740160 - Flags: review?(wmccloskey)
(Reporter)

Updated

3 years ago
Assignee: bob.silverberg → kmaglione+bmo
Status: NEW → ASSIGNED
Attachment #8740160 - Flags: review?(wmccloskey) → review+
(Reporter)

Updated

3 years ago
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 10

3 years ago
Kris, I don't think that patch attached to this bug has landed yet, or was the plan to land that at the same time as the changes to our code/tests?
Flags: needinfo?(kmaglione+bmo)
(Reporter)

Comment 11

3 years ago
I was planning to land it at the same time, but I can land it now if you need an artifact build.
Flags: needinfo?(kmaglione+bmo)
(Reporter)

Updated

3 years ago
Assignee: kmaglione+bmo → bob.silverberg
(Assignee)

Comment 12

3 years ago
It's alright. I'll build myself a copy with your patch.
(Assignee)

Comment 13

3 years ago
Created attachment 8745364 [details]
Bug 1240326 - Part 2: Utilize lazyWidth and lazyHeight properties from frame loaders for tab width and height.

Review commit: https://reviewboard.mozilla.org/r/49005/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49005/
Attachment #8745364 - Flags: review?(kmaglione+bmo)
(Reporter)

Updated

3 years ago
Attachment #8745364 - Flags: review?(kmaglione+bmo)
(Reporter)

Comment 14

3 years ago
Comment on attachment 8745364 [details]
Bug 1240326 - Part 2: Utilize lazyWidth and lazyHeight properties from frame loaders for tab width and height.

https://reviewboard.mozilla.org/r/49005/#review45877

This is a good start, but it needs tests.

We don't currently have any tests for `tab.width` or `tab.height`, so we need to test that this matches the `clientWidth` and `clientHeight` of the tabs in both query results and events.

We also need to test that the sizing is correct when scaling for high DPI displays, which means changing the value of the "layout.css.devPixelsPerPx" preference, and checking that the size is still correct. We have some other tests that do similar things that you can probably use for reference.

::: browser/components/extensions/ext-tabs.js:690
(Diff revision 1)
>          }
>  
>          let message = {
>            options,
> -          width: browser.clientWidth,
> -          height: browser.clientHeight,
> +          width: browser.frameLoader.lazyWidth,
> +          height: browser.frameLoader.lazyHeight,

We should still use `clientWidth` and `clientHeight` here.
(Assignee)

Comment 15

3 years ago
Comment on attachment 8745364 [details]
Bug 1240326 - Part 2: Utilize lazyWidth and lazyHeight properties from frame loaders for tab width and height.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49005/diff/1-2/
Attachment #8745364 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 16

3 years ago
https://reviewboard.mozilla.org/r/49005/#review46079

::: browser/components/extensions/test/browser/browser_ext_tabs_events.js:167
(Diff revision 2)
> +
> +add_task(function* testTabEventsSize() {
> +  function background() {
> +    browser.tabs.onCreated.addListener(tab => {
> +      browser.tabs.remove(tab.id);
> +      browser.test.sendMessage("width", tab.width);

There is a problem which is that both `width` and `height` are `0` at this point. This is the case regardless of whether I remove the tab before or after sending the messages. So this test is failing.

I wanted to write the whole test and submit it in case there is something I should be doing differently in my test which would solve the problem. Or perhaps this is a bug in `onCreated` or maybe even expected behaviour?
(Reporter)

Comment 17

3 years ago
https://reviewboard.mozilla.org/r/49005/#review46079

> There is a problem which is that both `width` and `height` are `0` at this point. This is the case regardless of whether I remove the tab before or after sending the messages. So this test is failing.
> 
> I wanted to write the whole test and submit it in case there is something I should be doing differently in my test which would solve the problem. Or perhaps this is a bug in `onCreated` or maybe even expected behaviour?

I think we may need to add a special case to use clientWidth/clientHeight when `tab.width` or `tab.height` is 0. That should only happen just after the tab has been created, and there hasn't been a reflow yet. I wish we could avoid it, but I'm not sure it's worth the trouble.
(Reporter)

Comment 18

3 years ago
Comment on attachment 8745364 [details]
Bug 1240326 - Part 2: Utilize lazyWidth and lazyHeight properties from frame loaders for tab width and height.

https://reviewboard.mozilla.org/r/49005/#review46829

Looks good aside from the issue you brought up, but I also think we could use a couple more tests, just to be safe.

::: browser/components/extensions/test/browser/browser_ext_tabs_events.js:197
(Diff revision 2)
> +  for (let resolution of [2, 1]) {
> +    SpecialPowers.setCharPref(RESOLUTION_PREF, String(resolution));
> +    is(window.devicePixelRatio, resolution, "window has the required resolution");
> +    extension.sendMessage("create-tab");
> +    is(yield extension.awaitMessage("width"), gBrowser.clientWidth, "tab reports expected width");
> +    is(yield extension.awaitMessage("height"), gBrowser.clientHeight, "tab reports expected height");

We should test this for other events too, and tab objects returned by tabs.create().

Also, we should use `gBrowser.selectedTab` rather than `gBrowser` here (and in the other test).
Attachment #8745364 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 19

3 years ago
Comment on attachment 8745364 [details]
Bug 1240326 - Part 2: Utilize lazyWidth and lazyHeight properties from frame loaders for tab width and height.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49005/diff/2-3/
Attachment #8745364 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 20

3 years ago
https://reviewboard.mozilla.org/r/49005/#review46829

> We should test this for other events too, and tab objects returned by tabs.create().
> 
> Also, we should use `gBrowser.selectedTab` rather than `gBrowser` here (and in the other test).

I added tests for the other events that provide a `tab` object, which is just `onUpdated`. I tried to use `gBrowser.selectedTab` rather than `gBrowser`, but I kept getting test failures because I think it was seeing the selectedTab as the one in the smaller window that opens when running the tests, so the sizes of `gBrowser.selectedTab` did not match the sizes returned from the background script. Do you have a suggestion about how to fix that?
(Reporter)

Comment 21

3 years ago
Comment on attachment 8745364 [details]
Bug 1240326 - Part 2: Utilize lazyWidth and lazyHeight properties from frame loaders for tab width and height.

https://reviewboard.mozilla.org/r/49005/#review47327

::: browser/components/extensions/ext-utils.js:493
(Diff revision 3)
>        active: tab.selected,
>        pinned: tab.pinned,
>        status: TabManager.getStatus(tab),
>        incognito: PrivateBrowsingUtils.isBrowserPrivate(tab.linkedBrowser),
> -      width: tab.linkedBrowser.clientWidth,
> -      height: tab.linkedBrowser.clientHeight,
> +      width: tab.linkedBrowser.frameLoader.lazyWidth || tab.linkedBrowser.clientWidth,
> +      height: tab.linkedBrowser.frameLoader.lazyHeight || tab.linkedBrowser.clientHeight,

Would be nice to add `let browser = tab.linkedBrowser` above, and change the rest of the uses of `tab.linkedBrowser` to use it as well.

::: browser/components/extensions/test/browser/browser_ext_tabs_events.js:167
(Diff revision 3)
> +
> +add_task(function* testTabEventsSize() {
> +  function background() {
> +    function sendSizeMessages(tab, type) {
> +      browser.test.sendMessage(`${type}-width`, tab.width);
> +      browser.test.sendMessage(`${type}-height`, tab.height);

Can we send an object with width and height properties in one message instead?

::: browser/components/extensions/test/browser/browser_ext_tabs_query.js:147
(Diff revision 3)
> +      browser.test.onMessage.addListener((msg) => {
> +        browser.tabs.query({active: true}).then(tabs => {
> +          browser.test.assertEq(tabs.length, 1, "should have one tab");
> +
> +          browser.test.sendMessage("width", tabs[0].width);
> +          browser.test.sendMessage("height", tabs[0].height);

Can we send an object with width and height properties in one message, instead?

::: browser/components/extensions/test/browser/browser_ext_tabs_query.js:156
(Diff revision 3)
> +    },
> +  });
> +
> +  const RESOLUTION_PREF = "layout.css.devPixelsPerPx";
> +  registerCleanupFunction(() => {
> +    SpecialPowers.clearUserPref(RESOLUTION_PREF);

We should explicitly clear this at the end of the task too, so any non-default value doesn't affect the rest of the tasks in this file.
Attachment #8745364 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 22

3 years ago
Comment on attachment 8745364 [details]
Bug 1240326 - Part 2: Utilize lazyWidth and lazyHeight properties from frame loaders for tab width and height.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49005/diff/3-4/
Attachment #8745364 - Flags: review?(kmaglione+bmo)
(Reporter)

Comment 23

3 years ago
Comment on attachment 8745364 [details]
Bug 1240326 - Part 2: Utilize lazyWidth and lazyHeight properties from frame loaders for tab width and height.

https://reviewboard.mozilla.org/r/49005/#review47341

Thanks!
Attachment #8745364 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Updated

3 years ago
Iteration: --- → 49.1 - May 9
Keywords: checkin-needed
(Assignee)

Comment 27

3 years ago
It looks like the numbers being reported for the tabs are just slightly off those being compared. Kris, do you think we need to build some flexibility into the test to address this, or is this something else? Also, we didn't see these on try, which seems odd.
Flags: needinfo?(bob.silverberg) → needinfo?(kmaglione+bmo)
(Assignee)

Comment 28

2 years ago
Kris, I feel like we discussed this on IRC or in a Monday meeting, but I don't recall what was said and don't seem to have any notes on it. How do you suggest I proceed with this?
(Reporter)

Comment 29

2 years ago
This change fixes the failures for me: https://hg.mozilla.org/try/rev/68d816048876e9a1c29bae38eb6731155676d6fe
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 30

2 years ago
Created attachment 8760249 [details]
Bug 1240326: Part 1: Add lazyWidth and lazyHeight properties to frame loaders.

Review commit: https://reviewboard.mozilla.org/r/57904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57904/
Attachment #8745364 - Attachment description: MozReview Request: Bug 1240326 - Part 2: Utilize lazyWidth and lazyHeight properties from frame loaders for tab width and height. r?kmag → Bug 1240326 - Part 2: Utilize lazyWidth and lazyHeight properties from frame loaders for tab width and height.
Attachment #8760249 - Flags: review?(wmccloskey)
Attachment #8745364 - Flags: review?(bob.silverberg)
(Assignee)

Comment 31

2 years ago
Comment on attachment 8745364 [details]
Bug 1240326 - Part 2: Utilize lazyWidth and lazyHeight properties from frame loaders for tab width and height.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49005/diff/4-5/
(Assignee)

Updated

2 years ago
Attachment #8760249 - Attachment is obsolete: true
Attachment #8760249 - Flags: review?(wmccloskey)
(Assignee)

Updated

2 years ago
Attachment #8745364 - Flags: review?(bob.silverberg)
(Assignee)

Comment 33

2 years ago
Thanks Kris. I made that change and have submitted a new try push.
(Assignee)

Comment 34

2 years ago
This seems to be ready to land now.
Iteration: 49.1 - May 9 → 49.3 - Jun 6
Keywords: checkin-needed
Part 1 needs rebasing.
Flags: needinfo?(bob.silverberg)
(Reporter)

Comment 36

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/6fcedeb6142a01f96c7b7f9bba39fae3d174e3c3
Bug 1240326: Part 1: Add lazyWidth and lazyHeight properties to frame loaders. r=billm
has problems to apply :

patching file dom/base/nsFrameLoader.cpp
Hunk #1 FAILED at 2331
Hunk #2 FAILED at 2361
2 out of 2 hunks FAILED -- saving rejects to file dom/base/nsFrameLoader.cpp.rej
patching file dom/base/nsFrameLoader.h
Hunk #1 FAILED at 369
1 out of 1 hunks FAILED -- saving rejects to file dom/base/nsFrameLoader.h.rej
patching file dom/base/nsIFrameLoader.idl
Hunk #1 FAILED at 221
1 out of 1 hunks FAILED -- saving rejects to file dom/base/nsIFrameLoader.idl.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue
Tomcats-MacBook-Pro-2:fx-team Tomcat$
(Assignee)

Comment 38

2 years ago
Passing this back to Kris to address the issue...
Flags: needinfo?(bob.silverberg) → needinfo?(kmaglione+bmo)

Comment 39

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6fcedeb6142a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Reporter)

Comment 41

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/91bd1c000101c281233cd1340faf18a8a374d8cc
Bug 1240326 - Part 2: Utilize lazyWidth and lazyHeight properties from frame loaders for tab width and height. r=kmag
(Reporter)

Updated

2 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 42

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/91bd1c000101
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Flags: needinfo?(kmaglione+bmo)
(Reporter)

Updated

2 years ago
Depends on: 1358415

Updated

4 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.