308917 - SVG crash [@ nsSVGPatternFrame::GetOuterSVGFrame] [@ nsSVGPathGeometryFrame::GetOuterSVGFrame]

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[Jesse Ruderman]

Attachment: testcase (not reduced)

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050916
Firefox/1.6a1

Crashes when status bar counter says 468.

Comment 2

[Jesse Ruderman]

Attachment: apple crash report with stack trace

Crash report for the testcase in comment 1.

Comment 3

[Jesse Ruderman]

Attachment: crash report for debug build

My debug build crashes earlier, while the status bar counter says 97, and with
a different stack trace.  Might be a different bug.

Comment 4

[Jesse Ruderman]

Attachment: testcase 2 (not reduced)

As I was trying to make a reduced testcase for the original (non-debug) crash,
I made a change that made it crash earlier, and with junk at the top of the
stack (demonstrating that it is likely to be exploitable).  This crashes while
the status bar counter says 393.

Top of the stack:
0   <<00000000>>	0x02356c10 0 + 37055504
1   nsSVGPatternFrame::GetOuterSVGFrame() + 204
2   nsSVGPathGeometryFrame::GetOuterSVGFrame() + 76
3   nsSVGPathGeometryFrame::InitSVG() + 172

Comment 5

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

Worth retesting once bug 306915 is fixed.

Comment 6

[Jesse Ruderman]

Or after bug 308615 is fixed.  I hit that bug a few times while trying to
testcase this.

Comment 7

[Jesse Ruderman]

Bug 306915 is fixed, but both testcases in this bug still crash Firefox as
described.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050922
Firefox/1.6a1

Comment 8

[Jesse Ruderman]

WFM Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b5)
Gecko/20050923 Firefox/1.4

Comment 9

[Jesse Ruderman]

Still crashes in my trunk opt build from a few hours ago.

Comment 10

[Jesse Ruderman]

Attachment: testcase 3 (mostly reduced)

Comment 11

[Scooter Morris]

Attachment: Combined Pattern patch

Ugh.  It took me a long time, but this was just a stupid logic error on my
part.  I was carefully cacheing mSource, but mSource should only be cached for
a particular user of the pattern (a single pattern frame could have multiple
geometric parents).  This patch changes things so that mSource is saved only
during the rendering of a particular pattern.

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 198880 [details] [diff] [review]
Combined Pattern patch

Some initial comments:

> #include "nsContainerFrame.h"
>+#include "nsSVGGenericContainerFrame.h"

Should remove the nsContainerFrame.h include.

>+  nsISVGGeometrySource                   *mSource;

Can you add a comment saying what mSource is used for? Perhaps:
  // temporary reference to the frame of elements that references our pattern

>+  nsISVGOuterSVGFrame *aOuterSVG = nsnull;
>   NS_ASSERTION(mParent, "null parent");
>+  if (mLoopFlag) {
>+    // We have some form of loop.  We need

Can you s/loop/reference loop/

>+    // to remove ourselves from the calling frame's
>+    // observer list or we'll attempt to call ourselves
>+    // again when our frame goes away
>+    NS_WARNING("Pattern Loop detected");
>+    nsCOMPtr<nsISVGValueObserver> aVO = do_QueryInterface(mSource);
>+    nsISVGValue *v;
>+    CallQueryInterface(this, &v);
>+    if (v && aVO)
>+      v->RemoveObserver(aVO);

No need to QI |this|. Simply |if (aVO) RemoveObserver(aVO);| should do the trick.

>+  mSource = aSource;
>+
>   // OK, now render -- note that we use "firstKid", which
>   // we got at the beginning because it takes care of the
>   // referenced pattern situation for us
>   nsRect dummyRect;

Would you mind putting the |mSource = aSource;| here to keep the two assignments to mSource as close together as possible?

>+#ifdef DEBUG_scooter
>+  printf("NS_GetSVGPattern: content not a descendant\n");
>+#endif
>   return result->QueryInterface(NS_GET_IID(nsISVGPattern), (void **)aPattern);
> }

Is that printf all that useful given that you printf on failure?


One other thing. At all the call points for NS_GetSVGPattern can you put an |if (x)| before all the |NS_ADD_SVGVALUE_OBSERVER(x)| to prevent bogus assertions.

I'll have more comments a bit later.

Comment 13

[Scooter Morris]

