Closed Bug 1389743 Opened 2 years ago Closed 2 years ago

Never do sync frame construction when removing content from the DOM.

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Stylo])

Attachments

(2 files, 1 obsolete file)

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.
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #8896568 - Flags: feedback?(cam)
Duplicate of this bug: 1377848
Blocks: 1365783, 1374235
Depends on: 1389936
No longer depends on: 1389936
Attachment #8896568 - Attachment is obsolete: true
Attachment #8896568 - Flags: feedback?(cam)
Summary: Never do sync frame construction when removing content from the DOM. → stylo: Never do sync frame construction when removing content from the DOM.
(This is not stylo-only, though it unblocks a bunch of stylo cleanups)
Summary: stylo: Never do sync frame construction when removing content from the DOM. → Never do sync frame construction when removing content from the DOM.
Whiteboard: [Stylo]
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
Attachment #8896798 - Flags: review?(mats) → review+
Comment on attachment 8896799 [details]
Bug 1389743: Remove various Stylo hacks to deal with XBL edge cases.

https://reviewboard.mozilla.org/r/168084/#review176660
Attachment #8896799 - Flags: review?(cam) → review+
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
Blocks: 1392964
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
(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.
Flags: needinfo?(emilio+bugs)
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
Flags: needinfo?(emilio+bugs)
Depends on: 1395719
Depends on: 1405796
Blocks: 1404789
You need to log in before you can comment on or make changes to this bug.