Closed Bug 1096267 Opened 5 years ago Closed 3 years ago

XMLHttpRequest.send({}) ends up calling send(InputStream data); variant, not send(DOMString? data);

Categories

(Core :: DOM: Core & HTML, defect)

36 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: smaug, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

See Bug 1096263
No longer blocks: 1096263
Blocks: 1096263
Given that this interface can be implemented via XPCWrappedJS (as far as xpconnect is concerned), how are bindings supposed to know to not use this overload?  :(
yup, I know this is hard given that we still support xpidl interfaces in webidl.
Perhaps we should never allow script implemented xpidl interfaces in webidl (as a first step)?
So if QIing to wrappedjs succeeds, use the string variant.
Can we just mark nsIInputStream builtinclass?
> Perhaps we should never allow script implemented xpidl interfaces in webidl

Looking at the set of xpidl interfaces we allow in webidl right now, it is:

imgINotificationObserver
imgIRequest
mozilla::dom::EventTarget
mozilla::dom::IndirectlyImplementedInterface
mozilla::dom::TestExternalInterface
nsIBrowserDOMWindow
nsIChannel
nsIDOMCSSRule
nsIDOMDataChannel
nsIDOMMozMmsMessage
nsIDOMMozSmsMessage
nsIDOMMozWakeLockListener
nsIDOMWindow
nsIFile
nsIFrameRequestCallback
nsIInputStream
nsIInputStreamCallback
nsIJSID
nsIMenuBuilder
nsIObserver
nsIOutputStream
nsIPrincipal
nsISelectionListener
nsISupports
nsITreeView
nsIURI
 
imgINotificationObserver is already builtinclass. So is imgIRequest.  EventTarget is clearly builtinclass.  IndirectlyImplementedInterface and TestExternalInterface are test-only.

nsIBrowserDOMWindow is JS-implemented and used on ChromeWindow.  But maybe we can switch to using a Web IDL callback interface here?

nsIChannel is only used for loadImageWithChannel.  I don't know why we even expose this in webidl; probably for compat reasons.  I would be happy trying to remove it.

nsIDOMCSSRule is clearly in no need of JS impls.  Same for nsIDOMDataChannel, nsIDOMMozMmsMessage, 
nsIDOMMozSmsMessage, nsIDOMWindow, nsIFile, nsIJSID.

nsIDOMMozWakeLockListener probably has JS implementations, but maybe we can switch it to being a WebIDL callback?

nsIFrameRequestCallback is JS-implemented but could become a Web IDL callback type.

nsIInputStreamCallback, maybe similar?

nsIMenuBuilder, no idea.

nsIObserver has JS impls but we could use a Web IDL callback with enough surgery.

nsIOutputStream, not sure.

nsIPrincipal is builtinclass.

nsISelectionListener is JS-implemented.

nsISupports ... well.

nsITreeView, not sure.

nsIURI extensions can probably provide JS-implemented ones.

But yes, in the long term this seems like a good goal.

> Can we just mark nsIInputStream builtinclass?

We could try.  See bug 1087633 comment 5 last paragraph.  This would need bake time and may have extension compat issues...
Even more conservative approach as a first step:  don't change the behavior when the caller is chrome, 
but for content js require non-wrappedJS xpidl implementation.
That would fail for nsIFrameRequestCallback until we convert it over.

But yes, if we nix this stuff from web-facing interfaces, then we could do that.
This bug has since been solved. Firefox now sends "[object Object]" as per spec (both for web and chrome content).
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
Thomas, this bug is not "solved".  The bug isn't about what we _send_: that was hacked around in bug 1096263; then this bug was filed to fix things properly.  See comment 0.

As the bug summary says, this bug is about which overload gets called.  And we're still calling the wrong overload, and still have likely problems for the various things listed in comment 4...
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Odd, sorry about that. My wires got crossed somehow and I thought this was bug 1096263.

Have your thoughts changed on what should be done to properly fix this? As in, should we bite the bullet and try to make nsIInputStream a builtinclass now? I suspect it's worth trying to do something a bit more drastic while the addon system is in flux, to try to discover what addon's needs are for WebExtensions and the like (especially given the upcoming WhatWG streams spec). Unless of course there's a simple way to do telemetry on which addons rely on this feature.
> nsIChannel is only used for loadImageWithChannel.  I don't know why we even expose this in webidl;
> probably for compat reasons.  I would be happy trying to remove it.

Patch for that in bug 1304515.  nsIChannel is still used as a _return_ value for XHR, of course.

> nsIInputStreamCallback, maybe similar?

No longer used.  See bug 1304516.

> Have your thoughts changed on what should be done to properly fix this?

I think we should probably just go ahead and do what comment 5 suggests.  Patch coming up in a second.

> I suspect it's worth trying to do something a bit more drastic while the addon system is in flux

nsIBrowserDOMWindow is not addons.  It's core Firefox UI...  Same for nsIObserver.  We could hold out for those being fixed in a nicer way, but I'm not sure that's getting us anywhere.
Assignee: nobody → bzbarsky
Status: REOPENED → ASSIGNED
We can also remove XHR.send from the implicitJSContext list in Bindings.conf while we're here, no? I think it was only added to that list for the nsIInputStream case in bug 1096263...
Oh, good catch.  Checking.
The worker version of send() needs a JSContext.
Comment on attachment 8793509 [details] [diff] [review]
Stop calling into the nsIInputStream overload of XMLHttpRequest.send() if a random object is passed in (except in chrome code, where we will keep doing that)

Ah, this doesn't break nsIDOMMozWakeLockListener usage since MozPowerManager is ChromeOnly
Attachment #8793509 - Flags: review?(bugs) → review+
> Ah, this doesn't break nsIDOMMozWakeLockListener usage since MozPowerManager is ChromeOnly

More importantly, Navigator.mozPower is chromeonly.  But yeah, good catch.  I better give this a nice try run.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d29f9abbeaca
Stop calling into the nsIInputStream overload of XMLHttpRequest.send() if a random object is passed in (except in chrome code, where we will keep doing that).  r=smaug
https://hg.mozilla.org/mozilla-central/rev/d29f9abbeaca
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.