Attachment: Updated diffs (incoporates jwatt's comments)

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 203804 [details] [diff] [review]
Updated diffs (incoporates jwatt's comments)

Scooter, can you give me a quick summary of where all the changes that weren't part of my review comments came from? Are they all to solve this bug? I'm going to have a look at this again later today.

Comment 15

[Jonathan Watt [:jwatt]]

You seem to have lost the change that moved |mSource = aSource;| further down and then nulled it out when PaintPattern is finished with it.

Comment 16

[Scooter Morris]

Attachment: Updated & cleaned up a little

Comment 17

[Jonathan Watt [:jwatt]]

Okay, so that's more like how GetOuterSVGFrame should ideally be. But now the loop checking stuff is completely bogus. You're just loop checking around a QI, and a loop can't occur anyway since you are going directly up the parent chain in document node order, not referenced order.

Comment 18

[Scooter Morris]

(In reply to comment #17)
> Okay, so that's more like how GetOuterSVGFrame should ideally be. But now the
> loop checking stuff is completely bogus. You're just loop checking around a QI,
> and a loop can't occur anyway since you are going directly up the parent chain
> in document node order, not referenced order.
> 

Right you are!  I'll get rid of all of the loop checking code and provide a new patch.

Comment 19

[Scooter Morris]

Attachment: Cleaned up GetOuterSVGFrame and fixed ownership problem in Cairo

Comment 20

[tor]

You probably want to remove the NEAREST filter setting or adjust to a higher level.

The pattern "swims" when w3c pattern testcase is loaded by itself, and repaints for partial drawing (dragging another window over the browser) are incorrect, which suggest the pattern matrix might not set up correctly.

Comment 21

[tor]

Attachment: locked patterns

Comment 22

[Scooter Morris]

Attachment: Encorporates tor's fixes

Comment 23

[tor]

Comment on attachment 211178 [details] [diff] [review]
Encorporates tor's fixes

>@@ -444,146 +421,133 @@ nsSVGPatternFrame::PaintPattern(nsISVGRe
...
>+  callerSVGFrame->SetMatrixPropagation(PR_TRUE);
>+  callerSVGFrame->NotifyCanvasTMChanged(PR_TRUE);

This isn't necessary, is it?

> cairo_pattern_t *
> CairoPattern(nsISVGRendererCanvas *canvas, nsISVGPattern *aPat,
>-             nsISVGGeometrySource *aSource)
>+             nsISVGGeometrySource *aSource, nsISVGRendererSurface **aSurface)
> {
...
>-  nsCOMPtr<nsISVGRendererSurface> aSurface;
>+  nsCOMPtr<nsISVGRendererSurface> renderSurface;
>   nsCOMPtr<nsIDOMSVGMatrix> pMatrix;
>-  if (NS_FAILED(aPat->PaintPattern(canvas, getter_AddRefs(aSurface), getter_AddRefs(pMatrix), aSource)))
>+  if (NS_FAILED(aPat->PaintPattern(canvas, getter_AddRefs(renderSurface), getter_AddRefs(pMatrix), aSource)))
>     return nsnull;

Instead of adding another nsCOMPtr for the render surface and playing
around with the refcount as you leave the method, why not use the
passed aSurface (just remove the getter_AddRefs)?

Comment 24

[Scooter Morris]

Attachment: Address tor's review comments

Comment 25

[tor]

Comment on attachment 211192 [details] [diff] [review]
Address tor's review comments

r=tor, but file a bug about the filled text pattern visual problems we discussed on irc.

Comment 26

[Scooter Morris]

Checked in on trunk:
Checking in layout/svg/base/src/nsSVGGlyphFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp,v  <--  nsSVGGlyphFrame.cpp
new revision: 1.40; previous revision: 1.39
done
Checking in layout/svg/base/src/nsSVGPathGeometryFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGPathGeometryFrame.cpp,v  <--  nsSVGPathGeometryFrame.cpp
new revision: 1.42; previous revision: 1.41
done
Checking in layout/svg/base/src/nsSVGPattern.h;
/cvsroot/mozilla/layout/svg/base/src/nsSVGPattern.h,v  <--  nsSVGPattern.h
new revision: 1.2; previous revision: 1.1
done
Checking in layout/svg/base/src/nsSVGPatternFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGPatternFrame.cpp,v  <--  nsSVGPatternFrame.cpp
new revision: 1.9; previous revision: 1.8
done
Checking in layout/svg/renderer/src/cairo/nsSVGCairoGlyphGeometry.cpp;
/cvsroot/mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGlyphGeometry.cpp,v  <--  nsSVGCairoGlyphGeometry.cpp
new revision: 1.26; previous revision: 1.25
done
Checking in layout/svg/renderer/src/cairo/nsSVGCairoPathGeometry.cpp;
/cvsroot/mozilla/layout/svg/renderer/src/cairo/nsSVGCairoPathGeometry.cpp,v  <--  nsSVGCairoPathGeometry.cpp
new revision: 1.30; previous revision: 1.29
done
Checking in layout/svg/renderer/src/cairo/nsSVGCairoPattern.cpp;
/cvsroot/mozilla/layout/svg/renderer/src/cairo/nsSVGCairoPattern.cpp,v  <--  nsSVGCairoPattern.cpp
new revision: 1.3; previous revision: 1.2
done
Checking in layout/svg/renderer/src/cairo/nsSVGCairoPattern.h;
/cvsroot/mozilla/layout/svg/renderer/src/cairo/nsSVGCairoPattern.h,v  <--  nsSVGCairoPattern.h
new revision: 1.2; previous revision: 1.1
done

Comment 27

[Daniel Veditz [:dveditz]]

Is this fix needed on the 1.8 branch? The initial testcase was fairly soon after the 1.8 branch was cut (about a month) so you'd think the code would be similar. But none of the testcases crash for me on ff1.5, ff1.5.0.4 or current ff2 dev build.

Comment 28

[tor]

(In reply to comment #27)
> Is this fix needed on the 1.8 branch? The initial testcase was fairly soon
> after the 1.8 branch was cut (about a month) so you'd think the code would be
> similar. But none of the testcases crash for me on ff1.5, ff1.5.0.4 or current
> ff2 dev build.

Not needed for 1.8 branches, as those do not have support for <pattern>.

Comment 29

[Daniel Veditz [:dveditz]]

Thanks tor!

Comment 30

[Jesse Ruderman]

Crashtest checked in.