Closed Bug 1448096 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Inspector, defect, P3)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(Performance Impact:high, firefox62 wontfix, firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: dholbert, Assigned: Gijs)

References

Details

(Keywords: perf, Whiteboard: [fxperf:p1])

Attachments

(2 files, 1 obsolete file)

example.gif
3.46 MB, image/gif
Details
Bug 1448096 - use promiseDocumentFlushed to avoid sync reflows when resizing devtools windows,
59 bytes, text/x-review-board-request
bgrins
: review+
mconley
: review+
Details 
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.
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
Whiteboard: [qf] → [qf][fxperf]
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.
Attached image example.gif — 

    
    

    
  
(I spun off bug 1448130 on the fact that we may be overpainting here, BTW.)
See Also:  → 1448130
Whiteboard: [qf][fxperf] → [qf:p1][qf:f64][fxperf]

    
    

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

    
    

    
  
ni? :ochameau, for the devtools perf effort
Flags: needinfo?(poirot.alex)

    
    

    
  
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.
Flags: needinfo?(poirot.alex)
Assignee: nobody → gl
Status: NEW → ASSIGNED
Flags: needinfo?(gl)
Priority: -- → P3

    
    

    
      
  


  

    
    

    
  
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.

        Attachment #8965497 -
        Flags: review?(bgrinstead) → review+

    
    

    
  
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db6ccdb84816
Throttle the inspector window resize handler with requestIdleCallback. r=bgrins

    
    

    
  
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef4c55400ab1
Throttle the inspector window resize handler with requestIdleCallback. r=bgrins

    
    

    
  
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f983fa3a60ca
Throttle the inspector window resize handler with requestIdleCallback. r=bgrins

    
    

    
  
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
Flags: needinfo?(gl)

    
    

    
  
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?
Flags: needinfo?(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.
Flags: needinfo?(gl)

    
    

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

    
    

    
  
: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?
Flags: needinfo?(gl)
Whiteboard: [qf:p1][qf:f64][fxperf] → [qf:p1][qf:f64][fxperf:p2]

    
    

    
  
(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.
Flags: needinfo?(gl)
Assignee: gl → nobody
Status: ASSIGNED → NEW

    
    

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

Moving this patch to Bug 1457114.

        Attachment #8965497 -
        Attachment is obsolete: true
No longer blocks: 1433716

    
  
Whiteboard: [qf:p1][qf:f64][fxperf:p2] → [qf:p1:f64][fxperf:p2]

    
  
Product: Firefox → DevTools

    
    

    
      
  


  

    
    

    
  
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 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 }

        Attachment #8986255 -
        Flags: review?(gl) → review?(bgrinstead)

    
    

    
  
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.

        Attachment #8986255 -
        Flags: review?(mconley) → review+

    
    

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

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

        Attachment #8986255 -
        Flags: review?(bgrinstead) → review+

    
    

    
  
(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.

    
  
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

    
    

    
      
  

    
    

    
  
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/48f880cffaed
use promiseDocumentFlushed to avoid sync reflows when resizing devtools windows, r=bgrins,mconley

    
    

    
  
Backout by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/31ac168c31d6
Backed out changeset 48f880cffaed for devtools mass failure on a CLOSED TREE

    
    

    
  
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
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
      
  

    
    

    
  
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
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/47192c4defeb
use promiseDocumentFlushed to avoid sync reflows when resizing devtools windows, r=bgrins,mconley
Whiteboard: [qf:p1:f64][fxperf:p2] → [qf:p1:f64][fxperf:p1]

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/47192c4defeb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox63:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63

    
  
Depends on: 1472364

    
    

    
  
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.

          status-firefox62:
          --- → ?
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
(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.

          status-firefox62:
          ? → affected
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(magicp.jp)

    
    

    
  
(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.

    
    

    
  
(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...)
Flags: needinfo?(magicp.jp)

    
    

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

          status-firefox62:
          affected → fix-optional
Performance Impact: --- → P1
Whiteboard: [qf:p1:f64][fxperf:p1] → [fxperf:p1]

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: