Add a way of reliably dirtying the frame tree so that a layout flush will be necessary during reflow tests

RESOLVED FIXED in Firefox 56

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
3 months ago
18 days ago

People

(Reporter: mconley, Unassigned)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

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

Attachments

(5 attachments)

(Reporter)

Description

3 months ago
I'm working on tests in bug 1354194 that monitor various key interactions for layout flushes.

In order to do this semi-deterministically, I've been using an "all event listener" to manually dirty the browser window DOM before each event fires.

This seems to work, but there are problems.

For one thing, I seem to need to special case dirtying things in side the scroll-able tab strip, because apparently even if I dirty the outer browser window DOM, the scrollable tab strip might _not_ cause layout flushes during some tab operations. The only way I seemed to be able to get around this is to dirty a frame _inside_ the scrollable tab strip.

The other problem is that the default dirtying I'm doing has the capacity to fail intermittently. The way it works is that it slowly increases the margin of the tab-view-deck DOM element in browser.xul until it reaches some maximum, and then loops back to 0. It is possible, with this strategy, for the right number of events to fire such that the margin is the _same_ from the last reflow to the next, meaning that (I believe) that layout will believe that the DOM is not dirty, and no reflow will occur. It's a bit like Russian roulette on that one.

So far, these two cases haven't stopped me, but they make me uneasy. This latter problem seems ripe for intermittent failure, if not now, then down the line.

Could we add a method to nsIDOMWindowUtils that takes some frame, finds the closest scroll-able ancestor and dirties that ancestor? I think that would help here, and then I could avoid all of this weird manual DOM manipulation.
(Reporter)

Updated

3 months ago
Summary: Add a way of reliably dirtying the frame tree so that a layout flush will be necessary → Add a way of reliably dirtying the frame tree so that a layout flush will be necessary during reflow tests
(Reporter)

Comment 1

3 months ago
Hey tnikkel, got any ideas on how I could do this reliably?

I've got a WIP that attempts to QI an nsIDOMElement to an nsIContent, get its primary frame, and then call FrameNeedsReflow on its PresShell.

That's the theory anyhow. In practice, I can't seem to get the primary frame - it's nullptr for the current element I'm passing in (in this case, the document.documentElement of a new browser window). Perhaps I'm confused about what primary frame means.

How can I ensure that the top-level frame is "dirtied"? And how can I also address the case where one of the child "reflow roots" needs to be dirtied?
Flags: needinfo?(tnikkel)
It seems most likely that you're running the script before we've constructed frames.  If you wanted to ensure there is a frame if there should be one, you'd need to call presShell->FlushPendingNotifications(FlushType::Frames) to force a flush.



Also, from #layout:

10:32:54 <mconley> dbaron: Hey - suppose I wanted to take a crack at bug 1363361. Would the right approach be to accept some nsIDOMElement, find its primary frame, find my way to its PresShell, can then call PresShell::FrameNeedsReflow?

18:31:09 <~dbaron> mconley: sorry, totally missed your question about 1363361 amid the other mentions.  That approach seems fine, modulo FrameNeedsReflow having a bunch of options that do different things, and it's not clear to me which you want
Flags: needinfo?(tnikkel)

Updated

2 months ago
Blocks: 1376822
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 months ago
mozreview-review
Comment on attachment 8882174 [details]
Bug 1363361 - Make reflow tests use nsIDOMWindowUtils.ensureDirtyRootFrame to avoid manual frame dirtying hack.

https://reviewboard.mozilla.org/r/153290/#review158504

Nice! :-)

