Closed Bug 1240326 Opened 6 years ago Closed 5 years ago

Don't trigger reflow on tab events

Categories

(WebExtensions :: Untriaged, defect, P3)

defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Iteration:
49.3 - Jun 6
Tracking Status
firefox50 --- fixed

People

(Reporter: kmag, Assigned: bsilverberg, Mentored)

References

Details

(Whiteboard: [tabs]triaged)

Attachments

(2 files, 1 obsolete file)

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?
Severity: normal → major
Priority: -- → P3
Whiteboard: [tabs][good first bug] → [tabs][good first bug]triaged
Assignee: kmaglione+bmo → nobody
Flags: blocking-webextensions? → blocking-webextensions+
Whiteboard: [tabs][good first bug]triaged → [tabs]triaged
Hi Kris, I'd like to work on this bug. Can you guide me through?
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.
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)
(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)
(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)
Ok thanks, mate. Bob, its all yours.
Assignee: nobody → bob.silverberg
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)
Assignee: bob.silverberg → kmaglione+bmo
Status: NEW → ASSIGNED
Attachment #8740160 - Flags: review?(wmccloskey) → review+
Flags: needinfo?(kmaglione+bmo)
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)
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)
Assignee: kmaglione+bmo → bob.silverberg
It's alright. I'll build myself a copy with your patch.
Attachment #8745364 - Flags: review?(kmaglione+bmo)
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 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)
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?
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 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)
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)
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 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)
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)
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+
Iteration: --- → 49.1 - May 9
Keywords: checkin-needed
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)
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?
This change fixes the failures for me: https://hg.mozilla.org/try/rev/68d816048876e9a1c29bae38eb6731155676d6fe
Flags: needinfo?(kmaglione+bmo)
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)
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/
Attachment #8760249 - Attachment is obsolete: true
Attachment #8760249 - Flags: review?(wmccloskey)
Attachment #8745364 - Flags: review?(bob.silverberg)
Thanks Kris. I made that change and have submitted a new try push.
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)
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$
Passing this back to Kris to address the issue...
Flags: needinfo?(bob.silverberg) → needinfo?(kmaglione+bmo)
https://hg.mozilla.org/mozilla-central/rev/6fcedeb6142a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/91bd1c000101
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Flags: needinfo?(kmaglione+bmo)
Depends on: 1358415
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.