792663 - BasicBuffer holds on to AutoOpenSurface buffers when it shouldn't

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

nrc found this.  Easy fix.

It's a violation of the new invariants in this code, but I don't believe it would cause any problems in the field.  Needs to be fixed though.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Don't SyncFrontBufferToBackBuffer() while a BufferTracker isn't around to revoke our buffer provider

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

An invariant in ThebesBuffers that are using mBufferProvider is, !mBufferProvider => !mBuffer.  When the code here was called outside of BasicShadowableThebesLayer::Paint(), then we don't use a buffer provider and there's no buffer tracker around the buffer set on the ThebesBuffer.  So we end up in a case where !mBufferProvider && mBuffer.

Comment 3

[Nick Cameron [:nrc]]

Comment on attachment 662796 [details] [diff] [review]
Don't SyncFrontBufferToBackBuffer() while a BufferTracker isn't around to revoke our buffer provider

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

Can we assert that mLayer->mBufferTracker is non-null in SyncFrontBufferToBackBuffer? Also can we assert that mBuffer is null in ThebesLayerBuffer::SetBufferProvider if aProvider is non-null?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Absolutely.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

With requested assertions

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ba44c7c860a

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

nrc, was going to ping on IRC --- did this incidentally fix the leak you were seeing?

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I ran all the crashtest-ipc and reftest-ipc tests locally and tested on a b2g device, but there were aborts on some mediastorage tests.  They're probably not using MOZ_LAYERS_FORCE_SHMEM_SURFACES, which is unfortunate because the shadow-layers code isn't in a shippable state on X11.  Oh well.  Backed out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0b08694ac6d

Comment 8

[Nick Cameron [:nrc]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> nrc, was going to ping on IRC --- did this incidentally fix the leak you
> were seeing?

Yes, it did, or at least a version modified inline with the refactorings did. Thanks!

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I wasn't able to reproduce the failure locally with bug 784244 applied.  Perhaps they need to be a pair.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

tryserver for this and bug 784244

https://tbpl.mozilla.org/?tree=Try&rev=f58910e8053c

and an additional push that hacks off xlib surfaces, in case some xlib-specific problem is the cause of the crashes

https://tbpl.mozilla.org/?tree=Try&rev=6e80c32f87bd

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Still broken on tryserver, but only on opt builds apparently.  I was testing debug locally.  That would be odd.

Still awaiting results with the patch that disables cross-process sharing of xlib surfaces.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Looks good so far.  Sorry xlib.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Temporary require opting in to allocating cross-process xlib surfaces

I know that there's a real bug here, but we're not shipping xlib shadow layers for a while and b2g is under a crunch.  We can hunt this down in a followup.

Will try to engineer a review steal while you enjoy your weekend :).

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is a transitive blocker now.

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> With requested assertions
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1ba44c7c860a

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> I ran all the crashtest-ipc and reftest-ipc tests locally and tested on a
> b2g device, but there were aborts on some mediastorage tests.  They're
> probably not using MOZ_LAYERS_FORCE_SHMEM_SURFACES, which is unfortunate
> because the shadow-layers code isn't in a shippable state on X11.  Oh well. 
> Backed out.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a0b08694ac6d

The mochitest-2 failure at
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1ba44c7c860a ,
the one marked with the backout changeset id, looks like bug 782505, but just happened to be during a different test.
The reason for that crash is described in bug 763449 comment 19.

Do you know why attachment 662796 [details] [diff] [review] would make that more common?

If there is a reason why switching to shmem surfaces would address that problem, then can you explain please?  Would DestroySharedSurface not destroy a shmem surface that is still in use by the other process?

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Karl Tomlinson (:karlt) from comment #15)
> Do you know why attachment 662796 [details] [diff] [review] would make that
> more common?
> 

I couldn't reproduce the crashes locally.  I think the patch just changes timing.  I don't have a guess beyond that.

> If there is a reason why switching to shmem surfaces would address that
> problem, then can you explain please?  Would DestroySharedSurface not
> destroy a shmem surface that is still in use by the other process?

Correct.  Each process has a "strong reference" to shmem through the mapped fd.

Comment 17

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #15)
> The mochitest-2 failure [...] looks like bug 782505, but
> just happened to be during a different test.

Actually it was the same test, so don't know why tbpl didn't suggest that bug.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 663630 [details] [diff] [review]
Temporary require opting in to allocating cross-process xlib surfaces

r+ based on comment 17, but this is not a prerequisite for landing attachment 662796 [details] [diff] [review].  Bug 782505 is a common failure, and I don't think there's any evidence that attachment 662796 [details] [diff] [review] made it more common.  However this patch is still a sensible solution to deal with bug 782505.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf7824a17b2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/780a4b8551e6

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cf7824a17b2b
https://hg.mozilla.org/mozilla-central/rev/780a4b8551e6