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

VERIFIED FIXED in Firefox 55

Status

()

Core
Audio/Video: Playback
P3
normal
VERIFIED FIXED
3 years ago
6 months ago

People

(Reporter: dholbert, Assigned: jwwang)

Tracking

Trunk
mozilla55
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(platform-rel +, firefox43 affected, firefox55 verified)

Details

(Whiteboard: [platform-rel-Soundcloud], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

3 years ago
+++ 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?
(Reporter)

Updated

3 years ago
Flags: needinfo?(ehsan)
(Reporter)

Updated

3 years ago
See Also: → bug 1193963

Comment 1

3 years ago
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.
Blocks: 572243
Flags: needinfo?(ehsan)
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

3 years ago
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.
Priority: -- → P2
Duplicate of this bug: 1122863
Component: Audio/Video → Audio/Video: Playback
Duplicate of this bug: 1242156
Duplicate of this bug: 1263898
Mass change P2 -> P3
Priority: P2 → P3
Duplicate of this bug: 1298916

Updated

a year ago
Duplicate of this bug: 1309642

Updated

a year ago
OS: Linux → All

Updated

a year ago
Duplicate of this bug: 1309105
platform-rel: --- → ?
Whiteboard: [platform-rel-Soundcloud]

Updated

11 months ago
platform-rel: ? → +
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?
Flags: needinfo?(bwu)
Thanks for this info. 
Ni JW to put this on his plate.
Flags: needinfo?(bwu) → needinfo?(jwwang)
(Assignee)

Comment 13

11 months ago
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?
Assignee: nobody → jwwang
Flags: needinfo?(jwwang) → needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
(Assignee)

Comment 15

11 months ago
Thanks! I will give it a shot.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8855612 - Flags: review?(cpearce)
Attachment #8855613 - Flags: review?(cpearce)
Attachment #8855614 - Flags: review?(cpearce)
Attachment #8855615 - Flags: review?(cpearce)

Comment 20

11 months ago
mozreview-review
Comment on attachment 8855612 [details]
Bug 1194891. P1 - plumb 'isPrivateBrowsing' down to MediaCacheStream.

https://reviewboard.mozilla.org/r/127464/#review130160
Attachment #8855612 - Flags: review?(cpearce) → review+

Comment 21

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

https://reviewboard.mozilla.org/r/127466/#review130162
Attachment #8855613 - Flags: review?(cpearce) → review+

Comment 22

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

https://reviewboard.mozilla.org/r/127468/#review130166
Attachment #8855614 - Flags: review?(cpearce) → review+

Comment 23

11 months ago
mozreview-review
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

11 months ago
mozreview-review
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().
Attachment #8855615 - Flags: review?(cpearce) → review+
(Assignee)

Comment 25

11 months ago
(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.
Flags: needinfo?(ehsan)
(Assignee)

Comment 26

11 months ago
https://archive.mozilla.org/pub/firefox/try-builds/jwwang@mozilla.com-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!
Flags: needinfo?(dholbert)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 31

11 months ago
(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.)
Flags: needinfo?(dholbert)

Comment 32

11 months ago
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/jwwang@mozilla.com-205221e9a8f5c44b93780f413dab808d4ec44a3d/try-win32/firefox-55.0a1.en-US.win32.installer.exe has fixed this problem for me.

Comment 33

11 months ago
Pushed by jwwang@mozilla.com:
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

11 months ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 35

10 months ago
(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.  :-)
Flags: needinfo?(ehsan)

Updated

10 months ago
Depends on: 1356514
(Assignee)

Comment 36

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

Comment 37

10 months ago
(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
Flags: qe-verify+
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
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
(Assignee)

Updated

6 months ago
See Also: → bug 1390458
You need to log in before you can comment on or make changes to this bug.