1194891 - Don't clear media cache when closing a private browsing window (or else e.g. soundcloud will have blips or pauses)

Status: VERIFIED FIXED

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

Description

[Daniel Holbert [:dholbert]]

+++ This bug was initially created as a clone of Bug #1193963 +++
(Spinning off this bug to cover the short pauses/blips described on bug 1193963 which seem to be caused by media cache being cleared)

STR:
 1. Visit https://soundcloud.com/kapslap/summer-mix-2015
   (This should start playing audio.)

 2. Open a private browsing window (Ctrl shift p)

 3. Close the private browsing window (Ctrl w)


ACTUAL RESULTS: Brief pause (or sudden blips of incorrect audio) in the SoundCloud playback from your main window.

EXPECTED RESULTS: No such pauses/blips.

Quoting jya from bug 1193963 comment 22:
> The core issue is that the media cache appears to be flushed
> when closing any private window.
> What's the rationale behind that?
> Why aren't private windows using their own media cache instead.

ehsan, perhaps you know the answer to this and whether this is possible?

Comment 1

[(no longer active)]

We originally did this in bug 572243, and we did that because it was easier than splitting the media cache into two, but ideally we would do that if this helps fixing the issue in comment 0.

Comment 2

[Jean-Yves Avenard [:jya]]

tbh, I don't understand what causes the blip during audio playback. A pause I can understand. But a blip ?

unless somehow the content once re-read didn't match what was there before (like data got shifted somehow).

But yeah, as a work-around not clearing the main media cache would certainly help.

Comment 3

[(no longer active)]

Yeah.  We may also very well find a better solution to fix this bug without separating the two caches, I think that would be fine too as far as I'm concerned.

Comment 7

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

Mass change P2 -> P3

Comment 11

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

Blake - this falls in the gap between private browsing and mediacache. I can see how it would be problematic for people using the browser for listening to music. We should try to fix this long standing issue. Do you have someone who can put this in their queue?

Comment 12

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

Thanks for this info. 
Ni JW to put this on his plate.

Comment 13

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

My initial idea is to associate each block stored in MediaCache with a flag to indicate whether this block belongs to a private window. Then we can only clear those private blocks when "last-pb-context-exited" is received.

Hi Ehsan,
Is there any way to tell if a media element belongs to a private window or not?

Comment 14

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

I'll answer this since I know Ehsan is very busy...

We do this in the GMP/EME code. We get the origin string from the nsIScriptObjecctPrincipal of the object's parent object:

https://dxr.mozilla.org/mozilla-central/rev/720b9177c6856c1c4339d0fac1bf5149c0d53950/dom/media/eme/MediaKeys.cpp#411

And then call OriginAttributes::IsPrivateBrowsing() to see whether it represents a context in private browsing mode.

It also looks like you may be able to just check if nsIPrincipal::GetPrivateBrowsingId()==0 directly to tell whether you are not in private browsing mode.

Comment 15

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

Thanks! I will give it a shot.

Comment 16

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

Attachment: Bug 1194891. P1 - plumb 'isPrivateBrowsing' down to MediaCacheStream.

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

Comment 17

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

Attachment: Bug 1194891. P2 - don't write any data for a closed stream.

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

Comment 18

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

Attachment: Bug 1194891. P3 - don't share data for elements in PB mode.

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

Comment 19

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

Attachment: Bug 1194891. P4 - close all streams associated with private browsing windows when PB is done.

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

Comment 20

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

Comment on attachment 8855612 [details]
Bug 1194891. P1 - plumb 'isPrivateBrowsing' down to MediaCacheStream.

https://reviewboard.mozilla.org/r/127464/#review130160

Comment 21

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

Comment on attachment 8855613 [details]
Bug 1194891. P2 - don't write any data for a closed stream.

https://reviewboard.mozilla.org/r/127466/#review130162

Comment 22

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

Comment on attachment 8855614 [details]
Bug 1194891. P3 - don't share data for elements in PB mode.

https://reviewboard.mozilla.org/r/127468/#review130166

Comment 23

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

Comment on attachment 8855612 [details]
Bug 1194891. P1 - plumb 'isPrivateBrowsing' down to MediaCacheStream.

https://reviewboard.mozilla.org/r/127464/#review130176

::: dom/html/HTMLMediaElement.cpp:4670
(Diff revision 1)
>      return NS_ERROR_FAILURE;
>    }
>  
>    LOG(LogLevel::Debug, ("%p Created decoder %p for type %s", this, decoder.get(), mimeType.get()));
>  
> -  RefPtr<MediaResource> resource =
> +  bool isPrivateBrowsing = NodePrincipal()->GetPrivateBrowsingId() > 0;

It seems most people use nsILoadContext::UsePrivateBrowsing() instead of checking the private browsing Id.

We should probably follow convention here. It looks like we can use nsContentUtils::IsInPrivateBrowsing() to get the load context.

Comment 24

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

Comment on attachment 8855615 [details]
Bug 1194891. P4 - close all streams associated with private browsing windows when PB is done.

https://reviewboard.mozilla.org/r/127470/#review130178

