I saw the crash. It seems to be a regression of Bug 1411472. https://crash-stats.mozilla.com/report/index/9a14b729-7add-4eee-97ab-8bb470171121

I do not have a specific STR, but it should to be related to video playback and canvas2d.

attachment 8930344 [details] [diff] [review] borrows code from ShadowLayerForwarder::~ShadowLayerForwarder(). https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayers.cpp#221

(In reply to Sotaro Ikeda [:sotaro] from comment #0) > I saw the crash. It seems to be a regression of Bug 1411472. > > https://crash-stats.mozilla.com/report/index/9a14b729-7add-4eee-97ab-8bb470171121 I think my [@ nsObserverService::RemoveObserver ] crashes in bug 1418201 look similar.

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #5) > (In reply to Sotaro Ikeda [:sotaro] from comment #0) > > I saw the crash. It seems to be a regression of Bug 1411472. > > > > https://crash-stats.mozilla.com/report/index/9a14b729-7add-4eee-97ab-8bb470171121 > > I think my [@ nsObserverService::RemoveObserver ] crashes in bug 1418201 > look similar. Yes, it is same crash.

Why is a MediaFormatReader holding on to a WebRenderBridgeChild? This looks like a bug.

(In reply to Nicolas Silva [:nical] from comment #7) > Why is a MediaFormatReader holding on to a WebRenderBridgeChild? This looks > like a bug. MediaFormatReader holds WebRenderBridgeChild as layers::KnowsCompositor. The KnowsCompositor is used to know backend type since ImageBridge is not connected to specific Compositor. The KnowsCompositor is passed to PlatformDecoder and is used by the PlatformDecoder. One example is WMFVideoMFTManager::InitializeDXVA() https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/wmf/WMFVideoMFTManager.cpp#523

Holding a WebRenderBridgeChild from multiple threads just to get access to a bit of constant information like the compositor backend is very dangerous for very little gains. So I'm working on changing that by adding a atomic ref-counted object that contains the TextureFactoryIdentifier for the off-main-thread objects to hold on to. That way the WebRenderBridgeChild doesn't have to be held alive by any decoder but the decoders can get the bits information they need without causing a mess during shutdown.

Comment on attachment 8930344 [details] [diff] [review] patch - Add non-MainThread case handling Review of attachment 8930344 [details] [diff] [review]: ----------------------------------------------------------------- Marking r- as I am working on an alternative solution that should remove the need to do this.

Assign to :nical, since he is working for it as in Comment 10.

> I'm working on changing that by adding a atomic ref-counted object that contains the TextureFactoryIdentifier for the off-main-thread objects to hold on to. This is turning out to be a big and messy refactoring, so I am looking for an alternative solution. My understanding is that the DXVA manager asks the WebRenderBridgeChild rather than the ImageBridge about the backend because the former has a better chance of knowing the backend since it is tied to a specific compositor. On the other hand we don't support having several OMTC compositor backends at the same time. If we fail to create a renderer we fallback to a non-webrender backend for all widgets so it should be safe to ask the compositor backend to the ImageBridge, unless we plan to support having several compositor backends running at the same time that can receive video frames. Sotaro, am I missing something here? We should get the off-main-thread pieces of media code to only directly interact with the image bridge, either by providing all of the information it needs in the IBC or by providing it in some other thread-safe way. I am hopeful that we can do the former because the latter needs a lot of changes.

Looks like The media code holding non-thread-safe KnowsCompositor implementations may have been introduced 4 months ago in bug 1380234.

> On the other hand we don't support having several OMTC compositor backends at the same time. So apparently we started at least trying to support that scenario in order to support extensions that need to render a video in a widget that can't hardware acceleration because of transparency on windows. > so it should be safe to ask the compositor backend to the ImageBridge Scratch that, I'm going to try a simpler version of my initial attempt.

The idea is to have something that knows about the proper compositor backend oof the main thread without having to hold a strong reference to non-thread-safe actors like WebRenderBridgeChild, and at the same time provide access to the actor that will do the texture allocations (the ImageBridge in this case, hence the class being specific to media). Ideally we would be more explicit and pass a concrete struct to the media stuff instead of hiding it under KnowsCompositor, but my attempt at doing that bubbled up into a large refactoring so I turned to this instead. One thing to watch out for during review is that these new KnowsCompositor instances have their own serial (before that the media code would hold on to an actor directly so it would see the serial of WebRenderBridge or ImageBridge, or something else) so there is a potential change of behavior in that regard. I think that it is not an issue because the media code doesn't seem to be using the serial, but if it turns out to be a problem we can make it so that the serial is not constant and have this proxy use the serial of the ImageBridge (or would we use the serial of the WebRenderBridgeChild?). try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c99e7f6b31d9e818a2ce35494b105a9eec23ac35

Comment on attachment 8933248 [details] [diff] [review] Add a media-specific KnowsCompositor proxy usable off the main thread Review of attachment 8933248 [details] [diff] [review]: ----------------------------------------------------------------- Oops, forgot to set the review flag.

Comment on attachment 8933248 [details] [diff] [review] Add a media-specific KnowsCompositor proxy usable off the main thread Review of attachment 8933248 [details] [diff] [review]: ----------------------------------------------------------------- If we want to always destroy WebRenderBridgeChild on Main thread, the GetForMedia() needs to be called on Main thread. Since when the GetForMedia() is called off main thread within media framework, a ref of WebRenderBridgeChild is held off main thread. We also need to add SupportsD3D11() to KnowsCompositor, it was added by Bug 1405562 recently. ::: dom/media/platforms/PlatformDecoderModule.h @@ +121,5 @@ > void Set(UseNullDecoder aUseNullDecoder) { mUseNullDecoder = aUseNullDecoder; } > void Set(OptionSet aOptions) { mOptions = aOptions; } > void Set(VideoFrameRate aRate) { mRate = aRate; } > void Set(layers::KnowsCompositor* aKnowsCompositor) > { This function seems not a good place to call GetForMedia(), since ref of KnowsCompositor is already held at off main thread. Instead MediaDecoder::GetCompositor() is better place to call GetForMedia(). MediaDecoder::GetCompositor() is always called on main thread and after the function call, ref of KnowsCompositor is moved to off main thread. ::: gfx/layers/ipc/ShadowLayers.cpp @@ +222,5 @@ > +} > + > +RefPtr<KnowsCompositor> > +ShadowLayerForwarder::GetForMedia() > +{ Can we add ASSERT to check if current thread is main thread? ::: gfx/layers/wr/WebRenderBridgeChild.cpp @@ +630,5 @@ > > + > +RefPtr<KnowsCompositor> > +WebRenderBridgeChild::GetForMedia() > +{ Can we add ASSERT to check if current thread is main thread?

> If we want to always destroy WebRenderBridgeChild on Main thread, the GetForMedia() needs to be called on Main thread. Since when the GetForMedia() is called off main thread within media framework, a ref of WebRenderBridgeChild is held off main thread. I agree. I thought that the CreateDecoderParam struct was created on the main thread and sent to the media threads to be consumed there, I'll look into a better place to do this and add the assertion you suggested.

This version makes sure that GetForMedia is called on the main thread and that CreateDecoderParams only ever get KnowsCompositor objects that advertise being okay to hold from multiple threads.

Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/49b95b5bf515 Add a Proxy KnowsCompositor implementation that can be used off the main thread. r=sotaro

No more crash on nightly 59 since the patch landed. But still 61 crashes in beta 58 for FennecAndroid (see http://bit.ly/2jwk8dL).

Those Android crashes have a different stack; no WebRenderBridgeChild stack frame. I believe they are covered by https://bugzilla.mozilla.org/show_bug.cgi?id=1276919

Per comment #24, mark 58 won't fix.