This is one of the issues that forces stylo to potentially style arbitrary already-styled parents in order to avoid resolving stale styles. There's no particular reason those reconstructs need to be synchronous.

The patch that's coming is pretty much green (need to tweak the LAYOUT_PHASE stuff, in order for it not to assert in a variety of situations in debug builds). The only failing test-cases for Gecko are three grid paginated fragmentation tests, which I'm moderately sure they're not my fault. There are also a few XBL test-cases that are failing on Stylo, but I know how to fix them. I'm uploading the patch for feedback, will probably finish it off tomorrow or on monday.

This is the try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8e248d75a85316f646823942e16a4c490cebd5c (disregard the debug-only layout phase assertions, since those are there since I forgot a few EXIT() / REENTER() calls). Do you know who should I poke at for the grid fragmentation stuff? (I guess mats, but he's on vacation... Maybe dholbert would be a good candidate too?) I'd ask bz for feedback, bug he doesn't accept feedback requests at the moment.

Pretty close: https://treeherder.mozilla.org/#/jobs?repo=try&revision=361daccdb530c08c5fde6b687ae850b3c2591a93

(This is not stylo-only, though it unblocks a bunch of stylo cleanups)

Comment on attachment 8896798 [details] Bug 1389743: Only reconstruct frames synchronously from ContentRemoved when called from frame construction. https://reviewboard.mozilla.org/r/168082/#review176512 This looks fine to me overall, nice work! r=mats with the nits and questions addressed. ::: commit-message-fb2e8:9 (Diff revision 1) > + > +There's only one case of sync frame construction from ContentRemoved now, and > +it's not on the element being removed, but on the whitespace siblings if needed, > +and _only_ when they don't support lazy frame construction. > + > +Basically, this switchs all the RecreateFramesForContent calls to use s/switchs/switches/ ::: layout/base/PresShell.cpp:2921 (Diff revision 1) > > ASSERT_REFLOW_SCHEDULED_STATE(); > } > > void > -PresShell::DestroyFramesFor(nsIContent* aContent, > +PresShell::DestroyFramesFor(Element* aElement) nit: remove one space please ::: layout/base/PresShell.cpp:2941 (Diff revision 1) > -} > + PostRecreateFramesFor(aElement); > - I'm surprised this works without GetLastCapturedLayoutHistoryState(). Maybe we simply don't have any tests for that? ;-) I'm also surprised it works with 'aElement' rather than 'aDestroyedFramesFor' -- are all cases where 'aDestroyedFramesFor' != 'aElement' covered by 'didReconstruct' being true and thus PostRecreateFramesFor posted restyle events on the right element for those cases? ::: layout/base/PresShell.cpp:2944 (Diff revision 1) > - // Don't call RecreateFramesForContent since that is not exported and we want > - // to keep the number of entrypoints down. > + mPresContext->RestyleManager()->PostRestyleEvent( > + aElement, eRestyle_Subtree, nsChangeHint(0)); I don't understand what this is for. Why is it needed? ::: layout/base/nsCSSFrameConstructor.h:325 (Diff revision 1) > void ContentRemoved(nsIContent* aContainer, > nsIContent* aChild, > nsIContent* aOldNextSibling, > RemoveFlags aFlags, > - bool* aDidReconstruct, > + bool* aDidReconstruct); We might want to return 'aDidReconstruct' as a return value rather than an out param. Ditto for DestroyFramesFor(). File a follow-up bug on that? ::: layout/base/nsCSSFrameConstructor.h:1812 (Diff revision 1) > - void > - RecreateFramesForContent(nsIContent* aContent, > - bool aAsyncInsert, > - RemoveFlags aFlags, > + void RecreateFramesForContent( > + nsIContent* aContent, > + InsertionKind aInsertionKind, > + RemoveFlags aFlags); I'd prefer: void RecreateFramesForContent(nsIContent* aContent, InsertionKind aInsertionKind, RemoveFlags aFlags); ::: layout/base/nsCSSFrameConstructor.cpp:6429 (Diff revision 1) > - RecreateFramesForContent(rootElement, false, REMOVE_FOR_RECONSTRUCTION, > - nullptr); > + RecreateFramesForContent( > + rootElement, InsertionKind::Sync, REMOVE_FOR_RECONSTRUCTION); The indentation of the RecreateFramesForContent args is a bit inconsistent in this patch. I'd prefer if we add them directly after the '(', add as many args as fits on each line, and then start a new line in the start column of the first arg. That way the indentation is consistent, and I think it's the current style used in this file. ::: layout/base/nsCSSFrameConstructor.cpp:7217 (Diff revision 1) > // NS_CREATE_FRAME_IF_NON_WHITESPACE flag) > return; > } > NS_ASSERTION(!aContent->GetPrimaryFrame(), > "Text node has a frame and NS_CREATE_FRAME_IF_NON_WHITESPACE"); > - ContentInserted(aParentContent, aContent, nullptr, false); > + ContentInserted(aParentContent, aContent, nullptr, true); While we're here, perhaps we should document what 'true' means by using a temp? bool allowLazyConstruction = true; ..., allowLazyConstruction); ::: layout/base/nsCSSFrameConstructor.cpp:10032 (Diff revision 1) > -nsCSSFrameConstructor::RecreateFramesForContent(nsIContent* aContent, > - bool aAsyncInsert, > - RemoveFlags aFlags, > +nsCSSFrameConstructor::RecreateFramesForContent(nsIContent* aContent, > + InsertionKind aInsertionKind, > + RemoveFlags aFlags) > - nsIContent** aDestroyedFramesFor) > { > - NS_PRECONDITION(!aAsyncInsert || aContent->IsElement(), > + MOZ_ASSERT_IF(aInsertionKind == InsertionKind::Async, aContent->IsElement()); I'd prefer MOZ_ASSERT(aInsertionKind != InsertionKind::Async || aContent->IsElement()). (IIRC, dbaron has veto'ed using MOZ_ASSERT_IF in layout code) ::: layout/base/nsCSSFrameConstructor.cpp:10126 (Diff revision 1) > - const bool reconstruct = aFlags != REMOVE_DESTROY_FRAMES; > - RemoveFlags flags = reconstruct ? REMOVE_FOR_RECONSTRUCTION : aFlags; > - ContentRemoved(container, aContent, nextSibling, flags, > - &didReconstruct, aDestroyedFramesFor); > - if (reconstruct && !didReconstruct) { > + ContentRemoved( > + container, > + aContent, > + nextSibling, > + aFlags, > + &didReconstruct); This indentation is rather odd, please list as many args as fits on each line instead, starting after the opening paren. ::: layout/base/nsCSSFrameConstructor.cpp (Diff revision 1) > + } else { > - // Now, recreate the frames associated with this content object. If > + // Now, recreate the frames associated with this content object. If > - // ContentRemoved triggered reconstruction, then we don't need to do this > + // ContentRemoved triggered reconstruction, then we don't need to do this > - // because the frames will already have been built. > + // because the frames will already have been built. > - if (aAsyncInsert) { > - // XXXmats doesn't frame state need to be restored in this case too? I'd like to keep the XXXmats comment above the PostRestyleEvent() call -- unless you have a reasonable explanation for why it's not needed. We should also add a similar comment before the PostRecreateFramesFor call in DestroyFramesFor I guess. (At some point we should probably refactor frame state handling and it might help to mark up the places we lose it.) ::: layout/base/nsCSSFrameConstructor.cpp:10153 (Diff revision 1) > -nsCSSFrameConstructor::DestroyFramesFor(nsIContent* aContent, > - nsIContent** aDestroyedFramesFor) > +nsCSSFrameConstructor::DestroyFramesFor( > + nsIContent* aContent, > + bool* aDidReconstruct) nit: seems like the first param would fit next to the '(', and the second param under it if it doesn't fit next to it. ::: layout/base/nsIPresShell.h:534 (Diff revision 1) > - */ > - virtual void DestroyFramesFor(nsIContent* aContent, > + * The frame tree state will be captured before the frames are destroyed in > + * the frame constructor. I think we can remove this note now, since it was only documenting why using GetLastCapturedLayoutHistoryState() in CreateFramesFor() was OK. Now that DestroyFramesFor() does PostRecreateFramesFor() instead, the comment seems misleading since we're in fact not using the captured frame state from the deleted frame tree, AFAICT. BTW, it might now be possible to condition the CaptureStateForFramesOf call in RecreateFramesForContent to the case where it's actually used locally. (it's a bit expensive, IIRC) Can we also remove the GetLastCapturedLayoutHistoryState() accessor? http://searchfox.org/mozilla-central/search?q=GetLastCapturedLayoutHistoryState

Comment on attachment 8896799 [details] Bug 1389743: Remove various Stylo hacks to deal with XBL edge cases. https://reviewboard.mozilla.org/r/168084/#review176660

Comment on attachment 8896798 [details] Bug 1389743: Only reconstruct frames synchronously from ContentRemoved when called from frame construction. https://reviewboard.mozilla.org/r/168082/#review176656 ::: layout/base/PresShell.cpp:2941 (Diff revision 1) > -} > + PostRecreateFramesFor(aElement); > - The cases where `aDestroyedFramesFor` != `aElement` are the cases where `didReconstruct` is true, so yes. ::: layout/base/PresShell.cpp:2944 (Diff revision 1) > - // Don't call RecreateFramesForContent since that is not exported and we want > - // to keep the number of entrypoints down. > + mPresContext->RestyleManager()->PostRestyleEvent( > + aElement, eRestyle_Subtree, nsChangeHint(0)); This is needed for stylo, since stylo attaches styles to elements instead of frames, so it won't restyle by default when reframing unless we tell it to. ::: layout/base/nsCSSFrameConstructor.h:325 (Diff revision 1) > void ContentRemoved(nsIContent* aContainer, > nsIContent* aChild, > nsIContent* aOldNextSibling, > RemoveFlags aFlags, > - bool* aDidReconstruct, > + bool* aDidReconstruct); Good call, will file. ::: layout/base/nsCSSFrameConstructor.h:1812 (Diff revision 1) > - void > - RecreateFramesForContent(nsIContent* aContent, > - bool aAsyncInsert, > - RemoveFlags aFlags, > + void RecreateFramesForContent( > + nsIContent* aContent, > + InsertionKind aInsertionKind, > + RemoveFlags aFlags); Sure.

Comment on attachment 8896798 [details] Bug 1389743: Only reconstruct frames synchronously from ContentRemoved when called from frame construction. https://reviewboard.mozilla.org/r/168082/#review176512 > We might want to return 'aDidReconstruct' as a return value rather than an out param. > Ditto for DestroyFramesFor(). > File a follow-up bug on that? Sure, will do. > I'd prefer: > void RecreateFramesForContent(nsIContent* aContent, > InsertionKind aInsertionKind, > RemoveFlags aFlags); Done :) > The indentation of the RecreateFramesForContent args is a bit inconsistent in this patch. I'd prefer if we add them directly after the '(', add as many args as fits on each line, and then start a new line in the start column of the first arg. That way the indentation is consistent, and I think it's the current style used in this file. Yup, I formatted them all the same way, good catch. > While we're here, perhaps we should document what 'true' means by using a temp? > bool allowLazyConstruction = true; > ..., allowLazyConstruction); Good call, done. > I'd prefer MOZ_ASSERT(aInsertionKind != InsertionKind::Async || aContent->IsElement()). > (IIRC, dbaron has veto'ed using MOZ_ASSERT_IF in layout code) Huh, sure thing, but... why is that?

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5c0a8f5eb8c9 Only reconstruct frames synchronously from ContentRemoved when called from frame construction. r=mats https://hg.mozilla.org/integration/autoland/rev/6406fed0a047 Remove various Stylo hacks to deal with XBL edge cases. r=heycam

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/6ccd5cf829e2 Update expected assertion count in layout/generic/crashtests/1059138-1.html. rs=heycam

https://hg.mozilla.org/mozilla-central/rev/5c0a8f5eb8c9 https://hg.mozilla.org/mozilla-central/rev/6406fed0a047 https://hg.mozilla.org/mozilla-central/rev/6ccd5cf829e2

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10) > > + mPresContext->RestyleManager()->PostRestyleEvent( > > + aElement, eRestyle_Subtree, nsChangeHint(0)); > > This is needed for stylo, since stylo attaches styles to elements instead of > frames, so it won't restyle by default when reframing unless we tell it to. OK, good. Should wrap it in "if (IsStylo())" then to avoid a performance penalty for Gecko? Or do we not care much about that anymore since we'll be Stylo-only soon anyway? (I assume) Anyway, I still think it's a bit surprising to see this restyle request here, since nothing in this method requires it AFAICT. The reason (I think) is that the callers of DestroyFramesFor (Element::CreateShadowRoot and nsXBLBindingRequest::DocumentLoaded) does things that requires it. It might be worth adding a code comment explaining a bit why it's there?

