1448096 - During inspector panel drag-to-resize operation, JS triggers two reflows per event loop (1 extra)

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Daniel Holbert [:dholbert]]

digitarald just sent me this profile from @mbalfanz:
  https://perfht.ml/2ptmlJZ

It shows janky responsiveness (~40ms between paints = 20-30fps) during a click-and-drag operation to resize inspector panels.

Each ~40ms paint cycle involves three large things, shown in this example snippet:
https://perfht.ml/2pxdgzP
 (1) A 11ms reflow triggered by the mouse-move event itself (nsBaseWidget::DispatchInputEvent, eventually calling into mozilla::EventStateManager::PreHandleEvent, which flushes layout)

 (2) Another 11ms reflow (just as expensive), this time triggered by a layout flush from "clientWidth" access during JS from the mousemove event. ("onPanelWindowResize" calling "useLandscapeMode" which uses clientWidth, here:
https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/devtools/client/inspector/inspector.js#454

 (3) An ~8.5ms paint ("rasterize")

We can optimize reflow and paint, but for the purposes of this bug, I'd like to focus on "(2)" -- the fact that we're triggering that extra reflow here (seemingly, just to tell us (on a per-paint granularity) whether we should switch to landscape mode).

Maybe we should be caching clientWidth from the previous mousemove, or from before we've made any style changes, so that it doesn't trigger an extra layout flush? I'll bet we've got some best practices here.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

So this looks like we're reorganizing the DOM based on the dimensions of the window.

Ideally, we'd use something like CSS grid / some responsive design stuff to offload all of this logic into Layout, and avoid having to think about it in JS at all. That'd be best.

If that's impossible for some reason, the next best thing is to avoid the flush by waiting until asking for clientWidth is cheap. It's now possible to wait until after the next refresh driver tick, where Style and Layout have been flushed "naturally", using window.promiseDocumentFlushed[1]. If offloading the computation to Layout is not possible, then we should fallback to using promiseDocumentFlushed (and hopefully React is designed to modify the DOM in requestAnimationFrame).

My 2c on how to proceed here, anyhow.

[1]: See https://developer.mozilla.org/en-US/Firefox/Performance_best_practices_for_Firefox_fe_engineers#Detecting_and_avoiding_synchronous_reflow

Comment 2

[Martin Balfanz [:mbalfanz]]

Some background info:

System: OS X 10.13.3, latest nightly
Flags: I have `devtools.inspector.split-rule-enabled` set to true to get the 3-pane inspector.  No other flags are modified.

Steps to reproduce:

1. Visit some page (I used https://mozilladevelopers.github.io/playground/) and open the inspector.  
2. Grab the bottom left corner of the browser and start resizing.

The lag is especially noticeable when you drag over a longer distance.  Please see the attached gif showing my test scenario.

Comment 3

[Martin Balfanz [:mbalfanz]]

Attachment: example.gif

Comment 4

[Daniel Holbert [:dholbert]]

(I spun off bug 1448130 on the fact that we may be overpainting here, BTW.)

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Hey gl, what are the chances someone from the DevTools team could take a crack at this? This kills the window resize performance. :/

Comment 6

[:Harald Kirschner :digitarald]

ni? :ochameau, for the devtools perf effort

Comment 7

[Alexandre Poirot [:ochameau]]

I made an update about resize perf during Perf standup. We don't have much resource on inspector. But if this slowness is specific to 3 panes panel as said in comment 2, may be someone from design-tools can look into this?

Otherwise, perf issues on resize is a global issue across tools.
I'll try to go through all popular panels and see if some are particularly affected by default.

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1448096 - Throttle the inspector window resize handler with requestIdleCallback.

Review commit: https://reviewboard.mozilla.org/r/234274/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234274/

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8965497 [details]
Bug 1448096 - Throttle the inspector window resize handler with requestIdleCallback.

https://reviewboard.mozilla.org/r/234274/#review239892

I haven't tested the perf impact but this will definitely help, and it seems reasonable since we don't need to be eagerly flipping the sidebar vertically/horizontally exactly when we hit PORTRAIT_MODE_WIDTH px. This also follows conventions from other tools as per https://bugzilla.mozilla.org/show_bug.cgi?id=1442531#c41.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db6ccdb84816
Throttle the inspector window resize handler with requestIdleCallback. r=bgrins

Comment 11

[Dorel Luca [:dluca]]

Backed out changeset db6ccdb84816 (bug 1448096) for Dev Tools failures on multiple files

Log of the failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=172217143&repo=mozilla-inbound&lineNumber=19857

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8514e653b71479ba22a307f58f79f563ed80e9d

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=db6ccdb848165514252bf3d7958d9679cc0fc062

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef4c55400ab1
Throttle the inspector window resize handler with requestIdleCallback. r=bgrins

Comment 13

[Andreea Pavel [:apavel]]

Backed out for failing devtools at  devtools/client/webconsole/test/browser_webconsole_split.js 

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ef4c55400ab17bcf2ff1d197fa8c9f9a1e0057b4

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=172237607&repo=mozilla-inbound&lineNumber=20801
https://treeherder.mozilla.org/logviewer.html#?job_id=172237611&repo=mozilla-inbound&lineNumber=17938

https://treeherder.mozilla.org/logviewer.html#?job_id=172238819&repo=mozilla-inbound&lineNumber=7092
https://treeherder.mozilla.org/logviewer.html#?job_id=172242554&repo=mozilla-inbound&lineNumber=4414


Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bac6aa98018c9948220713fb7cddb1f0c1096f

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f983fa3a60ca
Throttle the inspector window resize handler with requestIdleCallback. r=bgrins

Comment 15

[Stefan Hindli [:stefan_hindli]]

Backed out changeset f983fa3a60ca (bug 1448096) for dt failures in devtools/client/inspector/test/browser_inspector_breadcrumbs_visibility.js on a CLOSED TREE

Problematic push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f983fa3a60ca5194fb452545230aabd6463eb320&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=620a4eba9389d4cfebb8b45c913d7a13921dd49a&selectedJob=172364476
Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d3867cec39d1d139085584d67f9831b9d912b13f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=172364476&repo=mozilla-inbound&lineNumber=6980

20:32:58    ERROR -  284 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_breadcrumbs_visibility.js | div#aThirdVeryLongIdToExceedTheTruncationLimit crumb is visible - Got false, expected true
20:32:58     INFO -  Stack trace:
20:32:58     INFO -  chrome://mochikit/content/browser-test.js:test_is:1280
20:32:58     INFO -  chrome://mochitests/content/browser/devtools/client/inspector/test/browser_inspector_breadcrumbs_visibility.js:testBreadcrumbTransitions:92
20:32:58     INFO -  285 INFO Checking for visibility of crumb div#anotherVeryLongIdToExceedTheBreadcrumbTruncationLimit
20:32:58     INFO -  286 INFO Simulating click of start button
20:32:58     INFO -  Not taking screenshot here: see the one that was previously logged
20:32:58    ERROR -  287 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_breadcrumbs_visibility.js | div#anotherVeryLongIdToExceedTheBreadcrumbTruncationLimit crumb is visible - Got false, expected true
20:32:58     INFO -  Stack trace:
20:32:58     INFO -  chrome://mochikit/content/browser-test.js:test_is:1280
20:32:58     INFO -  chrome://mochitests/content/browser/devtools/client/inspector/test/browser_inspector_breadcrumbs_visibility.js:testBreadcrumbTransitions:92
20:32:58     INFO -  288 INFO Checking for visibility of crumb div#aVeryLongIdToExceedTheBreadcrumbTruncationLimit
20:32:58     INFO -  289 INFO Simulating click of start button
20:32:58     INFO -  Not taking screenshot here: see the one that was previously logged
20:32:58    ERROR -  290 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_breadcrumbs_visibility.js | div#aVeryLongIdToExceedTheBreadcrumbTruncationLimit crumb is visible - Got false, expected true

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Throttling the sync layout flushes is strictly better than what we were doing before - however, eliminating them entirely (some ideas on how to do that in comment 1) would be ideal.

Could we not attempt to avoid them entirely?

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #16)
> Throttling the sync layout flushes is strictly better than what we were
> doing before - however, eliminating them entirely (some ideas on how to do
> that in comment 1) would be ideal.
> 
> Could we not attempt to avoid them entirely?

Unfortunately, that is not as trivial of a fix at this moment. I would file a follow up or I can spin the current patch into a different bug.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Okay, that's fair. Let's spin the current patch into a different bug, I guess. Thanks!

Comment 19

[:Gijs (he/him)]

:gl, what's the current state of the patch here? Looks like it was backed out last time we tried to land this; are you still working on updating it, or did it already land somewhere else or something?

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to :Gijs (he/him) from comment #19)
> :gl, what's the current state of the patch here? Looks like it was backed
> out last time we tried to land this; are you still working on updating it,
> or did it already land somewhere else or something?

I am moving the current patch over to Bug 1457114. There is still some test failures with the current patch that I am backlogged on figuring out for the time being. I am still aiming to land the current patch to throttle the resize call for this release.

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8965497 [details]
Bug 1448096 - Throttle the inspector window resize handler with requestIdleCallback.

Moving this patch to Bug 1457114.

Comment 22

[:Gijs (he/him)]

Attachment: Bug 1448096 - use promiseDocumentFlushed to avoid sync reflows when resizing devtools windows,

Review commit: https://reviewboard.mozilla.org/r/251638/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251638/

Comment 23

[:Gijs (he/him)]

The attached cset seems to wfm, but I'd like some feedback if this is the right thing to do.

I moved away from the perpetual idle task stuff given bug 1469184 which implies that can lead to the resize not resulting in a change of inspector layout for a Long Time (which I could reproduce, even on a powerful mbp - not 100% sure what's up with that). As far as I can tell these changes fix that, but I'd be eager to hear if you think this is a correct fix or not.

Comment 24

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8986255 [details]
Bug 1448096 - use promiseDocumentFlushed to avoid sync reflows when resizing devtools windows,

https://reviewboard.mozilla.org/r/251638/#review258080

Gonna defer the final review to bgrins

::: devtools/client/inspector/inspector.js:585
(Diff revision 1)
> +    // Use window.top because promiseDocumentFlushed() in a subframe doesn't
> +    // work, see https://bugzilla.mozilla.org/show_bug.cgi?id=1441173
> +    const useLandscapeMode = await window.top.promiseDocumentFlushed(() => {
> +      return this.useLandscapeMode();
> +    });
> +    this.splitBox.setState({vert: useLandscapeMode});

s/{vert: useLandscapeMode}/{ vert: useLandscapeMode }

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8986255 [details]
Bug 1448096 - use promiseDocumentFlushed to avoid sync reflows when resizing devtools windows,

https://reviewboard.mozilla.org/r/251638/#review258588

Thanks! I don't really know Inspector that well, but I'm certainly familiar with DeferredTask and promiseDocumentFlushed, and the usage seems to make sense here. Just one note - see below.

::: devtools/client/inspector/inspector.js:598
(Diff revision 1)
>    onPanelWindowResize: function() {
> -    window.cancelIdleCallback(this._resizeTimerId);
> -    this._resizeTimerId = window.requestIdleCallback(() => {
> -      this.splitBox.setState({
> -        vert: this.useLandscapeMode(),
> -      });
> +    if (!this._lazyResizeHandler) {
> +      this._lazyResizeHandler = new DeferredTask(this._onLazyPanelResize.bind(this),
> +                                                 LAZY_RESIZE_INTERVAL_MS, 0);
> +    }
> +    this._lazyResizeHandler.arm();

I might be telling you something you already know, but I want to make sure it's clear:

Repeatedly calling `arm()` on a DeferredTask does not cause the internal timer to reset. This means that if we have a slew of resize events coming in over the course of several seconds, they're going to be coalesced to fire \_onLazyPanelResize every 200ms.

I say this, because I think some people expect DeferredTask to wait until `arm()`'s stop coming in for the delay period before firing the callback.

If coalescing every 200ms is what you're trying to do, then this is fine. If you wanted to only run 200ms after the last resize event without an `arm()` event after it, then you'll need to `disarm()` before `arm()`'ing.

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8986255 [details]
Bug 1448096 - use promiseDocumentFlushed to avoid sync reflows when resizing devtools windows,

https://reviewboard.mozilla.org/r/251638/#review259444

Comment 28

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos / reviews) from comment #26)
> Comment on attachment 8986255 [details]
> Bug 1448096 - use promiseDocumentFlushed to avoid sync reflows when resizing
> devtools windows,
> 
> https://reviewboard.mozilla.org/r/251638/#review258588
> 
> Thanks! I don't really know Inspector that well, but I'm certainly familiar
> with DeferredTask and promiseDocumentFlushed, and the usage seems to make
> sense here. Just one note - see below.
> 
> ::: devtools/client/inspector/inspector.js:598
> (Diff revision 1)
> >    onPanelWindowResize: function() {
> > -    window.cancelIdleCallback(this._resizeTimerId);
> > -    this._resizeTimerId = window.requestIdleCallback(() => {
> > -      this.splitBox.setState({
> > -        vert: this.useLandscapeMode(),
> > -      });
> > +    if (!this._lazyResizeHandler) {
> > +      this._lazyResizeHandler = new DeferredTask(this._onLazyPanelResize.bind(this),
> > +                                                 LAZY_RESIZE_INTERVAL_MS, 0);
> > +    }
> > +    this._lazyResizeHandler.arm();
> 
> I might be telling you something you already know, but I want to make sure
> it's clear:
> 
> Repeatedly calling `arm()` on a DeferredTask does not cause the internal
> timer to reset. This means that if we have a slew of resize events coming in
> over the course of several seconds, they're going to be coalesced to fire
> \_onLazyPanelResize every 200ms.
> 
> I say this, because I think some people expect DeferredTask to wait until
> `arm()`'s stop coming in for the delay period before firing the callback.
> 
> If coalescing every 200ms is what you're trying to do, then this is fine. If
> you wanted to only run 200ms after the last resize event without an `arm()`
> event after it, then you'll need to `disarm()` before `arm()`'ing.

Yep, this is what we do for the resize code in CUI as well. I'm happy to hear it if there's a compelling reason to do something else, but in this case I actually think coalescing every N (m)s is better than postponing indefinitely while resize events continue to come in (ie the thing you suggest in your second paragraphs). This is because if we continue to delay responding, then the window won't seem to deal with resize events at all - which would be a bit odd because other bits of the window (in the case of devtools, the browser content window itself) *will* respond to resizes, so it's confusing ("looks broken") to users if/when the other bit(s) of the window don't change.

Comment 29

[:Gijs (he/him)]

Comment on attachment 8986255 [details]
Bug 1448096 - use promiseDocumentFlushed to avoid sync reflows when resizing devtools windows,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251638/diff/1-2/

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48f880cffaed
use promiseDocumentFlushed to avoid sync reflows when resizing devtools windows, r=bgrins,mconley

Comment 31

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31ac168c31d6
Backed out changeset 48f880cffaed for devtools mass failure on a CLOSED TREE

Comment 32

[Arthur Iakab [arthur_iakab]]

Backed out changeset 48f880cffaed (bug 1448096) for devtools mass failure on a CLOSED TREE
Backout revision https://hg.mozilla.org/integration/autoland/rev/31ac168c31d6adce9a4f3d0fde8c21979dd0ab96
Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=48f880cffaed8d9432df7c48e599838228e515f2&filter-searchStr=dt

Next affected push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7e18d20c263f5ac0f99c6aa80bfb2e69d310805a&filter-searchStr=dt

Link to a recent log:https://treeherder.mozilla.org/logviewer.html#?job_id=184813661&repo=autoland&lineNumber=12729
Part of that log:[task 2018-06-25T23:35:00.364Z] 23:35:00     INFO - TEST-START | devtools/client/webconsole/test/mochitest/browser_webconsole_input_field_focus_on_panel_select.js
[task 2018-06-25T23:35:00.972Z] 23:35:00     INFO - GECKO(2842) | [Parent 2842, Main Thread] WARNING: /build/glib2.0-RnwmWL/glib2.0-2.48.2/./gobject/gsignal.c:3486: signal name 'load_complete' is invalid for instance '0xc2335700' of type 'MaiAtkType139': 'glib warning', file /builds/worker/workspace/build/src/toolkit/xre/nsSigHandlers.cpp, line 141
[task 2018-06-25T23:35:00.973Z] 23:35:00     INFO - GECKO(2842) | (firefox:2842): GLib-GObject-WARNING **: /build/glib2.0-RnwmWL/glib2.0-2.48.2/./gobject/gsignal.c:3486: signal name 'load_complete' is invalid for instance '0xc2335700' of type 'MaiAtkType139'
[task 2018-06-25T23:35:02.065Z] 23:35:02     INFO - GECKO(2842) | JavaScript error: resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js, line 32: Error: Minified React error #188; visit http://facebook.github.io/react/docs/error-decoder.html?invariant=188 for the full message or use the non-minified dev environment for full errors and additional helpful warnings.
[task 2018-06-25T23:35:02.418Z] 23:35:02     INFO - TEST-INFO | started process screentopng
[task 2018-06-25T23:35:02.920Z] 23:35:02     INFO - TEST-INFO | screentopng: exit 0
[task 2018-06-25T23:35:02.921Z] 23:35:02     INFO - Buffered messages logged at 23:35:00
[task 2018-06-25T23:35:02.923Z] 23:35:02     INFO - Entering test bound 
[task 2018-06-25T23:35:02.923Z] 23:35:02     INFO - Adding a new tab with URL: data:text/html;charset=utf8,<p>Test console input focus
[task 2018-06-25T23:35:02.925Z] 23:35:02     INFO - Tab added and finished loading
[task 2018-06-25T23:35:02.926Z] 23:35:02     INFO - Opening the toolbox
[task 2018-06-25T23:35:02.927Z] 23:35:02     INFO - Buffered messages logged at 23:35:01
[task 2018-06-25T23:35:02.928Z] 23:35:02     INFO - Toolbox opened and focused
[task 2018-06-25T23:35:02.929Z] 23:35:02     INFO - Focus after console is opened
[task 2018-06-25T23:35:02.931Z] 23:35:02     INFO - TEST-PASS | devtools/client/webconsole/test/mochitest/browser_webconsole_input_field_focus_on_panel_select.js | input node is focused after console is opened - 
[task 2018-06-25T23:35:02.932Z] 23:35:02     INFO - TEST-PASS | devtools/client/webconsole/test/mochitest/browser_webconsole_input_field_focus_on_panel_select.js | filter input should be focused - 
[task 2018-06-25T23:35:02.934Z] 23:35:02     INFO - TEST-PASS | devtools/client/webconsole/test/mochitest/browser_webconsole_input_field_focus_on_panel_select.js | input node is not focused anymore - 
[task 2018-06-25T23:35:02.935Z] 23:35:02     INFO - Go to the inspector panel
[task 2018-06-25T23:35:02.940Z] 23:35:02     INFO - Go back to the console
[task 2018-06-25T23:35:02.941Z] 23:35:02     INFO - TEST-PASS | devtools/client/webconsole/test/mochitest/browser_webconsole_input_field_focus_on_panel_select.js | input node is focused when coming from a different panel - 
[task 2018-06-25T23:35:02.942Z] 23:35:02     INFO - Leaving test bound 
[task 2018-06-25T23:35:02.943Z] 23:35:02     INFO - Buffered messages logged at 23:35:02
[task 2018-06-25T23:35:02.944Z] 23:35:02     INFO - Console message: [JavaScript Error: "Error: Minified React error #188; visit http://facebook.github.io/react/docs/error-decoder.html?invariant=188 for the full message or use the non-minified dev environment for full errors and additional helpful warnings." {file: "resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/vendor/react-dom.js" line: 32}]
[task 2018-06-25T23:35:02.945Z] 23:35:02     INFO - Removing tab.
[task 2018-06-25T23:35:02.946Z] 23:35:02     INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2018-06-25T23:35:02.947Z] 23:35:02     INFO - Got event: 'TabClose' on [object XULElement].
[task 2018-06-25T23:35:02.948Z] 23:35:02     INFO - Tab removed and finished closing
[task 2018-06-25T23:35:02.949Z] 23:35:02     INFO - Buffered messages finished
[task 2018-06-25T23:35:02.950Z] 23:35:02     INFO - TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/mochitest/browser_webconsole_input_field_focus_on_panel_select.js | A promise chain failed to handle a rejection: window.top is null - stack: _onLazyPanelResize@chrome://devtools/content/inspector/inspector.js:593:11
[task 2018-06-25T23:35:02.951Z] 23:35:02     INFO - Async*_runTask@resource://gre/modules/DeferredTask.jsm:308:15
[task 2018-06-25T23:35:02.954Z] 23:35:02     INFO - async*_timerCallback/<@resource://gre/modules/DeferredTask.jsm:278:13
[task 2018-06-25T23:35:02.954Z] 23:35:02     INFO - async*_timerCallback@resource://gre/modules/DeferredTask.jsm:276:30
[task 2018-06-25T23:35:02.955Z] 23:35:02     INFO - Rejection date: Mon Jun 25 2018 23:35:02 GMT+0000 (UTC) - false == true - JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoUncaughtRejections :: line 257
[task 2018-06-25T23:35:02.956Z] 23:35:02     INFO - Stack trace:
[task 2018-06-25T23:35:02.957Z] 23:35:02     INFO - resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:257
[task 2018-06-25T23:35:02.958Z] 23:35:02     INFO - chrome://mochikit/content/browser-test.js:nextTest:740
[task 2018-06-25T23:35:02.959Z] 23:35:02     INFO - chrome://mochikit/content/browser-test.js:testScope/test_finish/<:1392
[task 2018-06-25T23:35:02.959Z] 23:35:02     INFO - chrome://mochikit/content/browser-test.js:run:1329
[task 2018-06-25T23:35:02.960Z] 23:35:02     INFO - GECKO(2842) | JavaScript error: chrome://devtools/content/inspector/inspector.js, line 593: TypeError: window.top is null
[task 2018-06-25T23:35:02.961Z] 23:35:02     INFO - GECKO(2842) | MEMORY STAT | vsize 1095MB | residentFast 353MB | heapAllocated 125MB
[task 2018-06-25T23:35:02.963Z] 23:35:02     INFO - TEST-OK | devtools/client/webconsole/test/mochitest/browser_webconsole_input_field_focus_on_panel_select.js | took 2074ms


This affected dt2, dt3, dt4. dt5, dt5, dt6,dt7, dt8  and failed on - browser_fontinspector_theme-change.js
                                                                   - browser_animation_panel_exists.js
                                                                   - test/browser_net_background_update.js
                                                                   - Assertion failure: Should have an event loop
                                                                   - browser_dbg_pause-warning.js
                                                                   - browser_inspector_destroy-after-navigation.js

Comment 33

[:Gijs (he/him)]

Comment on attachment 8986255 [details]
Bug 1448096 - use promiseDocumentFlushed to avoid sync reflows when resizing devtools windows,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251638/diff/2-3/

Comment 34

[:Gijs (he/him)]

Well, that's embarrassing. The issue seems to be that `window.top` is null, and the cause seems to be that the window has been closed by the time the deferred task fires. A check for `window.closed` fixes things for me locally. Let's check if that's enough on infra, too:

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

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47192c4defeb
use promiseDocumentFlushed to avoid sync reflows when resizing devtools windows, r=bgrins,mconley

Comment 36

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/47192c4defeb

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

I notice in duplicate bug 1469184 that 62 is affected. Does this also affect 62?  If so, maybe this is a candidate for uplift to beta.

Comment 38

[:Gijs (he/him)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #37)
> I notice in duplicate bug 1469184 that 62 is affected. Does this also affect
> 62?  If so, maybe this is a candidate for uplift to beta.

Yes, but there's an outstanding regression. I don't know that I have time to look at that very soon, and I don't really understand if bug 1472364 is a better or worse thing to ship in practice than bug 1469184, because I don't resize devtools myself very often... Maybe magicp can clarify how they perceive these two bugs and whether they think we should uplift this.

Comment 39

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #38)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #37)
> > I notice in duplicate bug 1469184 that 62 is affected. Does this also affect
> > 62?  If so, maybe this is a candidate for uplift to beta.
> 
> Yes, but there's an outstanding regression. I don't know that I have time to
> look at that very soon, and I don't really understand if bug 1472364 is a
> better or worse thing to ship in practice than bug 1469184, because I don't
> resize devtools myself very often... Maybe magicp can clarify how they
> perceive these two bugs and whether they think we should uplift this.

FWIW, I put up a patch in bug 1472364, but I don't know if it's the right fix.

Comment 40

[magicp]

(In reply to :Gijs (he/him) from comment #38)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #37)
> > I notice in duplicate bug 1469184 that 62 is affected. Does this also affect
> > 62?  If so, maybe this is a candidate for uplift to beta.
> 
> Yes, but there's an outstanding regression. I don't know that I have time to
> look at that very soon, and I don't really understand if bug 1472364 is a
> better or worse thing to ship in practice than bug 1469184, because I don't
> resize devtools myself very often... Maybe magicp can clarify how they
> perceive these two bugs and whether they think we should uplift this.

I don't need uplifting this patch to 62 beta, because bug 1469184 cannot reproduce on latest 62 beta. (It is reproduced on 62a1. I don't know why...)

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, sounds like it isn't certain, and isn't going to block 62 release. I'll keep an eye on bug 1472364.