1340197 - MP3 radio stream works only once in a Firefox session

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

+++ This bug was initially created as a clone of Bug #1326066 +++

This is not a relevant problem (very few person open the direct MP3 stream, usualy it is served trough the web online player), but it is strange and I report it.
Steps to reproduce:
1. Open a direct MP3 online radio stream, for example:
http://icestreaming.rai.it/1.mp3
http://icecast.unitedradio.it/Virgin.mp3
(NOte: the default volume of the second stream   is sometimes high)
2. Close the tab with the stream
3. Try to open the stream again

What happens?
Firefox try to connect to the stream without reaching it . No error messages are shown but the stream is not opened.

In Firefox ESR the problem is not present.
I have the problem on FDE on Windows, but some friends have confirmed it also on beta, nightly and with other platforms.

How to fix it?
Restart Firefox and the stream is reached.



Analyzes (https://bugzilla.mozilla.org/show_bug.cgi?id=1326066#c3):

The stream doesn't start to play on the second load because nsHttpChannel waits for entry completion. The first nsHttpChannel didn't finalize the entry because the channel was suspended right before the channel was canceled.


Solution: let the media code specify LOAD_BYPASS_CACHE_IF_BUSY load flag on the loading channel.

Comment 1

[Alastor Wu [:alwu]]

Hi, Honza,
You proposed the solution in comment0, specifying LOAD_BYPASS_CACHE_IF_BUSY flag in media code.
However, the media code has specified it [1] for a long time, but issue still exists.
Could you give me some other suggestion?
Thanks!

[1] http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/dom/html/HTMLMediaElement.cpp#1183

Comment 2

[Honza Bambas (:mayhemer)]

I'll take this bug, but unless explicitly said, I won't be diagnosing this for 57.

Comment 3

[Bulk Bug Changes for mreavy's org]

Mass change P1->P2 to align with new Mozilla triage process

Comment 4

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

Honza,
Any luck on this bug? Is this bug still a problem?

Comment 5

[Honza Bambas (:mayhemer)]

So, I looked once more to the log in bug 1326066.  The Michal's conclusion was put too vaguely so I cam to a wrong conclusion.

The problem is elsewhere, in the http code.  We don't respect BYPASS_WHEN_BUSY when the entry is in write progress by the previous channel.

It should be easy to fix.

Comment 6

[Honza Bambas (:mayhemer)]

No!  The problem really is in not setting the LOAD_BYPASS_LOCAL_CACHE_IF_BUSY on the channel.  In the log I can see it very clearly.

This is not a media bug, it's a broader thing.  The first channel is suspended and canceled.  It still keeps the entry, OK.  The second top level load channel is opened and, as it's NOT a media channel - it's a top level load channel and has no notion of loading a media yet, it IS NOT set the BYPASS_IF_BUSY flag...

From a user point of view we may want to specify LOAD_BYPASS_LOCAL_CACHE_IF_BUSY for all top-level loads or do some kind of a compromise (already have an idea for a patch).  When there is a concurrent and resumable response in progress we will do a concurrent cache streaming.  When the concurrent response is non-resumable, we wait.  And here we can make the change - when the second (depending) load is a top level load, just bypass this wait.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 8

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> This is not a media bug, it's a broader thing.  The first channel is
> suspended and canceled.  It still keeps the entry, OK.  The second top level
> load channel is opened and, as it's NOT a media channel - it's a top level
> load channel and has no notion of loading a media yet, it IS NOT set the
> BYPASS_IF_BUSY flag...

Maybe this patch fixes this particular load, but is it OK that cancelled channel related to a closed tab can keep the cache entry open so it's not usable by other channel?

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #8)
> (In reply to Honza Bambas (:mayhemer) from comment #6)
> > This is not a media bug, it's a broader thing.  The first channel is
> > suspended and canceled.  It still keeps the entry, OK.  The second top level
> > load channel is opened and, as it's NOT a media channel - it's a top level
> > load channel and has no notion of loading a media yet, it IS NOT set the
> > BYPASS_IF_BUSY flag...
> 
> Maybe this patch fixes this particular load, but is it OK that cancelled
> channel related to a closed tab can keep the cache entry open so it's not
> usable by other channel?

regardless of what an answer could be, I would rather separate it to yet another bug.  the fix I propose has other advantages, like when users opens the same page more than once and the response is not resumable we stream the second (and on) responses from network rather than waiting for completion of the first one.  it's likely to happen with streamed chunked responses that are naturally not resumable.

I've also made some preliminary objections against dooming from cancel at https://bugzilla.mozilla.org/show_bug.cgi?id=1326066#c6

to be honest, this is a media (or some combination) bug since we leak all top-level loading channels playing a media content and with it we block (are made to block) the cache entry.  i'll file a bug.

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #9)
> i'll file a bug.

bug 1421004


https://treeherder.mozilla.org/#/jobs?repo=try&revision=6594efabc6f07542a2ce7f386dd89a0a90e3ff79

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42fc2739a333
Don't wait for concurrent HTTP cache entry when the depending load is a top-level navigation. r=michal

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/42fc2739a333

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/38f49346a200
Backed out 1 changesets for failing reftests on /tests/dom/base/crashtests/504224.html r=backout a=backout on a CLOSED TREE

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 1 changesets (bug 1340197) for failing reftests on /tests/dom/base/crashtests/504224.html

https://treeherder.mozilla.org/logviewer.html#?job_id=148526830&repo=mozilla-inbound&lineNumber=2367

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=38f49346a200cc25492236c7b3c536fc835fe031

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=0e4e79435223abf6c5edf5320c11f3432b579fd3&filter-searchStr=87574beda25c5cecd334d247bf32817e4c9bea69&filter-tier=1

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Bogdan Tara[:bogdan_tara] from comment #14)
> Backed out 1 changesets (bug 1340197) for failing reftests on
> /tests/dom/base/crashtests/504224.html
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=148526830&repo=mozilla-
> inbound&lineNumber=2367
> 
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> central&revision=38f49346a200cc25492236c7b3c536fc835fe031
> 
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=0e4e79435223abf6c5edf5320c11f3432b579fd3&filter-
> searchStr=87574beda25c5cecd334d247bf32817e4c9bea69&filter-tier=1

This is a known failure (https://bugzilla.mozilla.org/show_bug.cgi?id=1385298), have I made it perma-failing?

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

It was perma-failing on Android.

https://mzl.la/2j8GPEJ

Comment 17

[Honza Bambas (:mayhemer)]

bug 1385298 is now fixed, we can reland.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16643f939e8f
Don't wait for concurrent HTTP cache entry when the depending load is a top-level navigation. r=michal

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/16643f939e8f