Closed Bug 1159042 Opened 9 years ago Closed 5 years ago

treat frames with fixed width and height as reflow roots

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox66 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

(Depends on 1 open bug, Blocks 6 open bugs)

Details

Attachments

(6 files)

Bug 1159042 - p1. Replace rootFrame variable with isRoot boolean in PresShell::DoReflow - r?dbaron
46 bytes, text/x-phabricator-request
Details | Review
Bug 1159042 - p2. Allow reflow roots to have overflow, and allow that overflow to change during reflow - r?dbaron,dholbert!
46 bytes, text/x-phabricator-request
Details | Review
Bug 1159042 - p3. Refactor mDirtyRoots type into a class - r?dbaron
46 bytes, text/x-phabricator-request
Details | Review
Bug 1159042 - p4. Make mDirtyRoots manage roots in preferred depth order - r?dbaron
46 bytes, text/x-phabricator-request
Details | Review
Bug 1159042 - p5. Add NS_FRAME_DYNAMIC_REFLOW_ROOT on frames that we can dynamically make reflow roots - r?dbaron
46 bytes, text/x-phabricator-request
Details | Review
Bug 1159042 - p6. Use NS_FRAME_DYNAMIC_REFLOW_ROOT - r?dbaron
46 bytes, text/x-phabricator-request
Details | Review 
One of the optimizations we want for CSS containment (bug 1150081) is treating frames that have 'contain' set as reflow roots, so that changes inside of them don't require a reflow that starts from the top of the tree.

However, we can do this for many other frames that have a fixed width and height as well, once we update reflow root handling to propagate overflow area changes (which should be trivial).

So we can do this, and then adding support for containment would be a simple change on top of this.
Assignee: dbaron → kzentner
I assigned this to myself because I actually had patches; I think mine were further along than Kyle's (we just discussed it), so taking this back, at least for now.
Assignee: kzentner → dbaron
My last try run was: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33b3e90f1283 .  I don't think I'd looked into any of the failures in it yet.
OK, looked into the failure of:
  layout/reftests/position-dynamic-changes/horizontal/leftA-widthN-rightA.html
in that try run, and I think the problem is the following:

The test case is dynamically changing 'width' on an absolutely positioned element with both 'height' and 'width' set, and thus subject to this optimization.  Per nsStylePosition::CalcDifference, the change hint for 'width' is:  NS_SubtractHint(nsChangeHint_AllReflowHints,
                                      NS_CombineHint(nsChangeHint_ClearDescendantIntrinsics,
                                                     nsChangeHint_NeedDirtyReflow))).
This is equivalent to nsChangeHint_NeedReflow | nsChangeHint_ClearAncestorIntrinsics.  Note that nsChangeHint_NeedDirtyReflow is not set.  This means that RestyleManager::StyleChangeReflow calls PresShell::FrameNeedsReflow(aFrame, nsIPresShell::eTreeChange, NS_FRAME_HAS_DIRTY_CHILDREN).  Because it's NS_FRAME_HAS_DIRTY_CHILDREN and not NS_FRAME_IS_DIRTY, PresShell::FrameNeedsReflow sets targetFrameDirty to false, which means it uses the reflow root optimization in this case, where it's actually not valid.

We need a way to know that these geometry style changes that don't set NS_FRAME_IS_DIRTY to still have targetFrameDirty true in FrameNeedsReflow so that we don't do the reflow root behavior in that case.

I believe this is a preexisting bug related to changing 'width' and similar properties on reflow roots.
I made that problem a separate bug, bug 1169440.
Is this being actively worked on? If not, what work in addition to dbaron's patches would be required?

This optimization significantly improve the performance of the new adwords.com in Firefox, so if necessary I or another Google engineer would be willing to work on it.
I'm not actively working on it, although I was hoping to get back to finishing it at some point soon.  The thing that needs to be done is getting the patches into some condition that will pass our tests.  (And even then, there might be bugs that show up after landing that aren't covered by our tests.)

