Closed Bug 1167235 Opened 4 years ago Closed 3 years ago

Use a fast method of double buffering for canvas

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: bas.schouten, Assigned: nical)

References

Details

Attachments

(17 files, 8 obsolete files)

13.10 KB, patch
nical
: review+
Details | Diff | Splinter Review
4.37 KB, patch
nical
: review+
Details | Diff | Splinter Review
17.55 KB, patch
nical
: review+
Details | Diff | Splinter Review
3.48 KB, patch
nical
: review+
Details | Diff | Splinter Review
4.85 KB, patch
nical
: review+
Details | Diff | Splinter Review
758 bytes, patch
nical
: review+
Details | Diff | Splinter Review
4.57 KB, patch
nical
: review+
Details | Diff | Splinter Review
2.61 KB, patch
nical
: review+
Details | Diff | Splinter Review
42.02 KB, patch
nical
: review+
Details | Diff | Splinter Review
32.23 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
10.73 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
6.59 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
4.88 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
3.21 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
2.07 KB, patch
gw280
: review+
Details | Diff | Splinter Review
1.56 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
13.86 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
Currently we copy and create surfaces a lot in order to facilitate double buffering of Canvas. We should do something smarter there.
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.
Attachment #8620651 - Flags: review?(nical.bugzilla)
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)
Attachment #8620651 - Flags: review?(nical.bugzilla) → review+
Attachment #8620654 - Flags: review?(nical.bugzilla) → review+
(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
Blocks: 1100744
Some cleanup.
Attachment #8621328 - Attachment is obsolete: true
Attachment #8621328 - Flags: review?(nical.bugzilla)
Attachment #8621329 - Flags: review?(nical.bugzilla)
Removal of now dead code.
Attachment #8621330 - Flags: review?(nical.bugzilla)
Attachment #8621352 - Flags: review?(nical.bugzilla)
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.
Attachment #8621329 - Flags: review?(nical.bugzilla) → review+
Attachment #8621330 - Flags: review?(nical.bugzilla) → review+
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 (
Attachment #8621331 - Flags: review?(nical.bugzilla) → review+
Attachment #8621337 - Flags: review?(nical.bugzilla) → review+
(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.
No longer blocks: shumway-m3
Keywords: leave-open
Does this fix bug 1126954?
(In reply to Markus Stange [:mstange] from comment #17)
> Does this fix bug 1126954?

Don't see it doing anything to affect that..
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.).
Attachment #8621352 - Flags: review?(nical.bugzilla) → review-
Attachment #8621355 - Flags: review?(nical.bugzilla) → review+
Attachment #8621352 - Attachment is obsolete: true
Fixed up some mistakes.
Attachment #8629989 - Attachment is obsolete: true
Attachment #8629989 - Flags: review?(nical.bugzilla)
Attachment #8630334 - Flags: review?(nical.bugzilla)
Attachment #8629990 - Flags: review?(nical.bugzilla) → review+
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 }
Attachment #8630334 - Flags: review?(nical.bugzilla) → review+
Depends on: 1188752
Blocks: 1181006
Depends on: 1192159
Depends on: 1206076
Depends on: 1246775
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".
Flags: needinfo?(bas)
(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.
Flags: needinfo?(bas)
Depends on: 1272600
Assignee: bas → nical.bugzilla
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.
Attachment #8752308 - Attachment is obsolete: true
Attachment #8755508 - Attachment is obsolete: true
Attachment #8755513 - Attachment is obsolete: true
Attachment #8759562 - Flags: review?(bas)
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.
Attachment #8759567 - Flags: review?(bas)
Attachment #8759562 - Attachment description: Part B - Detach DrawTargetr snapshots before unlocking TextureClient → Part B - Detach DrawTarget snapshots before unlocking TextureClient
Blocks: 1252835
No longer blocks: 1252835
See Also: → 1252835
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.
Attachment #8759561 - Flags: review?(bas) → review+
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.
Attachment #8759562 - Flags: review?(bas) → review-
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.
Attachment #8759564 - Flags: review?(bas) → review+
Attachment #8759565 - Flags: review?(bas) → review+
Attachment #8759567 - Flags: review?(bas) → review+
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.
Attachment #8760736 - Flags: review?(bas)
Attachment #8760736 - Flags: review?(bas) → review+
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.
Attachment #8764218 - Flags: review?(gwright)
Attachment #8764218 - Flags: review?(gwright) → review+
Pushed by nsilva@mozilla.com:
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
Keywords: leave-open
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
Flags: needinfo?(nical.bugzilla)
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.
Flags: needinfo?(nical.bugzilla)
Attachment #8766365 - Flags: review?(lsalzman)
Addresses the review comments.
Attachment #8759562 - Attachment is obsolete: true
Attachment #8766385 - Flags: review?(bas)
Attachment #8766365 - Flags: review?(lsalzman) → review+
Duplicate of this bug: 1008221
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
Attachment #8766385 - Flags: review?(bas) → review+
Pushed by nsilva@mozilla.com:
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
Depends on: 1284721
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.)
Flags: needinfo?(nical.bugzilla)
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.
Flags: needinfo?(nical.bugzilla)
Depends on: 1284856
Depends on: 1285207
Depends on: 1284705
Depends on: 1285263
Depends on: 1285730
Depends on: 1292870
You need to log in before you can comment on or make changes to this bug.