New crash signature that is showing up only with e10s enabled: RtlEnterCriticalSection | PR_Lock | mozilla::MediaEventSourceImpl<T>::ConnectInternal<T> See https://crash-stats.mozilla.com/search/?signature=~RtlEnterCriticalSection+%7C+PR_Lock+%7C+mozilla%3A%3AMediaEventSourceImpl%3CT%3E%3A%3AConnectInternal%3CT%3E&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports for a list of recent crashes. These crashes are happening at poison+offset 0xe5e5e615 and so I'm going to file this as sec-sensitive and I think this needs to block M9.

Maire, can you find an assignee?

This is a playback bug. (The stack says this is called from MediaDecoderStateMachine::MediaDecoderStateMachine().) Anthony, Chris - I know this is public holiday for you; when you're back, can one of you find an owner for this? Thanks

I don't see any crashes after 2016-03-21 -- something fixed in 46.0b2? All the stacks I looked at had mozilla::OggDecoder::CreateStateMachine() in them. Any Ogg-specific fixes?

This isn't showing up at all in the current 46 apz experiment.

This looks like it's stopped happening in beta 46.

Also noteworthy; we've not changed anything significant in the Ogg code for a while.

jw: does the crash happening here make any sense to you?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #0) https://crash-stats.mozilla.com/search/?signature=~RtlEnterCriticalSection+%7C+PR_Lock+%7C+mozilla%3A%3AMediaEventSourceImpl%3CT%3E%3A%3AConnectInternal%3CT%3E&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports No results are returned from the search. I got 4 records from https://crash-stats.mozilla.com/search/?signature=~MediaEventSourceImpl&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports None of them are about 0xe5e5e...

Here's a search which includes date fields so you're not looking at just the last 7 days: https://crash-stats.mozilla.com/search/?signature=~RtlEnterCriticalSection+%7C+PR_Lock+%7C+mozilla%3A%3AMediaEventSourceImpl%3CT%3E%3A%3AConnectInternal%3CT%3E&date=%3E2016-03-09&date=%3C2016-03-22&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address#crash-reports I don't know whether this has gone away or changed signature.

It looks like we are using an object (MediaDecoderStateMachine::mAudioQueue) that is not initialized yet. But I wonder how that is possible... mAudioQueue should have been constructed by the time we invoke its methods in the constructor of MediaDecoderStateMachine. Is it possible to know whether vs2003 or 2005 is used for the builds?

We use vs2013 for all builds except m-c (which was only recently switched to vs2015u1)

I think it's much more likely that the MediaDecoderStateMachine is being destroyed in the middle of this method. I don't know what the MediaDecoderStateMachine::InitializationTask does, but is it possible that this new object is being destroyed on another thread even before the constructor is finished running?

Good point! I will try and see what will happen if failing to dispatch MediaDecoderStateMachine::InitializationTask in the constructor.

When failing to dispatch MediaDecoderStateMachine::InitializationTask, it is something like: MediaDecoderStateMachine::MediaDecoderStateMachine() { { RefPtr<MediaDecoderStateMachine> self = this; } // Boom! ~MediaDecoderStateMachine() runs before exiting constructor. } It is generally a bad idea to pass |this| somewhere in the constructor. It is even worse to addref/release a refcounted type in its constructor. I wonder if this can be caught automatically by static-analysis.

Bug 1259473 - per comment 14, move actions involving |this| to Init() from the constructor. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b21a7c9c8fe0

Comment on attachment 8739650 [details] [diff] [review] 1259473_dont_dispatch_this.patch Will try factory method.

MediaDecoder::CreateStateMachine has too many overloads. Will stick to the original plan. Since Chris is loaded with GMP, I will find jya for review.

Comment on attachment 8740258 [details] [diff] [review] 1259473_dont_dispatch_this.patch v2 Review of attachment 8740258 [details] [diff] [review]: ----------------------------------------------------------------- what a beauty ! I'm fairly certain I have seen other constructor dispatching a task to initialise themselves...

Thanks for the review!

https://hg.mozilla.org/mozilla-central/rev/52fd577f808f but - this was sec-high so should have sec-approval before that landed initially on inbound i guess

Comment on attachment 8740258 [details] [diff] [review] 1259473_dont_dispatch_this.patch v2 [Security approval request comment] How easily could an exploit be constructed based on the patch? Very hard according to the volume in crash reports. This seems to only happen during shutdown phase. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Which older supported branches are affected by this flaw? 40 and later. If not all supported branches, which bug introduced the flaw? Bug 1160064. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This bug seems to be manifested by e10s. Backports to aurora and beta should be enough. How likely is this patch to cause regressions; how much testing does it need? Very unlikely as the change is as simple as moving code around without introducing new logic. Treeherder should be sufficient.

[Tracking Requested - why for this release]: Can we get this uplifted to 47 too? We'd like it in the beta 47 e10s experiments as a stability fix.

Sec-approval+ now since it is already in. This should not have landed on inbound without sec-approval and we're now exposed...

Sorry. My head was dizzy and didn't notice this is a sec-high bug. It might be help to add a hg hook to detect there should be a "sec-approval=xxx" string in the commit message to prevent accidental commits.

No, we explicitly don't want sec-approval in the commit message to avoid drawing attention to the bug. Also, hg would have to somehow know that the bug met all the requirements for needing sec-approval, which would be non-trivial. https://wiki.mozilla.org/Security/Bug_Approval_Process

[Tracking Requested - why for this release]: kind of late to be taking in Fx46 but now the fix is public in the tree.

We don't need this on 46 now that e10s is disabled (and it won't be enabled for release)

jwwang, we have bug 1158178 which outlines something in the spirit of what you're suggesting to prevent people from accidentally checking in patches for sec-high or sec-critical bugs.

Thanks for the info. I will request uplift to 47.

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #28) > We don't need this on 46 now that e10s is disabled (and it won't be enabled > for release) I see no reason why this problem couldn't potentially occur without e10s.

It is more likely to happen in e10s I guess...

(In reply to JW Wang [:jwwang] from comment #30) > Thanks for the info. I will request uplift to 47. can we uplift this?

47 is beta now. Need to rebase before uplifting.

Approval Request Comment [Feature/regressing bug #]:unknown [User impact if declined]:rare crash might be experienced during shutdown when e10s is on. [Describe test coverage new/current, TreeHerder]:Has been on central for weeks without issues reported. Also tested on TreeHerder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c56aa907c0d1 [Risks and why]: Low. The fix basically move code around without changing logic. [String/UUID change made/needed]:none

Comment on attachment 8745815 [details] [diff] [review] 1259473_fix_beta_47.patch e10s crash fix, Beta47+