I got crash in [@ mozilla::ipc::MessageChannel::AssertWorkerThread | mozilla::ipc::MessageChannel::CxxStackFrame::CxxStackFrame | mozilla::ipc::MessageChannel::Send | mozilla::layers::PLayerTransactionChild::SendShutdown ] when I was opening some amount (about 10) movies in tabs, only one was playing, other were inactive yet, after some time going to another tabs to see how movie downloading going, one of it crashes with the signature written above. I want to add that this is with: - e10s enabled - GPU process (layers) enabled - GPU process (media decoder) disabled [since I'm under diagnosing it for a other issue) Crashlog report: https://crash-stats.mozilla.com/report/index/58f9d934-644b-4777-85d1-5c9112170104 [Tracking Requested - why for this release]: Regression

Started with 20161221030226 build. Possible regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=567894f026558e6dada617a3998f29aed06ac7d8&tochange=c36fbe84042debef0a5d58b7fc88185b401762ce New crash regression that we will track for 53.

From the stack, it looks like MediaFormatReader is releasing some object off the main thread which causes a ShadowLayerForwarder to be destroyed, which tries to send a message, but it can't, because it is on the wrong thread.

David, anything related to your recent work, or something that will get better with your recent work?

In the commit range from comment 1, bug 1319992 looks like it touched related code. jya, could this crash be a regression from that bug? Thanks. (In reply to Milan Sreckovic [:milan] from comment #3) > David, anything related to your recent work, or something that will get > better with your recent work? I think the bug here is media code releasing a main thread object on some random other thread, rather than layers code failing to handle some case. That's just a hunch though.

(In reply to Andrew McCreight [:mccr8] from comment #4) > In the commit range from comment 1, bug 1319992 looks like it touched > related code. jya, could this crash be a regression from that bug? Thanks. > > (In reply to Milan Sreckovic [:milan] from comment #3) > > David, anything related to your recent work, or something that will get > > better with your recent work? > > I think the bug here is media code releasing a main thread object on some > random other thread, rather than layers code failing to handle some case. > That's just a hunch though. Agree, that's exactly what's happening. It's weird that the layers ref was "leaked" on the main thread without having forcefully called Disconnect. We try pretty hard not to do that.

(In reply to Andrew McCreight [:mccr8] from comment #4) > I think the bug here is media code releasing a main thread object on some > random other thread, rather than layers code failing to handle some case. > That's just a hunch though. Which object must be released on the main thread? Is it possible to add some assertions to catch that?

It doesn't have to be released on the main thread, though that would fix this bug if media can do that. The intent is that the main thread has to call Destroy() before it releases its last reference.

https://hg.mozilla.org/mozilla-central/annotate/57ac9f63fc69/gfx/layers/ipc/LayerTransactionChild.cpp#l37 Do you mean LayerTransactionChild::Destroy() should be called on the main thread?

Yeah exactly. Very strange that it's not.

http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/dom/media/MediaFormatReader.h#520 mKnowsCompositor will be released off the main thread if MediaFormatReader is deleted off the main thread. http://searchfox.org/mozilla-central/search?q=RefPtr%3Clayers%3A%3AKnowsCompositor%3E+mKnowsCompositor%3B&case=false&regexp=false&path= Seeing there are several places in media code that hold reference to KnowsCompositor, I would suggest ShadowLayerForwarder to provide a custom release function (NS_IMPL_RELEASE_WITH_DESTROY) to ensure the object is deleted on the main thread.

next crash with same STR - https://crash-stats.mozilla.com/report/index/0fe2e677-0059-4950-bf6c-c8c452170119

Tracking 54+ for the same reason in Comment 1.

dvander - can you handle this, perhaps with jwwang's idea from the comment above? THanks

Matt should weigh in on that approach.

Pushed by danderson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/843e450763b6 Only call LayerTransactionChild::Destroy on the main thread. (bug 1328633, r=mattwoodrow)

Please request Aurora approval on this when you get a chance.

Comment on attachment 8833598 [details] [diff] [review] fix Approval Request Comment [Feature/Bug causing the regression]: bug 1319992 [User impact if declined]: potential crashes [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: can't see any new nightly crashes on crash-stats so far [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: no [Is the change risky?]: no [Why is the change risky/not risky?]: This simply delays a resource being freed, so that it happens on the correct thread. Can't think of any potential risks. [String changes made/needed]:

Comment on attachment 8833598 [details] [diff] [review] fix Fix a crash. Aurora53+.

No crashes since Firefox 54.0a1 (2017-02-09) & Firefox 53 (2017-02-11) with [@ mozilla::dom::IdleRequest::IdleRun ] signature, so I'm marking this as VERIFIED.