Closed Bug 1363361 Opened 8 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)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(5 files)

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)
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)
Assignee: nobody → mconley
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: