1329432 - Intermittent dom/media/test/test_background_video_suspend.html | application crashed [@ RefPtr<mozilla::layers::ImageClient>::~RefPtr()][@ mozilla::layers::ImageLayerComposite::~ImageLayerComposite()]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Treeherder Bug Filer]

Filed by: rvandermeulen [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=67002778&repo=mozilla-inbound

https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-macosx64-debug/1483800355/mozilla-inbound_yosemite_r7-debug_test-mochitest-media-e10s-bm106-tests1-macosx-build946.txt.gz

Comment 1

[Vincent Liu[:vliu]]

From low log, an assertion failure happens by dupping release on ImageClient.

08:55:40     INFO - Assertion failure: int32_t(mRefCnt) > 0 (dup release), at /builds/slave/m-in-m64-d-0000000000000000000/build/src/obj-firefox/dist/include/mozilla/layers/CompositableClient.h:75 

ImageClient was created by ImageContainer and possibly held reference by SharedPlanarYCbCrImage or SharedRGBImage. 
Since they all allocate another RefPtr to hold ImageClient, it won't be a problem if SharedPlanarYCbCrImage or SharedRGBImage was destroyed by some reasons.
Other possible was someone operated release() to reduce reference count directly. But from code tracking, I don't see this.
The last possible would be memory corruption.

Soraro, can you please have more input to figure out the possible reason if I have any missing? Really thanks.

Comment 2

[Sotaro Ikeda [:sotaro]]

It seems like a regression of Bug 1325784. A pointer of ImageContainer in mImageContainers might become invalid already.

Comment 3

[Sotaro Ikeda [:sotaro]]

Conflict between ImageContainer::~ImageContainer() and ImageBridgeChild::RecvDidComposite() seems not work well.

ImageBridgeChild::RecvDidComposite() tries to keep ref count of ImageContainer. But When ImageContainer::~ImageContainer() is called, ref count became already 0. Increment from 0 does not work and could cause dup release.

Comment 4

[Sotaro Ikeda [:sotaro]]

Gecko does not support multi threaded weak pointer. And mImageContainers needs to hold raw pointers of ImageContainer to prevent leaking ImageContainer.

Simple solution seems to use lock to prevent to exit ImageContainer::~ImageContainer() during calling imageContainer->NotifyComposite().

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Holds mContainerMapLock during calling NotifyComposite()

Comment 7

[Sotaro Ikeda [:sotaro]]

attachment 8825634 [details] [diff] [review] seems not work well with GPU process restart :( If there is gpu process restart, there is a risk of dead lock.

Comment 8

[Sotaro Ikeda [:sotaro]]

AssertNotCurrentThreadIn() is not implemented yet. There is no way to check if monitor is not locked.
  https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/ReentrantMonitor.h#136

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Holds mContainerMapLock during calling NotifyComposite()

Addressed potential dead lock.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Holds mContainerMapLock during calling NotifyComposite()

correct patch.

Comment 11

[Sotaro Ikeda [:sotaro]]

attachment 8825791 [details] [diff] [review] addressed the potential dead lock. But it could be broken easily. It seems better to try another way.

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> attachment 8825791 [details] [diff] [review] addressed the potential dead
> lock. But it could be broken easily. 

Since AssertNotCurrentThreadIn() is not implemented yet as comment 8.

Comment 15

[Sotaro Ikeda [:sotaro]]

ImageContainer is declared as to support weak pointer. But it could be used only when ImageContainer is used as non multi-threaded usage.

> class ImageContainer final : public SupportsWeakPtr<ImageContainer>

ImageContainer's weak pointer is used in RasterImage.
  https://dxr.mozilla.org/mozilla-central/source/image/RasterImage.h#409

gecko does not support thread safe weak pointer. It was removed by Bug 923554. There is a bug to support thread safe weak pointer as Bug 1258263. But it is not clear when it is addressed.

Comment 16

[Sotaro Ikeda [:sotaro]]

We could not use the following to address the problem for now.
- Use multi-threaded weak pointer(Bug 1258263)
- Use AtomicRefCountedWithFinalize as ancestor of ImageContainer and use recycle callback.
    We could not replace the ancestor since weak pointer is used already used by RasterImage.

attachment 8825791 [details] [diff] [review] seems like simple fix, but we do not have a way to check the possible dead lock in future, since AssertNotCurrentThreadIn() is not implemented yet(Bug 1027772). It seems not be implemented even in future.

Comment 17

[Sotaro Ikeda [:sotaro]]

Another way to address the problem is to move ImageContainer::NotifyComposite() related stuffs to a different refcounted object.

Comment 18

[Sotaro Ikeda [:sotaro]]

:nical, what do you prefer to address the problem?

Comment 20

[Nicolas Silva [:nical]]

I hate WeakPtr :(

(In reply to Sotaro Ikeda [:sotaro] from comment #18)
> :nical, what do you prefer to address the problem?

Is this issue affecting all Image implementations? It looks like this unfortunate bi-directional dependency between Image and ImageContainer is true for most image classes since they implement GetImageContainer.

I would rather not rely on the recycle callback mechanism because it is already a fair bit complicated so I'd prefer to not make it have to support too many use cases.
That's my preference but if you feel strongly for doing it this way I'm nit going to oppose it.
I'm fine with the other solutions. Ideally we would not have this bi-directional dependency between Image and ImageContainer (In my opinion it is asking for trouble) but I have no idea how much work it would be to change that so I don't want to corner you with that approach.

Comment 23

[Sotaro Ikeda [:sotaro]]

It seems simpler if ImageContainer::NotifyComposite() is split to different ref counted object.

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add ImageContainerListener

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add ImageContainerListener

Fix build failure.

Comment 26

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df958f5c096f0a540f5953cc3d23ea84ce3d0de3&selectedJob=68711292

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b5b5d24d975
Add ImageContainerListener r=nical

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6b5b5d24d975

Comment 32

[Intermittent Failures Robot]

7 failures in 777 pushes (0.009 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 5
* mozilla-inbound: 2

Platform breakdown:
* osx-10-10: 7

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1329432&startday=2017-03-13&endday=2017-03-19&tree=all