1423756 - Nullptr crash in MediaPipeline

Status: RESOLVED INCOMPLETE

(Core :: WebRTC, defect, P2)

Description

[Nils Ohlmeier [:drno]]

Low frequency null pointer crash in MediaPipeline: https://crash-stats.mozilla.com/report/index/822d599b-a9ed-4cf1-9723-4a6280171205

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: Bug 1423756: check for conduit before trying to use it.

Review commit: https://reviewboard.mozilla.org/r/206024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206024/

Comment 2

[Nils Ohlmeier [:drno]]

The idea behind the initial patch is that it should be easily upliftable to stop the bleeding.

Not sure if we can figure out the root cause and fix that as well.

Comment 3

[Nils Ohlmeier [:drno]]

The only point where conduit_ appears to be set to null is here https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#1259

Which means the destructor for PipelineListener ran on MainThread, and therefor the desctructor for MediaPipelineTransmit ran as well. What I fail to understand where this stuff gets connected to the MSG.
My assumption here is since the destructors ran MSG should no try to deliver any of this data any more.

bwc: do you have thoughts on why we get into this crash situation?

Comment 4

[Byron Campen [:bwc]]

Yeah, MSG should be holding onto a ref to the PipelineListener at this point, so it should not have been destroyed yet. Also, NotifyRealtimeTrackData is a virtual function; if PipelineListener had been destroyed, that vtable lookup should not have worked anyhow. Maybe there's some way we can init with a null conduit successfully?

Comment 5

[Byron Campen [:bwc]]

I don't see this happening on 59 at all; have we landed a fix there or something? The transceivers work was tripping over failure to create conduits in a different place, and now that _should_ be patched up.

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8935165 [details]
Bug 1423756: check for conduit before trying to use it.

https://reviewboard.mozilla.org/r/206024/#review211838

I fear that we're seeing some kind of undefined behavior here, so messing with it carries the risk of turning a safe nullptr crash into an unsafe one. On nightly, maybe this would be ok if it helped us diagnose what is going on, but I don't know if I like the idea of uplifting this.

Comment 7

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #5)
> I don't see this happening on 59 at all;

Well for some unknown reason this never crashed in Nightly very much. In the last 6 months I see:
- 2 crashes in 56 Nightly
- 6 crashes in 57 Nightly
- 2 crashes in 58 Nightly
- 0 crashes in 59 Nightly so far

> have we landed a fix there or
> something? 

I think it's to early in the Nightly cycle to assume that this got fixed in 59 somehow.

> The transceivers work was tripping over failure to create
> conduits in a different place, and now that _should_ be patched up.

Since the crash rate on beta is a lot higher it might make sense to wait for 59 to reach Beta and see what happens.

Comment 8

[Nils Ohlmeier [:drno]]

The only recent crashes happened with outdated version of Firefox, e.g. 55 or 56.
I see only a single crash in Fx 59, when it was still in the Nightly cycle.

I think it's safe to assume this got fixed through some other bug, for example the Transceivers landing in 59.