::: browser/base/content/test/performance/head.js:77
(Diff revision 1)
> +  let dirtyFrameFn = () => {
> +    try {
> +      dwu.ensureDirtyRootFrame();
> +    } catch(e) {
> +      // If this fails, we should probably make note of it, but it's not fatal.
> +      info("Note: ensureDirtyRootFrame threw an exception.");

When is this failing/why is it not fatal?
Attachment #8882174 - Flags: review?(florian) → review+

Comment 6

2 months ago
mozreview-review
Comment on attachment 8882173 [details]
Bug 1363361 - Add ability to dirty root frame from nsIDOMWindowUtils.

https://reviewboard.mozilla.org/r/153288/#review158784

r=me

::: dom/interfaces/base/nsIDOMWindowUtils.idl:2029
(Diff revision 1)
>    /**
> +   * Flips the NS_FRAME_IS_DIRTY bit on the root frame so that a layout flush
> +   * will be necessary.

This isn't quite correct -- this function does a bit more than just flipping that one bit. (FrameNeedsReflow is more than a bit flip.)

How about: "Calls FrameNeedsReflow on the root frame, [...]"
Attachment #8882173 - Flags: review?(dholbert) → review+
(Reporter)

Updated

a month ago
Blocks: 1356271
(Reporter)

Comment 7

a month ago
mozreview-review-reply
Comment on attachment 8882174 [details]
Bug 1363361 - Make reflow tests use nsIDOMWindowUtils.ensureDirtyRootFrame to avoid manual frame dirtying hack.

https://reviewboard.mozilla.org/r/153290/#review158504

> When is this failing/why is it not fatal?

At least for browser_windowopen_reflows.js, it's possible for this to run before the PresShell has been set up, which causes this to fail. We'll pick up dirtying the window when it's first made available on the DOMWindowCreated event.
(Reporter)

Comment 8

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b69743de8028
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a month ago
mozreview-review
Comment on attachment 8885900 [details]
Bug 1363361 - Update expected reflows on window open now that we're using nsIDOMWindowUtils to dirty the frame tree.

https://reviewboard.mozilla.org/r/156676/#review161780
Attachment #8885900 - Flags: review?(florian) → review+

Comment 15

a month ago
mozreview-review
Comment on attachment 8885901 [details]
Bug 1363361 - Disable browser_appmenu_reflows.js on Linux and Windows debug builds for intermittent failures.

https://reviewboard.mozilla.org/r/156678/#review161782

::: browser/base/content/test/performance/browser_appmenu_reflows.js:108
(Diff revision 1)
>    /**
>     * Please don't add anything new!
>     */
>  ];
>  
> +const WIN_DEBUG_E10S = Services.appinfo.OS == "WINNT" &&

Do we have any idea of why the behavior is different in this specific configuration? Should we file a bug to investigate and reference the bug number here?
Attachment #8885901 - Flags: review?(florian) → review+

Comment 16

a month ago
mozreview-review
Comment on attachment 8885902 [details]
Bug 1363361 - Disable browser_windowopen_reflows.js on Linux and Win 8 x64 for frequent failures.

https://reviewboard.mozilla.org/r/156680/#review161786
Attachment #8885902 - Flags: review?(florian) → review+
(Reporter)

Comment 17

a month ago
mozreview-review-reply
Comment on attachment 8885901 [details]
Bug 1363361 - Disable browser_appmenu_reflows.js on Linux and Windows debug builds for intermittent failures.

https://reviewboard.mozilla.org/r/156678/#review161782

> Do we have any idea of why the behavior is different in this specific configuration? Should we file a bug to investigate and reference the bug number here?

Good idea. I'll do that before landing. Thanks!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

a month ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da4b7c8c4ce5
Add ability to dirty root frame from nsIDOMWindowUtils. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/aea6941d1f7b
Make reflow tests use nsIDOMWindowUtils.ensureDirtyRootFrame to avoid manual frame dirtying hack. r=florian
https://hg.mozilla.org/integration/autoland/rev/cd87e778037e
Update expected reflows on window open now that we're using nsIDOMWindowUtils to dirty the frame tree. r=florian
https://hg.mozilla.org/integration/autoland/rev/d441805ccbbd
Adjust browser_appmenu_reflows.js now that we're using nsIDOMWindowUtils to dirty the frame tree. r=florian
https://hg.mozilla.org/integration/autoland/rev/81208848b53e
Disable browser_windowopen_reflows.js on Linux for frequent failures. r=florian
Backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=114047758&repo=autoland on at least Windows 8.

https://hg.mozilla.org/integration/autoland/rev/41bbd615a9a953ef0919242b191f2e6f6cce6ff8
Flags: needinfo?(mconley)
(Reporter)

Comment 22

a month ago
Pretty wacky stuff happening here. On Windows 8, it looks like this test causes the new window to not paint properly:

http://mozilla-releng-blobs.s3.amazonaws.com/blobs/autoland/sha512/adbe6216da03df8c6541b3fe612830a8a963811e6c1a4d8e9fd430c55c0141dc84741de47e49408b58076bfc776cb9d6eb8f9692d6e8f84bf6b95e7ca2c34f36

I'm going to file a follow-up bug to investigate. In the meantime, I'll disable this test for Windows 8.
Flags: needinfo?(mconley)
(Reporter)

Updated

a month ago
See Also: → bug 1381521
(Reporter)

Updated

a month ago
See Also: → bug 1380465
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 28

a month ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5c974dcdddd7
Add ability to dirty root frame from nsIDOMWindowUtils. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/4e3857566e76
Make reflow tests use nsIDOMWindowUtils.ensureDirtyRootFrame to avoid manual frame dirtying hack. r=florian
https://hg.mozilla.org/integration/autoland/rev/db627e28977e
Update expected reflows on window open now that we're using nsIDOMWindowUtils to dirty the frame tree. r=florian
https://hg.mozilla.org/integration/autoland/rev/355a7da0154c
Adjust browser_appmenu_reflows.js now that we're using nsIDOMWindowUtils to dirty the frame tree. r=florian
https://hg.mozilla.org/integration/autoland/rev/40505345a865
Disable browser_windowopen_reflows.js on Linux and Win 8 x64 for frequent failures. r=florian

Updated

a month ago
Priority: -- → P3
Backed out for running and failing browser_windowopen_reflows.js on Windows 8 x64:

https://hg.mozilla.org/integration/autoland/rev/8ff4f17b266db9a780efe06f7fbdae629e49f5bc
https://hg.mozilla.org/integration/autoland/rev/8acc1ed07ff9cd10c329f7b86fc010498c4c78b0
https://hg.mozilla.org/integration/autoland/rev/1bce5b6bf1b712451345a9ab9e632b4c708ae34e
https://hg.mozilla.org/integration/autoland/rev/7443be3a1a0ed2b26a1f10e8fa747ab3fc5ac3fe
https://hg.mozilla.org/integration/autoland/rev/8c0a5834a1d5e978042edabe57ceeadb0a9da0e8

First push with test run and failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5fb7a6989ca46671043f859247d5e3440bc8068b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=115050092&repo=autoland

17:44:34     INFO - TEST-START | browser/base/content/test/performance/browser_windowopen_reflows.js
17:44:34     INFO - TEST-INFO | started process screenshot
17:44:34     INFO - TEST-INFO | screenshot: exit 0
17:44:34     INFO - Buffered messages logged at 17:44:34
17:44:34     INFO - Entering test bound 
17:44:34     INFO - Skipping this test because of perma-failures on Windows 8 x64 (bug 1381521)
17:44:34     INFO - Leaving test bound 
17:44:34     INFO - Buffered messages finished
17:44:34     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_windowopen_reflows.js | This test contains no passes, no fails and no todos. Maybe it threw a silent exception? Make sure you use waitForExplicitFinish() if you need it. -
Flags: needinfo?(mconley)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

a month ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2f02ad2c82b
Add ability to dirty root frame from nsIDOMWindowUtils. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/4433f993d523
Make reflow tests use nsIDOMWindowUtils.ensureDirtyRootFrame to avoid manual frame dirtying hack. r=florian
https://hg.mozilla.org/integration/autoland/rev/5b631acee0b8
Update expected reflows on window open now that we're using nsIDOMWindowUtils to dirty the frame tree. r=florian
https://hg.mozilla.org/integration/autoland/rev/7a7d6d2d2b2d
Adjust browser_appmenu_reflows.js now that we're using nsIDOMWindowUtils to dirty the frame tree. r=florian
https://hg.mozilla.org/integration/autoland/rev/d94de30b99a3
Disable browser_windowopen_reflows.js on Linux and Win 8 x64 for frequent failures. r=florian
https://hg.mozilla.org/mozilla-central/rev/b2f02ad2c82b
https://hg.mozilla.org/mozilla-central/rev/4433f993d523
https://hg.mozilla.org/mozilla-central/rev/5b631acee0b8
https://hg.mozilla.org/mozilla-central/rev/7a7d6d2d2b2d
https://hg.mozilla.org/mozilla-central/rev/d94de30b99a3
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Backed out for frequently failing browser_appmenu_reflows.js on OS X:

https://hg.mozilla.org/mozilla-central/rev/68046a58f82913eb7804e4796ec981f6f8ea490e
https://hg.mozilla.org/mozilla-central/rev/c37e118e823b15e9569acf3b51d15f584ab1e81d
https://hg.mozilla.org/mozilla-central/rev/804cedeeb0a848c5cb21e1a8d1d68700c1cc7a7a
https://hg.mozilla.org/mozilla-central/rev/c59bc35309aa0821625f8ac99f474a28146994ff
https://hg.mozilla.org/mozilla-central/rev/9a37323f4289ba8830f3e570a248358200c5a57c

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=115776946&repo=autoland

19:44:42     INFO - Click appMenu-library-button
19:44:42     INFO - Buffered messages finished
19:44:42     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_appmenu_reflows.js | unexpected uninterruptible reflow 
19:44:42     INFO - [
19:44:42     INFO - 	"get_alignmentPosition@chrome://global/content/bindings/popup.xml:158:11",
19:44:42     INFO - 	"adjustArrowPosition@chrome://global/content/bindings/popup.xml:428:13",
19:44:42     INFO - 	"onxblpopuppositioned@chrome://global/content/bindings/popup.xml:525:9",
19:44:42     INFO - 	""
19:44:42     INFO - ]
19:44:42     INFO -  - false == true - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/performance/head.js :: reflow :: line 114
19:44:42     INFO - Stack trace:
19:44:42     INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:reflow:114
19:44:42     INFO - chrome://global/content/bindings/popup.xml:get_alignmentPosition:158
19:44:42     INFO - chrome://global/content/bindings/popup.xml:adjustArrowPosition:428
19:44:42     INFO - chrome://global/content/bindings/popup.xml:onxblpopuppositioned:525
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 45

28 days ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd3c2bb5c787
Add ability to dirty root frame from nsIDOMWindowUtils. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/65fbaf1abd1d
Make reflow tests use nsIDOMWindowUtils.ensureDirtyRootFrame to avoid manual frame dirtying hack. r=florian
https://hg.mozilla.org/integration/autoland/rev/51c18c1a9eef
Update expected reflows on window open now that we're using nsIDOMWindowUtils to dirty the frame tree. r=florian
https://hg.mozilla.org/integration/autoland/rev/b05b1b40f5d2
Disable browser_appmenu_reflows.js on Linux and Windows debug builds for intermittent failures. r=florian
https://hg.mozilla.org/integration/autoland/rev/31409d8a0483
Disable browser_windowopen_reflows.js on Linux and Win 8 x64 for frequent failures. r=florian
https://hg.mozilla.org/mozilla-central/rev/bd3c2bb5c787
https://hg.mozilla.org/mozilla-central/rev/65fbaf1abd1d
https://hg.mozilla.org/mozilla-central/rev/51c18c1a9eef
https://hg.mozilla.org/mozilla-central/rev/b05b1b40f5d2
https://hg.mozilla.org/mozilla-central/rev/31409d8a0483
Status: REOPENED → RESOLVED
Last Resolved: a month ago28 days ago
Resolution: --- → FIXED
(Reporter)

Updated

25 days ago
Flags: needinfo?(mconley)
See Also: → bug 1369959
You need to log in before you can comment on or make changes to this bug.