895311 - "Assertion failure: false (should have already reflowed the anonymous block child)" with overflow

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: false (should have already reflowed the anonymous block child), at /Users/jruderman/trees/mozilla-central/layout/svg/nsSVGTextFrame2.cpp:429

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Cameron McCormack (:heycam)]

The change from bug 890783 must not be quite right.  The style change causes the inline frame for the <tspan> to be recreated, but we do need to get it to be reflowed.

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #2)
> The change from bug 890783 must not be quite right.

The change from bug 893510 that is.

Comment 4

[Robert Longson [:longsonr]]

I don't think first_reflow is ever cleared from non-display frames.

Comment 5

[Cameron McCormack (:heycam)]

I think what I really want to know is whether we are currently reflowing the children of the nsSVGTextFrame2.  I have a patch that I think works, but I'll attach it in the morning.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: patch

OK.  What I want to achieve here is that when a frame is being given a new style context during reflow -- which is going to be when the reflow causes a new frame to be created -- we want to avoid calling ScheduleReflowSVGNonDisplayText, since we're reflowing now anyway.

I had thought that I could check the anonymous block frame's NS_FRAME_IN_REFLOW bit, but it turns out that for layout/svg/crashtests/893510-1.svg, the frame gets created during the GetPrefWidth() call made in nsSVGTextFrame2::DoReflow.  That's when it does the bidi resolution.  At that point, we haven't called WillReflow yet, and I'm not sure how safe it is to move that call earlier.  So it seems neater to just record the fact that we're in the reflow-y bit of DoReflow directly with a state bit on the nsSVGTextFrame2.

The alternative I was thinking of was to call nsSVGUtils::OuterSVGIsCallingReflowSVG from ScheduleReflowSVGNonDisplayText and return early if that's true, but that needs a bunch of traversing up the tree which we can avoid with the state bit approach.

Comment 7

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]

Comment 8

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=58fdb21a6cd4

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 778267 [details] [diff] [review]
patch

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

::: layout/generic/nsFrame.cpp
@@ +697,5 @@
>  // Subclass hook for style post processing
>  /* virtual */ void
>  nsFrame::DidSetStyleContext(nsStyleContext* aOldStyleContext)
>  {
> +  if (IsSVGText()) {

Can we keep the NS_FRAME_FIRST_REFLOW check, but check that bit on the nsSVGTextFrame2's anon block rather than on this frame? Avoiding unnecessary ScheduleReflowSVGNonDisplayText calls during document load would be nice.

I'm not sure if that would then mean that the NS_STATE_SVG_TEXT_IN_REFLOW bit is no longer needed or not. It seems like it would solve the 893510-1.svg case that you're introducing it for though.

Comment 10

[Cameron McCormack (:heycam)]

Indeed, you're right.  I don't think we need NS_STATE_SVG_TEXT_IN_REFLOW then.

Comment 11

[Cameron McCormack (:heycam)]

Attachment: patch (v1.1)

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 780117 [details] [diff] [review]
patch (v1.1)

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

::: layout/generic/nsFrame.cpp
@@ +702,3 @@
>      nsSVGTextFrame2* svgTextFrame = static_cast<nsSVGTextFrame2*>(
>          nsLayoutUtils::GetClosestFrameOfType(this, nsGkAtoms::svgTextFrame2));
> +    nsIFrame* kid = svgTextFrame->GetFirstPrincipalChild();

Can s/kid/anonBlock/?

@@ +705,4 @@
>      // Just as in nsSVGTextFrame2::DidSetStyleContext, we need to ensure that
>      // any non-display nsSVGTextFrame2s get reflowed when a child text frame
> +    // gets new style.  We don't need to do this when the frame has not yet
> +    // been reflowed, since that will happen soon anyway.

Can you also tack on something like:

    // Note that we must check NS_FRAME_FIRST_REFLOW on our nsSVGTextFrame2's
    // anonymous block frame rather than our self, since NS_FRAME_FIRST_REFLOW
    // may be set on us if we're a new frame that has been inserted after the
    // document's first reflow. (In which case this DidSetStyleContext call may
    // be happening under frame construction under a Reflow() call.)

Comment 13

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18f56188b6b4

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/18f56188b6b4