Closed Bug 910329 Opened 8 years ago Closed 5 years ago

BaseMediaResource::ModifyLoadFlags makes wrong assumptions about channel being in a loadgroup

Categories

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

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: mayhemer, Assigned: kinetik)

References

Details

Attachments

(1 file, 2 obsolete files)

It causes inconsistency of the nsLoadGroup::mForegroundCount counter, it may be decreased twice for the same channel.

Imagine following scenario:
- start a media resource load from some http://foo.bar/baz.ogg
- during progress, let the MediaResource move that load to background (i.e. set LOAD_BACKGROUND by call to BaseMediaResource::ModifyLoadFlags)
- this will drop the nsLoadGroup::mForegroundCount counter by 1
- the http load fails from whatever reason (network failure, cache read error)
- ChannelMediaResource::OnStopRequest:
  - *doesn't* remove the channel from its load group
  - drops the LOAD_BACKGROUND flag from the channel
  - *doesn't* re-add the channel back to the load group
- these 3 steps cause that the channel is not re-added as a foreground load to the load group
- when the http channel then removes it self from the loadgroup (it's done after call of the consumer's OnStopRequest), it will drop the nsLoadGroup::mForegroundCount again and produce inconsistency of the counter and of the load group it self too


Thus, BaseMediaResource::ModifyLoadFlags should first check whether the request is actually in the load group and not make assumptions based on the channel error state.  It's no problem to try to call RemoveRequest on a load group, it will just fail with NS_ERROR_FAILURE.
To make it yet a bit more clear, ChannelMediaResource::OnStopRequest *doesn't* remove/re-add the channel from its load group since state of the channel is a failure.
Component: Audio/Video → Audio/Video: Playback
Attached patch bug910329_v0.patch (obsolete) — Splinter Review
If I understand this correctly, I think this is what you're suggesting as a fix.  Hitting the "ForegroundCount messed up" assert happens with a stream failing after it's backgrounded before this change, and doesn't afterwards.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Attachment #8761059 - Flags: review?(odvarko)
Comment on attachment 8761059 [details] [diff] [review]
bug910329_v0.patch

I can't do this review. Perhaps you wanted to ask Jan Bambas (:mayhemer)?

Honza
Attachment #8761059 - Flags: review?(odvarko)
Comment on attachment 8761059 [details] [diff] [review]
bug910329_v0.patch

Review of attachment 8761059 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, that's who I meant to ask.  I bet that happens a lot. :-)
Attachment #8761059 - Flags: review?(honzab.moz)
Comment on attachment 8761059 [details] [diff] [review]
bug910329_v0.patch

Review of attachment 8761059 [details] [diff] [review]:
-----------------------------------------------------------------

I'll quickly check a final to-land version.

::: dom/media/MediaResource.cpp
@@ +1570,4 @@
>    }
>  
>    rv = mChannel->SetLoadFlags(aFlags);
>    NS_ASSERTION(NS_SUCCEEDED(rv), "SetLoadFlags() failed!");

maybe when you are here, NS_ASSERTIONS in this method to MOZ_ASSERT

::: media/libcubeb/src/cubeb_wasapi.cpp
@@ -891,5 @@
>  
>  bool
>  refill_callback_output(cubeb_stream * stm)
>  {
> -  bool rv;

please remove this change from this patch, it doesn't belong here.
Attachment #8761059 - Flags: review?(honzab.moz) → feedback+
Attached patch bug910329_v1.patch (obsolete) — Splinter Review
Attachment #8761059 - Attachment is obsolete: true
Attachment #8761406 - Flags: review?(honzab.moz)
Attachment #8761406 - Attachment is obsolete: true
Attachment #8761406 - Flags: review?(honzab.moz)
Attachment #8761449 - Flags: review?(honzab.moz)
Comment on attachment 8761449 [details] [diff] [review]
bug910329_v2.patch

Review of attachment 8761449 [details] [diff] [review]:
-----------------------------------------------------------------

thanks.
Attachment #8761449 - Flags: review?(honzab.moz) → review+
Pushed by mgregan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe7afe87a99
Don't use the channel status as a proxy for load group membership.  r=mayhemer
Blocks: 1276529
https://hg.mozilla.org/mozilla-central/rev/ebe7afe87a99
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Matthew - is this worth uplifting?
Flags: needinfo?(kinetik)
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #13)
> Matthew - is this worth uplifting?

It looks like the crashes reported in bug 1276529 have dropped off since the fix in this bug landed, so it does look like it's worth uplifting.
Flags: needinfo?(kinetik)
Comment on attachment 8761449 [details] [diff] [review]
bug910329_v2.patch

Approval Request Comment
[Feature/regressing bug #]: bug 633051
[User impact if declined]: potential crash during video load if a network error occurs
[Describe test coverage new/current, TreeHerder]: covered by existing tests (test_closing_connections.html)
[Risks and why]: simple patch, low risk.  potential regressions could be related to video behaviour with BF cache.
[String/UUID change made/needed]: none
Attachment #8761449 - Flags: approval-mozilla-aurora?
Comment on attachment 8761449 [details] [diff] [review]
bug910329_v2.patch

May fix some crashes in 49, OK to uplift.
Attachment #8761449 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.