Closed Bug 895311 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: SVG, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jruderman, Assigned: heycam)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 1 obsolete file)

Attached image testcase
Assertion failure: false (should have already reflowed the anonymous block child), at /Users/jruderman/trees/mozilla-central/layout/svg/nsSVGTextFrame2.cpp:429
Attached file stack
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.
(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.
I don't think first_reflow is ever cleared from non-display frames.
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.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #778267 - Flags: review?(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.
Flags: needinfo?(cam)
Indeed, you're right.  I don't think we need NS_STATE_SVG_TEXT_IN_REFLOW then.
Flags: needinfo?(cam)
Attached patch patch (v1.1)Splinter Review
Attachment #778267 - Attachment is obsolete: true
Attachment #778267 - Flags: review?(jwatt)
Attachment #780117 - Flags: review?(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.)
Attachment #780117 - Flags: review?(jwatt) → review+
https://hg.mozilla.org/mozilla-central/rev/18f56188b6b4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.