884034 - SourceSurfaceSkia's pointer to DrawTargetSkia goes dangling

Status: RESOLVED WORKSFORME

(Core :: Graphics: Canvas2D, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Fix a crash in SourceSurfaceSkia: should have used a RefPtr, not a raw pointer, to the DrawTargetSkia

While working on Skia/GL I got crashes caused by SourceSurfaceSkia's pointer to DrawTargetSkia being dangling, which were easy enough to fix by making it strong. I don't see a downside to that, either, as DrawTargetSkia doesn't seem to possibly have strong references back to SourceSurfaces --- all I can see is

   std::vector<SourceSurfaceSkia*> mSnapshots;

which is not strong, hence not a problem.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 763784 [details] [diff] [review]
Fix a crash in SourceSurfaceSkia: should have used a RefPtr, not a raw pointer, to the DrawTargetSkia

Review of attachment 763784 [details] [diff] [review]:
-----------------------------------------------------------------

This just looks like it's wallpapering over a different bug.

When the DrawTargetSkia is destroyed, it should call DrawTargetDestroyed() on all the snapshots, which will cause them to clear the DT pointer.

How are we ending up with a SourceSurfaceSkia that retains a pointer to the DrawTarget after it has been destroyed?

Also, DrawTargetSkia doesn't need a std::vector of snapshots. It can just use a single pointer.

Any time we modify the DrawTarget, we notify all the snapshots and disconnect them. If we have multiple, they must be identical.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Have a look at what DrawTargetCG and DrawTargetD2D do.

We could probably share this code on DrawTarget too.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> Comment on attachment 763784 [details] [diff] [review]
> Fix a crash in SourceSurfaceSkia: should have used a RefPtr, not a raw
> pointer, to the DrawTargetSkia
> 
> Review of attachment 763784 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This just looks like it's wallpapering over a different bug.
> 
> When the DrawTargetSkia is destroyed, it should call DrawTargetDestroyed()
> on all the snapshots, which will cause them to clear the DT pointer.

It actually does!
http://hg.mozilla.org/projects/graphics/file/6b8fbb41aaa9/gfx/2d/DrawTargetSkia.cpp#l88

So I suppose then, that the SourceSurfaceSkia that is causing my crash isn't one of these mSnapshots, or isn't anymore at the time the DrawTargetSkia is destroyed?

> 
> How are we ending up with a SourceSurfaceSkia that retains a pointer to the
> DrawTarget after it has been destroyed?

If I remember correctly, that was using the AdjustedTarget machinery. I can try to debug this again to give you the entire story.

> 
> Also, DrawTargetSkia doesn't need a std::vector of snapshots. It can just
> use a single pointer.

Sounds like good material for a separate bug.

> 
> Any time we modify the DrawTarget, we notify all the snapshots and
> disconnect them. If we have multiple, they must be identical.

That sounds fragile.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Alright, re-debugged.

The problem is actually with CanvasPattern, not with AdjustedTarget.

The testcase that triggers this is:

http://philip.html5.org/tests/canvas/suite/tests/framed.initial.reset.pattern.html

This calls CreatePattern, which creates a SourceSurfaceSkia referencing the current DrawTargetSkia, and then resizes the canvas, which destroys the current DrawTargetSkia.

The SourceSurfaceSkia continues with a dangling mDrawTarget pointer, which does get dereferenced in the destructor calling MarkIndependent (as mDrawTarget is not NULL).

However you make a good point about the right solution being to clear out the mDrawTarget pointer rather than keeping no-longer-useful DrawTarget's alive.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

So what's happening is that the SourceSurface returned in the CanvasPattern object is NOT the one that is recorded in the DrawTarget's mSnapshots!  _that_ one correctly gets notified when the DrawTarget goes away.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Oh wait, no, ignore comment 5... moar debugging.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

I can't reproduce a crash at the moment; I thought did reproduce a bad-pointer-dereference in comment 4, but now I can't reproduce it anymore. I guess I'll work again on this if this is an issue again. Meanwhile, I've got a valgrind-green run without my 'fix' so I guess it's good enough for now. I'll remove my patch from the graphics branch.