486646 - nsMediaChannelStream::OnStopRequest nulls out mChannel too early

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

This causes GetPrincipal() to return null after the load completes.  This causes the two video canvas reftests to fail with a security error.  The tests have been marked random for now, so they need to be reenabled when this bug is fixed.

Comment 2

[Honza Bambas (:mayhemer)]

Are STR from that bug reliable way to reproduce?

Comment 3

[Matthew Gregan [:kinetik]]

The steps from that bug can be simplified:

1. Visit http://tapper-ware.net/devel/js/JS.StereoVideo/index2.xhtml
2. Wait for the video to completely download
3. Click play

You should see the canvas below the video update as the video plays.  Instead, due to this bug, the canvas won't update, and you will see security errors logged in the error console.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Note that nulling out the channel is probably correct (though not really necessary to avoid leaks).  The real issue is not saving off the principal before doing that.

Comment 6

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

Does the channel start returning null from GetPrincipal?

Comment 7

[Boris Zbarsky [:bzbarsky]]

No, if you have an nsIChannel, even one that's done getting data, you can get its principal via the security manager API.

So if this code did hold on to mChannel that would certainly fix the principal issue.

Comment 8

[Honza Bambas (:mayhemer)]

I have a patch for this. Soon will submit after I check there are no leaks/crashes.

Comment 9

[Honza Bambas (:mayhemer)]

Hmm... when I run mochitest and open the URL in the mochitest profile (i.e. clear one) video is not streaming (there is no pre-load). In a completely new profile it works. Having trunk build few hours old from now. Is it intention, some setting for mochitest environment? user_pref("media.cache_size", 100)?

Comment 10

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

That setting is to provide better testing of the media cache. Video should still play correctly though, what exactly is the problem you're seeing?

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to comment #10)
> That setting is to provide better testing of the media cache. Video should
> still play correctly though, what exactly is the problem you're seeing?

OK, just checked it's not caused by my patch. When I open the URL from comment 3, video first frame appears. Then, when I do this in a clean profile I can see the buffer indicator is growing as the whole video file is being downloaded, immediately after I open the URL. But, when I open the URL in mochitest profile, the video file is not being automatically downloaded; only after I press play button it starts to download and play with it.

Comment 12

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

In the mochitest profile, a small amount of data will be downloaded and then we'll pause because the media cache is full. Then when you press play we'll be able to discard some data and continue the download. This is intended behaviour when the cache size is small.

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v1

I'm saving mPrincipal in OnStartRequest(). I am not sure if you expect this method be called only on the main thread.

Comment 14

[Boris Zbarsky [:bzbarsky]]

So what happens with the principal in the cached-data case, then?

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to comment #14)
> So what happens with the principal in the cached-data case, then?

cache-data case means to read data from mCacheStream or when an underlying channel is loading from a cache, e.g. an http cache?

Comment 16

[Boris Zbarsky [:bzbarsky]]

The former: the case where you're clearing mPrincipal and mChannel in this patch.

Comment 17

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

I'm confused ... where is he clearing mPrincipal and mChannel in this patch? The only place I see is in nsMediaFileStream::Close, which seems OK ... nsMediaFileStream is for file: URIs and has nothing to do with caching...

Comment 18

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

Comment on attachment 373115 [details] [diff] [review]
v1

This is fine as far as I'm concerned but I'd like Boris to sign off too

Comment 19

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  I was fooled by the NS_ERROR_PARSED_DATA_CACHED nsresult used there.

I'll look in more detail tomorrow, assuming I get through all of Blake's wrapper stuff...

Comment 20

[Honza Bambas (:mayhemer)]

