Closed Bug 1363361 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, enhancement, P3)

Product:
Core
Component:
Layout
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(5 files)

Bug 1363361 - Add ability to dirty root frame from nsIDOMWindowUtils.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1363361 - Make reflow tests use nsIDOMWindowUtils.ensureDirtyRootFrame to avoid manual frame dirtying hack.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1363361 - Update expected reflows on window open now that we're using nsIDOMWindowUtils to dirty the frame tree.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1363361 - Disable browser_appmenu_reflows.js on Linux and Windows debug builds for intermittent failures.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1363361 - Disable browser_windowopen_reflows.js on Linux and Win 8 x64 for frequent failures.
59 bytes, text/x-review-board-request
florian
: review+
Details 
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.
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
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)
Blocks: 1376822
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 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+
Blocks: 1356271
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.
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 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 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+
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!
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
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)
See Also: → 1381521
See Also: → 1380465
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
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)
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
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 → ---
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
Flags: needinfo?(mconley)
See Also: → 1369959
Assignee: nobody → mconley
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: