1416862 - Crash in mozilla::gfx::StoredPattern::Assign

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P3)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-36a47eef-d55c-412e-8768-23a0d0171112.
=============================================================

Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::gfx::StoredPattern::Assign(mozilla::gfx::Pattern const&) 	gfx/2d/DrawCommand.h:113
1 	xul.dll 	mozilla::gfx::FillRectCommand::FillRectCommand(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::Pattern const&, mozilla::gfx::DrawOptions const&) 	gfx/2d/DrawCommand.h:328
2 	xul.dll 	mozilla::layers::FillRectWithMask(mozilla::gfx::DrawTarget*, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::SourceSurface*, mozilla::gfx::SamplingFilter, mozilla::gfx::DrawOptions const&, mozilla::gfx::ExtendMode, mozilla::gfx::SourceSurface*, mozilla::gfx::BaseMatrix<float> const*, mozilla::gfx::BaseMatrix<float> const*) 	gfx/layers/basic/BasicLayersImpl.cpp:148
3 	xul.dll 	mozilla::layers::FillRectWithMask(mozilla::gfx::DrawTarget*, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::SourceSurface*, mozilla::gfx::SamplingFilter, mozilla::gfx::DrawOptions const&, mozilla::layers::Layer*) 	gfx/layers/basic/BasicLayersImpl.cpp:172
4 	xul.dll 	mozilla::layers::BasicCanvasLayer::Paint(mozilla::gfx::DrawTarget*, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::layers::Layer*) 	gfx/layers/basic/BasicCanvasLayer.cpp:66
5 	xul.dll 	mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) 	gfx/layers/basic/BasicLayerManager.cpp:711
6 	xul.dll 	mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) 	gfx/layers/basic/BasicLayerManager.cpp:891
7 	xul.dll 	mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) 	gfx/layers/basic/BasicLayerManager.cpp:731
8 	xul.dll 	mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) 	gfx/layers/basic/BasicLayerManager.cpp:891
9 	xul.dll 	mozilla::layers::BasicLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) 	gfx/layers/basic/BasicLayerManager.cpp:616
10 	xul.dll 	mozilla::PaintInactiveLayer 	layout/painting/FrameLayerBuilder.cpp:3706
11 	xul.dll 	mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem>&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, gfxContext*, nsDisplayListBuilder*, nsPresContext*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const&, float, float, int) 	layout/painting/FrameLayerBuilder.cpp:6020
12 	xul.dll 	mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*) 	layout/painting/FrameLayerBuilder.cpp:6200
13 	xul.dll 	mozilla::layers::ClientPaintedLayer::PaintOffMainThread() 	gfx/layers/client/ClientPaintedLayer.cpp:243
14 	xul.dll 	mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor*) 	gfx/layers/client/ClientPaintedLayer.cpp:282
15 	xul.dll 	mozilla::layers::ClientContainerLayer::RenderLayer() 	gfx/layers/client/ClientContainerLayer.h:58
16 	xul.dll 	mozilla::layers::ClientLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor*) 	gfx/layers/client/ClientLayerManager.h:389
17 	xul.dll 	mozilla::layers::ClientContainerLayer::RenderLayer() 	gfx/layers/client/ClientContainerLayer.h:58
18 	xul.dll 	mozilla::layers::ClientLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) 	gfx/layers/client/ClientLayerManager.cpp:384
19 	xul.dll 	mozilla::FrameLayerBuilder::ComputeGeometryChangeForItem(mozilla::DisplayItemData*) 	layout/painting/FrameLayerBuilder.cpp:4637
20 	xul.dll 	mozilla::layers::ClientLayerManager::EndTransaction(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) 	gfx/layers/client/ClientLayerManager.cpp:442
21 	xul.dll 	nsDisplayList::PaintRoot(nsDisplayListBuilder*, gfxContext*, unsigned int) 	layout/painting/nsDisplayList.cpp:2548
22 	xul.dll 	RepeatOrStretchMirroredSurface 	gfx/thebes/gfxBlur.cpp:824
23 	xul.dll 	mozilla::SVGPathData::BuildPath(mozilla::gfx::PathBuilder*, unsigned char, float) 	dom/svg/SVGPathData.cpp:496
24 		@0x17fe7fff

