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

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: jruderman, Assigned: jwatt)

Tracking

({assertion, testcase})

Trunk
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
Created attachment 265378 [details]
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
#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()...
Flags: blocking1.9?
Also trigger "ASSERTION: Unknown value for meetOrSlice" now.
(Assignee)

Updated

12 years ago
Duplicate of this bug: 383296
(Assignee)

Comment 4

12 years ago
Note bug 383296 comment 1.
Assignee: general → jwatt
OS: Mac OS X → All
Hardware: PC → All
(Assignee)

Comment 5

12 years ago
Created attachment 269386 [details] [diff] [review]
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.
Attachment #269386 - Flags: superreview?(tor)
Attachment #269386 - Flags: review?(dbaron)
(Assignee)

Comment 6

12 years ago
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.
Attachment #269386 - Flags: superreview?(tor)
Attachment #269386 - Flags: review?(dbaron)
(Assignee)

Comment 7

12 years ago
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?
(Assignee)

Comment 8

12 years ago
Created attachment 269473 [details] [diff] [review]
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).
Attachment #269386 - Attachment is obsolete: true
Attachment #269473 - Flags: review?(dbaron)
(Assignee)

Comment 9

12 years ago
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.
(Assignee)

Comment 10

12 years ago
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?)
(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.
(Assignee)

Comment 12

12 years ago
> (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?
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.
> 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...
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 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?
Attachment #269473 - Flags: review?(dbaron) → review+

Comment 17

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

Updated

11 years ago
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
(Assignee)

Comment 18

11 years ago
I have a patch to implement comment 15. It needs a little more work though.
(Assignee)

Comment 19

11 years ago
Created attachment 289119 [details] [diff] [review]
patch using the register approach
Attachment #289119 - Flags: review?(roc)
(Assignee)

Updated

11 years ago
Attachment #269473 - Attachment description: patch to reflow synchronously if the presshell is handling reflow events → patch to reflow synchronously if the presshell is handling reflow events [not used]
Attachment #269473 - Attachment is obsolete: true
+  // 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.
(Assignee)

Comment 21

11 years ago
Created attachment 289250 [details] [diff] [review]
patch now using a hash-set
Attachment #289119 - Attachment is obsolete: true
Attachment #289250 - Flags: review?(roc)
Attachment #289119 - Flags: review?(roc)
(Assignee)

Comment 22

11 years ago
Oh! I had to move the MaybeReflowFromOuterSVGFrame call to DidReflow instead of Reflow to fix one of my reftests.
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.
Attachment #289250 - Flags: superreview+
Attachment #289250 - Flags: review?(roc)
Attachment #289250 - Flags: review+
(Assignee)

Updated

11 years ago
Attachment #289250 - Flags: approval1.9?
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/
(Assignee)

Comment 25

11 years ago
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.
I guess you're right on the first two points then.

Updated

11 years ago
Attachment #289250 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 27

11 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Comment 28

11 years ago
Created attachment 289484 [details] [diff] [review]
patch as checked in (with new reftest)
(Assignee)

Updated

11 years ago
Blocks: 389198
You need to log in before you can comment on or make changes to this bug.