890782 - Assertion failure: "should have already reflowed the kid" (nsSVGTextFrame2::UpdateGlyphPositioning)

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: !(((kid)->GetStateBits() & ((nsFrameState(1) << (10)) | (nsFrameState(1) << (12)))) != 0) (should have already reflowed the kid), at layout/svg/nsSVGTextFrame2.cpp:4835

This assertion was added in bug 885642.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Cameron McCormack (:heycam)]

Unfortunately we crash just after the assertion, even in release builds.  Let's fix that first.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: return early from UpdateGlyphPositioning if frames are dirty

We should be returning early from UpdateGlyphPositioning, with mPositions empty, when we encounter this assertion failure.

There's also a problem with CharIterator's passing in of nullptr to TextFrameIterator's constructor as the root frame.  We need the TextFrameIterator constructor to not call GetContent() on aRoot when it's null, and also for Init() to not call StyleSVGReset() on the null mRootFrame.

Comment 4

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=96f91409dbd3

Comment 5

[Cameron McCormack (:heycam)]

We don't end up reflowing the <text> since ReflowSVGNonDisplayText doesn't go down into the nsSVGForeignObjectFrame, because it isn't an nsSVGContainerFrame.  We might even have HTML elements between the <foreignObject> and the <text>, so I think we should be traversing down any nsSVGForeignObjectFrame and any non-SVG frame too.

Comment 6

[Robert Longson [:longsonr]]

Don't forget to check in the testcase as a crashtest :-)

Comment 7

[Cameron McCormack (:heycam)]

We'll still crash in the crashtest because I'm using MOZ_ASSERT, so I'll need to solve the underlying issues in comment 5 before adding the document as a crashtest.  Do you think it makes sense to continue using MOZ_ASSERT, or should I switch to NS_ASSERTION?

Comment 8

[Robert Longson [:longsonr]]

MOZ_ASSERT is OK provided you fix the underlying problem.

Comment 9

[Cameron McCormack (:heycam)]

There are some other reasons why we don't end up calling ReflowSVGNonDisplayText.  We at least need to do something in nsSVGDisplayContainerFrame::InsertFrames to make sure we schedule a ReflowSVG when we insert a (non-display) nsSVGContainerFrame.  I'll keep looking into that tomorrow.

Comment 10

[Cameron McCormack (:heycam)]

Attachment: WIP reflow SVG text inserted into non-display containers (v0.1)

jwatt, I'd like your feedback here.

Here's a summary of the problem.  Assume the document begins like this:

  <svg ...>
    <defs>
      <foreignObject/>
    </defs>
  </svg>

defs is a non-display container, as is the foreignObject.  We then insert this subtree as a child of the foreignObject:

  <svg>
    <text>...</text>
  </svg>

and immediately after try to access some text-layout dependent information on the <text>.  We should be able to flush layout and have that cause the non-display text to be reflowed, but that isn't happening currently.

It's not happening, because when we insert the frames into the non-display container frame for the foreignObject, we don't schedule a reflow that would be needed to find the non-display text frames and call ReflowSVGNonDisplayText on them.  We need to have nsSVGContainerFrame::InsertFrames (and nsSVGDisplayContainerFrame::InsertFrames) find any nsSVGTextFrame2s and call SchedulReflowSVGNonDisplayText on them.

I first tried to do this without the changes to nsSVGTextFrame2::ScheduleReflowSVGNonDisplayText that I have in the patch, but this didn't work.  I put some reasoning into the comment of my changes to that function.  Let me know if it makes sense or not.

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 772530 [details] [diff] [review]
WIP reflow SVG text inserted into non-display containers (v0.1)

Review of attachment 772530 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/nsSVGContainerFrame.cpp
@@ +80,5 @@
>    NS_ASSERTION(!aPrevFrame || aPrevFrame->GetParent() == this,
>                 "inserting after sibling frame with different parent");
>  
> +  nsIFrame* firstOldFrame = aPrevFrame ?
> +    aPrevFrame->GetNextSibling() : GetChildList(aListID).FirstChild();

