584238 - Add an API to clear the media cache

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[(no longer active)]

This is needed for private browsing support (see bug 572243 comment 5).

Comment 1

[(no longer active)]

It should be noted that the depending bug 572243 has been set to block betaN.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What should this API look like? Is there an observer notification that fires when we enter/leave private browsing mode? I suppose I could create an XPCOM service that exposes a single "clear media cache" API, but that would be a bit annoying.

I'm also not sure how this should affect media elements that are playing when you request the media cache to be cleared. Should they just break? Should we flush data but allow the element to put new data into the cache? Or is it OK to somehow keep around the data for those elements?

Comment 3

[(no longer active)]

(In reply to comment #2)
> What should this API look like? Is there an observer notification that fires
> when we enter/leave private browsing mode? I suppose I could create an XPCOM
> service that exposes a single "clear media cache" API, but that would be a bit
> annoying.

Yes, the "private-browsing" topic with "enter" or "exit" as its data.  I could write the patch, I just don't know what the correct internal API to call is to empty the media cache.  If we choose to go through this route, then this bug can just be marked as a dupe of bug 572243 I think.

> I'm also not sure how this should affect media elements that are playing when
> you request the media cache to be cleared. Should they just break? Should we
> flush data but allow the element to put new data into the cache? Or is it OK to
> somehow keep around the data for those elements?

Generally, when we're leaving the private browsing mode, we close all open tabs and restore the user's old session, which means that we don't have to worry about media elements that are playing.

There are two concerns here though.  The first one is that add-ons may use media elements (possibly in their own UI) which survive the private browsing mode transitions.  The other concern is that we have a hidden pref which allows the session to remain untouched when we're exiting or entering the private browsing mode.

The latter is not something to worry about, because we don't officially support that pref, and it only exists to facilitate automated tests.  I'm not sure how to address the first concern though.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: add cache Flush() API

Completely untested, but this should flush the cache without breaking any existing elements, even if they're currently playing.

Ehsan, I'll leave the rest of this bug in your capable hands :-)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, you should probably add a call to QueueUpdate() in there.

Comment 6

[(no longer active)]

OK, so I'm stealing this from you.

Comment 7

[(no longer active)]

Comment on attachment 478191 [details] [diff] [review]
add cache Flush() API

Boris, is this something which you can review?

Comment 8

[Boris Zbarsky [:bzbarsky]]

I was going to say that roc should, but he wrote the patch.  I can do it if really needed, but I'd be happier if someone familiar with the media cache reviewed instead.

Comment 9

[(no longer active)]

Comment on attachment 478191 [details] [diff] [review]
add cache Flush() API

Fair enough.  I'll forward the request to Chris.  Chris, feel free to suggest other people if needed.

Comment 10

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

Comment on attachment 478191 [details] [diff] [review]
add cache Flush() API

I think Matthew has a better understanding of the media cache than I, passing it onto him.

Comment 11

[Matthew Gregan [:kinetik]]

+nsMediaCache::FlushInternal()
+{
[...]
+  PR_Close(mFD);
+  Init();
+}

What if |Init()| fails here?  |InitMediaCache()| handles this by deleting and NULLing gMediaCache, making it impossible to call nsMediaCache methods after |Init()| has failed.  In this case, |Init()| may fail silently, mFD is now bogus, and nothing prevents further nsMediaCache method calls.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We can't just null out gMediaCache since we already have nsMediaCacheStreams active which will continue calling methods on gMediaCache.

How about I set mFD to 0 if Init fails? Then all the I/O operations on mFD will return errors, but we already handle I/O errors.

Comment 13

[Matthew Gregan [:kinetik]]

Sounds good.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Flush() API v2

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/a4ac5ee6d3d1