Closed Bug 1072758 Opened 5 years ago Closed 8 months ago

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

Categories

(Core :: SVG, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: jruderman, Assigned: violet.bugreport)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(3 files)

Attached image testcase
Assertion failure: false (should have already reflowed the anonymous block child), at layout/svg/SVGTextFrame.cpp:432

This reminds me of bug 890783.
Attached file stack

The cause is here: https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1062

For a non-display element like mask, we rely on the the DidSetComputedStyle() hook to notify the closest ancestor that is not a non-display to schedule a reflow. This works for normal style change, but doesn't work when the style changes to "display:none". Because then the frame is gone and won't receive a SetComputedStyle from RestyleManager, thus DidSetComputedStyle won't be called.

So we will miss the opportunity to schedule a reflow when the style changes to "display:none", leaving the anon block dirty.

Do you know how to hook the style change to "display:none"? If such a hook exists, we can easily fix this bug by calling ScheduleReflowSVGNonDisplayText there.

Flags: needinfo?(longsonr)

I don't know, you'd want to ask a style system expert like heycam (or dbaron).

Flags: needinfo?(longsonr) → needinfo?(cam)

Hi Emilio,

I have a question about the style/layout system. The situation is that when a frame has changed style, we need to notify some of its ancestor to schedule a reflow. nsIFrame::DidSetComputedStyle() works well in most of the case, but it doesn't work when style is changed to "display:none". Do you know how to get notified about a style change even if it changes to "display:none"?

For more context, please read the commend#2 in this thread.

Flags: needinfo?(emilio)

I guess the problem you're describing is also a problem when the <text> element is removed from the DOM, right?

The right way to do it is probably overriding DestroyFrom. Does that help?

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

I guess the problem you're describing is also a problem when the <text> element is removed from the DOM, right?

The right way to do it is probably overriding DestroyFrom. Does that help?

Thanks for the pointers. The handling for removal is actually fine, after some debugging I find it has registered an MutationObserver to listen ContentRemoved event. So I think probably the bug can be fixed by listening on AttributeChanged, then check the new style, if it becomes "display:none" then to something.

No, that doesn't work, you can change the style by changing any attribute, and sometimes without attribute changes, like :hover.

DidSetComputedStyle won't be called if the style changes to "display:none".
To ensure the reflow is properly scheduled, we need to also hook DestroyFrom.

Assignee: nobody → violet.bugreport
Keywords: checkin-needed

Receiving this while attempting to land:
"We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmptn1a_g\npatching file layout/generic/nsFrame.cpp\nHunk #3 FAILED at 1050\n1 out of 3 hunks FAILED -- saving rejects to file layout/generic/nsFrame.cpp.rej\nabort: patch failed to apply', '')"

Flags: needinfo?(violet.bugreport)

I've manually rebased the patch to current trunk.

Flags: needinfo?(violet.bugreport)
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2717903072b6
ScheduleReflowSVGNonDisplayText when changing style to display none r=emilio
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Flags: in-testsuite+
Flags: needinfo?(cam)
You need to log in before you can comment on or make changes to this bug.