381285 - 323656-5-ref.svg triggers "ASSERTION: can't mark frame dirty during reflow"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: a copy of the reftest that triggers this assertion

Loading layout/reftests/bugs/323656-5-ref.svg triggers:

###!!! ASSERTION: can't mark frame dirty during reflow: '!mIsReflowing', file /Users/jruderman/trunk/mozilla/layout/base/nsPresShell.cpp, line 3069

Comment 1

[Boris Zbarsky [:bzbarsky]]

#4  0x17efab0c in PresShell::FrameNeedsReflow (this=0x243b400, aFrame=0x2455aa8, aIntrinsicDirty=eResize, aBitToAdd=1024) at ../../../mozilla/layout/base/nsPresShell.cpp:3069
#5  0x18403db6 in nsSVGForeignObjectFrame::RequestReflow (this=0x2455934, aType=eResize) at ../../../../../mozilla/layout/svg/base/src/nsSVGForeignObjectFrame.cpp:497
#6  0x18405173 in nsSVGForeignObjectFrame::NotifyCanvasTMChanged (this=0x2455934, suppressInvalidation=0) at ../../../../../mozilla/layout/svg/base/src/nsSVGForeignObjectFrame.cpp:362
#7  0x183f73ba in nsSVGOuterSVGFrame::NotifyViewportChange (this=0x2455678) at ../../../../../mozilla/layout/svg/base/src/nsSVGOuterSVGFrame.cpp:589
#8  0x183f6ff6 in nsSVGOuterSVGFrame::Reflow (this=0x2455678, aPresContext=0x3fa89720, aDesiredSize=@0xbfffb804, aReflowState=@0xbfffb758, aStatus=@0xbfffba00) at ../../../../../mozilla/layout/svg/base/src/nsSVGOuterSVGFrame.cpp:293

That's just not a good idea at all.  This should happen as a post-reflow callback or something.  Or maybe this stuff should participate in Reflow()...

Comment 2

[Martijn Wargers (dead)]

Also trigger "ASSERTION: Unknown value for meetOrSlice" now.

Comment 4

[Jonathan Watt [:jwatt]]

Note bug 383296 comment 1.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

I'm really not sure how to make this stuff participate in reflow, so here's a patch to make us reflow off a post-reflow callback when necessary.

Comment 6

[Jonathan Watt [:jwatt]]

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

Yikes. Well this gets rid of the assertion, but the deferred reflow never actually happens. One weird thing is that MSVC 2005 claims mDeferredReflow doesn't exist (CXX0017: Error: symbol "mDeferredReflow" not found). Just as annoyingly DidReflow is never called on nsSVGForeignObjectFrame, perhaps because it is a reflow root? I'm not sure where else I can hook in a callback.

Comment 7

[Jonathan Watt [:jwatt]]

dbaron: if nsPresShell::IsReflowLocked returns true, should we just do a synchronous reflow by calling DoReflow rather than an asynchronous reflow by calling nsPresShell::FrameNeedsReflow?

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: patch to reflow synchronously if the presshell is handling reflow events [not used]

This seems to work (the reftests pass and the assertion is gone).

Comment 9

[Jonathan Watt [:jwatt]]

