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

VERIFIED FIXED in Firefox 55

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: rjward0, Assigned: dao)

Tracking

(Blocks 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified, firefox57 verified)

Details

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

Attachments

(1 attachment)

Reporter

Description

2 years 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

2 years 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

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

Updated

2 years ago
Assignee: nobody → dao+bmo
Component: Session Restore → DOM: Core & HTML
Product: Firefox → Core
Assignee

Comment 5

2 years 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.
Status: NEW → ASSIGNED
Flags: qe-verify?
Priority: -- → P1

Comment 6

2 years 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

2 years 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)
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
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).
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 12

2 years ago
mozreview-review
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

2 years 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

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1be01419533e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Iteration: --- → 55.4 - May 1
Duplicate of this bug: 892154
Assignee

Comment 17

2 years 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

2 years 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

2 years ago
Flags: needinfo?(dao+bmo)
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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.