crash reports with this signature are newly showing up on windows since 58.0a1, the first one was in build 20171101104430. most of the time the crashes are happening in the content process and for 32bit installations. a portion of them are UAF crashes.

Comment 1

[Daniel Veditz [:dveditz]]

I see 14 out of 15 crashes in the past week are UAFs -- quite a portion!

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

milan: you or jet/layout?

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Graphics.
Ryan, Bas, feels like it's OMTP related?

Comment 4

[Bas Schouten (:bas.schouten)]

Yup, definitely OMTP related.

Comment 5

[Bas Schouten (:bas.schouten)]

This is very odd.. I'm guessing based on the stack the surface in this pattern is freed. But I don't understand yet how that could be.. David, have you seen anything like this?

Comment 6

[Bas Schouten (:bas.schouten)]

I'm thinking this may be related to bug 1416864. Let's see if that fixes it.

Comment 7

[Ryan Hunt [:rhunt] (on leave until early May)]

Huh, I'm not sure what's going on here.

I'm not sure if it's related to bug 1416864 because the feature notes has 'D2D? D2D1-'. I'm not sure why the stack frame cuts off at 24. JIT'ed code?

This could be something odd with AsyncCanvasRenderer because it looks like theres some interesting threading stuff in there, but I'm not familiar with that code.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

I don't think it's bug 1416864 either. Keeping ni, I think we need to look at some crash dumps.

Comment 9

[Bas Schouten (:bas.schouten)]

I agree, I noticed the D2D1- as well after I posted that comment.

Comment 10

[Bas Schouten (:bas.schouten)]

I agree, fwiw, this probably has something to do with CopyableCanvasRenderer::ReadbackSurface()? But I wasn't involved with this code, so I'm still working on understanding how this is supposed to work.

Comment 11

[Bas Schouten (:bas.schouten)]

So, I'd like to know on these systems, what codepath in CopyableCanvasRenderer::ReadbackSurface we are supposed to take, and if anyone has ideas on how that might lead us to get a race condition on a SourceSurface, that would be even better! :)

Comment 12

[Bas Schouten (:bas.schouten)]

Looking at the crash dump, surfPat->mSurface points to 0x150ecc0, this seems like a reasonable address based on the stack to contain the surface. When it loads the vtable ptr from that address the result it got was 0xe5e5e5e5, confirming the problem is a destroyed surface being passed in.

Comment 13

[Bas Schouten (:bas.schouten)]

Based on what I'm seeing in the crash report this does not appear to be using the canvasRenderer codepath, but rather the PersistentBufferProvider codepath.

Comment 14

[Bas Schouten (:bas.schouten)]

I believe I've figured out the problem. Patch upcoming.

Comment 15

[Bas Schouten (:bas.schouten)]

Attachment: Ensure mSnapshot access is always protected by mSnapshotLock

I strongly suspect this is yet another race condition surrounding mSnapshot.

I believe we're getting the Snapshot off a DrawTarget just after the paint thread is destroying that same Snapshot, since we've already transferred the pointer into a local before the Snapshot destructor cleared out mSnapshot, we'll then return the destroyed snapshot from DrawTargetSkia::Snapshot().

Comment 16

[Bas Schouten (:bas.schouten)]

Comment on attachment 8931471 [details] [diff] [review]
Ensure mSnapshot access is always protected by mSnapshotLock

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Very difficult, this is a very rare race condition, one could attempt to trigger it by having a 1000 canvasses and drawing to them at once or something, but that still wouldn't really put you in a great place to actually exploit it. 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The patch itself does. Even without a comment it would be pretty obvious what's going on here.

Which older supported branches are affected by this flaw? None

If not all supported branches, which bug introduced the flaw? OMTP, bug 1403935

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This patch should apply to Beta as is.

How likely is this patch to cause regressions; how much testing does it need? Extremely unlikely, at the worst this would cause a performance regression but even that seems farfetched for a single, rarely contended lock.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

sec-approval+. Please nominate for beta.

Comment 18

[Bas Schouten (:bas.schouten)]

Comment on attachment 8931471 [details] [diff] [review]
Ensure mSnapshot access is always protected by mSnapshotLock

Approval Request Comment
[Feature/Bug causing the regression]: 1403935
[User impact if declined]: UAF
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No, in progress.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Single line added grabbing a lock that is rarely contended with no deadlock risk.
[String changes made/needed]: None

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/da1a37d96966

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8931471 [details] [diff] [review]
Ensure mSnapshot access is always protected by mSnapshotLock

Fix a sec-high. Beta58+.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4acde7e42314

Comment 22

[Bas Schouten (:bas.schouten)]

Bad news, although this may have helped, this crash does -not- appear to be fixed.

Comment 23

[Bas Schouten (:bas.schouten)]

I'm an idiot. There's still an obvious race condition here, if Snapshot() grabs the snapshot lock when the destructor has already been started. When the snapshot lock is grabbed by the SourceSurface the DT will still get out the source surface that's in the process of being destroyed. I'm thinking about how we can address this.

Comment 24

[Bas Schouten (:bas.schouten)]

So, I can only see two ways forward here, either we need to make SourceSurfaces use a virtual release method, that can be overloaded and hold the lock while release is called. This may be a simpler patch, but it seems like the less desirable solution.

The other option is to reverse the ownership model to the model I used originally in D2D. I don't know if there was ever a good reason it was done the other way around in Skia. The patch for this will be a little more complicated, it will need to bake on nightly for at least 2-3 days before we can uplift it.

I'm tempted to say the second solution is the best, and I will construct a patch for it.

Comment 25

[David Anderson [:dvander] - inactive, e-mail if emergency]

For what it's worth, I think the Skia ownership model is kind of weird. I can see why it might make sense, but inverting it is a lot clearer.

Comment 26

[David Anderson [:dvander] - inactive, e-mail if emergency]

So the race is that: SourceSurfaceSkia's refcount drops to 0, but DrawTargetSkia wins the race to get the lock, and this brings the refcount back up to 1. On a dead object. That does seem likely, really the only option is that it should own a ref to the snapshot.

Do we still have the same problem if ownership is reversed? In that scenario, DrawTargetSkia owns a ref to the snapshot, and the snapshot does not own a ref to the DrawTarget. As long as the snapshot doesn't try to revive the DrawTarget, I think it's okay - it just has to acquire the lock before trying to use it, since potentially DrawTargetWillChange/MarkIndependent have been called while the lock is held.

Comment 27

[David Anderson [:dvander] - inactive, e-mail if emergency]

Also, potentially a workaround for uplift (not a long-term solution) would be for DrawTargetSkia::Snapshot() to check if |mSnapshot && mSnapshot->refcount == 0|. That should be okay since, either we lost the race and mSnapshot is null, or we won the race and the object is not destroyed yet. The important thing is we just can't revive it.

Comment 28

[Bas Schouten (:bas.schouten)]

