1259473 - [e10s] new crash with e10s enabled in MediaEventSourceImpl::ConnectInternal

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Benjamin Smedberg]

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.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Maire, can you find an assignee?

Comment 2

[Maire Reavy [:mreavy]]

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

Comment 3

[Daniel Veditz [:dveditz]]

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?

Comment 4

[Jim Mathies [:jimm]]

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

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

(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...

Comment 9

[Benjamin Smedberg]

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.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

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?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Benjamin Smedberg]

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?

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1259473_dont_dispatch_this.patch

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 16

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8739650 [details] [diff] [review]
1259473_dont_dispatch_this.patch

Will try factory method.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

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 18

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1259473_dont_dispatch_this.patch v2

Comment 19

[Jean-Yves Avenard [:jya]]

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...

Comment 20

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

Comment 21

[Carsten Book [:Tomcat]]

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 22

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 23

[Jim Mathies [:jimm]]

[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.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

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

Comment 25

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Daniel Veditz [:dveditz]]

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

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 30

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 31

[Jean-Yves Avenard [:jya]]

(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.

Comment 32

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 33

[Jim Mathies [:jimm]]

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

can we uplift this?

Comment 34

[JW Wang [:jwwang] [:jw_wang]]

47 is beta now. Need to rebase before uplifting.

Comment 35

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1259473_fix_beta_47.patch

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 36

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8745815 [details] [diff] [review]
1259473_fix_beta_47.patch

e10s crash fix, Beta47+

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4f8761914836