Jank from frame reconstruction & layout, when tweaking "overflow" on root of deeply-nested frame tree

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout
RESOLVED FIXED
3 months ago
4 hours ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 affected, firefox55 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

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

Attachments

(6 attachments, 1 obsolete attachment)

Created attachment 8843501 [details]
testcase 1

STR:
 1. Load attached testcase (and then wait for alert to appear)

EXPECTED RESULTS:
Small duration reported, comparable to other browsers.

ACTUAL RESULTS:
Large duration reported (over 1 second)
(This is very similar to bug 1344377 BTW, except for 'overflow' instead of 'position')
See Also: → bug 1344377
Created attachment 8843503 [details]
testcase 2 (with an actual scrollbar)

Here's another testcase similar to testcase 1, but with an actual scrollbar, so that the (temporary) 'overflow' change has a non-trivial effect on layout (at least on platforms where scrollbars take up space).
testcase 1   testcase 2   Browser Version
==========   ==========   ================
 2100ms      2500ms       Firefox Nightly 54 (2017-03-03)
 250ms       2800ms       Edge 14
 1ms         600ms        Chrome 58


So in cases where scrollbars aren't going from showing <--> visible (testcase 1), other browsers clearly beat us.

In cases where scrollbars *are* going from showing <--> visible, it's a bit more even (at least between us & Edge -- and while Chrome is faster, they still take hundreds of ms).

So to a first approximation, we might want to focus on optimizing the first case here and care less about the second.
This is in the service of bug 1342220 -- and on that bug, we're actually just going between:
  overflow-y: scroll;
  overflow-x: auto;
...and:
  overflow-x: hidden;
  overflow-y: hidden;

Both of those involve a scroll frame wrapper (at least for blocks[1]), so there shouldn't really be any reason to do frame reconstruction[2].

For now, let's just consider that restricted case in this bug (going between overflow:hidden & overflow:scroll/auto), and we can leave changes to/from "overflow:visible" as triggering frame reconstruction, since handling that gracefully would require more intricate surgery I think.

[1] dbaron says there might be a few frame types (maybe table-cells) where we *don't* create a scrollframe for 'overflow:hidden'; need some investigation to follow up on that.
[2] Maybe we do need frame construction/destruction for the scrollbars themselves (which are present/potentially-present in overflow:scroll/auto, vs. absent in overflow:hidden) -- but that shouldn't require reconstructing the whole subtree.
Created attachment 8843521 [details]
testcase 3 (with initial "overflow-y: scroll" on body to better match Twitter)
Approximate times for testcase 3 (back-of-the-envelope averaged from a few runs, on Win10):

testcase 3   Browser Version
==========   ================
 3000ms      Firefox Nightly 54 (2017-03-03)
 3000ms      Edge 14
 500ms       Chrome 58

Updated

3 months ago
Blocks: 1344531
(In reply to Daniel Holbert [:dholbert] from comment #4)
> This is in the service of bug 1342220 -- and on that bug, we're actually
> just going between:
>   overflow-y: scroll;
[...]
>   overflow-y: hidden;
> 
> Both of those involve a scroll frame wrapper (at least for blocks[1]), so
> there shouldn't really be any reason to do frame reconstruction[2].
> 
> For now, let's just consider that restricted case in this bug

bug 1344157 is a version of this same issue, on Wikipedia -- but there, the <body> is going from "overflow:visible" (the default) to "overflow-y:auto".  On most elements, this change **would require** some frametree surgery (or reconstruction) to add/remove a scrollframe -- BUT on the <body> I believe it doesn't technically need that, since the body just gets to use the root viewport's scrollframe, IIRC.  So we might want to add some special cases to make this fast even for visible <--> non-visible changes, for <body> & <html> nodes.)
I wonder if all these numbers get somewhat better with dbaron's patch in bug 1308876?

