bug 1110277 patch 6 adds a FIXME in RebuildAllStyleData, noting that the initial bug isn't completely fixed in the RebuildAllStyleData case, because the scope of the ReframingStyleContexts is wrong. To fix this, we should refactor things so that RebuildAllStyleData uses the standard restyle processing rather than rolling its own; it's been a bit of a pain for quite a while since there are always two places to fix.

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e4fafe0b6ce0 https://tbpl.mozilla.org/?tree=Try&rev=e4fafe0b6ce0 https://hg.mozilla.org/try/pushloghtml?changeset=e4fafe0b6ce0

With one more null-check, a combined try run for this and bug 1110277: https://hg.mozilla.org/try/pushloghtml?changeset=e2a4013cb469 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e2a4013cb469 https://tbpl.mozilla.org/?tree=Try&rev=e2a4013cb469

The patches in this series refactor the process of rebuilding all style data (RestyleManager::RebuildAllStyleData and RestyleManager::DoRebuildAllStyleData) so that the process of rebuilding all style data uses the existing restyle processing loops in ProcessPendingRestyles. (Rebuilding all style data is what we do when we need to throw away the rule tree because something has invalidated the cached data in it.) This removes (increasing, especially with bug 960465 coming) code duplicated between the two codepaths, fixes some omissions from the separate rebuild-all codepath, and (more immediately) allows fixing lifetime issues of ReframingStyleContexts objects in bug 1110277 so that we can have a single ReframingStyleContexts for all of the restyle processing in each restyle processing operation. In other words, the goal is to change the rebuild-all process from a separate codepath to a few variables that modify the way ProcessPendingRestyles works (and make it do the extra work). This is just a small first step in that process, which moves one piece of code from a chunk of duplicated and to-be-removed code into a chunk of code that will be preserved.

Part of this refactoring involves the ability to start the rebuild-all process within the processing of restyles. This means we can't pass parameters directly from RebuildAllStyleData into DoRebuildAllStyleData. So this continues storing the hints as member variables a little bit deeper into the process. (I tried to move in a different direction in this patch queue, and store these hints in mPendingRestyles, for the root element. But that broke layout/style/test/test_counter_style.html and layout/style/test/test_font_loading_api.html, and I didn't want to figure out why. It would be somewhat better in the long run, since currently these hints will get processed if we do a rebuild-all on a RestyleTracker other than mPendingRestyles, which can happen if we have 'rem' units and have a root element font size change in the animation-only update or in mPendingAnimationRestyles.)

This is the variable that says we *need to* rebuild style data. Since the next patch will introduce a variable that says we're *currently* rebuilding all style data, renaming this one makes things clearer.

This adds a member variable that is currently only used within a single function, but that function will be split apart so that different parts of it can be called from different places within ProcessPendingRestyles.

This is needed for the following patch, so that it can access a member variable of RestyleManager.

This is needed for patch 9 (once patch 9 is used via the ProcessPendingRestyles() codepath); it ensures that when we use the new way of rebuilding, we don't bail out early because we think we have nothing to do.

This fixes one of the omissions in the rebuild-all codepaths (where it incorrectly differs from the regular ProcessPendingRestyles codepath). Note that the explicit FlushOverflowChangedTracker() is no longer needed because that's part of EndProcessingRestyles. (This will all get refactored more substantially in the following patches.)

This moves the code that finishes the rebuild-all process into EndProcessingRestyles(), which is part of the main restyling codepath. Patch 7 ensures that we'll always get to EndProcessingRestyles in this case, when we're going through the normal ProcessPendingRestyles() codepath rather than the special DoRebuildAllStyleData() codepath (which will be removed later in this patch series).

This is needed in patch 11.

Here we call StartRebuildAllStyleData from BeginProcessingRestyles (much like patch 9 and EndProcessingRestyles). But we will later also call it from the code that handles a root element font size change when we have 'rem' units. That's because it's fine to *start* the rebuild process in the middle of processing the queue of pending restyles. (We have to end after the whole process is done, though, in order to avoid wanting to destroy the old rule tree while we still have style contexts referencing it.) We only call StartRebuildAllStyleData in this case when we're processing our primary restyle queue (mPendingRestyles), not the animation restyles (to be removed in bug 960465) or the animation-only restyles, since a rebuild-all should be processed (in terms of animation phases, or in terms of having an animation-only update before it) like a normal restyle. (This isn't true for the 'rem' unit restyle, which could happen during any sort of update.)