Nit: I think "old" is too easy to link to the idea of replacing and removing. Maybe call this "nextFrame" to compliment "aPrevFrame"? Alternatively iterate over aFrameList directly?

@@ +142,5 @@
>  }
>  
> +/**
> + * Traverses a frame tree, marking any nsSVGTextFrame2 frame as dirty
> + * and calling InvalidateRenderingObservers() on it.

While you're moving this, s/frame as/frames/ and s/it/them/, I'd think.

::: layout/svg/nsSVGTextFrame2.cpp
@@ +3156,5 @@
> +  // choice for the frame to call FrameNeedsReflow on.  However, if that
> +  // outer SVG frame's parent is dirty, which can be the case when the
> +  // whole subtree from that parent frame has just been created and inserted
> +  // into the frame tree, then calling FrameNeedsReflow on the outer SVG
> +  // will return early once it finds that dirty parent.

Why is that a problem? I'm just guessing here, but did you find your testcase wasn't fixed without this and, if so, isn't the real problem that the nsSVGOuterSVGFrame is also NS_STATE_SVG_NONDISPLAY_CHILD? In that case it seems like this comment could be changed, and you could also be checking for NS_STATE_SVG_NONDISPLAY_CHILD in the code below, as appropriate.

Comment 12

[Cameron McCormack (:heycam)]

(In reply to Jonathan Watt [:jwatt] from comment #11)
> ::: layout/svg/nsSVGTextFrame2.cpp
> @@ +3156,5 @@
> > +  // choice for the frame to call FrameNeedsReflow on.  However, if that
> > +  // outer SVG frame's parent is dirty, which can be the case when the
> > +  // whole subtree from that parent frame has just been created and inserted
> > +  // into the frame tree, then calling FrameNeedsReflow on the outer SVG
> > +  // will return early once it finds that dirty parent.
> 
> Why is that a problem? I'm just guessing here, but did you find your
> testcase wasn't fixed without this and, if so, isn't the real problem that
> the nsSVGOuterSVGFrame is also NS_STATE_SVG_NONDISPLAY_CHILD? In that case
> it seems like this comment could be changed, and you could also be checking
> for NS_STATE_SVG_NONDISPLAY_CHILD in the code below, as appropriate.

The goal of ScheduleReflowSVGNonDisplayText is to ensure a layout flush has been queued up that will result in ReflowSVGNonDisplayText being called on that frame when the layout flush is processed.

Normally, it is fine for FrameNeedsReflow to return early and not call mDocument->SetNeedLayoutFlush() when it finds a frame that is already dirty while moving up the tree, since that would mean that its reflow root (or root of the frame tree) has already been enqueued for a layout flush.  In this case, it's not true, since the frames are dirty because they've just been created, and not because they've been marked up the tree as dirty by a prior call to FrameNeedsReflow.

When a non-SVG frame subtree is created and inserted, FrameNeedsReflow gets called on the new subtree.  This happens in nsContainerFrame::InsertFrames:

  https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.cpp#156

We could just do the same in our nsSVGContainerFrame::InsertFrames, but the frame we're inserting in to might be non-display, in which case FrameNeedsReflow will still return early, because our non-display frames always remain dirty.

Comment 13

[Cameron McCormack (:heycam)]

I've discovered however that the ScheduleReflowSVGNonDisplayText calls in nsSVGContainerFrame.cpp are unnecessary, and all that is needed is the changes to nsSVGTextFrame2::ScheduleReflowSVGNonDisplayText.  This is because when the newly constructed frames get Init called on them, this will call nsSVGTextFrame2::DidSetStyleContext, which will call ScheduleReflowSVGNonDisplayText itself.

Comment 14

[Cameron McCormack (:heycam)]

Attachment: reflow SVG text inserted into non-display containers (v1.0)

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 773016 [details] [diff] [review]
reflow SVG text inserted into non-display containers (v1.0)

Review of attachment 773016 [details] [diff] [review]:
-----------------------------------------------------------------

Marking r- based on the requested NS_STATE_SVG_NONDISPLAY_CHILD change, but if you don't want to do that for some reason then feel free to reset to r?.

::: layout/svg/nsSVGContainerFrame.cpp
@@ +125,5 @@
>    // pending, then we need to schedule one for our new children:
>    if (!(GetStateBits() &
>          (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN |
>           NS_STATE_SVG_NONDISPLAY_CHILD))) {
> +    for (nsIFrame* kid = firstNewFrame; kid != nextFrame;

Heh. And now I guess "nextFrame" seems weird here. Oh well...

@@ +243,5 @@
>    return nsSVGUtils::GetCoveredRegion(mFrames);
>  }
>  
>  /**
> + * Traverses a frame tree, marking any nsSVGTextFrame2 frame dirty

s/frame dirty/frames dirty/

@@ +271,5 @@
>  {
>    NS_ASSERTION(aContainer->GetStateBits() & NS_FRAME_IS_DIRTY,
>                 "expected aContainer to be NS_FRAME_IS_DIRTY");
> +  NS_ASSERTION((aContainer->GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD) ||
> +               !aContainer->IsFrameOfType(nsIFrame::eSVG),

I think it would be better to move the NS_STATE_SVG_NONDISPLAY_CHILD define to nsIFrame.h and add it to the state bits of _all_ frames when it's set on the parent here:

https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?rev=04d8c309fe72#517

That should also considerably simplify the code in nsSVGTextFrame2::ScheduleReflowSVGNonDisplayText. Can you do that?

(Having NS_STATE_SVG_NONDISPLAY_CHILD set on all frames will also allow us to simplify other code in future and easily add some optimizations.)

Comment 16

[Cameron McCormack (:heycam)]

Currently nsSVGOuterSVGFrame does not inherit NS_STATE_SVG_NONDISPLAY_CHILD from its parent.  Is there a reason for this?

Comment 17

[Robert Longson [:longsonr]]

Sounds like a bug. Most often an outer frame's parent is null or html so it's only when its outer frame is a foreignObject that it could inherit. You'd have to be careful that you tested for a html frame and ignoreed those.

Comment 18

[Cameron McCormack (:heycam)]

Attachment: make NS_STATE_SVG_NONDISPLAY_CHILD a global state bit and rename it

Comment 19

[Cameron McCormack (:heycam)]

Attachment: inherit NS_FRAME_IS_NONDISPLAY by default

Comment 20

[Cameron McCormack (:heycam)]

Attachment: reflow SVG text inserted into non-display containers (v1.1)

nsSVGTextFrame2::ScheduleReflowSVGNonDisplayText is now simplified.

Comment 21

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #17)
> Sounds like a bug. Most often an outer frame's parent is null or html so
> it's only when its outer frame is a foreignObject that it could inherit.
> You'd have to be careful that you tested for a html frame and ignoreed those.

Yeah, that's what I was doing before in:

  https://hg.mozilla.org/mozilla-central/file/tip/layout/svg/nsSVGOuterSVGFrame.cpp#l171

That's removed in this patch series, now that we propagate that state bit on non-SVG frames.

Comment 22

[Cameron McCormack (:heycam)]

Green try run: https://tbpl.mozilla.org/?tree=Try&rev=3d1a8a22a9dc

Comment 23

[Jonathan Watt [:jwatt]]

Comment on attachment 773817 [details] [diff] [review]
make NS_STATE_SVG_NONDISPLAY_CHILD a global state bit and rename it

Review of attachment 773817 [details] [diff] [review]:
-----------------------------------------------------------------

Lovely.

::: layout/generic/nsIFrame.h
@@ +321,5 @@
>  // rect stored to invalidate.
>  #define NS_FRAME_HAS_INVALID_RECT                   NS_FRAME_STATE_BIT(52)
>  
> +// Frame is within a subtree that is not displayed due to being under an
> +// SVG <defs> element or resource element (<mask>, <pattern>, etc.)

Maybe:

// Frame is not displayed directly due to it being - or being under - an SVG
// <defs> element or an SVG resource element (<mask>, <pattern>, etc.)

::: layout/svg/nsSVGUtils.h
@@ +66,2 @@
>  // If this bit is set, we are a <clipPath> element or descendant.
> +#define NS_STATE_SVG_CLIPPATH_CHILD              NS_FRAME_STATE_BIT(21)

I'd have probably put a comment after the last define noting that there's an empty slot at #21 rather than change blame. (I actually have another semi written patch that would fill it.) Not a big deal though.

Comment 24

[Jonathan Watt [:jwatt]]

Comment on attachment 773818 [details] [diff] [review]
inherit NS_FRAME_IS_NONDISPLAY by default

Review of attachment 773818 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/nsSVGOuterSVGFrame.cpp
@@ +165,5 @@
>    // frame will be recreated via an nsChangeHint_ReconstructFrame restyle if
>    // the value returned by PassesConditionalProcessingTests changes.
>    SVGSVGElement *svg = static_cast<SVGSVGElement*>(aContent);
> +  if ((aParent->GetStateBits() & NS_FRAME_IS_NONDISPLAY) ||
> +      !svg->PassesConditionalProcessingTests()) {

The nsSVGOuterSVGFrameBase::Init() call below is going to take care of adding the bit if the parent has it, so just have the PassesConditionalProcessingTests check here (or add a comment noting why it's needed).

@@ -175,5 @@
> -    // as copying a state bit from the parent, since non-SVG frames do
> -    // not use NS_FRAME_IS_NONDISPLAY.
> -    for (nsIFrame* f = aParent; f; f = f->GetParent()) {
> -      if (f->IsFrameOfType(eSVG)) {
> -        AddStateBits(f->GetStateBits() & NS_FRAME_IS_NONDISPLAY);

Nice to kill off. :)

Comment 25

[Jonathan Watt [:jwatt]]

Comment on attachment 773819 [details] [diff] [review]
reflow SVG text inserted into non-display containers (v1.1)

Review of attachment 773819 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/nsSVGTextFrame2.cpp
@@ +3145,5 @@
> +  // on that will cause the document to be marked as needing relayout,
> +  // and for that ancestor (or some further ancestor) to be marked as
> +  // a root to reflow.  We choose the closest ancestor frame that is not
> +  // NS_FRAME_IS_NONDISPLAY and which is either an outer SVG frame or a
> +  // non-SVG frame.

It's probably worth noting that the reason that we go up to an outer SVG frame is because calling FrameNeedsReflow on other SVG frames would do a bunch of unnecessary work on the SVG frames up to the nsSVGOuterSVGFrame.

@@ +3157,4 @@
>          return;
>        }
> +      if ((f->GetStateBits() & NS_STATE_IS_OUTER_SVG) ||
> +          !f->IsFrameOfType(eSVG)) {

This happens to be okay, but in general the eSVG check should come before the NS_STATE_IS_OUTER_SVG check since multiple bit flags are set to NS_FRAME_STATE_BIT(20).

Comment 26

[Jonathan Watt [:jwatt]]

Thanks for doing that, and thanks for nicely splitting up the changes into separate patches!

Comment 27

[Cameron McCormack (:heycam)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/44cf1e213d06
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dd488e0cc1fd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/403b12180519
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8d715a147cd8

Thanks for the reviews!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/44cf1e213d06
https://hg.mozilla.org/mozilla-central/rev/dd488e0cc1fd
https://hg.mozilla.org/mozilla-central/rev/403b12180519
https://hg.mozilla.org/mozilla-central/rev/8d715a147cd8