Closed Bug 1194891 Opened 9 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Platform:
Unspecified
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
platform-rel --- +
firefox43 --- affected
firefox55 --- verified

People

(Reporter: dholbert, Assigned: jwwang)

References

(
URL
)

Details

(Whiteboard: [platform-rel-Soundcloud])

Attachments

(4 files)

Bug 1194891. P1 - plumb 'isPrivateBrowsing' down to MediaCacheStream.
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1194891. P2 - don't write any data for a closed stream.
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1194891. P3 - don't share data for elements in PB mode.
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1194891. P4 - close all streams associated with private browsing windows when PB is done.
59 bytes, text/x-review-board-request
cpearce
: review+
Details 
+++ 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?
Flags: needinfo?(ehsan)
See Also: → 1193963
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.
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
Component: Audio/Video → Audio/Video: Playback
Mass change P2 -> P3
Priority: P2 → P3
OS: Linux → All
platform-rel: --- → ?
Whiteboard: [platform-rel-Soundcloud]
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)
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)
Thanks! I will give it a shot.
Attachment #8855612 - Flags: review?(cpearce)
Attachment #8855613 - Flags: review?(cpearce)
Attachment #8855614 - Flags: review?(cpearce)
Attachment #8855615 - Flags: review?(cpearce)
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 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 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 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 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+
(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)
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)
(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)
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.
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
(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)
Depends on: 1356514
Thanks! I will file a follow-up bug to fix that.
(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+
See Also: → 1390458
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: