1240326 - Don't trigger reflow on tab events

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Kris Maglione [:kmag]]

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.

Comment 1

[Sourabh Shrivastava]

Hi Kris, I'd like to work on this bug. Can you guide me through?

Comment 2

[Kris Maglione [:kmag]]

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.

Comment 3

[Kris Maglione [:kmag]]

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?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, sounds good to me.

Comment 5

[Andy McKay]

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

Comment 6

[Sourabh Shrivastava]

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

Comment 7

[Andy McKay]

Ok thanks, mate. Bob, its all yours.

Comment 8

[Bob Silverberg [:bsilverberg]]

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?

Comment 9

[Kris Maglione [:kmag]]

Attachment: Part 1: Add lazyWidth and lazyHeight properties to frame loaders

Comment 10

[Bob Silverberg [:bsilverberg]]

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?

Comment 11

[Kris Maglione [:kmag]]

I was planning to land it at the same time, but I can land it now if you need an artifact build.

Comment 12

[Bob Silverberg [:bsilverberg]]

It's alright. I'll build myself a copy with your patch.

Comment 13

[Bob Silverberg [:bsilverberg]]

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

Comment 14

[Kris Maglione [:kmag]]

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.

Comment 15

[Bob Silverberg [:bsilverberg]]

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/

Comment 16

[Bob Silverberg [:bsilverberg]]

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?

Comment 17

[Kris Maglione [:kmag]]

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.

Comment 18

[Kris Maglione [:kmag]]

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

Comment 19

[Bob Silverberg [:bsilverberg]]

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/

Comment 20

[Bob Silverberg [:bsilverberg]]

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?

Comment 21

[Kris Maglione [:kmag]]

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.

Comment 22

[Bob Silverberg [:bsilverberg]]

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/

Comment 23

[Kris Maglione [:kmag]]

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!

Comment 24

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdcbcc9eb6cf

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4399e8609c74
https://hg.mozilla.org/integration/mozilla-inbound/rev/c057ea186ced

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9a5c69a8a7ed for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=27328607&repo=mozilla-inbound

Comment 27

[Bob Silverberg [:bsilverberg]]

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.

Comment 28

[Bob Silverberg [:bsilverberg]]

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?

Comment 29

[Kris Maglione [:kmag]]

This change fixes the failures for me: https://hg.mozilla.org/try/rev/68d816048876e9a1c29bae38eb6731155676d6fe

Comment 30

[Bob Silverberg [:bsilverberg]]

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

Comment 31

[Bob Silverberg [:bsilverberg]]

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/

Comment 32

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b029dba4a152

Comment 33

[Bob Silverberg [:bsilverberg]]

Thanks Kris. I made that change and have submitted a new try push.

Comment 34

[Bob Silverberg [:bsilverberg]]

This seems to be ready to land now.

Comment 35

[Tim Nguyen :ntim]

Part 1 needs rebasing.

Comment 36

[Kris Maglione [:kmag]]

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

Comment 37

[Carsten Book [:Tomcat]]

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$

Comment 38

[Bob Silverberg [:bsilverberg]]

Passing this back to Kris to address the issue...

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fcedeb6142a

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fcedeb6142a

Comment 41

[Kris Maglione [:kmag]]

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

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91bd1c000101