This way MarkIntrinsicWidthsDirty isn't called on the foreignObject's descendants (or the other magic done in nsPresShell::FrameNeedsReflow. I'm not sure how much that matters in the general case.

Comment 10

[Jonathan Watt [:jwatt]]

Hmm, and maybe we should only do the synchronous reflow if the foreignObject isn't already marked for reflow. Will bad things happen if a reflow root is reflowed before the presshell gets to it? (Will it get confused if it finds the reflow root is (no longer) marked as dirty?)

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #9)
> This way MarkIntrinsicWidthsDirty isn't called on the foreignObject's
> descendants (or the other magic done in nsPresShell::FrameNeedsReflow. I'm not
> sure how much that matters in the general case.

This seems like a problem if aType is anything other than eResize. (But in this case eTreeChange doesn't really make sense, so it's just eResize vs. eStyleChange.)

(In reply to comment #10)
> Will bad things happen if a reflow root is
> reflowed before the presshell gets to it? (Will it get confused if it finds the
> reflow root is (no longer) marked as dirty?)

That shouldn't be a problem.

Comment 12

[Jonathan Watt [:jwatt]]

> (But in this case eTreeChange doesn't really make sense, so
> it's just eResize vs. eStyleChange.)

dbaron: this is one thing I still can't make sense of. Changes to a foreignObject or its descendants should never affect its ancestors intrinsic widths (the size of its parent <svg> is independent of its contents), so I don't understand why we'd ever want to use eStyleChange. In fact, I don't understand why changing the width of a foreignObject should even require the intrinsic widths of its descendants to be recalculated. The computed widths, yes, but the intrinsic widths? Can you shed any light on this?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Foreign objects are reflow roots, so the style change shouldn't affect ancestors.  (Although we should probably make the outer SVG frame a reflow root too, since a style change to the foreignObject itself would propagate up.)

But if you change, say, the 'font-size' property on a foreignObject, you definitely need to recompute intrinsic widths on descendants.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I'm really not sure how to make this stuff participate in reflow

Frankly, the simplest way would be to implement Reflow() on all SVG frames and have it do nothing for most of them other than check dirty bits and reflow kids as needed if there are dirty kids.  Then have the foreign object frame's Reflow() method reflow the foreign content as a reflow root (you can basically copy the code from nsPreShell::DoReflow, I bet).

I think that would work...

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Rather than comment 14, I'd suggest making the foreign objects register themselves with their containing outer SVG frame, and having it reflow them directly.  Foreign objects are rare relative to the number of SVG frames.

In reply to comment 12, really, we could probably set the reflow root bit on *all* SVG frames, and thus the containing of a foreign object would always be a reflow root as well, which would suppress the style change propagating outwards.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 269473 [details] [diff] [review]
patch to reflow synchronously if the presshell is handling reflow events [not used]

That said, I think this patch is ok, so r=dbaron, but could you get a bug filed on doing comment 15 or such?

Comment 17

[jag (Peter Annema)]

Just a note, this patch was checked in and backed out (well, commented out, really) to fix reftest failures.

Comment 18

[Jonathan Watt [:jwatt]]

I have a patch to implement comment 15. It needs a little more work though.

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: patch using the register approach

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  // If our width/height have a percentage value then we need to reflow if the
+  // width/height of our parent coordinate context changes. Perhaps
+  // unexpectedly we also need to reflow if our CTM changes. This is because
+  // glyph metrics do not necessarily scale uniformly with change in scale and,
+  // as a result, CTM changes may require text to break at different points.

This is actually bad. We shouldn't reflow on CTM changes. We should just set our text glyph positions based on an identity CTM. For example, it would be silly to be rotating a foreignobject and observe changes in line breaking. That is not something to fix here, but change the comment. (And requesting a resize reflow will not actually recompute the glyph positions, FWIW.)

Storing the foreignobject frames in an array means that you will get O(N^2) behaviour tearing down an SVG frame subtree with N foreignobjects in it. It would make more sense to use a hash-set here.

Other than that this looks OK.

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: patch now using a hash-set

Comment 22

[Jonathan Watt [:jwatt]]

Oh! I had to move the MaybeReflowFromOuterSVGFrame call to DidReflow instead of Reflow to fix one of my reftests.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 289250 [details] [diff] [review]
patch now using a hash-set

+ReflowForeignObjects(nsVoidPtrHashKey *aEntry, void* aUserArg)

This should be "ReflowForeignObject" (singular)

+    // Now that all viewport establishing descendants have their correct size,
+    // tell our foreignObject descendants to reflow their children.
+    if (mForeignObjectHash.Count() > 0) {

I think this check is a bit of a pointless optimization. Just remove it.

Comment 24

[Robert Longson [:longsonr]]

I wonder if you could make MaybeReflowFromOuterSVGFrame const and thereby remove the const cast in ReflowForeignObjects?

You could make the Register and Unregister methods protected and nsSVGForeignObjectFrame a friend of the nsSVGOuterSVGFrame then you might have fewer worries with "who on earth is calling us"

+   * unregister themselves with their nearest nsSVGOuterSVGFrame anscester so

s/ancester/ancestor/

Comment 25

[Jonathan Watt [:jwatt]]

MaybeReflowFromOuterSVGFrame touches mState, so making it const would be a lie. Even if it didn't, the const_cast would still be necessary since the type of nsVoidPtrHashKey::KeyType is |const void*|.

I wouldn't be in favor of using friend here since nsSVGForeignObjectFrame only needs very limited access to nsSVGOuterSVGFrame, and friend makes it more difficult to reason about what it might be accessing. (I'm in favor of avoiding friend in general.) I think the chances of anyone writing code that calls those methods incorrectly is just about zero. I just threw in the assertions for bonus points.

Thanks for the spelling correction. I've fixed that locally and it'll go in when I commit.

Comment 26

[Robert Longson [:longsonr]]

I guess you're right on the first two points then.

Comment 27

[Jonathan Watt [:jwatt]]

Checked in.

I also added the reftest layout/reftests/svg/foreignObject-ancestor-style-change-01.svg which is the test that was failing when the MaybeReflowFromOuterSVGFrame call was in Reflow instead of DidReflow.

Comment 28

[Jonathan Watt [:jwatt]]

Attachment: patch as checked in (with new reftest)