Mainly, when we have overflow: auto, I think we reflow the scrolled frame up to four times? If the frame is dirty, then right now we'd be doing a dirty reflow of all the frame tree four times instead of once
(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> I wonder if all these numbers get somewhat better with dbaron's patch in bug
> 1308876? [...] four times instead of once

Maybe! But even if we optimized this to reflow once (rather than four times) and that dropped us to taking 1/4 of the time we currently take, that'd still leave us at ~500ms for testcase 1, which is still 2x as long as Edge and 500x as long as Chrome. (per numbers for "testcase 1" in comment 3)

So, we do really need to avoid frame reconstruction in order to be competitive here, at least in that case (testcase 1).
Whiteboard: [qf:p1]

Updated

2 months ago
Assignee: nobody → bugs
dholbert has work in progress patches for this.
Assignee: bugs → dholbert
> [1] dbaron says there might be a few frame types (maybe table-cells) where we *don't* create a scrollframe for 'overflow:hidden'; need some investigation to follow up on that.

That's correct, but note that we don't support overflow:scroll on those either, iirc.
Created attachment 8860199 [details] [diff] [review]
wip v1

Here's my current WIP patch.

For simplicity, I'm *only* targeting <body>-or-<html>-that-is-using-the-root-scrollport, since:
 - that lets our optimization work for changes to/from "overflow:visible" too (since we always have the root scrollframe even when we're overflow:visible)
 - a general solution would need to handle body/html as a special case anyway (since they sometimes propagate their scrollbars to the viewport)
 - body/html are the only place where dynamic "overflow" tweaks cause actual known pain out there on the web (at Twitter and Wikipedia).

(So we *could* try to gracefully handle "overflow" changes on arbitrary nodes, too, as I was suggesting in comment 4 -- but it's not clear there's much to be gained from that in the real world.)

This is nearly ready for review, I think - just needs some wordsmithing, per a few XXXdholbert comments, and I want to add some reftests for scenarios where we dynamically end up with overflow:scroll on *both* <body> and <html>, to be sure we do the right thing (and paint both scrollbars) in that case.

Numbers-wise: I tried this on a version of testcase 3 locally (with the testcase's depth 16 reduced to 14 so it doesn't explode my debug build). It shaves off 2/3 of the time consumed by that version of the testcase (my debug build goes from reporting ~9 seconds to reporting ~3 seconds).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8860199 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Just pushed a "part 3" patch with some reftests to test our handling of dynamic changes to "overflow" on <body> and <html> (in the presence/absence of similar styles/changes on the other of those two nodes).

These tests should pass before *and* after this patch -- they're just "breaktests" to be sure we build/destroy scrollframes on these elements appropriately, even after this change.
Results from Testcase 3, using a locally-compiled Linux enable-opt disable-debug build on my laptop...
...without patch-stack:  1330ms
...with patch-stack:      460ms

I ran the benchmark 3 times with each build, and I'm reporting the rough average (all 3 samples were +/- 20ms from my reported value).

Conclusions:
 - We're a good bit faster on Linux than on Windows (at least the Win10 build I used in comment 6).
 - The patches here seem to give us a ~65% improvement in the metric reported by this testcase (the time to flush changes after tweaking "overflow" on the body of a deeply-nested DOM).

Comment 18

28 days ago
mozreview-review
Comment on attachment 8861741 [details]
Bug 1344398 part 3: Add reftests for dynamic changes to "overflow" on html & body elements.

https://reviewboard.mozilla.org/r/133724/#review137166
Attachment #8861741 - Flags: review?(tnikkel) → review+

Comment 19

27 days ago
mozreview-review
Comment on attachment 8861655 [details]
Bug 1344398 part 1: Move presContext variable a little earlier in a nsCSSFrameConstructor method.

https://reviewboard.mozilla.org/r/133600/#review137164

::: layout/base/nsCSSFrameConstructor.cpp:8519
(Diff revision 1)
> -    mPresShell->GetPresContext()->UpdateViewportScrollbarStylesOverride();
> +    presContext->UpdateViewportScrollbarStylesOverride();
>    }
>  
>    // XXXldb Do we need to re-resolve style to handle the CSS2 + combinator and
>    // the :empty pseudo-class?
>  

Hmm, there is a ContentRemoved (that we don't always return right after) call between when we get the prescontext and when we use it now. Are we certain that call can't kill the prescontext?

Comment 20

27 days ago
mozreview-review
Comment on attachment 8861656 [details]
Bug 1344398 part 2: React to some CSS 'overflow' changes on body/html by simply calling UpdateViewportScrollbarStylesOverride() instead of reframing.

https://reviewboard.mozilla.org/r/133602/#review137208

::: layout/base/nsPresContext.h:1402
(Diff revision 1)
>    nscolor               mBodyTextColor;
>  
> +  // This is a non-owning pointer. May be null.  If non-null, it's guaranteed
> +  // to be pointing to a node that's still alive, because
> +  // nsCSSFrameConstructor::ContentRemoved() will update this pointer if & when
> +  // that node is removed from the document.

Is that true? What if we remove the parent of this element?
Comment on attachment 8861655 [details]
Bug 1344398 part 1: Move presContext variable a little earlier in a nsCSSFrameConstructor method.

https://reviewboard.mozilla.org/r/133600/#review137164

> Hmm, there is a ContentRemoved (that we don't always return right after) call between when we get the prescontext and when we use it now. Are we certain that call can't kill the prescontext?

I'm pretty sure, yeah.

The PresShell owns this nsCSSFrameConstructor (whose method we're inside) and it co-owns the PresContext that we're getting here [in a RefPtr which it sets inside of PresShell::Init, and which it never clears AFAICT].

So: as long as the PresShell is alive (which it is, assuming the nsCSSFrameConstructor "this"-pointer is alive in this code) then the PresShell's GetPresContext() method should return the same value throughout this method (and for its whole post-Init lifetime, really).

Comment 22

27 days ago
mozreview-review
Comment on attachment 8861655 [details]
Bug 1344398 part 1: Move presContext variable a little earlier in a nsCSSFrameConstructor method.

https://reviewboard.mozilla.org/r/133600/#review137592
Attachment #8861655 - Flags: review?(tnikkel) → review+
Comment on attachment 8861656 [details]
Bug 1344398 part 2: React to some CSS 'overflow' changes on body/html by simply calling UpdateViewportScrollbarStylesOverride() instead of reframing.

https://reviewboard.mozilla.org/r/133602/#review137208

> Is that true? What if we remove the parent of this element?

Ah, you're right -- this is not true after all! I'd thought we called nsCSSFrameConstructor::ContentRemoved() recursively (it superficially looks like it, but now I'm seeing that's just for display:contents-specific section).

I tried viewing a testcase with <body style="overflow:scroll">, and doing document.documentElement.remove(), and indeed, we only got a single call to ContentRemoved (for <html>) and we never trigger a UpdateViewportScrollbarStylesOverride call.

I'll take a closer look at what more cleanup might be worth doing here...
ContentRemoved() for <html> should call UpdateViewportScrollbarStylesOverride, no?  The code in ContentRemoved does:

  if (aChild->IsHTMLElement(nsGkAtoms::body) ||
      (!aContainer && aChild->IsElement())) {
    // This might be the element we propagated viewport scrollbar
    // styles from.  Recompute those.
    mPresShell->GetPresContext()->UpdateViewportScrollbarStylesOverride();
  }

In the <html> case !aContainer should be true and hence we should call UpdateViewportScrollbarStylesOverride().

I mean, this _has_ to work like that, because if the element which might have been propagated to viewport is removed, however it's removed, we need to call UpdateViewportScrollbarStylesOverride().

That said, the actual change to that condition in ContentRemoved in the patch is wrong.  The condition needs to stay as it was.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #24)
> ContentRemoved() for <html> should call
> UpdateViewportScrollbarStylesOverride, no?  The code in ContentRemoved does:
[..] 
> In the <html> case !aContainer should be true and hence we should call
> UpdateViewportScrollbarStylesOverride().

In comment 23, I was using a build with my patch applied which has the !aContainer condition removed (because I mistakenly thought ContentRemoved would get called for each element) 

> I mean, this _has_ to work like that, because if the element which might
> have been propagated to viewport is removed, however it's removed, we need
> to call UpdateViewportScrollbarStylesOverride().

Agreed.  And that makes me think the current condition around the call (explicitly checking for ::body and !aContainer) is perhaps a bit fragile?  For example, it doesn't get triggered if we have a fullscreen element that's providing the scrollbars, and that fullscreen element is part of a subtree that gets removed.

(Happily, we're saved in that condition because Element::UnbindFromTree DOES get called for each removed element (including the fullscreen one), and that calls ExitFullscreenInDocTree which ultimately calls UpdateViewportScrollbarStylesOverride, after drilling down through several layers of other fullscreen-cleanup-methods).

Now that we'll have a cheap way to check "am I providing the scrollbars as far as nsPresContext is aware", maybe we should move this UpdateViewportScrollbarStylesOverride call over to Element::UnbindFromTree? (perhaps as a followup)

> That said, the actual change to that condition in ContentRemoved in the
> patch is wrong.  The condition needs to stay as it was.

Yeah, I didn't understand the significance of the !aContainer check.  I'll restore it back to the way it was.
> For example, it doesn't get triggered if we have a fullscreen element that's providing
> the scrollbars, and that fullscreen element is part of a subtree that gets removed.

Ah, fullscreen elements propagate their scrollbar state?  Fun.

> maybe we should move this UpdateViewportScrollbarStylesOverride call over to Element::UnbindFromTree? 

Worth thinking about, at least.  Definitely a followup.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Never version of part 2 posted. Changes:
 (a) I've reverted the ContentRemoved() condition tweak that bz pointed out was wrong, and instead I've now added a clearer code-comment there, to explain what that tweak does.
 (b) I've clarified the comment that tnikkel asked about in comment 20, to explain the other place where we might clean up the pointer.
 (c) I've added a check to Element::UnbindFromTree(), to verify that the element we're unbinding isn't referenced by a dangling pointer in this new nsPresContext scrollbars-provider member-var.  (Importantly, this check happens *after* we've done the fullscreen cleanup stuff in that function.  So we will have already called UpdateViewportScrollbarStylesOverride to accomodate for ourselves being removed if we're fullscreen & the scrollbar-provider.)
Comment on attachment 8861656 [details]
Bug 1344398 part 2: React to some CSS 'overflow' changes on body/html by simply calling UpdateViewportScrollbarStylesOverride() instead of reframing.

bz, I'd be interested in your thoughts on this patch as well. (tnikkel, feel free to punt entirely to bz if you'd like, too -- though I'm also just as glad to get feedback from each of you on this.)
Attachment #8861656 - Flags: feedback?(bzbarsky)
Hmm, it looks like my new assert (part "c") fails on some XUL reftests!  (dom/xul/crashtests/386914-1.html at least)

In that case, scrollbars are propagated from a <window> element, and we get a call to nsCSSFrameConstructor::ContentRemoved for that element (with aContainer = null), and so we correctly call UpdateViewportScrollbarStylesOverride, BUT that still leaves us with the same result -- with the element that we're in the process of removing.

I don't believe this happens for similar cases with <html> being removed.  Maybe XUL document root nodes are stickier or something...
(In reply to Daniel Holbert [:dholbert] (AFK May 3-7, 13-21) from comment #32)
> I don't believe this happens for similar cases with <html> being removed. 
> Maybe XUL document root nodes are stickier or something...

Nope, I was wrong here -- this assert fires for HTML document.documentElement removals as well.

This happens for a sneaky reason -- our call to UpdateViewportScrollbarStylesOverride actually *finds and returns the HTML node that's being returned (even in our current tree, I think), even though it's already been removed from its parent (the document)'s child list!  It gets discovered because the document is still tracking it as nsIDocument::mCachedRootElement -- that cached pointer doesn't get updated until the very end of nsDocument::RemoveChildAt, *after* we've invoked nsCSSFrameConstructor::ContentRemoved and Element::UnbindFromTree.

I think we should probably clear mCachedRootElement sooner -- and I think that'll fix these assertions discussed in comment 32. I'll file a helper bug on that, and resolve that as a prerequisite to this bug.
Depends on: 1361268
I filed bug 1361268 on the issue discussed in comment 32 - 33. If my patch there pans out, then the latest review request here should still be good.  Try run with that bug's patch & this bug's patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d36fcd18e2c76d943585d9c9b44aadd83528444b
Comment on attachment 8861656 [details]
Bug 1344398 part 2: React to some CSS 'overflow' changes on body/html by simply calling UpdateViewportScrollbarStylesOverride() instead of reframing.

https://reviewboard.mozilla.org/r/133602/#review138620

r=me

::: dom/base/Element.cpp:1806
(Diff revision 2)
>      }
>      SetParentIsContent(false);
>    }
>  
> +#ifdef DEBUG
> +  // If we can get access to the PresContext and we are of the right node type

This is Element::UnbindFromTree.  It's going to be an nsIContent by definition.

::: dom/base/Element.cpp:1810
(Diff revision 2)
> +#ifdef DEBUG
> +  // If we can get access to the PresContext and we are of the right node type
> +  // (nsIContent), then we sanity-check that we're not leaving behind a pointer
> +  // to ourselves as the PresContext's cached provider of the viewport's
> +  // scrollbar styles.
> +  if (document && IsContent()) {

So no need for the IsContent() check here.

::: dom/base/Element.cpp:1815
(Diff revision 2)
> +  if (document && IsContent()) {
> +    nsIPresShell* presShell = document->GetShell();
> +    if (presShell) {
> +      nsPresContext* presContext = presShell->GetPresContext();
> +      if (presContext) {
> +        MOZ_ASSERT(this->AsContent() !=

Just nix the AsContent() bit.  Compare `this` directly to the thing from the prescontext.

::: layout/base/RestyleManager.cpp:1357
(Diff revision 2)
> +  for (nsStyleChangeData& data : aChangeList) {
> +    if (data.mHint & nsChangeHint_CSSOverflowChange) {
> +      data.mHint &= ~nsChangeHint_CSSOverflowChange;
> +      bool doReconstruct = true; // assume the worst
> +
> +      // Only bother with this if we're html/body, since:

The real thing I'd like to see this comment explain is why limit to just html/body.  And the answer is presumably that we don't want to call UpdateViewportScrollbarStylesOverride() in every case when overflow changes, right?

::: layout/base/RestyleManager.cpp:1361
(Diff revision 2)
> +
> +      // Only bother with this if we're html/body, since:
> +      // - it's expensive to reframe their subtree (the whole frametree).
> +      // - their "overflow" styles are often propagated to the viewport, so
> +      //   they may not need us to create/destroy a scrollframe anyway.
> +      if (data.mContent->IsHTMLElement(nsGkAtoms::body) ||

data.mContent->IsAnyOfHTMLElements(nsGkAtoms::body, nsGkAtoms::html)
Attachment #8861656 - Flags: review+
Attachment #8861656 - Flags: feedback?(bzbarsky) → feedback+

Comment 36

22 days ago
mozreview-review
Comment on attachment 8861656 [details]
Bug 1344398 part 2: React to some CSS 'overflow' changes on body/html by simply calling UpdateViewportScrollbarStylesOverride() instead of reframing.

https://reviewboard.mozilla.org/r/133602/#review138664

::: layout/base/RestyleManager.cpp:1357
(Diff revision 2)
> +  for (nsStyleChangeData& data : aChangeList) {
> +    if (data.mHint & nsChangeHint_CSSOverflowChange) {
> +      data.mHint &= ~nsChangeHint_CSSOverflowChange;
> +      bool doReconstruct = true; // assume the worst
> +
> +      // Only bother with this if we're html/body, since:

It only works for root scrollframes because they always construct scrollbar frames. So to make it work for non-root scrollframes we'd need to either always make them construct scrollbar frames (leading to many useless scrollbar frames) or handle constructing them dynamically without re-creating the scrollframe.
Attachment #8861656 - Flags: review?(tnikkel) → review+
Comment on attachment 8861656 [details]
Bug 1344398 part 2: React to some CSS 'overflow' changes on body/html by simply calling UpdateViewportScrollbarStylesOverride() instead of reframing.

https://reviewboard.mozilla.org/r/133602/#review138620

> This is Element::UnbindFromTree.  It's going to be an nsIContent by definition.

Thanks. I'm fuzzy on the Element/nsIContent distinction, but I'll take your word for it & clean up this chunk.

> The real thing I'd like to see this comment explain is why limit to just html/body.  And the answer is presumably that we don't want to call UpdateViewportScrollbarStylesOverride() in every case when overflow changes, right?

tn's response kind of covered that, yeah.

These lines of code were meant to explain why these nodes were special in allowing this simple optimization, but they didn't do a very good job:
      // - their "overflow" styles are often propagated to the viewport, so
      //   they may not need us to create/destroy a scrollframe anyway.

I'm replacing this whole comment with the following clearer explanation:
      // Only bother with this if we're html/body, since:
      //  (a) It'd be *expensive* to reframe these particular nodes.  They're
      //      at the root, so reframing would mean rebuilding the world.
      //  (b) It's often *unnecessary* to reframe for "overflow" changes on
      //      these particular nodes.  In general, the only reason we reframe
      //      for "overflow" changes is so we can construct (or destroy) a
      //      scrollframe & scrollbars -- and the html/body nodes often don't
      //      need their own scrollframe/scrollbars because they coopt the ones
      //      on the viewport (which always exist). So depending on whether
      //      that's happening, we can skip the reframe for these nodes.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
> I'm fuzzy on the Element/nsIContent distinction

Element is a subclass of nsIContent.  All Elements instances are also nsIContent.

There are also non-element nsIContent things: textnodes, doctypes, document fragments, comments, processing instructions.

> I'm replacing this whole comment with the following clearer explanation:

Thanks, that's much clearer.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #41)
> > I'm fuzzy on the Element/nsIContent distinction
> 
> Element is a subclass of nsIContent.  All Elements instances are also
> nsIContent.

Ah, right. I thought I remembered that being the case, but I got confused by an IsContent()/AsContent() call that I saw somewhere else (which may've been on a non-Element; I don't recall).

Thanks!

I'll land this later today. Happily, the new reftests pass on Stylo now (they didn't last week), so I've pushed one final revision without the fails-if(stylo) annotations and I'm running that through Try.

Try run before I made that change (Rs13 oranges are expected):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ccf4e889b9a
Try run after I made that change, stylo-only (no oranges are expected):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=398ec1597de6

Comment 46

14 days ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/647d0bb3714d
part 1: Move presContext variable a little earlier in a nsCSSFrameConstructor method. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/d70f9de401d1
part 2: React to some CSS 'overflow' changes on body/html by simply calling UpdateViewportScrollbarStylesOverride() instead of reframing. r=bz,tnikkel
https://hg.mozilla.org/integration/autoland/rev/d0e5a5ba01b5
part 3: Add reftests for dynamic changes to "overflow" on html & body elements. r=tnikkel
Backed out for debug crashtest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=98190321&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/750e808c6331
Flags: needinfo?(dholbert)
Yup, sorry & thanks for the backout -- clearly I forgot to run crashtests with all the patches after addressing the issue uncovered in comment 32.

Comment 47's crashtest failure is another instance of something like bug 1361268 -- mCachedRootElement being cleared too late in a different codepath of our existing code, and this makes GetRootElement() return something that we've just removed from the tree.

I've got a patch running through Try now, with this bug's patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e20dca9d2756d9db24ba74269555335b7b15234
Flags: needinfo?(dholbert)
Depends on: 1364180
That try run looks good! I spun off bug 1364180 for that crashtest fix, and I added one additional assertion on top of what I was testing in comment 48's Try run, and I triggered another Try run as noted on bug 1364180.

Assuming that's successful and bug 1364180 gets r+, then this can re-land.

Comment 50

13 days ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a6907fad992
part 1: Move presContext variable a little earlier in a nsCSSFrameConstructor method. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/276e3d5e9085
part 2: React to some CSS 'overflow' changes on body/html by simply calling UpdateViewportScrollbarStylesOverride() instead of reframing. r=bz,tnikkel
https://hg.mozilla.org/integration/autoland/rev/621a828de744
part 3: Add reftests for dynamic changes to "overflow" on html & body elements. r=tnikkel

Comment 51

13 days ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 67cbf91fb2fd -d 621a828de744: rebasing 395364:67cbf91fb2fd "Bug 1344398 part 1: Move presContext variable a little earlier in a nsCSSFrameConstructor method. r=tnikkel"
merging layout/base/nsCSSFrameConstructor.cpp
warning: conflicts while merging layout/base/nsCSSFrameConstructor.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
It looks like this landed (based on TreeHerder), so I'm hoping comment 51 was from the autoland robot attempting to land this twice, or something like that.  (Maybe due to human error on my part, not sure)

Comment 53

13 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0a6907fad992
https://hg.mozilla.org/mozilla-central/rev/276e3d5e9085
https://hg.mozilla.org/mozilla-central/rev/621a828de744
Status: NEW → RESOLVED
Last Resolved: 13 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1367568
You need to log in before you can comment on or make changes to this bug.