292391 - [FIXr]XMLHttpRequest callbacks should forward to original ones

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Right now, people trying to set notification callbacks on the channel that
XMLHttpRequest uses get surprised because their callbacks get clobbered by
XMLHttpRequest.  I think the right thing to do is to cache the callbacks they
sent and:

1)  Forward GetInterface to them for interfaces XMLHttpRequest doesn't implement
2)  Forward interface calls to them for the interfaces it does implement

Anyone see any obvious problems with the proposal?

Comment 1

[Darin Fisher]

So, how would you handle the case where an implementation of nsIChannelEventSink
is already present in the notification callbacks assigned to the channel in use
by the XMLHttpRequest object?  Would XMLHttpRequest just trust the
implementation it finds, or would it still want to impose its security
restrictions for redirects?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I should have clarified that forwarding should be in addition to whatever it
normally does.  So we should let the other sink know we're redirecting, but only
if we're allowing the redirect to start with.  For the other notifications we
may also want to do something smart....

Comment 3

[Darin Fisher]

OK, I figure we'll just special case things.  Maybe if the consumer specifies
their own nsIAuthPrompt, then we'll just use that instead of the one we'd
normally use.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 182239 [details] [diff] [review]
Proposed patch

wanna use /** */ comment style in nsXMLHttpRequest.h? that'd allow automatic
documentation extracting by doxygen :-)

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

(and sorry about the delay, I suck at doing reviews lately)

Comment 7

[Johnny Stenback (:jst)]

What about code that removes the callbacks in the middle of the load, they'll
now continue to get notifications after they removed the callbacks. Maybe we
need to hide the actual channel that XMLHttpRequest uses from its consumers and
return a wrapper instead that they can mess with, and we'll forward the
appropriate data through that wrapper?

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

well, nsIChannel says:
http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIChannel.idl#98
"                                                                              A
100      * channel may ignore changes to the notificationCallbacks attribute after
101      * it has been opened.  This rule also applies to notificationCallbacks
102      * queried from the channel's loadgroup.

Comment 9

[Darin Fisher]

Comment on attachment 182239 [details] [diff] [review]
Proposed patch

>Index: extensions/xmlextras/base/src/nsXMLHttpRequest.cpp

>+  if (mProgressEventSink) {
>+    return mProgressEventSink->OnProgress(aRequest, aContext, aProgress,
>+                                          aProgressMax);
>+  }
>+
>   return NS_OK;
> }

micro nit: the return from OnProgress means nothing, so maybe better
not to propogate it.  returning NS_OK in all cases is fine.



> nsXMLHttpRequest::OnStatus(nsIRequest *aRequest, nsISupports *aContext, nsresult aStatus, const PRUnichar *aStatusArg)
> {
>+  if (mProgressEventSink) {
>+    return mProgressEventSink->OnStatus(aRequest, aContext, aStatus,

same micro nit


This patch looks good, but what about the notification callbacks
associated with the channel's load group?  Should you use 
NS_NewNotificationCallbacksAggregation to build an interface
requestor aggregation of the notification callbacks from the
channel as well as from its load group?  Or is the load group
never going to have notification callbacks defined on it?

Comment 10

[Boris Zbarsky [:bzbarsky]]

jst, the wrapper would have to implement all the channel interfaces we have, for
all protocols.  That's just not doable sanely.  nsIChannel clearly says that
changing callbacks after the channel has been opened is unsupported, and our
channels already ignore such changes, so I think we're safe there.  ;)

darin, I'm not sure how the loadgroup is a problem here... That's already
handled by the channel impl, no?  What am I missing?

Comment 11

[Darin Fisher]

bz: you're right.  i was confused a bit.  but now that i think about it more,
the concern i had was actually with the fact that we only inspect
nsIProgressEventSink and nsIChannelEventSink from mNotificationCallbacks,
whereas the channel would normally inspect both its notification callbacks as
well as the notification callbacks of its loadgroup.  so, if a consumer defined
notification callbacks on the loadgroup but not on the channel, we would tell
the channel that we (XMLHttpRequest) implement those interfaces thus preventing
the channel from getting the interfaces from the loadgroup.  does this make any
sense?  it's hard to explain in brief.  this isn't that big of a concern to me.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Darin, that does make sense but then the consumer is generally running the risk
that someone will set notificationCallbacks on the channel and the loadgroup
callbacks won't get called...  I think that's just a built-in part of our
design, for better or worse.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Requesting approval.  People seem to be running into this problem with
nsXMLHttpRequest a good bit, and it'd be good if they didn't have to worry
about it...

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 182680 [details] [diff] [review]
Updated to comments

a=asa

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed.