615146 - "ASSERTION: prev sibling not in line list" with <svg requiredExtensions>

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

###!!! ASSERTION: prev sibling not in line list: 'Not Reached', file layout/generic/nsBlockFrame.cpp, line 4811

Crash [@ nsRect::nsRect]

Related to bug 615012?

Comment 1

[Jesse Ruderman]

Attachment: stack traces (assertion and crash)

Comment 2

[Timothy Nikkel (:tnikkel)]

Bisected this
The first bad revision is:
changeset:   58309:bb43b6a9b621
user:        Cameron McCormack <[e-mail]>
date:        Fri Nov 26 16:49:08 2010 +1300
summary:     Bug 614265 - Make paint server references to elements within an unknown SVG element subtree work again. r=longsonr a=b

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Ensure outer SVG elements get an nsSVGOuterSVGFrame even they fail conditional processing attributes

The crash was because the bug 614265 patch made outer <svg> elements with
failing conditional processing attribtues get a plain nsSVGContainerFrame
and nsSVGUtils::GetOuterSVGFrame assumes that it is always an
nsSVGOuterSVGFrame (which nsSVGForeignObjectFrame::Init tripped up
on).

This patch makes such failing outer <svg> elements get an
nsSVGOuterSVGFrame again, but handles their non-rendering of children in the
same way that nsSVGSwitchFrame does -- i.e., by overriding a bunch of methods
from nsISVGChildFrame.  (I borrowed that approach without understanding
really why there are calls to UpdateGraphic in a couple of the methods, so
please verify that what I've done is sane.)

The other approach I consdered, making all nsSVGUtils::GetOuterSVGFrame call
sites be aware that an nsSVGContainerFrame might instead be returned, seemed
too invasive for what is probably going to be a case that comes up rarely
anyway.  (Although I noticed that some caller of GetOuterSVGFrame check for
null, while others, like in nsSVGForeignObjectFrame, don't.)

Comment 4

[Robert Longson [:longsonr]]

Comment on attachment 494283 [details] [diff] [review]
Ensure outer SVG elements get an nsSVGOuterSVGFrame even they fail conditional processing attributes

What about dynamic changes? Init is only called on construction. If I do a setAttribute and make the conditions pass when they failed earlier or vice versa then mIsFailingConditionalAttributes would need to be recalculated. I think you may need something in nsSVGOuterSVGFrame::AttributeChanged

Secondly you should use the property NS_STATE_NONDISPLAY_CHILD rather than have a mIsFailingConditionalAttributes boolean. If you do this you won't need all those overrides (though you will need some of them) as many of the overridden methods have logic that checks for NONDISPLAY_CHILD already. Some others that need NONDISPLAY_CHILD handling should probably be handled without overriding anyway by changing the overridden method itself to cope with NONDISPLAY_CHILD if necessary, although those that are special like GetFrameForPoint will need to stay with your extra logic.

Thirdly, no else after return in nsCSSFrameConstructor.cpp

Comment 5

[Robert Longson [:longsonr]]

(In reply to comment #3)
> 
> This patch makes such failing outer <svg> elements get an
> nsSVGOuterSVGFrame again, but handles their non-rendering of children in the
> same way that nsSVGSwitchFrame does -- i.e., by overriding a bunch of methods
> from nsISVGChildFrame.  (I borrowed that approach without understanding
> really why there are calls to UpdateGraphic in a couple of the methods, so
> please verify that what I've done is sane.)

If you don't need the method coming from nsSVGIChildFrame to do anything special then best not to override it at all. That's likely to be most of them once you switch to a NONDISPLAY_CHILD implementation.

> 
> The other approach I consdered, making all nsSVGUtils::GetOuterSVGFrame call
> sites be aware that an nsSVGContainerFrame might instead be returned, seemed
> too invasive for what is probably going to be a case that comes up rarely
> anyway.  (Although I noticed that some caller of GetOuterSVGFrame check for
> null, while others, like in nsSVGForeignObjectFrame, don't.)

You chose the right approach IMHO.

Comment 6

[Robert Longson [:longsonr]]

I was thinking more about this.

Dynamic changes will work with your patch as-is as the frames get reconstructed whenever the conditional processing attributes change.

So we've two choices.

a) you could fix the outersvgframe to be always the right type and non-display or not.
b) we could revert to failing conditional processing having no frame and therefore not working as a paint server for FX4 and fix this all up post FX4.

Post FX4 the correct way to do this would be to always create the right frame type and whatever frame we have to make it non-display or not depending on whether the conditional processing passes or not. That's way too risky for FX4 though.

Comment 7

[Cameron McCormack (:heycam)]

Yeah I noticed dynamic changes were working, but couldn't understand why.  In fact I'm not sure a new frame actually gets constructed for the outer svg frame when twiddling one of the conditional processing attributes: I see there's something in nsSVGElement::GetAttributeChangeHint but I didn't see that acted upon.  (I saw that the <rect> child's frame was reconstructed, but didn't see FindSVGData called again for the <svg>.)  So I'm not sure why it is working. :)

So (a) doesn't seem far away: I just need to make it work by using NONDISPLAY_CHILD rather than overriding a bunch of things, right?  Is your assessment that that is too risky, or just that the more extensive "make all elements only ever get the one kind of frame, non-display or not" one is?

Comment 8

[Robert Longson [:longsonr]]

Just the more extensive all frames right type but non-display is too risky.

Comment 9

[Cameron McCormack (:heycam)]

(In reply to comment #4)
> Secondly you should use the property NS_STATE_NONDISPLAY_CHILD rather than have
> a mIsFailingConditionalAttributes boolean. If you do this you won't need all
> those overrides (though you will need some of them) as many of the overridden
> methods have logic that checks for NONDISPLAY_CHILD already. Some others that
> need NONDISPLAY_CHILD handling should probably be handled without overriding
> anyway by changing the overridden method itself to cope with NONDISPLAY_CHILD
> if necessary, although those that are special like GetFrameForPoint will need
> to stay with your extra logic.

In looking into exactly which of the nsISVGChildFrame methods need to be overridden, I'm coming to the conclusion that the answer is "none", given the fact that the frame will be recreated when the conditional processing attributes change and that the inherited ones have the NONDISPLAY_CHILD logic.

I am not even sure that GetFrameForPoint (which exists currently on nsSVGOuterSVGFrame) needs to be there, either; I can't find when it would ever be called -- GetFrameForPoint only gets called from nsSVGClipPathFrame::ClipHitTest and nsSVGUtils::HitTestChildren, and both of those only call GetFrameForPoint on frames that are children of an SVG element frame, which thus could never be an nsSVGOuterSVGFrame.

The only issue I've found with using NONDISPLAY_CHILD is that nsSVGPathGeometryFrame::GetFrameForPoint uses it as an optimization to check whether it is being called on a <clipPath> child.  Would it problematic to change this back to using the slower IsClipChild?  Should we track whether the element is inside a clipPath another way?

Comment 10

[Robert Longson [:longsonr]]

(In reply to comment #9)
> (In reply to comment #4)
> 
> In looking into exactly which of the nsISVGChildFrame methods need to be
> overridden, I'm coming to the conclusion that the answer is "none", given the
> fact that the frame will be recreated when the conditional processing
> attributes change and that the inherited ones have the NONDISPLAY_CHILD logic.

That's pretty much what I expected ;-)

> 
> I am not even sure that GetFrameForPoint (which exists currently on
> nsSVGOuterSVGFrame) needs to be there, either; I can't find when it would ever
> be called -- GetFrameForPoint only gets called from
> nsSVGClipPathFrame::ClipHitTest and nsSVGUtils::HitTestChildren, and both of
> those only call GetFrameForPoint on frames that are children of an SVG element
> frame, which thus could never be an nsSVGOuterSVGFrame.

It's called whenever you mouse move or click on the document. I think it gets called from the root down so it's up to the hierarchy of frames to exclude themselves if they don't cover the point. It's used for <title> popups, mousein, mouseout events etc.

> 
> The only issue I've found with using NONDISPLAY_CHILD is that
> nsSVGPathGeometryFrame::GetFrameForPoint uses it as an optimization to check
> whether it is being called on a <clipPath> child.  Would it problematic to
> change this back to using the slower IsClipChild?  Should we track whether the
> element is inside a clipPath another way?

You could create a new CLIPCHILD_PROPERTY and initialise it on the ::Init() method of the frame similar to how NONDISPLAY_CHILD works. Since frames are torn down and recreated whenever the hierarchy changes you wouldn't even have to keep it dynamically up to date - if it's true initially it will be true throughout the life of the frame.

Comment 11

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

(In reply to comment #10)
> > I am not even sure that GetFrameForPoint (which exists currently on
> > nsSVGOuterSVGFrame) needs to be there, either; I can't find when it would ever
> > be called -- GetFrameForPoint only gets called from
> > nsSVGClipPathFrame::ClipHitTest and nsSVGUtils::HitTestChildren, and both of
> > those only call GetFrameForPoint on frames that are children of an SVG element
> > frame, which thus could never be an nsSVGOuterSVGFrame.
> 
> It's called whenever you mouse move or click on the document. I think it gets
> called from the root down so it's up to the hierarchy of frames to exclude
> themselves if they don't cover the point. It's used for <title> popups,
> mousein, mouseout events etc.

GetFrameForPoint is SVG-only now. I think Cameron's right.

Comment 12

[Cameron McCormack (:heycam)]

Attachment: Ensure outer SVG elements get an nsSVGOuterSVGFrame even they fail conditional processing attributes (v2) r=longsonr

(In reply to comment #10)
> You could create a new CLIPCHILD_PROPERTY and initialise it on the ::Init()
> method of the frame similar to how NONDISPLAY_CHILD works. Since frames are
> torn down and recreated whenever the hierarchy changes you wouldn't even have
> to keep it dynamically up to date - if it's true initially it will be true
> throughout the life of the frame.

Great, I preemptively tried this on last night and it seems to work.

(In reply to comment #11)
> GetFrameForPoint is SVG-only now. I think Cameron's right.

OK, in the patch I've dropped the existing nsSVGOuterSVGFrame::GetFrameFromPoint.

Comment 13

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/395fac6a7de4