However, one of the things I discovered while going through test failures that my initial patches triggered was that it's a valid optimization in substantially fewer cases than I was initially hoping.  In particular, for blocks, we can only do this for frames that establish new block formatting contexts.  Otherwise the patch would break behavior related to floats and behavior relating to margin collapsing.

The trickiest problem that I got stuck on was the failure:
REFTEST TEST-UNEXPECTED-FAIL | layout/base/crashtests/428113.xhtml | assertion count 1 is more than expected 0 assertions
The assertion was "Out-of-flow frame got reflowed before its placeholder", which seems like it might be a fundamental problem, since if we dynamically add an absolutely positioned element, with the placeholder and the primary frame separated by a dynamic reflow root, it's not clear what would make us reflow them in the correct order.  (The order is important because of the "hypothetical box" stuff that was a really stupid feature to begin with.)  Now that I think about this again, I suppose the right (or at least expedient) way around that would be to require that we only make the optimization on blocks that are also a containing block for absolutely positioned elements.  This would make it even less likely for the optimization to help existing Web content, but it would still be useful foundation work for CSS containment.  (However, the current CSS containment spec says that layout containment doesn't make an element a containing block for positioned descendants; only paint containment does.  I think that's probably a mistake)  I guess I should give that a try and see how many of the other test failures go away.

Help would be welcome, although this probably wouldn't normally be the sort of change I'd suggest working on to somebody writing their first Gecko patches.  It's probably not too difficult, though...
On :dholbert's suggestion, I've started looking at this, with the hope of adding CSS containment straight into it.

I've just managed to rebase and compile :dbaron's comment 12 patches (but please let me know if you have a more recent rebase yourself) and will now try to understand and debug them -- no promises of success.
Any further hints welcome!
Given that those urls have "/tip/" in them they should be pretty recent.

Note that the commit messages describe some known problems with the patches.
Update:

Got a fairly good-looking try [0].

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #14)
> Note that the commit messages describe some known problems with the patches.
None of those issues are present anymore. :-)

I silenced the issues I had with test_intersectionobservers.html [1] by preventing out-of-flow frames from being dynamic reflow roots [2], but I'm not certain this is the correct fix: if it was, wouldn't we have seen many more failures?
Unless there's a more obvious&correct solution, this current fix may still be fine, as in the worst case it just marks fewer frames as reflow roots.

I've also made contain-layout&size frames dynamic reflow roots [3]; no test failures, but I our coverage is still light. More testing needed.

And part 5 is not complete yet.


[0] https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=202825967&revision=1ec081b3646f82d15394fb03bb9babc7202c3c77
[1] https://treeherder.mozilla.org/testview.html?repo=try&revision=44285df0fffe800e802ee28961ec12c162757572
[2] https://hg.mozilla.org/try/rev/c8c745da0f28a6e423d1bf94d8f406cbc3515724#l1.72
[3] https://hg.mozilla.org/try/rev/c8c745da0f28a6e423d1bf94d8f406cbc3515724#l1.45
So the issue I was most concerned about was this one in part 4:
  > "Out-of-flow frame got reflowed before its placeholder", which seems like it might be a fundamental problem, since if we dynamically add an absolutely positioned element, with the placeholder and the primary frame separated by a dynamic reflow root, it's not clear what would make us reflow them in the correct order

If we have a placeholder and an out-of-flow frame separated by a dynamic reflow root, it basically means (I think) that one of two things is true:
 * the placeholder is inside the dynamic reflow root and the out-of-flow is outside it.  I don't think part 5 helps with this part at all -- in fact, it probably hurts.  Part 5 will make the outer reflow always happen first; this could make the problem worse since we avoid the problem when the inner reflow happens first and we thus reflow the placeholder first.  (We also avoid the problem if the outer reflow happens first but the inner reflow is triggered by the outer reflow rather than being separated, which is probably reasonably common.)
 * the out-of-flow *is* the dynamic reflow root in question.  In this case I think we can safely weaken the assertion (if it fires at all in this case), since if we're reflowing it *as* a reflow root, it shouldn't matter that we haven't reflowed its placeholder yet.  However, I think part 5 probably fully fixes this case, so we shouldn't need to worry about it.

