Closed
Bug 1358809
Opened 8 years ago
Closed 8 years ago
0.94ms uninterruptible reflow at ssi_getWindowDimension@resource:///modules/sessionstore/SessionStore.jsm:4265:7
Categories
(Core :: DOM: Core & HTML, defect, P1)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: rjward0, Assigned: dao)
References
(Blocks 1 open bug)
Details
(Whiteboard: [ohnoreflow][photon-performance][qa-commented])
Attachments
(1 file)
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
Comment 1•8 years ago
|
||
This is:
dimension = aWindow.outerWidth;
at http://searchfox.org/mozilla-central/rev/f225dbcb15ca2e38f7d434a9278a41d2340e7cf3/browser/components/sessionstore/SessionStore.jsm#4265
I wonder if this could use lazyWidth instead (http://searchfox.org/mozilla-central/rev/f225dbcb15ca2e38f7d434a9278a41d2340e7cf3/dom/base/nsIFrameLoader.idl#246)
Component: Untriaged → Session Restore
Assignee | ||
Comment 2•8 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•8 years ago
|
||
Otherwise I agree that we could use lazyWidth here.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dao+bmo
Component: Session Restore → DOM: Core & HTML
Product: Firefox → Core
Assignee | ||
Comment 5•8 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.
Updated•8 years ago
|
Status: NEW → ASSIGNED
Flags: qe-verify?
Priority: -- → P1
Comment 6•8 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•8 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) |
Assignee | ||
Comment 9•8 years ago
|
||
Updated•8 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
Updated•8 years ago
|
Attachment #8860693 -
Flags: review?(bzbarsky)
Comment 10•8 years ago
|
||
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•8 years ago
|
Attachment #8860693 -
Flags: review?(tnikkel)
Comment 11•8 years ago
|
||
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•8 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•8 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•8 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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•8 years ago
|
Iteration: --- → 55.4 - May 1
Assignee | ||
Comment 17•8 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•8 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•8 years ago
|
Flags: needinfo?(dao+bmo)
Comment 19•8 years ago
|
||
bugherder |
Updated•8 years ago
|
No longer blocks: photon-performance-triage
Updated•8 years ago
|
Blocks: photon-performance-triage
Comment 20•7 years ago
|
||
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]
Comment 21•7 years ago
|
||
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.
Updated•3 years ago
|
Performance Impact: --- → ?
Whiteboard: [ohnoreflow][qf][photon-performance][qa-commented] → [ohnoreflow][photon-performance][qa-commented]
You need to log in
before you can comment on or make changes to this bug.
Description
•