(In reply to David Anderson [:dvander] from comment #27)
> Also, potentially a workaround for uplift (not a long-term solution) would
> be for DrawTargetSkia::Snapshot() to check if |mSnapshot &&
> mSnapshot->refcount == 0|. That should be okay since, either we lost the
> race and mSnapshot is null, or we won the race and the object is not
> destroyed yet. The important thing is we just can't revive it.

Nope, sadly, that's not true, suppose this scenario:

Thread 1:
EnterLock()
if (mSnapshot && mSnapshot->refCnt != 0) {

Thread 2:
if (--mRefCnt == 0) {
  delete this;

Thread 1:
RefPtr<SourceSurfaceSkia> snapshot = mSnapshot;

Thread 2:
SourceSurfaceSkia::~SourceSurfaceSkia()

You really have to be holding the lock while you do --mRefCnt == 0.

Comment 29

[Bas Schouten (:bas.schouten)]

Attachment: Reverse ownership model in Skia.

Comment 30

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Bas Schouten (:bas.schouten) from comment #28)
> (In reply to David Anderson [:dvander] from comment #27)
> > Also, potentially a workaround for uplift (not a long-term solution) would
> > be for DrawTargetSkia::Snapshot() to check if |mSnapshot &&
> > mSnapshot->refcount == 0|. That should be okay since, either we lost the
> > race and mSnapshot is null, or we won the race and the object is not
> > destroyed yet. The important thing is we just can't revive it.
> 
> Nope, sadly, that's not true, suppose this scenario:
> 
> Thread 1:
> EnterLock()
> if (mSnapshot && mSnapshot->refCnt != 0) {
> 
> Thread 2:
> if (--mRefCnt == 0) {
>   delete this;
> 
> Thread 1:
> RefPtr<SourceSurfaceSkia> snapshot = mSnapshot;
> 
> Thread 2:
> SourceSurfaceSkia::~SourceSurfaceSkia()
> 
> You really have to be holding the lock while you do --mRefCnt == 0.

Oh, of course - you're right. Looping a compare-and-swap to increment the refcount could address that, I guess, where we'd stop trying if the refcount drops to 0.

Comment 31

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8934204 [details] [diff] [review]
Reverse ownership model in Skia.

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +295,5 @@
> +    MutexAutoLock lock(mSnapshotLock);
> +    // We may hold the only reference. MarkIndependent will clear mSnapshot;
> +    // keep the snapshot object alive so it doesn't get destroyed while
> +    // DrawTargetWillChange is running.
> +    RefPtr<SourceSurfaceSkia> deathGrip = mSnapshot;

I think we want to do something like DrawTargetCairo does instead. Call a function called MarkSnapshotIndependent or something, and then only call DrawTargetWillChange if the snapshot has > 1 ref. We'd want to do this in MarkChanged() as well.

Otherwise I think we're potentially going to copy a lot of surfaces.

@@ +2240,5 @@
>  {
> +  // I'm not entirely certain whether this lock is needed, as multiple threads
> +  // should never modify the DrawTarget at the same time anyway, but this seems
> +  // like the safest.
> +  MutexAutoLock lock(mSnapshotLock);

I agree - probably not needed, but harmless to have it.

Comment 32

[Bas Schouten (:bas.schouten)]

(In reply to David Anderson [:dvander] from comment #31)
> Comment on attachment 8934204 [details] [diff] [review]
> Reverse ownership model in Skia.
> 
> Review of attachment 8934204 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetSkia.cpp
> @@ +295,5 @@
> > +    MutexAutoLock lock(mSnapshotLock);
> > +    // We may hold the only reference. MarkIndependent will clear mSnapshot;
> > +    // keep the snapshot object alive so it doesn't get destroyed while
> > +    // DrawTargetWillChange is running.
> > +    RefPtr<SourceSurfaceSkia> deathGrip = mSnapshot;
> 
> I think we want to do something like DrawTargetCairo does instead. Call a
> function called MarkSnapshotIndependent or something, and then only call
> DrawTargetWillChange if the snapshot has > 1 ref. We'd want to do this in
> MarkChanged() as well.
> 
> Otherwise I think we're potentially going to copy a lot of surfaces.
> I agree - probably not needed, but harmless to have it.

Oh, this is silly, I meant to just hand off the skSurface, not do a full MarkIndependent. I will correct this.

Comment 33

[Bas Schouten (:bas.schouten)]

Attachment: Reverse ownership model in Skia. v2

Comment 34

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8934629 [details] [diff] [review]
Reverse ownership model in Skia. v2

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

Nice - this is so much simpler than the inverted model!

::: gfx/2d/DrawTargetSkia.cpp
@@ +295,5 @@
> +    MutexAutoLock lock(mSnapshotLock);
> +    // We may hold the only reference. MarkIndependent will clear mSnapshot;
> +    // keep the snapshot object alive so it doesn't get destroyed while
> +    // DrawTargetWillChange is running.
> +    RefPtr<SourceSurfaceSkia> deathGrip = mSnapshot;

I *think* this comment and the RefPtr are not necessary. GiveSurface doesn't call back into the DrawTarget, so there's no way the snapshot's refcount could drop to 0.

If you were calling MarkChanged or something then the RefPtr/comment would be necessary. Maybe it's worth keeping the RefPtr or leaving some kind of comment just in case someone does that in the future.

::: gfx/2d/SourceSurfaceSkia.h
@@ +61,5 @@
>    void DrawTargetWillChange();
>  
>    sk_sp<SkImage> mImage;
> +  // This keeps a surface alive if needed because its DrawTarget has gone away.
> +  sk_sp<SkSurface> mSurface;

Just so I understand - the SkImage is effectively a pointer to the SkSurface, so all we need to do is keep a reference to it alive?

Comment 35

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8934629 [details] [diff] [review]
Reverse ownership model in Skia. v2

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +291,5 @@
>  
>  DrawTargetSkia::~DrawTargetSkia()
>  {
> +  if (mSnapshot) {
> +    MutexAutoLock lock(mSnapshotLock);

Also, is the lock necessary here? I think not, *but* it might be necessary to lock mChangeMutex in DrawTargetSkia. Probably in GiveSurface().

Comment 36

[David Anderson [:dvander] - inactive, e-mail if emergency]

err, mChangeMutex in SourceSurfaceSkia*

Comment 37

[Bas Schouten (:bas.schouten)]

(In reply to David Anderson [:dvander] from comment #34)
> Comment on attachment 8934629 [details] [diff] [review]
> Reverse ownership model in Skia. v2
> 
> Review of attachment 8934629 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice - this is so much simpler than the inverted model!
> 
> ::: gfx/2d/DrawTargetSkia.cpp
> @@ +295,5 @@
> > +    MutexAutoLock lock(mSnapshotLock);
> > +    // We may hold the only reference. MarkIndependent will clear mSnapshot;
> > +    // keep the snapshot object alive so it doesn't get destroyed while
> > +    // DrawTargetWillChange is running.
> > +    RefPtr<SourceSurfaceSkia> deathGrip = mSnapshot;
> 
> I *think* this comment and the RefPtr are not necessary. GiveSurface doesn't
> call back into the DrawTarget, so there's no way the snapshot's refcount
> could drop to 0.
> 
> If you were calling MarkChanged or something then the RefPtr/comment would
> be necessary. Maybe it's worth keeping the RefPtr or leaving some kind of
> comment just in case someone does that in the future.

That was actually my reasoning. But you're right, I should improve the comment.

> ::: gfx/2d/SourceSurfaceSkia.h
> @@ +61,5 @@
> >    void DrawTargetWillChange();
> >  
> >    sk_sp<SkImage> mImage;
> > +  // This keeps a surface alive if needed because its DrawTarget has gone away.
> > +  sk_sp<SkSurface> mSurface;
> 
> Just so I understand - the SkImage is effectively a pointer to the
> SkSurface, so all we need to do is keep a reference to it alive?

The SkImage is a pointer to the -data- pointed to by the SkSurface, however the SkSurface is the object that -owns- that data in the 'normal' case.

(In reply to David Anderson [:dvander] from comment #35)
> Comment on attachment 8934629 [details] [diff] [review]
> Reverse ownership model in Skia. v2
> 
> Review of attachment 8934629 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetSkia.cpp
> @@ +291,5 @@
> >  
> >  DrawTargetSkia::~DrawTargetSkia()
> >  {
> > +  if (mSnapshot) {
> > +    MutexAutoLock lock(mSnapshotLock);
> 
> Also, is the lock necessary here? I think not, *but* it might be necessary
> to lock mChangeMutex in DrawTargetSkia. Probably in GiveSurface().

I don't think we do, as the surface should be threadsafe refcounted and this won't actually affect the content of the surface. Nor will any other party ever access mSurface on SourceSurfaceSkia.

Comment 38

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/286203c73311

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Please request approval on the new patch too once you're happy with how it's looking on m-c.

Comment 40

[Bas Schouten (:bas.schouten)]

Comment on attachment 8934629 [details] [diff] [review]
Reverse ownership model in Skia. v2

Approval Request Comment
[Feature/Bug causing the regression]: OMTP
[User impact if declined]: UAF crash
[Is this code covered by automated tests?]: Extensively
[Has the fix been verified in Nightly?]: 2 days
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Bug 1423281
[Is the change risky?]: Slightly, but the code is very widely used so nightly testing is fairly solid.
[Why is the change risky/not risky?]: Changes an ownership pattern
[String changes made/needed]: None

Comment 41

[Julien Cristau [:jcristau]]

Comment on attachment 8934629 [details] [diff] [review]
Reverse ownership model in Skia. v2

sec fix for omtp, beta58+

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/996b3de167b7