(On the other hand, we've managed to avoid fixing incremental layout bugs like this for a while, such as bug 1200585, though we probably should fix them...)


I think preventing out-of-flow frames from being dynamic reflow roots seems like it's removing one of the bigger categories of things that might actually be dynamic reflow roots in the real world, so I'm not crazy about that idea.  Can you explain what the problem was that it fixed?

Finishing part 5 may make things a little simpler since it would give us slightly stronger invariants about how things work that make things less likely to be different from the current setup.


I'd also note that I'd rather *not* make contain: layout+size frames be dynamic reflow roots.  Under the correct conditions they should be able to be *regular* reflow roots since changes of contain cause frame reconstruction.  That's simpler, since we can assume they'll be reflow roots for the lifetime of the frame.
(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #16)
> [...]
> I think preventing out-of-flow frames from being dynamic reflow roots seems
> like it's removing one of the bigger categories of things that might
> actually be dynamic reflow roots in the real world, so I'm not crazy about
> that idea.  Can you explain what the problem was that it fixed?

See links in comment 15 for details. In summary, the assection there failed:
https://searchfox.org/mozilla-central/source/dom/base/test/test_intersectionobservers.html#751
An absolutely-positioned and fixed-size square that should be just outside the element-to-compare (intersectionRatio should be 0) appears to be fully inside (intersectionRatio is 1)!

Looking at the frame tree, and knowing which frames were being marked as dynamic reflow roots, all four OOF boxes were marked, but not their placeholder.
Interestingly the one involved in the failure is an iframe while others are plain divs. Changing it into a div did silence the failure; is there something special about iframes that could explain this?


> Finishing part 5 may make things a little simpler since it would give us
> slightly stronger invariants about how things work that make things less
> likely to be different from the current setup.

I'll give it a shot next.


> I'd also note that I'd rather *not* make contain: layout+size frames be
> dynamic reflow roots.  Under the correct conditions they should be able to
> be *regular* reflow roots since changes of contain cause frame
> reconstruction.  That's simpler, since we can assume they'll be reflow roots
> for the lifetime of the frame.

Thanks. I've split that change into its own bug 1497414, to keep this one here focused on dynamic reflow roots.
Update:

Try with part 5 (now part 4) implemented, but not using dynamic reflows yet:
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=204795719&revision=23d97d75b8a5722f686649784b46419b0b3bea3a

I went in a different direction from your original part 5, which I believe was going to order reflows based on frame depths:
After picking the last frame in the list, I then find its shallowest ancestor in the list, if any, and reflow that one instead (keeping the descendant in the list for later). I think this has the desired effect of always reflowing roots before their descendant roots, avoiding potential double reflows.


Try with added dynamic roots and contain:layout+size roots:
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=204795719&revision=c00848c750119262f6242456523ab81a9b5917d8

We're still hitting the intersectionObserver issue, where an out-of-flow iframe has an incorrect intersectionRatio:
https://treeherder.mozilla.org/logviewer.html#?job_id=204760401&repo=try&lineNumber=8714

To be continued...
I found the location of the intersectionObserver issue, and with help from David got a fix for it.
(Separate bug 1499961, as it should get fixed anyway, regardless of this bug here.)
As mDirtyRoots will be accessed through a more cohesive API, this patch hides
the storage details (nsTArray) -- but provides almost the same API for now.

Depends on D9488
Prevent duplicate roots, and only schedule layout if a new root was added.
When popping a dirty root, make sure to take one that's not a descendant of
another one in the list (so we reflow from outer frames first, to avoid
duplicate reflow of inner frames).

Depends on D9489
Attachment #9019275 - Attachment description: Bug 1159042 - p2. Allow reflow roots to have overflow, and allow that overflow to change during reflow - r?dbaron → Bug 1159042 - p2. Allow reflow roots to have overflow, and allow that overflow to change during reflow - r?dbaron,dholbert!
Attachment #9019277 - Attachment description: Bug 1159042 - p4. Make mDirtyRoots more efficient at managing related roots - r?dbaron → Bug 1159042 - p4. Make mDirtyRoots manage roots in preferred depth order - r?dbaron
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/845bc8316b05
p1. Replace rootFrame variable with isRoot boolean in PresShell::DoReflow - r=dbaron,dholbert
https://hg.mozilla.org/integration/autoland/rev/3a86c3e65d44
p2. Allow reflow roots to have overflow, and allow that overflow to change during reflow - r=dbaron,dholbert
https://hg.mozilla.org/integration/autoland/rev/6a5191742ca1
p3. Refactor mDirtyRoots type into a class - r=dbaron
https://hg.mozilla.org/integration/autoland/rev/080b9ceee8ec
p4. Make mDirtyRoots manage roots in preferred depth order - r=dbaron
https://hg.mozilla.org/integration/autoland/rev/db0e173a6ed2
p5. Add NS_FRAME_DYNAMIC_REFLOW_ROOT on frames that we can dynamically make reflow roots - r=dholbert
https://hg.mozilla.org/integration/autoland/rev/fe77c09dee31
p6. Use NS_FRAME_DYNAMIC_REFLOW_ROOT - r=dholbert
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5a15c64bd40
Backed out 6 changesets for crashtest failures. CLOSED TREE
Backed out 6 changesets (bug 1159042) for crashtest failures. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=214038669&repo=autoland&lineNumber=60612

INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323386-1.html
[task 2018-11-27T03:52:08.881Z] 03:52:08     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323386-1.html | 1977 / 3639 (54%)
[task 2018-11-27T03:52:08.920Z] 03:52:08     INFO - ++DOMWINDOW == 105 (0x7f8b48f93000) [pid = 1144] [serial = 6430] [outer = 0x7f8b81061800]
[task 2018-11-27T03:52:08.944Z] 03:52:08     INFO - Block(div)(-1)@7f8b49c98578: Init: bad caller: width WAS 1426530368(0x55072040)
[task 2018-11-27T03:52:08.946Z] 03:52:08     INFO - [Child 1144, Main Thread] WARNING: Scrolled rect smaller than scrollport?: file /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp, line 6233
[task 2018-11-27T03:52:08.947Z] 03:52:08     INFO - [Child 1144, Main Thread] WARNING: Scrolled rect smaller than scrollport?: file /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp, line 6233
[task 2018-11-27T03:52:08.949Z] 03:52:08     INFO - nsLineLayout: nsTextControlFrame@7f8b49c97b70 metrics=1426530608,2400!
[task 2018-11-27T03:52:08.950Z] 03:52:08     INFO - Block(div)(-1)@7f8b49c98578: Init: bad caller: width WAS 1426530368(0x55072040)
[task 2018-11-27T03:52:08.952Z] 03:52:08     INFO - [Child 1144, Main Thread] WARNING: Scrolled rect smaller than scrollport?: file /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp, line 6233
[task 2018-11-27T03:52:08.955Z] 03:52:08     INFO - [Child 1144, Main Thread] WARNING: Scrolled rect smaller than scrollport?: file /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp, line 6233
[task 2018-11-27T03:52:08.960Z] 03:52:08     INFO - nsLineLayout: nsTextControlFrame@7f8b49c97b70 metrics=1426530608,2400!
[task 2018-11-27T03:52:08.960Z] 03:52:08     INFO - [Child 1144, Main Thread] WARNING: Scrolled rect smaller than scrollport?: file /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp, line 6233
[task 2018-11-27T03:52:08.965Z] 03:52:08     INFO - [Child 1144, Main Thread] WARNING: Scrolled rect smaller than scrollport?: file /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp, line 6233
[task 2018-11-27T03:52:08.965Z] 03:52:08     INFO - [Child 1144, Main Thread] WARNING: Scrolled rect smaller than scrollport?: file /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp, line 6233
[task 2018-11-27T03:52:08.966Z] 03:52:08     INFO - [Child 1144, Main Thread] WARNING: Scrolled rect smaller than scrollport?: file /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp, line 6233
[task 2018-11-27T03:52:08.966Z] 03:52:08     INFO - [Child 1144, Main Thread] WARNING: Scrolled rect smaller than scrollport?: file /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp, line 6233
[task 2018-11-27T03:52:08.966Z] 03:52:08     INFO - [Child 1144, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /builds/worker/workspace/build/src/dom/base/nsContentUtils.cpp, line 4035
[task 2018-11-27T03:52:08.986Z] 03:52:08     INFO - [Child 1144, Main Thread] WARNING: Scrolled rect smaller than scrollport?: file /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp, line 6233
[task 2018-11-27T03:52:09.012Z] 03:52:09     INFO - REFTEST TEST-PASS | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323386-1.html | (LOAD ONLY)
[task 2018-11-27T03:52:09.014Z] 03:52:09     INFO - REFTEST TEST-END | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323386-1.html
[task 2018-11-27T03:52:09.022Z] 03:52:09     INFO - ++DOMWINDOW == 106 (0x7f8b499b5000) [pid = 1144] [serial = 6431] [outer = 0x7f8b81061800]
[task 2018-11-27T03:52:09.044Z] 03:52:09     INFO - REFTEST TEST-UNEXPECTED-PASS | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323386-1.html | assertion count 0 is less than expected 1 assertions
[task 2018-11-27T03:52:09.045Z] 03:52:09     INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323389-1.html
[task 2018-11-27T03:52:09.047Z] 03:52:09     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323389-1.html | 1978 / 3639 (54%)
[task 2018-11-27T03:52:09.065Z] 03:52:09     INFO - ++DOMWINDOW == 107 (0x7f8b49dcfc00) [pid = 1144] [serial = 6432] [outer = 0x7f8b81061800]
[task 2018-11-27T03:52:09.127Z] 03:52:09     INFO - [Child 1144, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /builds/worker/workspace/build/src/dom/base/nsContentUtils.cpp, line 4035
[task 2018-11-27T03:52:09.182Z] 03:52:09     INFO - REFTEST TEST-PASS | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323389-1.html | (LOAD ONLY)
[task 2018-11-27T03:52:09.184Z] 03:52:09     INFO - REFTEST TEST-END | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323389-1.html
[task 2018-11-27T03:52:09.206Z] 03:52:09     INFO - ++DOMWINDOW == 108 (0x7f8b7773cc00) [pid = 1144] [serial = 6433] [outer = 0x7f8b81061800]
[task 2018-11-27T03:52:09.236Z] 03:52:09     INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323389-2.html
[task 2018-11-27T03:52:09.238Z] 03:52:09     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323389-2.html | 1979 / 3639 (54%)
[task 2018-11-27T03:52:09.264Z] 03:52:09     INFO - ++DOMWINDOW == 109 (0x7f8b77741c00) [pid = 1144] [serial = 6434] [outer = 0x7f8b81061800]
[task 2018-11-27T03:52:09.310Z] 03:52:09     INFO - [Child 1144, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /builds/worker/workspace/build/src/dom/base/nsContentUtils.cpp, line 4035
[task 2018-11-27T03:52:09.323Z] 03:52:09     INFO - REFTEST TEST-PASS | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323389-2.html | (LOAD ONLY)
[task 2018-11-27T03:52:09.323Z] 03:52:09     INFO - REFTEST TEST-END | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323389-2.html
[task 2018-11-27T03:52:09.339Z] 03:52:09     INFO - ++DOMWINDOW == 110 (0x7f8b77765800) [pid = 1144] [serial = 6435] [outer = 0x7f8b81061800]
[task 2018-11-27T03:52:09.375Z] 03:52:09     INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323493-1.html
[task 2018-11-27T03:52:09.377Z] 03:52:09     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323493-1.html | 1980 / 3639 (54%)
[task 2018-11-27T03:52:09.507Z] 03:52:09     INFO - ++DOMWINDOW == 111 (0x7f8b7776d400) [pid = 1144] [serial = 6436] [outer = 0x7f8b81061800]
[task 2018-11-27T03:52:09.569Z] 03:52:09     INFO - [Child 1144, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /builds/worker/workspace/build/src/dom/base/nsContentUtils.cpp, line 4035
[task 2018-11-27T03:52:09.592Z] 03:52:09     INFO - REFTEST TEST-PASS | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323493-1.html | (LOAD ONLY)
[task 2018-11-27T03:52:09.592Z] 03:52:09     INFO - REFTEST TEST-END | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323493-1.html
[task 2018-11-27T03:52:09.600Z] 03:52:09     INFO - ++DOMWINDOW == 112 (0x7f8b7776f800) [pid = 1144] [serial = 6437] [outer = 0x7f8b81061800]
[task 2018-11-27T03:52:09.639Z] 03:52:09     INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323495-1.html
[task 2018-11-27T03:52:09.641Z] 03:52:09     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323495-1.html | 1981 / 3639 (54%)
[task 2018-11-27T03:52:09.658Z] 03:52:09     INFO - ++DOMWINDOW == 113 (0x7f8b782acc00) [pid = 1144] [serial = 6438] [outer = 0x7f8b81061800]
[task 2018-11-27T03:52:09.709Z] 03:52:09     INFO - [Child 1144, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /builds/worker/workspace/build/src/dom/base/nsContentUtils.cpp, line 4035
[task 2018-11-27T03:52:09.740Z] 03:52:09     INFO - REFTEST TEST-PASS | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323495-1.html | (LOAD ONLY)
[task 2018-11-27T03:52:09.744Z] 03:52:09     INFO - REFTEST TEST-END | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323495-1.html
[task 2018-11-27T03:52:09.764Z] 03:52:09     INFO - ++DOMWINDOW == 114 (0x7f8b48f89000) [pid = 1144] [serial = 6439] [outer = 0x7f8b81061800]
[task 2018-11-27T03:52:09.793Z] 03:52:09     INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/324318-1.html
[task 2018-11-27T03:52:09.794Z] 03:52:09     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/324318-1.html | 1982 / 3639 (54%)
[task 2018-11-27T03:52:09.814Z] 03:52:09     INFO - ++DOMWINDOW == 115 (0x7f8b499b3000) [pid = 1144] [serial = 6440] [outer = 0x7f8b81061800]
[task 2018-11-27T03:52:09.837Z] 03:52:09     INFO - ++DOCSHELL 0x7f8b77609000 == 23 [pid = 1144] [id = {524a3722-f3fd-43f2-91ed-703478c2a93d}]
[task 2018-11-27T03:52:09.839Z] 03:52:09     INFO - ++DOMWINDOW == 116 (0x7f8b782ae000) [pid = 1144] [serial = 6441] [outer = (nil)]
[task 2018-11-27T03:52:09.839Z] 03:52:09     INFO - ++DOCSHELL 0x7f8b7760d800 == 24 [pid = 1144] [id = {7736cbe4-4db7-408e-ab97-2c6bfd53991e}]
[task 2018-11-27T03:52:09.841Z] 03:52:09     INFO - ++DOMWINDOW == 117 (0x7f8b782af400) [pid = 1144] [serial = 6442] [outer = (nil)]

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fe77c09dee3107e675162b760a6efa9b5f0bcce3

Backout:
https://hg.mozilla.org/integration/autoland/rev/d5a15c64bd401768c581663b1b13f45e66080616
Flags: needinfo?(gsquelart)
Comment 28 was a lot of text, but I think the relevant log-quote was this one line:
>  REFTEST TEST-UNEXPECTED-PASS | file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/crashtests/323386-1.html | assertion count 0 is less than expected 1 assertions

That unexpected-pass seems to have been reported on all linux crashtest runs for the push.

Looks like we can just remove the corresponding "asserts" annotation entirely from that test, as part of this patch stack. The annotation is:
> asserts-if(gtkWidget,1) asserts-if(Android&&asyncPan,1) load 323386-1.html # Bug 718883
https://dxr.mozilla.org/mozilla-central/source/layout/generic/crashtests/crashtests.list#28

...and we just need to remove that first gtkWidget-specific annotation, probably in the final patch here (the one that makes us start using dynamic reflow roots), because that's probably where the assertion stops happening (due to the change in reflow depth/behavior).
(And while you're at editing that final patch, there's a reviewbot suggestion that'd be nice to address, too -- just replacing _f with (_f) in the body of the macro that you're editing there.  See https://phabricator.services.mozilla.com/D9492#319261 )
(and in case you haven't used phabricator post-backout before, there's a note about how to reopen revisions to let you update them here: https://wiki.mozilla.org/Phabricator/FAQ#Phabricator )
Depends on: 1510369
I think it was just unlucky timing, and there's a windows update dialog that's likely stealing focus and causing the problem here. bug 1509787 seems to be tracking the failure, and I've posted more info in bug 1509787 comment 3.

Also: assuming the test-failure has persisted for a few days, then it's impossible for this bug to have been involved, because this bug's patches were backed out after a few hours (see comment 27).
(In reply to Dorel Luca [:dluca] from comment #28)
> Backed out 6 changesets (bug 1159042) for crashtest failures. CLOSED TREE
> ...
> [task 2018-11-27T03:52:09.044Z] 03:52:09     INFO - REFTEST
> TEST-UNEXPECTED-PASS |
> file:///builds/worker/workspace/build/tests/reftest/tests/layout/generic/
> crashtests/323386-1.html | assertion count 0 is less than expected 1
> assertions

Thank you Dorel.
Fixed in next push.
Flags: needinfo?(gsquelart)
(Note: I suspect this is ready (or nearly-ready) to re-land -- but given that it's "soft code freeze" day, and this patch stack has nonzero risk, I guess we should hold off on landing this until after the branch, on Dec 10th.)
It looks like this needs an unbitrotting before it does land, though.

(I just tried & failed to apply the patch-stack locally, with "arc patch D9492" -- I hit conflicts in PresShell.cpp for one of the patches, presumably due to the recent full-tree whitespace reformatting.)
Flags: needinfo?(gsquelart)
Good news!  "hg rebase" is able to rebase these patches, with no user interaction required (no merge conflicts).

I diffed the old vs. rebased patches, and the changes all seem to be correct/expected reformatting differences (in both the removed & added code, and in contextual code).

I tried to "moz-phab submit" the rebased patches, but moz-phab doesn't seem to want to allow me to do that unless I "commandeer" them.

So: Gerald, would you mind rebasing & resubmitting via moz-phab?  Hopefully should only take a few min, given the lack of merge conflicts (woohoo tooling!)
Try run with the automatically-rebased patches looks good, too:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a5dd12a78d3a97ec549caeb6c0152054ce7c532
(this is "-b do -p linux,macosx64 -u crashtest,mochitests,reftest,web-platform-tests")
Thanks Daniel.
The rebase automagically worked for me as well. I'll now do a quick sanity build & try, then resubmit and push soon... (And bug 1497414 after that.)
Flags: needinfo?(gsquelart)
Oh you already did a Try run; I'll skip it then.
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f27e36655777
p1. Replace rootFrame variable with isRoot boolean in PresShell::DoReflow - r=dbaron,dholbert
https://hg.mozilla.org/integration/autoland/rev/29d8ae277caa
p2. Allow reflow roots to have overflow, and allow that overflow to change during reflow - r=dbaron,dholbert
https://hg.mozilla.org/integration/autoland/rev/e5b84b579c54
p3. Refactor mDirtyRoots type into a class - r=dbaron
https://hg.mozilla.org/integration/autoland/rev/6c1abb10067f
p4. Make mDirtyRoots manage roots in preferred depth order - r=dbaron
https://hg.mozilla.org/integration/autoland/rev/60524976c8f6
p5. Add NS_FRAME_DYNAMIC_REFLOW_ROOT on frames that we can dynamically make reflow roots - r=dholbert
https://hg.mozilla.org/integration/autoland/rev/c07fb8a8d472
p6. Use NS_FRAME_DYNAMIC_REFLOW_ROOT - r=dholbert
Depends on: 1517067
Regressions: 1547305

Note: we ended up disabling this in Firefox 66 due to some known regressions (in bug 1527120), and I filed bug 1547852 on making that disabling persistent (in post-early-beta builds) until we're more confident that this is safe to ship.

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

Attachment

General

Creator:
Created:
Updated:
Size: