1167235 - Use a fast method of double buffering for canvas

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

Currently we copy and create surfaces a lot in order to facilitate double buffering of Canvas. We should do something smarter there.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Add code exposing a PersistentBufferProvider

In order to do this I'm going to introduce a new class called the 'PersistentBufferProvider' this is an object a DT can be gotten from and returned to, and when getting it the amount of content from the last return that needs to be persisted can be specified.

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Add support for the basic PersistentBufferProvider to canvas.

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 8620651 [details] [diff] [review]
Part 1: Add code exposing a PersistentBufferProvider

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

So far so good although I'd like to see more of the ipdl stuff (in this patch there's only the descriptor structure but's not used)

Comment 4

[Bas Schouten (:bas.schouten)]

(In reply to Nicolas Silva [:nical] from comment #3)
> Comment on attachment 8620651 [details] [diff] [review]
> Part 1: Add code exposing a PersistentBufferProvider
> 
> Review of attachment 8620651 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So far so good although I'd like to see more of the ipdl stuff (in this
> patch there's only the descriptor structure but's not used)

Oh that's weird, that doesn't even belong in this patch, consider it removed from this patch :P

Comment 5

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Switch CanvasRenderingContext2D to use the new BufferProvider API

Comment 6

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Switch CanvasRenderingContext2D to use the new BufferProvider API v2

Some cleanup.

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Part 4: Remove DrawTarget as a possible means of initializing Canvas layers

Removal of now dead code.

Comment 8

[Bas Schouten (:bas.schouten)]

Attachment: Part 5: Make CanvasLayerComposite ImageHost type agnostic

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: Part 6: Fix up a HasInternalBuffer value on TextureHostDirectUpload

Comment 10

[Bas Schouten (:bas.schouten)]

Attachment: Part 7: Implement PersistentBufferProvider Client and Host code

Comment 11

[Bas Schouten (:bas.schouten)]

Attachment: Part 8: Make ClientCanvasLayer use PersistentBufferProviderClient when receiving a PersistentBufferProviderShared

Comment 12

[Nicolas Silva [:nical]]

Comment on attachment 8621329 [details] [diff] [review]
Part 3: Switch CanvasRenderingContext2D to use the new BufferProvider API v2

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

