889821 - Custom JS nsIChannel.asyncOpen implementation gets called with invalid parameters when request comes from an HTMLObjectElement

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[caribou]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.116 Safari/537.36

Steps to reproduce:

In an extension define a custom nsIChannel implementation. Make an HTML page with an object tag whose data attribute makes a request to the custom nsIChannel.


Actual results:

The request fails because onStartRequest method is not defined on the nsIStreamListener passed to the nsIChannel.asyncOpen method.


Expected results:

The request should not have failed and should have answered back to the object the channel content. 

Once a request is bound to this channel implementation, its asyncOpen is called with an nsIStreamListener as the first parameter. When the request issuer is an HTMLObjectElement, this first argument does not have the nsIStreamListener methods visible (onStartRequest, onStopRequest) since FF22. Previous FF version worked very well. Requests issued by other means (HTMLIFrameElement, HTMLImageElement, top level window,...) work very well.

Comment 1

[caribou]

Attachment: xpi that defined a custom nsIProtocolHandler (test://) bound a custom nsIChannel implementation (TestChannel)

To reproduce the wrong behaviour, install this minimimalist extension which defines a custom nsIProtocolHandler (test://) bound a custom nsIChannel implementation(TestChannel)

Comment 2

[caribou]

Attachment: Test page to view once the customchannel.xpi is installed

This test page reproduces the bug. Take a look at the Error Console to view the message raised by the CustomChannel.

Comment 3

[caribou]

Just anothor comment to be more precise : these custom JS channels worked very well before FF 22 (and still do when not requested from an <object> tag !)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Fun.  This is like bug 883968 but for the argument to asyncOpen.

John, do you have time to fix this, or should I?  We can reuse the same shim object, I would expect and just have it forward along more interfaces...

Comment 5

[John Schoenick [:johns]]

Sure I'll take a look

Comment 6

[John Schoenick [:johns]]

Attachment: Extend interface shim to allow nsIStreamListener callbacks to object/embed/applet tags to be used from script

Well the static_cast thing is awful, but this is already a mess for this interface. I guess the alternative would be to add these interfaces to nsIObjectLoadingContent or restore nsIInterfaceRequestor to it, or maybe have the shim have two redundant pointers to the same class. Given that this is file-local this seems like the sanest approach.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 773411 [details] [diff] [review]
Extend interface shim to allow nsIStreamListener callbacks to object/embed/applet tags to be used from script

>+  nsCOMPtr<nsIObjectLoadingContent> mContent;

How about nsRefPtr<nsObjectLoadingContent> and then you don't need the casts?

Also, I think you need nsIRequestObserver here too.

And no need to add anything to ObjectInterfaceRequestorShim::GetInterface.

Comment 8

[John Schoenick [:johns]]

Attachment: Extend interface shim to allow nsIStreamListener callbacks to object/embed/applet tags to be used from script

(In reply to Boris Zbarsky (:bz) from comment #7)
> Comment on attachment 773411 [details] [diff] [review]
> Extend interface shim to allow nsIStreamListener callbacks to
> object/embed/applet tags to be used from script
>
> >+  nsCOMPtr<nsIObjectLoadingContent> mContent;
>
> How about nsRefPtr<nsObjectLoadingContent> and then you don't need the casts?

nsRefPtr fails, we discussed this on IRC and decided keeping the static cast was the least-ugly option. Added a comment

> Also, I think you need nsIRequestObserver here too.

Fixed

> And no need to add anything to ObjectInterfaceRequestorShim::GetInterface.

Removed

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 773427 [details] [diff] [review]
Extend interface shim to allow nsIStreamListener callbacks to object/embed/applet tags to be used from script

No need for the bug comment in the checkin comment here.  ;)

You need nsIRequestObserver in the QI impl for ObjectInterfaceRequestorShim too.

r=me with that

Comment 10

[John Schoenick [:johns]]

Attachment: Extend interface shim to allow nsIStreamListener callbacks to object/embed/applet tags to be used from script. r=bz

Comment 11

[John Schoenick [:johns]]

Comment on attachment 773469 [details] [diff] [review]
Extend interface shim to allow nsIStreamListener callbacks to object/embed/applet tags to be used from script. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddebb5983363

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ddebb5983363

Comment 13

[John Schoenick [:johns]]

Comment on attachment 773469 [details] [diff] [review]
Extend interface shim to allow nsIStreamListener callbacks to object/embed/applet tags to be used from script. r=bz

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
bug 827158

User impact if declined: 
Addons will be unable to intercept certain channel functions for object/embed/applet elements that worked properly in FF21, and attempting to do so may break said element.

Testing completed (on m-c, etc.):
On m-c, verified this fixes provided testcase.

Risk to taking this patch (and alternatives if risky):
Low, just shuffles around interfaces to ensure we're exposing an XPIDL object to some calls.

String or IDL/UUID changes made by this patch:
None

Comment 14

[bhavana bajaj [:bajaj]]

Comment on attachment 773469 [details] [diff] [review]
Extend interface shim to allow nsIStreamListener callbacks to object/embed/applet tags to be used from script. r=bz

Given the low risk of the patch and this is recent regression on Fx22, approving on aurora

Comment 15

[Alex Keybl [:akeybl]]

Is there a use case that warrants taking this on Beta?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Mostly extension compatibility: an extension that implements a protocol handler and channel won't work for <object>.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 773469 [details] [diff] [review]
Extend interface shim to allow nsIStreamListener callbacks to object/embed/applet tags to be used from script. r=bz

In the interest of extension compatibility and given the low risk assessment here, let's take this on beta too.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/16ee09f2fb7e
https://hg.mozilla.org/releases/mozilla-beta/rev/1ae00cd78569

Comment 19

[Paul Silaghi, QA [:pauly]]

(In reply to caribou from comment #2)
> Created attachment 770794 [details]
> Test page to view once the customchannel.xpi is installed
> 
> This test page reproduces the bug. Take a look at the Error Console to view
> the message raised by the CustomChannel.

Reproduced on FF 22. Verified fixed FF 23b10 Win 7 x64.

Comment 20

[caribou]

Thank you very much to all of you for the rapidity for this bug fix and the quick integration to FF23.
Regards,

Vincent

Comment 21

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 24b4 Win 7 x64.

Comment 22

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 25.0a2 (2013-09-13), 26.0a1 (2013-09-12) Win 7 x64