485471 - Channel references are stale on redirect

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

When we load an HTTP channel which has a 3XX redirect, nsHTTPChannel will create a new channel with the same listener to handle the new channel. But in such cases, the references to the original channel held by nsHTMLMediaElement and nsMediaStream will still point to the old channel, not the one actually fetching the data.

This is bad because we can't remove the channel from the owner document loadgroup properly, and if we need to cancel the channel, we may not cancel the right one.

To implement this, we need to detect redirect by having an nsIChannelEventSink implementation which we'd pass to NS_NewChannel(), similar to NotificationCallbacks in netwerk/test/TestProtocols.cpp.

This is spun off from bug 482885. When you implement this, make sure that the assertions in nsMediaStream::MoveLoadsToBackground() (to land in bug 482885) are not firing, e.g. ensure that the mChannel reference is current.

Comment 1

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

Attachment: Patch v1

Patch v1
* Use an nsIChannelEventSink implementation to listen for OnChannelRedirect() for nsHTMLMediaElement and nsMediaChannelStream.
* Fixed some assertions in nsMediaStream.cpp so that the message matched what was being asserted. I think the assert condition was correct, but the message incorrect.

Comment 2

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

Why do we need to add ChannelSink, can't we just add interfaces to MediaLoadListener, like you did for nsMediaChannelStream::Listener?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Where do we actually set a nsMediaChannelStream::Listener as the notification callbacks on a channel?

The correct error code for an unknown interface from GetInterface is NS_NOINTERFACE.

Comment 4

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

Attachment: Patch v2

Comments addressed. Tests pass on Windows & Linux.

Comment 5

[Boris Zbarsky [:bzbarsky]]

At the point where we call SetNotificationCallbacks there, are we guaranteed that we're not clobbering someone else's callbacks?  Can we assert existing callbacks are null?  Or do we need to pass GetInterface calls along to those callbacks?

Comment 6

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

(In reply to comment #5)
> At the point where we call SetNotificationCallbacks there, are we guaranteed
> that we're not clobbering someone else's callbacks?

Looks like not. Possibly it's the loadgroup's callback, or nsCrossSiteListenerProxy? I'll have to pass GetInterface() and OnChannelRedirect() calls along like nsCrossSiteListenerProxy.

Comment 7

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

Attachment: Patch v3

It looks like the channel being passed into nsMediaChannelStream::OpenChannel() is often the one created in nsHTMLMediaElement::LoadResource() - with a MediaLoadListener. MediaLoadListener::OnStartRequest() calls down into nsMediaChannelStream::OpenChannel() with this channel. There's not much use in adding another wrapped channel redirect listener to a channel that will no longer be redirected. So now I add the listener in nsMediaChannelStream if the channel QIs to an nsIHttpChannel, and if its GetStatus() returns NS_ERROR_NOT_AVAILABLE. This can happen only if the channel's response hasn't been processed yet. It shouldn't be possible for channels created by nsMediaStream to process their response before we add the listener, as we don't return control back to the event loop before adding the listener. This listener still wraps any existing listeners.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Is there no way to set the redirect observer up when we set up the channel, whereever that happens?  Or do we have consumers passing in channels that my or may not have seen OnStartRequest but that we don't set up ourselves?

Comment 9

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

(In reply to comment #8)
> Is there no way to set the redirect observer up when we set up the channel,
> whereever that happens?  Or do we have consumers passing in channels that my or
> may not have seen OnStartRequest but that we don't set up ourselves?

nsVideoDocument is passed a channel pointing to a video, which it passes down into nsHTMLMediaElement::LoadWithChannel() and friends. I'm not sure where that channel gets created.

In regards to specifying the callbacks when we create the channel in nsMediaStream::Open(), the problem is that we only need the nsIChannelEventSink for the nsMediaChannelStream subclass, not the nsMediaFileStream. We also create the channel in nsMediaStream::Open() and then QI the channel to nsIFileChannel in order to determine which subclass to instantiate. We could supply a stand alone nsIChannelEventSink to the NS_NewChannel() call, and have that call a virtual notification method on the nsMediaStream which the nsMediaChannelStream implemented, and the nsMediaFileStream ignored?

Or we could use the specifier of the URI to determine which subclass to open, and push the channel open down to the subclass?

We'd still need to set the notification callbacks to cover nsVideoDocument channels though.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> nsVideoDocument is passed a channel pointing to a video

nsVideoDocument only gets the channel once the channel has called OnStartRequest on its listener, so there is no need to listen for redirects on this channel.

If that's the only consumer of LoadWithChannel, I think it would be reasonable to assert that the channel passed to OnStartRequest has a response head.

> the problem is that we only need the nsIChannelEventSink
> for the nsMediaChannelStream subclass, not the nsMediaFileStream.

Because you don't actually use a channel in the latter case?  Does that mean that loading video from desktop shortcut files (.desktop on Linux, .url on windows) doesn't work?  In any case, that's a separate discussion; the virtual method approach sounds fine, though the case which ignores the notification should have a NS_NOTREACHED, I would think... or if it can be reached it perhaps shouldn't ignore it?

Comment 11

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

I think I reproduced a lot of this in bug 489415. Oops!

Comment 12

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

Attachment: Patch v4

(In reply to comment #11)
> I think I reproduced a lot of this in bug 489415. Oops!

Looks like it. The only thing not reproduced was handling redirect on nsHTMLMediaElement - this patch does that. This patch is based on patch from bug 489415. nsMediaStream seek result redirect is handled by Roc's patch.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 377558 [details] [diff] [review]
Patch v4

Looks fine, but more than just http 3xx can trigger redirects.  In particular, you get them any time you use a proxy, you get them for some file:// URIs, etc.

Comment 14

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

Attachment: Patch v5 - updated comment

As patch v4, but changed comment on nsHTMLMediaElement::OnChannelRedirect() as bz pointed out that it wasn't quite accurate. r+bz sr+ roc.

Comment 15

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

http://hg.mozilla.org/mozilla-central/rev/1523d386e613

Comment 16

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/63a7355e196d