Closed Bug 889821 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Plug-ins, defect)

22 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla25
Tracking Status
firefox22 --- wontfix
firefox23 + verified
firefox24 + verified
firefox25 --- verified

People

(Reporter: vincent.cariven, Assigned: johns)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

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.
To reproduce the wrong behaviour, install this minimimalist extension which defines a custom nsIProtocolHandler (test://) bound a custom nsIChannel implementation(TestChannel)
This test page reproduces the bug. Take a look at the Error Console to view the message raised by the CustomChannel.
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 !)
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...
Blocks: 827158
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jschoenick)
Sure I'll take a look
Assignee: nobody → jschoenick
Status: NEW → ASSIGNED
Component: DOM: Core & HTML → Plug-ins
Flags: needinfo?(jschoenick)
OS: Windows 7 → All
Hardware: x86_64 → All
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.
Attachment #773411 - Flags: review?(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.
Attachment #773411 - Flags: review?(bzbarsky) → review-
(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
Attachment #773411 - Attachment is obsolete: true
Attachment #773427 - Flags: review?(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
Attachment #773427 - Flags: review?(bzbarsky) → review+
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
Attachment #773469 - Flags: review+
Attachment #773469 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/ddebb5983363
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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
Attachment #773469 - Flags: approval-mozilla-beta?
Attachment #773469 - Flags: approval-mozilla-aurora?
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
Attachment #773469 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Is there a use case that warrants taking this on Beta?
Mostly extension compatibility: an extension that implements a protocol handler and channel won't work for <object>.
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.
Attachment #773469 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #770794 - Attachment mime type: text/plain → text/html
(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.
Thank you very much to all of you for the rapidity for this bug fix and the quick integration to FF23.
Regards,

Vincent
Verified fixed FF 24b4 Win 7 x64.
Verified fixed FF 25.0a2 (2013-09-13), 26.0a1 (2013-09-12) Win 7 x64
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.