In the new way of doing a rebuild-all, StartRebuildAllStyleData might be called directly from ProcessPendingRestyles rather than from RebuildAllStyleData (which null-checks the root frame) or from within processing restyles (which can only happen when there's a root frame). This means it needs its own null-check of the root frame.

This switches RebuildAllStyleData() to the normal ProcessPendingRestyles() manner of restyle processing. This means a rebuild-all going through this codepath (the main rebuild-all codepath) only sets up for non-animation restyle processing once rather than doing it twice (and potentially having reframes posted in DoRebuildAllStyleData() that don't get processed until ProcessPendingRestyles(), which causes a variant of bug 1110277 with transitions on reframed elements failing to start because it doesn't match the lifetime of the ReframingStyleContexts).

This changes what was probably a silly design choice when I wrote the code for 'rem'-basis handling; we shouldn't try continuing through the rest of RestyleElement() here, but instead repost the hint to the rebuild-all process.

This means that instead of recurring into DoRebuildAllStyleData, we'll call StartRebuildAllStyleData in the middle of processing the restyle queue (which is fine). StartRebuildAllStyleData will move the old rule tree out of the way and immediately do a full-tree restyle, before returning to any queue processing that might be left (the full-tree restyle should have consumed all remaining restyle hints, but might have posted some new ones for handling reframes that require reframing ancestors). And, more importantly, the EndReconstruct() call to get rid of the old rule tree won't happen until after we're done processing the containing RestyleTracker's queue of restyles, which reduces the risk of having dangling old style contexts and makes it easier (in bug 1110277) to have a ReframingStyleContexts with the right lifetime.

This fixes another pre-existing bug in the rebuild-all codepath; it didn't handle the animation-only update correctly, which could have caused bugs in transitions with OMT animations enabled.

If we discover that we've set mDoRebuildAllStyleData in the middle of ProcessPendingRestyles(), now that ProcessPendingRestyles() fully handles mDoRebuildAllStyleData, we only need to make a recursive call to ProcessPendingRestyles, rather than calling RebuildAllStyleData to call ProcessPendingRestyles.

If we need a strong reference to the pres shell, we may as well do it the first time rather than bothering with an extra variable.

Comment on attachment 8542422 [details] [diff] [review] patch 5 - Store the state of whether we're currently rebuilding all style data in a member variable, to prepare for future merging of the rebuild into other code Review of attachment 8542422 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/RestyleManager.h @@ +466,5 @@ > private: > nsPresContext* mPresContext; // weak, disconnected in Disconnect > > bool mDoRebuildAllStyleData : 1; > + bool mInRebuildAllStyleData : 1; These two could have a comment explaining the difference.

Comment on attachment 8542419 [details] [diff] [review] patch 2 - Move the eRestyle_ChangeAnimationPhaseDescendants hint in DoRebuildAllStyleData so that the new rebuild-all codepaths will keep it Review of attachment 8542419 [details] [diff] [review]: ----------------------------------------------------------------- Since RestyleManager::RestyleElement calls DoRebuildAllStyleData (if we're restyling the root and the font-size has changed), were we doing something wrong with animations and rem units that this fixes?

(In reply to Cameron McCormack (:heycam) from comment #23) > Since RestyleManager::RestyleElement calls DoRebuildAllStyleData (if we're > restyling the root and the font-size has changed), were we doing something > wrong with animations and rem units that this fixes? I think it probably does, actually, although constructing a testcase might be difficult. Without that change, we might end up with animation styles still around when our phase says we should have non-animation styles. (The whole issue goes away with bug 960465, which I'm hoping is actually soon now.)

Comment on attachment 8542420 [details] [diff] [review] patch 3 - Pass the hints to DoRebuildAllStyleData via the member variables, in preparation for future refactoring Review of attachment 8542420 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/RestyleManager.cpp @@ +948,5 @@ > if (oldContext->StyleFont()->mFont.size != > newContext->StyleFont()->mFont.size) { > // The basis for 'rem' units has changed. > newContext = nullptr; > + mRebuildAllRestyleHint |= aRestyleHint; Can mRebuildAllRestyleHint actually be anything other than 0 at this point?

Comment on attachment 8542421 [details] [diff] [review] patch 4 - Rename mRebuildAllStyleData to mDoRebuildAllStyleData Review of attachment 8542421 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/RestyleManager.h @@ +465,5 @@ > > private: > nsPresContext* mPresContext; // weak, disconnected in Disconnect > > + bool mDoRebuildAllStyleData : 1; Per Ms2ger's comment, please add a comment for this bit.

Comment on attachment 8542424 [details] [diff] [review] patch 7 - Always call DoProcessRestyles if mInRebuildAllStyleData Review of attachment 8542424 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/RestyleManager.h @@ +466,5 @@ > void ProcessRestyles(RestyleTracker& aRestyleTracker) { > // Fast-path the common case (esp. for the animation restyle > // tracker) of not having anything to do. > + if (aRestyleTracker.Count() || > + mInRebuildAllStyleData) { Can you explain further why we need to call into DoProcessRestyles if there are no pending restyles but mInRebuildAllStyleData is true? I can't tell from reading patch 9 why that is.

(In reply to Cameron McCormack (:heycam) from comment #27) > Can you explain further why we need to call into DoProcessRestyles if there > are no pending restyles but mInRebuildAllStyleData is true? I can't tell > from reading patch 9 why that is. Patch 11 makes me think this should actually be mDoRebuildAllStyleData?

Comment on attachment 8542435 [details] [diff] [review] patch 17 - Do animation-only update properly for a rebuild all Review of attachment 8542435 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/RestyleManager.cpp @@ +1589,5 @@ > // running entirely on the compositor thread) is up-to-date, so that > // if any style changes we cause trigger transitions, we have the > // correct old style for starting the transition. > if (nsLayoutUtils::AreAsyncAnimationsEnabled() && > + (mPendingRestyles.Count() > 0 || mDoRebuildAllStyleData)) { This (mPendingRestyles.Count() > 0 || mDoRebuildAllStyleData) is used in a couple of places. Maybe make a function for it?

Can we assert somewhere that both mDoRebuildAllStyleData and mInRebuildAllStyleData are false by the end of processing everything?

I think I find the naming of StartRebuildAllStyleData odd, because there's no corresponding EndRebuildAllStyleData. It also actually does the building of all the style data, so it's not just starting the process. I'm tempted to say it should be called DoRebuildAllStyleData.

(In reply to Cameron McCormack (:heycam) from comment #25) > Can mRebuildAllRestyleHint actually be anything other than 0 at this point? I'm not sure; there are some cases that can post restyles from the middle of the restyle processing queue (handling the change hints); it's possible some of them might be a rebuild-all, although I hope not. (In reply to Cameron McCormack (:heycam) from comment #27) > Comment on attachment 8542424 [details] [diff] [review] > patch 7 - Always call DoProcessRestyles if mInRebuildAllStyleData > > Review of attachment 8542424 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/RestyleManager.h > @@ +466,5 @@ > > void ProcessRestyles(RestyleTracker& aRestyleTracker) { > > // Fast-path the common case (esp. for the animation restyle > > // tracker) of not having anything to do. > > + if (aRestyleTracker.Count() || > > + mInRebuildAllStyleData) { > > Can you explain further why we need to call into DoProcessRestyles if there > are no pending restyles but mInRebuildAllStyleData is true? I can't tell > from reading patch 9 why that is. Because patches 9, 11, and 13 move the rebuilding to stuff that happens inside DoProcessRestyles; following those patches, if we bail out of ProcessRestyles, then no rebuilding will happen. (In the interim states, part of the work will happen and part won't.) (In reply to Cameron McCormack (:heycam) from comment #28) > (In reply to Cameron McCormack (:heycam) from comment #27) > > Can you explain further why we need to call into DoProcessRestyles if there > > are no pending restyles but mInRebuildAllStyleData is true? I can't tell > > from reading patch 9 why that is. > > Patch 11 makes me think this should actually be mDoRebuildAllStyleData? No, since we don't transform mDoRebuildAllStyleData into mInRebuildAllStyleData until BeginProcessingRestyles, which is called *inside* DoProcessRestyles. (In reply to Cameron McCormack (:heycam) from comment #30) > Can we assert somewhere that both mDoRebuildAllStyleData and > mInRebuildAllStyleData are false by the end of processing everything? Perhaps, although I'm not sure it will be true. (I don't think this patch changes how harmful it is or whether it would happen; it would mean that things aren't completely flushed in response to a flush, but I don't think we'd lose changes.) It's probably worth asserting if the assertion holds. (In reply to Cameron McCormack (:heycam) from comment #31) > I think I find the naming of StartRebuildAllStyleData odd, because there's > no corresponding EndRebuildAllStyleData. It also actually does the building > of all the style data, so it's not just starting the process. I'm tempted > to say it should be called DoRebuildAllStyleData. The EndRebuildAllStyleData is really just the call to EndReconstruct in RestyleManager::EndProcessingRestyles. I'd rather move that (and maybe the mInRebuildAllStyleData = false) into a function called FinishRebuildAllStyleData than rename StartRebuildAllStyleData to DoRebuildAllStyleData.

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #32) > > ::: layout/base/RestyleManager.h > > @@ +466,5 @@ > > > void ProcessRestyles(RestyleTracker& aRestyleTracker) { > > > // Fast-path the common case (esp. for the animation restyle > > > // tracker) of not having anything to do. > > > + if (aRestyleTracker.Count() || > > > + mInRebuildAllStyleData) { > > > > Can you explain further why we need to call into DoProcessRestyles if there > > are no pending restyles but mInRebuildAllStyleData is true? I can't tell > > from reading patch 9 why that is. > > Because patches 9, 11, and 13 move the rebuilding to stuff that happens > inside DoProcessRestyles; following those patches, if we bail out of > ProcessRestyles, then no rebuilding will happen. (In the interim states, > part of the work will happen and part won't.) I am getting muddled. As of patch 9, what is the control flow that sets mInRebuildAllStyleData to true and then calls ProcessRestyles (before EndProcessingRestyles, which sets it to false again)? > (In reply to Cameron McCormack (:heycam) from comment #30) > > Can we assert somewhere that both mDoRebuildAllStyleData and > > mInRebuildAllStyleData are false by the end of processing everything? > > Perhaps, although I'm not sure it will be true. (I don't think this patch > changes how harmful it is or whether it would happen; it would mean that > things aren't completely flushed in response to a flush, but I don't think > we'd lose changes.) It's probably worth asserting if the assertion holds. It would be a bug, right, if we didn't end up processing the mDoRebuildAllStyleData request? I think at least we should assert !mInRebuildAllStyleData by the end. > (In reply to Cameron McCormack (:heycam) from comment #31) > > I think I find the naming of StartRebuildAllStyleData odd, because there's > > no corresponding EndRebuildAllStyleData. It also actually does the building > > of all the style data, so it's not just starting the process. I'm tempted > > to say it should be called DoRebuildAllStyleData. > > The EndRebuildAllStyleData is really just the call to EndReconstruct in > RestyleManager::EndProcessingRestyles. I'd rather move that (and maybe the > mInRebuildAllStyleData = false) into a function called > FinishRebuildAllStyleData than rename StartRebuildAllStyleData to > DoRebuildAllStyleData. Yes that sounds fine to me.

(In reply to Cameron McCormack (:heycam) from comment #33) > I am getting muddled. As of patch 9, what is the control flow that sets > mInRebuildAllStyleData to true and then calls ProcessRestyles (before > EndProcessingRestyles, which sets it to false again)? It's not actually required until patch 13. (I think that's what I meant by the "once patch 9 is used via the ProcessPendingRestyles() codepath" in the commit message in patch 7.) (I reorganized the patch series after writing it to make it so that each step would still be fully functional and so that all of the steps would be simple; I tried to rewrite the commit messages appropriately afterwards, but I might not have gotten everything right.) > > (In reply to Cameron McCormack (:heycam) from comment #30) > > > Can we assert somewhere that both mDoRebuildAllStyleData and > > > mInRebuildAllStyleData are false by the end of processing everything? > > > > Perhaps, although I'm not sure it will be true. (I don't think this patch > > changes how harmful it is or whether it would happen; it would mean that > > things aren't completely flushed in response to a flush, but I don't think > > we'd lose changes.) It's probably worth asserting if the assertion holds. > > It would be a bug, right, if we didn't end up processing the > mDoRebuildAllStyleData request? > > I think at least we should assert !mInRebuildAllStyleData by the end. Yes, it would be a bug. I can see if the assertion sticks, although I'm not sure if it will.

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #34) > It's not actually required until patch 13. (I think that's what I meant by > the "once patch 9 is used via the ProcessPendingRestyles() codepath" in the > commit message in patch 7.) OK. Anyway, since it's replaced in patch 11 (with the check for mDoRebuildAllStyleData) I guess it doesn't matter. And I'm happy with how it is in patch 13. > (I reorganized the patch series after writing it to make it so that each > step would still be fully functional and so that all of the steps would be > simple; I tried to rewrite the commit messages appropriately afterwards, but > I might not have gotten everything right.) I appreciate the small individual patch sizes. > > It would be a bug, right, if we didn't end up processing the > > mDoRebuildAllStyleData request? > > > > I think at least we should assert !mInRebuildAllStyleData by the end. > > Yes, it would be a bug. I can see if the assertion sticks, although I'm not > sure if it will. Sounds good.

(In reply to Cameron McCormack (:heycam) from comment #29) > This (mPendingRestyles.Count() > 0 || mDoRebuildAllStyleData) is used in a > couple of places. Maybe make a function for it? I'm not going to do this. They're slightly different already (which isn't a big deal), so I could just turn the patch into: https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/9fff4311386a/rebuild-all-animation-only-update but they're going to become more different in bug 960465, which means I'd just need to revert the change there, in: https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/767e8b20d633/flush-animation-only-before-restyle

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #36) > (In reply to Cameron McCormack (:heycam) from comment #29) > > This (mPendingRestyles.Count() > 0 || mDoRebuildAllStyleData) is used in a > > couple of places. Maybe make a function for it? > > I'm not going to do this. [...] they're going to become more different in bug 960465 OK.

(In reply to Cameron McCormack (:heycam) from comment #35) > (In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from > comment #34) > > It's not actually required until patch 13. (I think that's what I meant by > > the "once patch 9 is used via the ProcessPendingRestyles() codepath" in the > > commit message in patch 7.) > > OK. Anyway, since it's replaced in patch 11 (with the check for > mDoRebuildAllStyleData) I guess it doesn't matter. And I'm happy with how > it is in patch 13. Yeah, I think patch 7 is probably a vestige of how I originally wrote the patch series, and shouldn't really exist as a separate patch, but rather just be replaced by what replaces it in patch 11. That said, I've done a bunch of testing that everything works each step of the patch series that I'd rather not redo, so I'm inclined to just leave things as they are.

https://hg.mozilla.org/try/pushloghtml?changeset=83d5de58d42a https://treeherder.mozilla.org/#/jobs?repo=try&revision=83d5de58d42a https://tbpl.mozilla.org/?tree=Try&rev=83d5de58d42a

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b5e114c159c https://hg.mozilla.org/integration/mozilla-inbound/rev/8927072cd0fb https://hg.mozilla.org/integration/mozilla-inbound/rev/4d145190cf56 https://hg.mozilla.org/integration/mozilla-inbound/rev/5f64ba26810f https://hg.mozilla.org/integration/mozilla-inbound/rev/722bcd3bcbbc https://hg.mozilla.org/integration/mozilla-inbound/rev/ce19dc161a0b https://hg.mozilla.org/integration/mozilla-inbound/rev/716fab262b56 https://hg.mozilla.org/integration/mozilla-inbound/rev/396a1fdfd686 https://hg.mozilla.org/integration/mozilla-inbound/rev/3758de260ac3 https://hg.mozilla.org/integration/mozilla-inbound/rev/ea9a26670eb9 https://hg.mozilla.org/integration/mozilla-inbound/rev/fa9e8fe352a1 https://hg.mozilla.org/integration/mozilla-inbound/rev/dee1be646918 https://hg.mozilla.org/integration/mozilla-inbound/rev/b939cbd6e0e1 https://hg.mozilla.org/integration/mozilla-inbound/rev/85c66f81ce8d https://hg.mozilla.org/integration/mozilla-inbound/rev/90dec68c50a4 https://hg.mozilla.org/integration/mozilla-inbound/rev/dbc67171fde2 https://hg.mozilla.org/integration/mozilla-inbound/rev/70ed6234b156 https://hg.mozilla.org/integration/mozilla-inbound/rev/8005d07007d5 https://hg.mozilla.org/integration/mozilla-inbound/rev/4c7428662fab https://hg.mozilla.org/integration/mozilla-inbound/rev/e12e9be31708

https://hg.mozilla.org/mozilla-central/rev/8b5e114c159c https://hg.mozilla.org/mozilla-central/rev/8927072cd0fb https://hg.mozilla.org/mozilla-central/rev/4d145190cf56 https://hg.mozilla.org/mozilla-central/rev/5f64ba26810f https://hg.mozilla.org/mozilla-central/rev/722bcd3bcbbc https://hg.mozilla.org/mozilla-central/rev/ce19dc161a0b https://hg.mozilla.org/mozilla-central/rev/716fab262b56 https://hg.mozilla.org/mozilla-central/rev/396a1fdfd686 https://hg.mozilla.org/mozilla-central/rev/3758de260ac3 https://hg.mozilla.org/mozilla-central/rev/ea9a26670eb9 https://hg.mozilla.org/mozilla-central/rev/fa9e8fe352a1 https://hg.mozilla.org/mozilla-central/rev/dee1be646918 https://hg.mozilla.org/mozilla-central/rev/b939cbd6e0e1 https://hg.mozilla.org/mozilla-central/rev/85c66f81ce8d https://hg.mozilla.org/mozilla-central/rev/90dec68c50a4 https://hg.mozilla.org/mozilla-central/rev/dbc67171fde2 https://hg.mozilla.org/mozilla-central/rev/70ed6234b156 https://hg.mozilla.org/mozilla-central/rev/8005d07007d5 https://hg.mozilla.org/mozilla-central/rev/4c7428662fab https://hg.mozilla.org/mozilla-central/rev/e12e9be31708