299351 - Check SVG frames ::Init for error exit

Status: RESOLVED WORKSFORME

(Core :: SVG, defect)

Description

[tor]

If SVG frame construction fails, it needs to make sure the frame has at least
the style context set before returning, or bad things happen (as in bug 296626).

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Seems like nsCSSFrameConstructor::InitAndRestoreFrame returns the rv from the SVG frame ctors to nsCSSFrameConstructor::ConstructFrameFromItemInternal, which expects construction for all frame types to succeed. Anyway, regardless of that point this bug seems to be about making sure the SVG frame ctors do not return an error code without calling the base class ctors. I've audited the code and in fact that is the case. In fact none of the SVG frame ctor methods return their own error code now.

This patch makes sure we always return any error code from the base class ctors. As mentioned in bug 296626, the subclass ctors sholdn't be touching the frame until the superclass ctors have completed. For bonus points I've changed that to be strictly true, although setting flags early is in reality probably always going to be harmless.

Comment 2

[Daniel Holbert [:dholbert]]

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

Looks good, just a few nits:

> nsSVGLeafFrame::Init(nsIContent* aContent, nsIFrame* aParent, nsIFrame* asPrevInFlow)
> {
>-  nsFrame::Init(aContent, aParent, asPrevInFlow);
>-  nsCOMPtr<nsIImageLoadingContent> imageLoader =
>-    do_QueryInterface(nsFrame::mContent);
>+  nsresult rv = nsFrame::Init(aContent, aParent, asPrevInFlow);
>+  if (NS_SUCCEEDED(rv)) {
>+    nsCOMPtr<nsIImageLoadingContent> imageLoader =
>+      do_QueryInterface(nsFrame::mContent);
> 
>-  if (imageLoader) {
>-    imageLoader->FrameCreated(this);
>+    if (imageLoader) {
>+      imageLoader->FrameCreated(this);
>+    }
>   }
>-
>-  return NS_OK;
>+  return rv;

IMHO this would be cleaner (saving both on indentation & hg-blame) as:
  nsresult rv = nsFrame::Init(aContent, aParent, asPrevInFlow);
  if (NS_FAILED(rv)) {
    return rv;
  }
and then the rest of the method can stay as it originally was.

I won't hold you to that, though.

Also, is there any reason we need "nsFrame::mContent" there instead of just "mContent"?  As long as you're touch this section, it might be nice to nix the unnecessary (I think?) nsFrame:: prefix there.

>diff --git a/layout/svg/base/src/nsSVGOuterSVGFrame.cpp b/layout/svg/base/src/nsSVGOuterSVGFrame.cpp
>--- a/layout/svg/base/src/nsSVGOuterSVGFrame.cpp
>+++ b/layout/svg/base/src/nsSVGOuterSVGFrame.cpp
>@@ -156,28 +156,28 @@ nsSVGOuterSVGFrame::Init(nsIContent* aCo
>                          nsIFrame* aParent,
>                          nsIFrame* aPrevInFlow)
> {
> #ifdef DEBUG
>   nsCOMPtr<nsIDOMSVGSVGElement> svgElement = do_QueryInterface(aContent);
>   NS_ASSERTION(svgElement, "Content is not an SVG 'svg' element!");

Might as well turn this assertion into an NS_ABORT_IF_FALSE, like you did with the others at the beginning of this patch.

Comment 3

[Robert Longson [:longsonr]]

NS_IMETHODIMP
 nsSVGGeometryFrame::Init(nsIContent* aContent,
                          nsIFrame* aParent,
                          nsIFrame* aPrevInFlow)
 {
+  nsresult rv = nsSVGGeometryFrameBase::Init(aContent, aParent, aPrevInFlow);
   AddStateBits(aParent->GetStateBits() &
                (NS_STATE_SVG_NONDISPLAY_CHILD | NS_STATE_SVG_CLIPPATH_CHILD));
-  nsresult rv = nsSVGGeometryFrameBase::Init(aContent, aParent, aPrevInFlow);
   return rv;
 }

Calling the base class Init can result in DidSetStyleContext getting called if the element is constructed by javascript. DidSetStyleContext generally causes us to repaint and if you change the code like this we may try to repaint without the NS_STATE_SVG_NONDISPLAY_CHILD bit being properly set for children of patterns which tends to make things crash.

Comment 4

[Jonathan Watt [:jwatt]]

Except that we can't paint during frame construction, right?

Comment 5

[Robert Longson [:longsonr]]

We can, see bug 713413 for a case involving foreignObject. In that case because foreignObject is involved in reflow I caught a break and could test for whether we've done a first reflow. 

nsSVGForeignObjectFrame::Init(nsIContent* aContent,
                              nsIFrame*   aParent,
                              nsIFrame*   aPrevInFlow)
{

  nsresult rv = nsSVGForeignObjectFrameBase::Init(aContent, aParent, aPrevInFlow);
  AddStateBits(aParent->GetStateBits() &
               (NS_STATE_SVG_NONDISPLAY_CHILD | NS_STATE_SVG_CLIPPATH_CHILD));

Is wrong and another, arguably better way to fix bug 713413 would have been to reorder the above code above.

If you change Init to work as it does in foreignObject Jesse will fuzz crash you and you won't be able to do the reflow trick I did.

Comment 6

[Jonathan Watt [:jwatt]]

The stack from bug 713413 is for invalidation, not painting, thankfully. That said, invalidating at that point in time seems like a bad thing to do too.

Also note that we use NS_FRAME_FIRST_REFLOW throughout the SVG frames, not just for fO. (It's added to all frames in the nsFrame ctor, and we clear it in InitialUpdate().)

Comment 7

[Robert Longson [:longsonr]]

We don't clear NS_FRAME_FIRST_REFLOW for frames that are children of <defs> though because we don't call InitialUpdate on them.

Comment 8

[Robert Longson [:longsonr]]

nsIFrame::Init is now void so there's nothing to return any longer. On top of that most of the AddStateBits calls have been moved to constructors.