r=me with my first two comment addressed

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +1527,5 @@
> +  if (mTarget && mBufferProvider) {
> +    CurrentState().transform = mTarget->GetTransform();
> +    DrawTarget* oldDT = mTarget;
> +    mTarget = nullptr;
> +    mBufferProvider->ReturnAndUseDT(oldDT);

So there is a contract that the BufferProvider will keep a string reference to the DrawTarget? If so let's document it, otherwise you need to keep oldRef as a strong reference to avoid a dangling pointer after mTarget is set to nullptr.

@@ +5391,5 @@
> +  if (mBufferProvider) {
> +    return mBufferProvider;
> +  }
> +
> +  gfxDebug() << "Only able to initialize buffer provider on first use.";

Please remove this logging (unless its appearance in the log actually means something went bad, but my understanding is that it's not the case unless it shows up twice in the log and you have one canvas in the page)

@@ -5365,5 @@
>      MarkContextClean();
>      return nullptr;
>    }
>  
> -  mTarget->Flush();

I am curious, why did we need this Flush call in the first place? I walked back the mercurial history and it's been around since the creation of the file.

Comment 13

[Nicolas Silva [:nical]]

Comment on attachment 8621331 [details] [diff] [review]
Part 5: Make CanvasLayerComposite ImageHost type agnostic

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

::: gfx/layers/composite/CanvasLayerComposite.cpp
@@ +111,5 @@
>  
>  CompositableHost*
>  CanvasLayerComposite::GetCompositableHost()
>  {
> +  if ( mCompositableHost && mCompositableHost->IsAttached()) {

lame nit: extra space after (

Comment 14

[Bas Schouten (:bas.schouten)]

(In reply to Nicolas Silva [:nical] from comment #12)
> Comment on attachment 8621329 [details] [diff] [review]
> Part 3: Switch CanvasRenderingContext2D to use the new BufferProvider API v2
> 
> Review of attachment 8621329 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with my first two comment addressed
> 
> ::: dom/canvas/CanvasRenderingContext2D.cpp
> @@ +1527,5 @@
> > +  if (mTarget && mBufferProvider) {
> > +    CurrentState().transform = mTarget->GetTransform();
> > +    DrawTarget* oldDT = mTarget;
> > +    mTarget = nullptr;
> > +    mBufferProvider->ReturnAndUseDT(oldDT);
> 
> So there is a contract that the BufferProvider will keep a string reference
> to the DrawTarget? If so let's document it, otherwise you need to keep
> oldRef as a strong reference to avoid a dangling pointer after mTarget is
> set to nullptr.

No, we simply pass it to return DT as a 'pointer' a reference. It's up to the buffer provider to check it and only use it if it has a reference to it. The reason for this is that texture clients don't allow you to keep a pointer around after unlock. We are -not- allowed to have a reference to the DT after we return it, but we -are- required to return it explicitly. This is mostly for debugging purposes. In theory ReturnAndUseDT already knows what DT it handed out, but this way it can verify it's getting back the expected one. I will document this.

> @@ +5391,5 @@
> > +  if (mBufferProvider) {
> > +    return mBufferProvider;
> > +  }
> > +
> > +  gfxDebug() << "Only able to initialize buffer provider on first use.";
> 
> Please remove this logging (unless its appearance in the log actually means
> something went bad, but my understanding is that it's not the case unless it
> shows up twice in the log and you have one canvas in the page)

That's why it's gfxDebug()... and not gfxWarning... gfxDebug is meant to be used for expected situations which may still be interesting.

> 
> @@ -5365,5 @@
> >      MarkContextClean();
> >      return nullptr;
> >    }
> >  
> > -  mTarget->Flush();
> 
> I am curious, why did we need this Flush call in the first place? I walked
> back the mercurial history and it's been around since the creation of the
> file.

Yeah, I don't know either, I don't think we did.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54ad73e3a027
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8cc59610b7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b062850ea136
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e42c13cc2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd11811fd67a
https://hg.mozilla.org/integration/mozilla-inbound/rev/eccde665c72a

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/54ad73e3a027
https://hg.mozilla.org/mozilla-central/rev/a8cc59610b7f
https://hg.mozilla.org/mozilla-central/rev/b062850ea136
https://hg.mozilla.org/mozilla-central/rev/d9e42c13cc2f
https://hg.mozilla.org/mozilla-central/rev/bd11811fd67a
https://hg.mozilla.org/mozilla-central/rev/eccde665c72a

Comment 17

[Markus Stange [:mstange]]

Does this fix bug 1126954?

Comment 18

[Bas Schouten (:bas.schouten)]

(In reply to Markus Stange [:mstange] from comment #17)
> Does this fix bug 1126954?

Don't see it doing anything to affect that..

Comment 19

[Nicolas Silva [:nical]]

Comment on attachment 8621352 [details] [diff] [review]
Part 7: Implement PersistentBufferProvider Client and Host code

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

I'd like to avoid inconsistent states, so please change the way the BufferProvider is created to not fail in the destructor (see comment), and handle TextureClient::Lock failure (handle could mean MOZ_CRASH for now if you want, but then I think it will show up in crash stats as soon as we start using it). I don't think think we need 2 ipc messages, but if there's a good reason for it this is fine. I think the shmem section is leaked, though.

::: gfx/layers/PersistentBufferProvider.cpp
@@ +30,5 @@
> +  , mWasUpdated(false)
> +{
> +  static_assert(sizeof(Atomic<int32_t>) == 4, "Atomic<int32_t> should be 4 bytes in size");
> +
> +  if (!aAllocator->AllocShmemSection(sizeof(Atomic<int32_t>), &mCurrentSection)) {

Where is the shmem section freed?

@@ +32,5 @@
> +  static_assert(sizeof(Atomic<int32_t>) == 4, "Atomic<int32_t> should be 4 bytes in size");
> +
> +  if (!aAllocator->AllocShmemSection(sizeof(Atomic<int32_t>), &mCurrentSection)) {
> +    gfxCriticalError() << "Failed to allocate ShmemSection for PersistentBufferProvider.";
> +    return;

Please avoid the pattern of fallible operations in constructors. In this case if we run into this we'll crash somewhere later from using the object in an inconsistent state. Either MOZ_CRASH here or replace the constructor with a static creation method that returns either a well initialized object or null (and make the constructor private).

@@ +95,5 @@
> +  }
> +
> +  MOZ_ASSERT(bufferToUse == kBufferOne || bufferToUse == kBufferTwo);
> +
> +  mTC[bufferToUse]->Lock(OpenMode::OPEN_READ_WRITE);

You need to check for the return value of TextureClient::Lock. It can fail sometimes (especially when we are running low on memory).

@@ +115,5 @@
> +{
> +  MOZ_ASSERT(aDT == mCurrentDT);
> +  aDT->Flush();
> +
> +  mTC[mLastBack]->Unlock();

as a sanity check, you should MOZ_ASSERT(mTC[mLastBack]->IsLocked());
In fact you'll probably want to do

if (mTC[mLastBack]->IsLocked()) { mTC[mLastBack]->Unlock(); }

because you have to handle the possibility of Lock having failed.

@@ +132,5 @@
> +    MOZ_CRASH();
> +  }
> +
> +  RefPtr<SourceSurface> snapshot;
> +  mTC[mQueuedFront]->Lock(OpenMode::OPEN_READ);

Same here, you need to handle the possibility of Lock() failing.

::: gfx/layers/PersistentBufferProvider.h
@@ +156,5 @@
> +  virtual void UsePersistentBufferProvider(const PersistentBufferProviderDescriptor& aDescriptor);
> +
> +  virtual void PrintInfo(std::stringstream& aStream, const char* aPrefix) { }
> +
> +  virtual bool Lock() { if (mFrontHost) { mFrontHost->Lock(); } return true; }

Shouldn't this return false if !mFrontHost?

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +205,5 @@
>          ScheduleComposition(op);
>        }
>        break;
>      }
> +    case CompositableOperation::TOpUsePersistentBufferProvider: {

Why do we need this message? couldn't we just have the message that we send in the transaction update the state of the host BufferProvider the way we do it for the other compositable types? Plus, this message is used at creation time but the name resemble the messages that other compositables send each transaction (UseTexture, UseTiledLayerBuffer, etc.).

Comment 20

[Bas Schouten (:bas.schouten)]

Attachment: Part 7: Implement PersistentBufferProvider Client and Host code v2

Comment 21

[Bas Schouten (:bas.schouten)]

Attachment: Part 6.5: Ensure Canvas does not have any clips pushed when returning the DrawTarget.

Comment 22

[Bas Schouten (:bas.schouten)]

Attachment: Part 7: Implement PersistentBufferProvider Client and Host code v3

Fixed up some mistakes.

Comment 23

[Nicolas Silva [:nical]]

Comment on attachment 8630334 [details] [diff] [review]
Part 7: Implement PersistentBufferProvider Client and Host code v3

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

r+ with nits. the return value of TextureHost::Lock should not be ignored (see comment).

::: gfx/layers/PersistentBufferProvider.h
@@ +88,5 @@
> +  virtual ~PersistentBufferProviderShared();
> +
> +  bool IsValid() {
> +    return !!mTC[0] && (!!mTC[1] || mTC[0]->HasInternalBuffer()) &&
> +           mCurrentFront != reinterpret_cast<Atomic<int32_t>*>(&mQueuedFront); 

nit: tailing space

@@ +174,5 @@
> +  virtual void UsePersistentBufferProvider(const PersistentBufferProviderDescriptor& aDescriptor);
> +
> +  virtual void PrintInfo(std::stringstream& aStream, const char* aPrefix) { }
> +
> +  virtual bool Lock() { if (mFrontHost) { mFrontHost->Lock(); return true; } else return false; }

It should be

{ return mFrontHost->Lock(); }

rather than

{ mFrontHost->Lock(); return true }

Comment 24

[Chris Peterson [:cpeterson]]

Bas, is there any work left to do for this canvas double-buffering? Your patches landed in comment 15, but you then marked the bug "leave-open".

Comment 25

[Bas Schouten (:bas.schouten)]

(In reply to Chris Peterson [:cpeterson] from comment #24)
> Bas, is there any work left to do for this canvas double-buffering? Your
> patches landed in comment 15, but you then marked the bug "leave-open".

Most of the work has never landed and has sort of been shelved. This was mostly an advantage to something like Shumway which ended up being shelved. Stabilizing this isn't easy and with no real demand here it's not been a high priority. Nicolas is interested in looking at reviving these patches though, we might still.

Comment 26

[Nicolas Silva [:nical]]

Attachment: Part A (Alternative approach) - copy-on-write updates not optimized yet.

Work in progress. This version tries to be smart about when we need to copy the texture to preserve the canvas content across frames. But it doesn't use tiling-style locks yet and allocates a texture every frame.

Comment 27

[Nicolas Silva [:nical]]

Attachment: Part A - Initial implementation of the copy-on-write canvas

Comment 28

[Nicolas Silva [:nical]]

Attachment: Part B - Use texture ReadLocks to optimize away allocations

Comment 29

[Nicolas Silva [:nical]]

Attachment: Part C - Avoid crashing when a snapshot outlives its TextureClient Lock/Unlock region

Comment 30

[Nicolas Silva [:nical]]

Attachment: Part A - Render canvas into a TextureClient and avoid copying data in some cases

Comment 31

[Nicolas Silva [:nical]]

Attachment: Part B - Detach DrawTarget snapshots before unlocking TextureClient

Comment 32

[Nicolas Silva [:nical]]

Attachment: Part C - Use TextureReadLock to optimize the copy-on-write scheme.

Comment 33

[Nicolas Silva [:nical]]

Attachment: Part D - Forward the shutdown notification to CanvasRenderingContext2D

Comment 34

[Nicolas Silva [:nical]]

Attachment: Part E - Unlock the canvas resources after drawing.

nsCOntentUtils lets us register a callback to be run when we enter the stable-state, which is at the end of the current JS event-loop spin. We use this to make sure canvas textures don't remain unlocked.

Comment 35

[Bas Schouten (:bas.schouten)]

Comment on attachment 8759561 [details] [diff] [review]
Part A - Render canvas into a TextureClient and avoid copying data in some cases

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +233,5 @@
> +    return false;
> +  }
> +
> +  if (state.patternStyles[aStyle] && state.patternStyles[aStyle]->mSurface) {
> +    return gfx::IsOpaqueFormat(state.patternStyles[aStyle]->mSurface->GetFormat());

Is there ambiguity that you're prefixing this gfx::?

::: gfx/layers/PersistentBufferProvider.cpp
@@ +69,5 @@
> +  if (!dt) {
> +    return nullptr;
> +  }
> +
> +  RefPtr<PersistentBufferProviderBasic> provider

nit: = goes on previous line.

@@ +94,5 @@
> +    return nullptr;
> +  }
> +
> +  RefPtr<PersistentBufferProviderShared> provider
> +    = new PersistentBufferProviderShared(aSize, aFormat, aFwd, texture);

idem

@@ +188,5 @@
> +    MOZ_ASSERT(false);
> +    return nullptr;
> +  }
> +
> +  if (!mFront->Lock(OpenMode::OPEN_READ)) {

Note purely in theory this could get a little hairy, if you'd then GetDataSourceSurface() on some snapshots and would try to lock them for writing, this would likely succeed. Anyway I don't really care.

::: gfx/layers/PersistentBufferProvider.h
@@ +139,5 @@
> +  PersistentBufferProvider* mBufferProvider;
> +  RefPtr<gfx::SourceSurface>* mSnapshot;
> +
> +  explicit AutoReturnSnapshot(PersistentBufferProvider* aProvider = nullptr)
> +  : mBufferProvider(aProvider)

nit: indentation

::: gfx/layers/client/CanvasClient.cpp
@@ +71,5 @@
> +CanvasClient2D::UpdateFromTexture(TextureClient* aTexture)
> +{
> +  MOZ_ASSERT(aTexture);
> +
> +  if (!aTexture->IsSharedWithCompositor() && !AddTextureClient(aTexture)) {

nit:

Personally I prefer:

if (!aTexture->IsSharedWithCompositor()) {
  if (!AddTextureClient(aTexture)) {
    return;
  }
}

Obviously there's no difference in compiled code but it makes it clearer what you're trying to do.

Comment 36

[Bas Schouten (:bas.schouten)]

Comment on attachment 8759562 [details] [diff] [review]
Part B - Detach DrawTarget snapshots before unlocking TextureClient

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

I don't see a D2D implementation.

::: gfx/layers/PersistentBufferProvider.cpp
@@ +83,5 @@
>                                         gfx::SurfaceFormat aFormat,
>                                         CompositableForwarder* aFwd)
>  {
> +  if (!aFwd || !aFwd->IPCOpen()) {
> +    return nullptr;

Seems odd to have this in this patch.

Comment 37

[Bas Schouten (:bas.schouten)]

Comment on attachment 8759564 [details] [diff] [review]
Part C - Use TextureReadLock to optimize the copy-on-write scheme.

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

::: gfx/layers/PersistentBufferProvider.cpp
@@ +227,5 @@
>  PersistentBufferProviderShared::BorrowSnapshot()
>  {
> +  // TODO[nical] currently we can't snapshot while drawing, looks like it does
> +  // the job but I am not sure whether we want to be able to do that.
> +  MOZ_ASSERT(!mDrawTarget);

Technically, mFront->IsLocked() will already cause us to assert 2 lines down from here.

Comment 38

[Nicolas Silva [:nical]]

Attachment: Part F - Avoid a copy of the BufferProvider's snapshot.

This lets us not have to store the BufferProvider's snapshot in CopyableCanvasLayer::mSurface (which resulted in the snapshot being lazily copied at the end of CopyableCanvasLayer::UpdateTarget when the BufferProvider gets unlocked.

Side note: CopyableCanvasLayer has grown into quite a mess (and this doesn't help). We should simplify everything so that canvas layers can only deal with PersistentBufferProvider (without storing mSurface, etc.). I'll file a followup.

Comment 39

[Nicolas Silva [:nical]]

Attachment: Part G - destroy textures before setting mCanSend to false during shutdown.

Calling TextureClient::Destroy can cause some code which asserts that IPCOpen() returns true to run. Right now we destroy the remaining textures just after setting CompositorBridgeChild's mCanSend to false which can make us run into these assertions. Moving this logic a few lines earlier fixes the issue.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14dfa550c783
Part 1 - Render canvas2D into TextureClient directly. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/b67548cc946e
Part 2 - Detach DrawTarget snapshots before unlocking TextureClient. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ee8762044bd
Part 3 - Use TextureReadLock to optimize canvas copy-on-writes. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/f534fcb785c9
Part 4 - Forward the shutdown notification to CanvasRenderingContext2D. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/61465f67b591
Part 5 - Unlock canvas2D resources after drawing. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/169b7053b22d
Part 6 - Destroy textures while TextureForwarder is still usable. r=gw280

Comment 41

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

Backed bug 1281780, bug 1281775 and bug 1167235 out for for OS X 10.10 debug for assertion in TextureClient.cpp during R(C) 1246775-1.html:

Bug 1281780:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9db95c763b66b79f5f46b497f7f769fc7692e6a3

Bug 1281775:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f5b0befdac9c69b63ea60183be171a193012f4f

Bug 1167235:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb7effd4b6fccb7a6ba6314968de49c218a41a77
https://hg.mozilla.org/integration/mozilla-inbound/rev/73deeeaaeb8644a8e1031e599aa2bcca4cdc047a
https://hg.mozilla.org/integration/mozilla-inbound/rev/b773c8510ff9e06f646e9797fd4265d2b0d13cd1
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dcc4d0ee3d713b40bd3347430b78542a15ee1c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/3267fb29a5e14feff10840dabbbcbeefe5ce1f58
https://hg.mozilla.org/integration/mozilla-inbound/rev/d66eb486e0f1d1a297e0aafc7c9c00d962416aff

First push which run the tests: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8ab05556d3264ee4799e25ec32baa21ae145fc95
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=30781702&repo=mozilla-inbound

07:18:27     INFO -  REFTEST TEST-START | file:///builds/slave/test/build/tests/reftest/tests/dom/canvas/crashtests/1246775-1.html
07:18:27     INFO -  REFTEST TEST-LOAD | file:///builds/slave/test/build/tests/reftest/tests/dom/canvas/crashtests/1246775-1.html | 279 / 3044 (9%)
07:18:27     INFO -  ++DOMWINDOW == 88 (0x11bc83000) [pid = 2001] [serial = 880] [outer = 0x12bd5e000]
07:18:27     INFO -  Assertion failure: mBorrowedDrawTarget->refCount() <= mExpectedDtRefs, at /builds/slave/m-in-m64-d-0000000000000000000/build/src/gfx/layers/client/TextureClient.cpp:513
07:18:27     INFO -  #01: XRE_main (in XUL) + 225
07:18:27     INFO -  #02: 0x0204612c (in XUL) + 252
07:18:27     INFO -  #03: 0x00f75a65 (in XUL) + 85
07:18:27     INFO -  #04: 0x00f83b28 (in XUL) + 472
07:18:27     INFO -  #05: 0x00f82e4b (in XUL) + 1195
07:18:27     INFO -  #06: 0x00f83a76 (in XUL) + 294
07:18:27     INFO -  #07: 0x00f82e4b (in XUL) + 1195
07:18:27     INFO -  #08: 0x00f81af9 (in XUL) + 1577
07:18:27     INFO -  #09: 0x030955c7 (in XUL) + 3767
07:18:27     INFO -  #10: 0x030d77e1 (in XUL) + 6577
07:18:27     INFO -  #11: 0x03105ad3 (in XUL) + 1267
07:18:27     INFO -  #12: 0x02058661 (in XUL) + 1681
07:18:27     INFO -  #13: 0x01a07faf (in XUL) + 1151
07:18:27     INFO -  #14: 0x01fe033a (in XUL) + 282
07:18:27     INFO -  #15: 0x04a225a0 (in XUL) + 96
07:18:27     INFO -  #16: 0x04a22282 (in XUL) + 690
07:18:27     INFO -  #17: 0x04bfe772 (in XUL) + 242
07:18:27     INFO -  #18: 0x04bfc681 (in XUL) + 897
07:18:27     INFO -  #19: 0x04a225a0 (in XUL) + 96
07:18:27     INFO -  #20: 0x04a22282 (in XUL) + 690
07:18:27     INFO -  #21: 0x04a1a287 (in XUL) + 36391
07:18:27     INFO -  #22: 0x04a11245 (in XUL) + 517
07:18:27     INFO -  #23: 0x04a22221 (in XUL) + 593
07:18:27     INFO -  #24: 0x04a229de (in XUL) + 46
07:18:27     INFO -  #25: 0x049358de (in XUL) + 990
07:18:27     INFO -  #26: 0x0492c7e1 (in XUL) + 241
07:18:27     INFO -  #27: 0x0492e0d6 (in XUL) + 166
07:18:27     INFO -  #28: 0x04a225a0 (in XUL) + 96
07:18:27     INFO -  #29: 0x04a22282 (in XUL) + 690
07:18:27     INFO -  #30: 0x04a229de (in XUL) + 46
07:18:27     INFO -  #31: 0x04938f62 (in XUL) + 194
07:18:27     INFO -  #32: 0x0491ffcf (in XUL) + 431
07:18:27     INFO -  #33: 0x0492c7e1 (in XUL) + 241
07:18:27     INFO -  #34: 0x0492e0d6 (in XUL) + 166
07:18:27     INFO -  #35: 0x04a225a0 (in XUL) + 96
07:18:27     INFO -  #36: 0x04a22282 (in XUL) + 690
07:18:27     INFO -  #37: 0x04a1a287 (in XUL) + 36391
07:18:27     INFO -  #38: 0x04a11245 (in XUL) + 517
07:18:27     INFO -  #39: 0x04a22221 (in XUL) + 593
07:18:27     INFO -  #40: 0x04a229de (in XUL) + 46
07:18:27     INFO -  #41: 0x047ab3d6 (in XUL) + 198
07:18:27     INFO -  #42: 0x01e08719 (in XUL) + 425
07:18:27     INFO -  #43: 0x021b4ac7 (in XUL) + 359
07:18:27     INFO -  #44: 0x021b3a37 (in XUL) + 1015
07:18:27     INFO -  #45: 0x0219d0df (in XUL) + 223
07:18:27     INFO -  #46: 0x0219dd89 (in XUL) + 1993
07:18:27     INFO -  #47: 0x0218e485 (in XUL) + 421
07:18:27     INFO -  #48: 0x0218df16 (in XUL) + 566
07:18:27     INFO -  #49: 0x0218facc (in XUL) + 4492
07:18:27     INFO -  #50: 0x030bb5c1 (in XUL) + 1617
07:18:27     INFO -  #51: 0x035b8acf (in XUL) + 927
07:18:27     INFO -  #52: 0x035b6d9b (in XUL) + 2955
07:18:27     INFO -  #53: 0x035ba010 (in XUL) + 16
07:18:27     INFO -  #54: 0x00d2e80d (in XUL) + 717
07:18:27     INFO -  #55: 0x00d2e0cb (in XUL) + 459
07:18:27     INFO -  #56: 0x00d2cab2 (in XUL) + 1010
07:18:27     INFO -  #57: 0x00d2d84d (in XUL) + 1149
07:18:27     INFO -  #58: 0x00d2de7d (in XUL) + 13
07:18:27     INFO -  #59: 0x001d87da (in XUL) + 1434
07:18:27     INFO -  #60: 0x01377e8c (in XUL) + 236
07:18:27     INFO -  #61: 0x013668f1 (in XUL) + 1665
07:18:27     INFO -  #62: 0x013e93f7 (in XUL) + 39
07:18:27     INFO -  #63: 0x000f7d90 (in XUL) + 896
07:18:27     INFO -  #64: 0x0013816f (in XUL) + 79
07:18:27     INFO -  #65: 0x02c9d411 (in XUL) + 113
07:18:27     INFO -  #66: CFRunLoopRunSpecific (in CoreFoundation) + 296
07:18:27     INFO -  #67: __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (in CoreFoundation) + 17
07:18:27     INFO -  #68: __CFRunLoopDoSources0 (in CoreFoundation) + 269
07:18:27     INFO -  #69: __CFRunLoopRun (in CoreFoundation) + 927
07:18:27     INFO -  #70: _BlockUntilNextEventMatchingListInModeWithFilter (in HIToolbox) + 71
07:18:27     INFO -  #71: RunCurrentEventLoopInMode (in HIToolbox) + 235
07:18:27     INFO -  #72: ReceiveNextEventCommon (in HIToolbox) + 431
07:18:27     INFO -  #73: -[NSApplication run] (in AppKit) + 594
07:18:27     INFO -  #74: _DPSNextEvent (in AppKit) + 978
07:18:27     INFO -  #75: 0x02d07d45 (in XUL) + 261
07:18:27     INFO -  #76: -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 346
07:18:27     INFO -  #77: 0x02d07036 (in XUL) + 86
07:18:28     INFO -  #78: 0x02d08495 (in XUL) + 421
07:18:28     INFO -  #79: 0x0393fd21 (in XUL) + 97
07:18:28     INFO -  #80: 0x039cc0cd (in XUL) + 7469
07:18:28     INFO -  #81: 0x039cc683 (in XUL) + 675
07:18:28     INFO -  #82: 0x0000000100002003 (in firefox) + 2227
07:18:28  WARNING -  TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/dom/canvas/crashtests/1246775-1.html | application terminated with exit code 1

Comment 42

[Nicolas Silva [:nical]]

Attachment: Part H - Don't DrawWindow directly into mTarget if using a shared BufferProvider.

This fixes the crashtest that caused the backout. The problem was that DrawWindow tries to render directly into mTarget while also calling the pre-transaction callback which calls ReturnTarget. With the shared buffer provider we assert that nobody kept (strong) references to the DrawTarget before nulocking/unmapping its buffer. So the solution is simply to not wrap a gfxContext around a mTarget which will be made unavailable in the pre-transaction callback.

Comment 43

[Nicolas Silva [:nical]]

Attachment: Part B - Detach DrawTargetr snapshots before unlocking TextureClient

Addresses the review comments.

Comment 45

[Bas Schouten (:bas.schouten)]

Comment on attachment 8766385 [details] [diff] [review]
Part B - Detach DrawTargetr snapshots before unlocking TextureClient

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

::: gfx/2d/2D.h
@@ +1177,5 @@
>  
> +  /**
> +   * Ensures that no snapshot is still pointing to this DrawTarget's surface data.
> +   *
> +   * This can be useful if the DrawTarget is wrapped around data that it doesn not

nit: typo

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fff0d7d295e5
Part 1 - Render canvas2D into TextureClient directly. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/53487e6b475a
Part 2 - Detach DrawTarget snapshots before unlocking TextureClient. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/31f3f2c5a759
Part 3 - Use TextureReadLock to optimize canvas copy-on-writes. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/f51f78388224
Part 4 - Forward the shutdown notification to CanvasRenderingContext2D. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/3210c503929d
Part 5 - Unlock canvas2D resources after drawing. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a31a3a2470a
Part 6 - Destroy textures while TextureForwarder is still usable. r=gw280
https://hg.mozilla.org/integration/mozilla-inbound/rev/56715a33b016
Part 7 - Don't paint directly into a canvas with DrawWindow when using a shared PersistentBufferProvider. r=lsalzman

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fff0d7d295e5
https://hg.mozilla.org/mozilla-central/rev/53487e6b475a
https://hg.mozilla.org/mozilla-central/rev/31f3f2c5a759
https://hg.mozilla.org/mozilla-central/rev/f51f78388224
https://hg.mozilla.org/mozilla-central/rev/3210c503929d
https://hg.mozilla.org/mozilla-central/rev/9a31a3a2470a
https://hg.mozilla.org/mozilla-central/rev/56715a33b016

Comment 48

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

The seven patches that landed (comment 47) - what do they respond to in terms of the above patches 1 through 8 (including 6.5) and A through H?  The comments don't quite match either.  Are all 17 patches attached to the bug obsolete, and there are different 7 that actually landed?  Let's clean this up, especially since we're tracking a regression (bug 1284721.)

Comment 49

[Nicolas Silva [:nical]]

The patches that landed recently are the ones with letters A through H. Among the other patches, some landed a while ago and some never landed, I don't know exactly which did or did not make it.

As far as recent regressions are concerned, we can focus on patches A -> H.