(In reply to comment #16)
> The former: the case where you're clearing mPrincipal and mChannel in this
> patch.

Sorry for later answer. As I understand how the media channel works it opens an underlying channel (a request) and caches the content in mCacheStream, that is actually a buffer. When user wants to play the media a decoder is using data in the cache stream. When there are not any data in the buffer for a segment user wants to play media channel opens a new request and fills the buffer - the cache stream. During all this time there is still mPrincipal filled (from the moment we get OnStartRequest) and if not it is always attempted to get it from a channel if there is a channel (as before).

I didn't find another place where the channel is cleared (and where mPrincipal should be discarded) then in nsMediaFileStream::Close. I am not sure it's the best place but as I see the code it seems appropriate to clear it on that place.

Comment 21

[Boris Zbarsky [:bzbarsky]]

So the point is that whenever we're doing Close() there that means we have a new channel we're using to load data?

Note that for this to work correctly for local files, the media code probably needs to be setting some owners on those channels; that should likely be a separate bug.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I guess a better question is... where is nsMediaFileStream::Close called from?  And wouldn't it make more sense to clear mPrincipal when assigning a new non-null mChannel if that's why it's ok to clear it?

Comment 23

[Honza Bambas (:mayhemer)]

We are assigning a new channel in nsMediaChannelStream::CacheClientSeek. :Close is called when decoder is being shut down what happens during removal of the media element or page close, not sure when src of video element is changed by a script.

The patch is wrong. I'd say we should null mPrincipal in all Close method implementations. When element is somehow replaced or URL is replaced channel may for some time keep a wrong principal.

bz, what do think?

Comment 24

[Boris Zbarsky [:bzbarsky]]

I have no idea.  How does the Close() method relate to the ability to get data out of the video element.

Whenever one can get data out of the element, we need to be able to get the principal of that data.  When the element has no data, it's fine to be returning null.

Comment 25

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

Close() is only called when we shut down the decoder. At that point we should not be painting video to the screen or the canvas anymore. So it's OK to null things out there and not return a principal. nsMediaChannelStream::CloseChannel() happens during CacheClientSeek, but we create a new channel there right away so setting the channel to null in CloseChannel shouldn't be a problem.

I think the main issue for this bug was just nulling out mChannel in OnStopRequest. If we just stop doing that, this bug should be fixed.

A deeper issue is that data in the cache might not all be associated with the same principal. We might read some data with one principal, seek and get redirected, then read data for some other principal. That's really a different bug; I'm not sure what the best way to handle it is. I suppose we should track all the principals of all the channels we've used and return their intersection or something like that.

Comment 26

[Honza Bambas (:mayhemer)]

Attachment: v2

A bit better patch. I clear mPrincipal in all Close() methods and when replacing a channel in CacheClientSeek().

Just for curiosity, leaving the channel in OnStopRequest fixes the bug too, and there are no leaks. Maybe do it that way?

Comment 27

[Boris Zbarsky [:bzbarsky]]

That sounds like what roc is proposing to me.

Comment 28

[Honza Bambas (:mayhemer)]

Attachment: v3 [Checkin comment 33]

OK, then this is the second variant. All content/media tests are passing with it and there are no leaks.

Comment 29

[Boris Zbarsky [:bzbarsky]]

That last patch looks fine to me on its own, but someone very familiar with the video networking stuff should review it to make sure that nothing is depeding on mChannel being null.

Comment 30

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

Comment on attachment 373716 [details] [diff] [review]
v3 [Checkin comment 33]

That'd be me!

Comment 31

[Honza Bambas (:mayhemer)]

Comment on attachment 373709 [details] [diff] [review]
v2

Looks like this approach is dead now.

Comment 32

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

Comment on attachment 373716 [details] [diff] [review]
v3 [Checkin comment 33]

actually, Boris said this was OK, so let's not delay getting this landed.

Comment 33

[Honza Bambas (:mayhemer)]

Comment on attachment 373716 [details] [diff] [review]
v3 [Checkin comment 33]

http://hg.mozilla.org/mozilla-central/rev/524233971e2c

Comment 34

[Boris Zbarsky [:bzbarsky]]

To be clear, that patch is OK from the necko end and from the "getting a principal" end.  I can't speak to its interaction with the other video stuff.

Also, this should have a test, no?

Comment 35

[Honza Bambas (:mayhemer)]

Attachment: test v1

Test based on http://tapper-ware.net/devel/js/JS.StereoVideo/index2.xhtml

Comment 36

[Honza Bambas (:mayhemer)]

Attachment: v3+test for 1.9.1

May land on 1.9.1 after the test gets reviewed.

Comment 37

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

Comment on attachment 374868 [details] [diff] [review]
test v1

+var currentMediaCache = prefs.getIntPref("media.cache_size");
+prefs.setIntPref("media.cache_size", 50*1024);

Just do <script src="use_large_cache.js"> like the other tests.

I think it would be best to do the drawing off the "load" event, not canplaythrough or timeupdate. It's quite likely that this test will pass on trunk if drawWindow fires while we're still downloading and holding the mChannel reference.

Comment 38

[Honza Bambas (:mayhemer)]

Robert, rorry for such a long time to react, but I missed bug mail with your last comment... 

Is there something different on 1.9.1 with the events? I'm using canplaythrough that is called when load of the data is complete (we are pass OnStopRequest, where the code change is). It perfectly works on trunk, is the behavior different on 1.9.1? Or you mean that canplaythrough can pass before we load all data?

Also, it seems the patch has already been landed on 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cb68da3b406a

So, we are just about to push the test.

Comment 39

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

canplaythrough can be called before the load of the data is complete. We'll call it as soon as we think we can play all the way through without stalling --- i.e., when we think we'll finish loading the data at about the same time as we finish playing.

Comment 40

[Honza Bambas (:mayhemer)]

I cannot reproduce the problem when I rollback the patch on trunk. Something has changed?

Comment 41

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

Yes, principals are now tracked through nsMediaCache. Your patch here is probably not needed anymore.

Comment 42

[Honza Bambas (:mayhemer)]

So, only thing left to do here is to have a test for 1.9.1.

Comment 43

[Matthew Gregan [:kinetik]]

Attachment: test v0

A mochitest that paints the video to a canvas after the video has completely loaded.

According to the spec, it's only legal to paint the video to the canvas when readyState >= HAVE_CURRENT_DATA (i.e. when the current frame is available), and it's possible for the load event to fire before that predicate holds, so the test will run the paint off of the loadeddata event if that occurs.

I don't think it's possible for load to fire before loadeddata in our current code, because we deliberately suppress the load event until we fire loadedmetadata, and we always fire loadedmetadata and loadeddata at the same time.  I can't see anything in the spec that requires the former, and I'm sure that the latter is not required either.  Am I right?  If so, should we be proposing this change/simplification to the HTML 5 WG?

Comment 44

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

You mean, should we propose that the spec require "load" to fire after "loadeddata"?

I don't feel very strongly about it, but I suppose it would be a good idea.

Comment 45

[Matthew Gregan [:kinetik]]

That, or at least after loadedmetadata.  That'd make a media load event consistent with an img load event (which fires when the image is "available", meaning the metadata is known), but that's the only argument I can come up with and it seems fairly week.  Looking at the other places a load event can be fired in HTML5:

link, style: when resource is fetched
script: when script created (including parsing)
object: once completely loaded (not really clear on this one)
iframe: when contented loaded and in frame loads have fired
img, input button: when metadata available
document: once no longer delayed

...it seems like there's scope but no strong argument to delay it for media elements.

I've emailed the WHATWG list.  I guess we won't need to take any action whatever the outcome is, but if we do I'll file a new bug.

Comment 46

[Matthew Gregan [:kinetik]]

Attachment: test v1

Anne van Kesteren pointed out on WHATWG that the spec already requires the events to be ordered, so you will always see loadedmetadata, loadeddata, then load.

Comment 47

[Matthew Gregan [:kinetik]]

Temporarily marking reopened so this shows up on the radar for [needs landing] or checkin-needed searches, since the tree is restricted and I might forget about this before I can check it in.

Comment 48

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

Can we move the tests to a new bug and close this one? It's blocking1.9.1+ so reopening it will confuse drivers.

Comment 49

[Matthew Gregan [:kinetik]]

Good point.  Sorry, that was dumb.  Bug 495165.

Comment 50

[Aakash Desai [:aakashd]]

verified FIXED using STR from comment #3

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090528 Minefield/3.6a1pre ID:20090528112613

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090602 Shiretoko/3.5pre ID:20090602031310

Comment 51

[Nochum Sossonko [:Natch]]

Bug 495165 added a mochitest for this, pushed in rev bd1d2c9c5799.
http://hg.mozilla.org/mozilla-central/rev/bd1d2c9c5799