(In reply to Emilio Cobos Álvarez [:emilio] from comment #11) > > I'd prefer MOZ_ASSERT(aInsertionKind != InsertionKind::Async || aContent->IsElement()). > > (IIRC, dbaron has veto'ed using MOZ_ASSERT_IF in layout code) > > Huh, sure thing, but... why is that? I don't recall exactly, but I think it was "unclear semantics". You have to ask him. The reason *I* don't like it is that it's almost the same as MOZ_ASSERT anyway and fewer variations is less cognitive load. Also, its name is similar to NS_WARN_IF which has completely different semantics.

(In reply to Mats Palmgren (:mats) from comment #18) > (In reply to Emilio Cobos Álvarez [:emilio] from comment #11) > > > I'd prefer MOZ_ASSERT(aInsertionKind != InsertionKind::Async || aContent->IsElement()). > > > (IIRC, dbaron has veto'ed using MOZ_ASSERT_IF in layout code) > > > > Huh, sure thing, but... why is that? > > I don't recall exactly, but I think it was "unclear semantics". > You have to ask him. > > The reason *I* don't like it is that it's almost the same as > MOZ_ASSERT anyway and fewer variations is less cognitive load. > Also, its name is similar to NS_WARN_IF which has completely > different semantics. I think it's nicer to read, but the argument about NS_WARN_IF is a good one.

(In reply to Mats Palmgren (:mats) from comment #17) > (In reply to Emilio Cobos Álvarez [:emilio] from comment #10) > > > + mPresContext->RestyleManager()->PostRestyleEvent( > > > + aElement, eRestyle_Subtree, nsChangeHint(0)); > > > > This is needed for stylo, since stylo attaches styles to elements instead of > > frames, so it won't restyle by default when reframing unless we tell it to. > > OK, good. Should wrap it in "if (IsStylo())" then to avoid > a performance penalty for Gecko? Or do we not care much about > that anymore since we'll be Stylo-only soon anyway? (I assume) It doesn't really matter, since the element has no frame anyway, so it'll get a full subtree restyle when reframed, that's why I didn't bother. > Anyway, I still think it's a bit surprising to see this restyle > request here, since nothing in this method requires it AFAICT. > The reason (I think) is that the callers of DestroyFramesFor > (Element::CreateShadowRoot and nsXBLBindingRequest::DocumentLoaded) > does things that requires it. It might be worth adding a code > comment explaining a bit why it's there? That's fair, will land a followup for this, I'll ni? miself since it's quite late already.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/22c304405b01 followup: Add a comment about why we post a restyle in DestroyFramesFor. r=comment-only