So we have this bit of code[1], and another similar one a few lines below, that make us restyle right before reconstructing frames. This is only done because from ContentRemoved, we may end up reconstructing, and we may not have totally up-to-date styles by then. However, it's fine for it to get slightly out of date styles, because we have a restyle scheduled anyway. So there are two ways to fix this: * Bite the bullet and allow resolving slightly out-to-date styles from ContentRemoved. I had a patch doing this that was green. * Put a bit of effort in making reconstruction calls in ContentRemoved async. This might be doable, though I looked into it and wasn't exactly trivial. [1]: http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/layout/base/nsCSSFrameConstructor.cpp#8066

This is the patch I used. This needs to tweak a debug assertion in glue.rs to check only for styles, not up-to-date styles, when resolving, but is otherwise green.

I'm not sure I follow. The StyleChildRangeForReconstruct calls are in ContentInserted/ContentAppended. That is, we're about to construct frames for the relevant content. Why do we not need up-to-date styles here? I would think we do in fact need them...

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #2) > I'm not sure I follow. The StyleChildRangeForReconstruct calls are in > ContentInserted/ContentAppended. That is, we're about to construct frames > for the relevant content. Well, they also happen from RecreateFramesForContent, which is on change hint processing, and we force to traverse the whole subtree in when the traversal is "for reconstruct". Also I bet we're doing dumb stuff with respect to lazy frame construction, which also call ContentRangeInserted. In both of those cases, we have the correct styles already, and it's assertable. The only case we don't right now (according to the try run with the posted patch) is on content removed notifications, where we may end up reconstructing some frame up in the hierarchy for an element that may have suffered a state change or attribute change, for example. > Why do we not need up-to-date styles here? I would think we do in fact need > them... My idea is that, if styles are not up-to-date on ContentRemoved because the parent or someone else needs a restyle, sure, we'll create the frames with the wrong style initially, but when the next style flush arrives we update them properly. It's not ideal (I think the best solution would be something like that notification posting a ReconstructFrames hint for the children instead, or something along those lines, but that may be non-tribial).

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3) > My idea is that, if styles are not up-to-date on ContentRemoved because the > parent or someone else needs a restyle, sure, we'll create the frames with > the wrong style initially, but when the next style flush arrives we update > them properly. It's not ideal (I think the best solution would be something > like that notification posting a ReconstructFrames hint for the children > instead, or something along those lines, but that may be non-tribial). Err, I meant for the parent here, when we do have to reconstruct. and s/tribial/trivial, sigh.

Are we in ContentRemoved due to a reframe from style, or a DOM mutation? Because in general, ContentRemoved needs to make sure frames go away. It doesn't have to create new frames synchronously. At least conceptually that's how it should work. There are some complications around frame state restoration, I guess... or at least that's the only reason I can think of for all those RecreateFramesForContent calls it makes saying "false" for aAsyncInsert.

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #5) > Are we in ContentRemoved due to a reframe from style, or a DOM mutation? DOM mutation. From style the styles are already up-to-date. > Because in general, ContentRemoved needs to make sure frames go away. It > doesn't have to create new frames synchronously. At least conceptually > that's how it should work. Yeah, that matches my mental model too, see the description. > There are some complications around frame state restoration, I guess... or > at least that's the only reason I can think of for all those > RecreateFramesForContent calls it makes saying "false" for aAsyncInsert. I can try to switch it and see whatever fails when I have the time, I guess...

I think we should be able to remove this now, since I fixed the ContentRemoved stuff.

It looks reasonably green even though it hasn't completely finished. Since I'm about to head out for the day I'll post the patches.

Comment on attachment 8903292 [details] Bug 1374235: style: Remove the for reconstruction traversals. https://reviewboard.mozilla.org/r/175086/#review180146 \o/ \o/ \o/ ::: layout/base/nsCSSFrameConstructor.cpp:7689 (Diff revision 1) > if (aContainer->IsStyledByServo()) { > - if (aForReconstruction) { > + if (!aForReconstruction) { > - StyleChildRangeForReconstruct(aFirstNewContent, nullptr); Nit: Make this a single predicate with &&? ::: layout/base/nsCSSFrameConstructor.cpp:8177 (Diff revision 1) > } > } > > // We couldn't construct lazily. Make Servo eagerly traverse the new content. > if (aContainer->IsStyledByServo()) { > - if (aForReconstruction) { > + if (!aForReconstruction) { Same here. ::: servo/components/style/traversal.rs:293 (Diff revision 1) > // > // In aggressively forgetful traversals (where we seek out and clear damage > // in addition to not computing it) we also need to traverse nodes with > // explicit damage and no other restyle data, so that this damage can be cleared. This comment needs updating.

The assertions failing are because I removed the servo bits from bug 1394935, sigh. Will push again before going to sleep. Thanks for the reviews!

Comment on attachment 8903292 [details] Bug 1374235: style: Remove the for reconstruction traversals. https://reviewboard.mozilla.org/r/175086/#review180146 > Nit: Make this a single predicate with &&? I used isNewlyAddedContentForServo instead, which is equivalent.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/35afffcb4182 style: Remove the for reconstruction traversals. r=bholley

Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/be68eab4aba7 Backed out changeset 35afffcb4182 for build bustage because it landed before its servo commit. r=backout

hg error in cmd: hg rebase -s 35afffcb4182 -d 9134ee2f8dcf: abort: source is ancestor of destination

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a0bef0340a9d style: Remove the for reconstruction traversals. r=bholley