0.94ms uninterruptible reflow at ssi_getWindowDimension@resource:///modules/sessionstore/SessionStore.jsm:4265:7

VERIFIED FIXED in Firefox 55

Status

()

Core
DOM: Core & HTML
P1
normal
VERIFIED FIXED
10 months ago
6 months ago

People

(Reporter: Robbie Ward, Assigned: dao)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 verified, firefox57 verified)

Details

(Whiteboard: [ohnoreflow][qf][photon-performance][qa-commented])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

10 months ago
Here's the stack:

ssi_getWindowDimension@resource:///modules/sessionstore/SessionStore.jsm:4265:7
ssi_updateWindowFeatures/<@resource:///modules/sessionstore/SessionStore.jsm:3023:24
ssi_updateWindowFeatures@resource:///modules/sessionstore/SessionStore.jsm:3022:5
ssi_collectWindowData@resource:///modules/sessionstore/SessionStore.jsm:3213:5
getCurrentState/<@resource:///modules/sessionstore/SessionStore.jsm:3061:11
ssi_forEachBrowserWindow@resource:///modules/sessionstore/SessionStore.jsm:4138:9
getCurrentState@resource:///modules/sessionstore/SessionStore.jsm:3057:7
getCurrentState@resource:///modules/sessionstore/SessionStore.jsm:362:12
_saveState@resource:///modules/sessionstore/SessionSaver.jsm:246:17
_saveStateAsync@resource:///modules/sessionstore/SessionSaver.jsm:309:5
runDelayed/this._timeoutID<@resource:///modules/sessionstore/SessionSaver.jsm:184:40
setTimeout_timer@resource://gre/modules/Timer.jsm:30:5
(Assignee)

Comment 2

10 months ago
It's not clear to why outerWidth on a top-level window would flush layout. Feels like this should be avoidable like it was for mozInnerScreenX (bug 1307134).
(Assignee)

Comment 3

10 months ago
Otherwise I agree that we could use lazyWidth here.
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Assignee: nobody → dao+bmo
Component: Session Restore → DOM: Core & HTML
Product: Firefox → Core
(Assignee)

Comment 5

10 months ago
nsGlobalWindow::CheckSecurityLeftAndTop is using the same pattern and should probably get the same treatment, but I'm not quite sure what the purpose of that function is.

Updated

10 months ago
Status: NEW → ASSIGNED
Flags: qe-verify?
Priority: -- → P1

Comment 6

10 months ago
mozreview-review
Comment on attachment 8860693 [details]
Bug 1358809 - Remove unnecessary layout flush for outerWidth and outerHeight.

https://reviewboard.mozilla.org/r/132640/#review135988

This is the outersize, it has nothing to do with the current document (unless the current document happens to be the root document). And I'm not even sure a flush could change that value. So _if_ we need to flush here it seems like we are flushing the right thing.
Attachment #8860693 - Flags: review?(tnikkel) → review-
(Assignee)

Comment 7

10 months ago
Bug 26673 added the layout flush in a bulk patch with no particular explanation why outerWidth/Height would need this.
Comment hidden (mozreview-request)

Updated

10 months ago
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu

Updated

10 months ago
Attachment #8860693 - Flags: review?(bzbarsky)
I'm not familiar with where the size of a docshell tree owner comes from, so I can't say if we need a flush or not to get an up to date value. So I tried to forward the review to bz (or anyone else who might know).

Updated

10 months ago
Attachment #8860693 - Flags: review?(tnikkel)
The size comes from the bounds of the nsIWidget for the nsXULWindow.

So as long as we don't have cases where the nsXULWindow auto-resizes to its content (as opposed to explicit sizeToContent calls), not flushing here should be OK.  I _think_ we never do that, but if we do I'm hoping Dão would actually know about it...
Comment on attachment 8860693 [details]
Bug 1358809 - Remove unnecessary layout flush for outerWidth and outerHeight.

https://reviewboard.mozilla.org/r/132640/#review137002

r=me
Attachment #8860693 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 13

10 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #11)
> The size comes from the bounds of the nsIWidget for the nsXULWindow.
> 
> So as long as we don't have cases where the nsXULWindow auto-resizes to its
> content (as opposed to explicit sizeToContent calls), not flushing here
> should be OK.  I _think_ we never do that, but if we do I'm hoping Dão would
> actually know about it...

As far as I know, we don't do that.

Comment 14

10 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1be01419533e
Remove unnecessary layout flush for outerWidth and outerHeight. r=bz

Comment 15

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1be01419533e
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

10 months ago
Iteration: --- → 55.4 - May 1
(Assignee)

Comment 17

10 months ago
from bug 892154 comment 1:

> When this bug is fixed, you should update browser_windowopen_reflows.js to
> not mention the sync reflow coming from Session Store.
Flags: needinfo?(dao+bmo)

Comment 18

10 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa210d7d1b43
followup: update browser_windowopen_reflows.js to account for the reflow in ssi_getWindowDimension having been removed (no review)
(Assignee)

Updated

10 months ago
Flags: needinfo?(dao+bmo)

Updated

10 months ago
No longer blocks: 1348289

Updated

10 months ago
Blocks: 1348289
For QA:

For testing this, I'd have multiple windows open, and ensure that we properly save their heights and widths when closing those windows. When restored from the History menu, those closed windows should always have the same dimensions as when they closed.
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf][photon-performance][qa-commented]
Depends on: 1393013
No longer depends on: 1393013
Environment:

Ubuntu 16.04 x64, Windows 10 Pro x64, Mac OsX 10.12
57.0a1 20170822171841
55.0.2 20170814072924

With bug 1393013 identified as not a regression caused by this bug and also by taking in account the bugs blocking bug 1330633, I will mark this bug as verified from the point of view that it hasn't introduced any new regression.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
status-firefox57: --- → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.