1276529 - Crash in mozilla::BaseMediaResource::ModifyLoadFlags

Status: RESOLVED DUPLICATE of bug 1329403

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

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

This bug was filed from the Socorro interface and is 
report bp-245d5eae-ede9-4e06-bd0e-252ca2160529.
=============================================================

There are a decent number of these ModifyLoadFlags crashes across all channels:
https://crash-stats.mozilla.com/signature/?product=Firefox&date=%3E%3D2016-05-01&signature=mozilla%3A%3ABaseMediaResource%3A%3AModifyLoadFlags&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=email&_columns=url&_columns=user_comments&page=2

A bugzilla search makes me wonder if they're at all related to the issue in bug 910329.

In any case, if you have private crash-stats access, the URLs for these crashes actually look generally useful; they seem to point to various video sites, the most common of which has URLs of the form (from a different crash):
 http://layarkaca21.tv/london-fallen-2016/2/
 http://layarkaca21.tv/captain-america-civil-war-2016/
etc.

I debugged the minidump for bp-245d5eae-ede9-4e06-bd0e-252ca2160529 , and the problem there is that (as the MSVC dialog says):
this->mChannel.mRawPtr was nullptr.

Is it expected that the call to loadGroup->RemoveRequest() can cause this->mChannel to become null?  Because it certainly looks like that's what's happening.


This isn't at the top of the topcrash list, but seems like it might be straightforward to fix and is worth looking into.

Comment 1

[Matthew Gregan [:kinetik]]

(In reply to David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) (vacation June 4-12) from comment #0)
> A bugzilla search makes me wonder if they're at all related to the issue in
> bug 910329.

That seems plausible, but I didn't dig enough to prove it's the cause.  I posted a patch on that bug, since it (seems) simple and we can see if it solves this problem.

> Is it expected that the call to loadGroup->RemoveRequest() can cause
> this->mChannel to become null?  Because it certainly looks like that's
> what's happening.

I think we only clear mChannel when the channel is closed, which we don't expect to happen as a result of removing the request from the load group.

Comment 2

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

Looks like Matthew handled this, so I'm clearing my ni?.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'mozilla::BaseMediaResource::ModifyLoadFlags':
 - nightly (version 50): 4 crashes from 2016-06-06.
 - aurora  (version 49): 30 crashes from 2016-06-07.
 - beta    (version 48): 392 crashes from 2016-06-06.
 - release (version 47): 1115 crashes from 2016-05-31.
 - esr     (version 45): 123 crashes from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          0          0          1          3          0          0          0
 - aurora           3          1          3          5          5         12          1
 - beta            63         49         51         68         54         66         20
 - release        170        139        182        157        169        195         46
 - esr             19          4          5         15          9         15         10

Affected platforms: Windows, Mac OS X, Linux

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Matthew - does bug 910329 fix the issue?

Comment 5

[Matthew Gregan [:kinetik]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #4)
> Matthew - does bug 910329 fix the issue?

Doesn't look like it, there are still crashes (null mChannel) showing up in 50.0a2/20160822004020 builds, e.g.: https://crash-stats.mozilla.com/report/index/6c4cbdb8-0b65-4b78-8c7d-fb2322160823

Comment 6

[Blake Wu [:bwu][:blakewu]]

JW, 
Chris is always busy. :-) 
Can you help on this one?

Comment 7

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

http://searchfox.org/mozilla-central/rev/0f254a30d684796bcc8b6e2a102a0095d25842bb/dom/media/MediaResource.cpp#1561

Is it possible for nsLoadGroup::RemoveRequest() to spin the event loop of the main thread such that something funny happens in between?

I wonder if this has something to do with bug 1329403.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #7)
> http://searchfox.org/mozilla-central/rev/
> 0f254a30d684796bcc8b6e2a102a0095d25842bb/dom/media/MediaResource.cpp#1561
> 
> Is it possible for nsLoadGroup::RemoveRequest() to spin the event loop of
> the main thread such that something funny happens in between?

This is very hard to answer, but there certainly is a possibility.  If the request being removed is the last foreground request, we trigger OnStopRequest on the listener of the load group.  That can do number of things.

Personally, I would forbid/remove/burn any API allowing spinning the thread loop this way.  But it's of course not that easy.

So, to answer - yes, it's possible.

> 
> I wonder if this has something to do with bug 1329403.

Didn't check that bug much deeply.  I don't have answer to that, sorry.