::: dom/media/MediaCache.cpp:363
(Diff revision 1)
>  
>  NS_IMETHODIMP
>  MediaCacheFlusher::Observe(nsISupports *aSubject, char const *aTopic, char16_t const *aData)
>  {
>    if (strcmp(aTopic, "last-pb-context-exited") == 0) {
> -    MediaCache::Flush();
> +    if (gMediaCache) {

The existing code has the convention of null checking inside a static function rather than before calling a non-static function. For example MediaCache::Flush().

Comment 25

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

(In reply to Chris Pearce (:cpearce) from comment #23)
> It seems most people use nsILoadContext::UsePrivateBrowsing() instead of
> checking the private browsing Id.
> 
> We should probably follow convention here. It looks like we can use
> nsContentUtils::IsInPrivateBrowsing() to get the load context.

It is kinda confusing to have many ways to do one thing. We have at least 3 ways check PB in HTMLMediaElement:
1. NodePrincipal()->GetPrivateBrowsingId() > 0
2. nsContentUtils::IsInPrivateBrowsing(OwnerDoc())
3. OwnerDoc()->GetLoadContext()->UsePrivateBrowsing()

Hi Ehsan,
Is any of them preferred? If not, I will just pick 2 and let the helper handle the chores for me.

Comment 26

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

https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-205221e9a8f5c44b93780f413dab808d4ec44a3d/try-win32/firefox-55.0a1.en-US.win32.installer.exe

Hi Daniel,
Since I can't repro the issue, can you try if this build fix the problem for you? Thanks!

Comment 27

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

Comment on attachment 8855612 [details]
Bug 1194891. P1 - plumb 'isPrivateBrowsing' down to MediaCacheStream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127464/diff/1-2/

Comment 28

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

Comment on attachment 8855613 [details]
Bug 1194891. P2 - don't write any data for a closed stream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127466/diff/1-2/

Comment 29

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

Comment on attachment 8855614 [details]
Bug 1194891. P3 - don't share data for elements in PB mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127468/diff/1-2/

Comment 30

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

Comment on attachment 8855615 [details]
Bug 1194891. P4 - close all streams associated with private browsing windows when PB is done.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127470/diff/1-2/

Comment 31

[Daniel Holbert [:dholbert]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #26)
> Hi Daniel,
> Since I can't repro the issue, can you try if this build fix the problem for
> you? Thanks!

Sorry, I can't repro in current Nightly (without the patch), so I won't be much help in verifying the fix. This must've gotten fixed (or much harder to trigger) at some point in the time since it was filed, I guess...

If you want to followup further on reproducibility / what fixed this, perhaps see if you can reproduce with a nightly from around when this was filed, via e.g. mozregression --launch 2015-08-20 .  I can reproduce quite easily with that build, at least.

(I tried a brief "mozregression --find-fix" run to see if I could identify the fix, and I ended up with https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3cc3b1968524248450c465c4ea2ee5596ffa65f2&tochange=0eaf345983b3afc2b426e25a3be93ebf0d93e6c1  , but I'm not at all confident about that range because it's pretty hard to trigger in builds at the start of that range, and I can still very-rarely trigger blips in the "fixed" version at the end of that range by performing the STR. If that happens to be a valid fix range, though, then it would suggest that bug 1223599 might've helped here.  But again, not at all confident about that range.)

Comment 32

[michal256]

Hi,

I was able to reproduce this bug on version Firefox 52.0.2 64-bit and 32-bit, when listening to radio stream:
http://www.antyradio.pl/sluchaj/online/Antyradio-Ballads.html

Running version https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-205221e9a8f5c44b93780f413dab808d4ec44a3d/try-win32/firefox-55.0a1.en-US.win32.installer.exe has fixed this problem for me.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29565a381d50
P1 - plumb 'isPrivateBrowsing' down to MediaCacheStream. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/3fd356d16072
P2 - don't write any data for a closed stream. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/9ab67769cf0a
P3 - don't share data for elements in PB mode. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/0d2ab3612342
P4 - close all streams associated with private browsing windows when PB is done. r=cpearce

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29565a381d50
https://hg.mozilla.org/mozilla-central/rev/3fd356d16072
https://hg.mozilla.org/mozilla-central/rev/9ab67769cf0a
https://hg.mozilla.org/mozilla-central/rev/0d2ab3612342

Comment 35

[(no longer active)]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #25)
> (In reply to Chris Pearce (:cpearce) from comment #23)
> > It seems most people use nsILoadContext::UsePrivateBrowsing() instead of
> > checking the private browsing Id.
> > 
> > We should probably follow convention here. It looks like we can use
> > nsContentUtils::IsInPrivateBrowsing() to get the load context.
> 
> It is kinda confusing to have many ways to do one thing. We have at least 3
> ways check PB in HTMLMediaElement:
> 1. NodePrincipal()->GetPrivateBrowsingId() > 0
> 2. nsContentUtils::IsInPrivateBrowsing(OwnerDoc())
> 3. OwnerDoc()->GetLoadContext()->UsePrivateBrowsing()
> 
> Hi Ehsan,
> Is any of them preferred? If not, I will just pick 2 and let the helper
> handle the chores for me.

So sorry for my delay.  The right way is #1, the other two are older APIs that we want to gradually switch away from.  Unfortunately I was too slow to get back to you here so the addition of nsContentUtils::IsInPrivateBrowsing() call is my fault.  :-)

Comment 36

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

Thanks! I will file a follow-up bug to fix that.

Comment 37

[(no longer active)]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #36)
> Thanks! I will file a follow-up bug to fix that.

Oh I already did: bug 1356514

Comment 38

[Ciprian Georgiu, Desktop QA]

I've managed to reproduce this issue using an affected Nightly build (2015-08-14).

This issue is verified fixed on 55 beta 2 (20170615133456) across platforms:
- Windows 10 x64
- Mac OS X 10.11.6
- Ubuntu 16.04 x64 LTS