536292 - e10s HTTP: callbacks infrastructure for e10s

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

Our interface for HTTP channel callbacks maps pretty badly onto e10s.  We allow users of a channel to set an arbitrary nsIInterfaceRequestor and/or loadgroup as "callbacks", which can then be GetInterface'd into any old XPCOM object.  Nice and flexible--except that when we're in a different process, we can't just access XPCOM objects in another process.

We don't really care about arbitrary necko clients, just gecko, so this isn't a huge problem:  what we can do it audit all the existing uses of this mechanism, and implement an nsIInterfaceRequestor in HttpChannelParent that "does the right thing" for each interface.  Things like alert and authentication windows we'll need to handle in chrome itself.  Other things like notifications will result in delivering an IPDL message to the child, which can then perform the logic on the original callback.

I've started working on this.

Comment 1

[Jason Duell]

Attachment: Infrastructure for callbacks

Attached patch implements enough of the callback infrastructure needed for HTTP auth (bug 537782), Redirects (bug 536294), application cache (bug 536295) and nsIProgressEventSink (bug 561692).  

Currently dies if we encounter other interfaces, so we know and can implement those, too.

Only need 2 reviews, but have flagged 3 of you, in hopes that I'll get 2 faster that way :)

Comment 2

[Jason Duell]

Renaming.  Progress notifications now covered by bug 561692.   Load groups should be working in content already, and don't need any e10s/IPDL work, unless I'm mistaken.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> We don't really care about arbitrary necko clients, just gecko

Yes, but within Gecko there are some very different necko clients.  For example, we have some clients that want HTTP auth to silently fail and do NOT hand out auth prompts...  See nsDocument::LoadgroupCallbacks.

Comment 4

[Jason Duell]

Comment on attachment 441430 [details] [diff] [review]
Infrastructure for callbacks

> we have some clients that want HTTP auth to silently fail and do NOT
hand out auth prompts...  See nsDocument::LoadgroupCallbacks.

Ah, good to know.  So it sounds like whoever implements bug 537782 will want to first check the HttpChannelChild's mCallbacks/mListener to see if they hand out auth prompts.  If they do, we should hand them out in HttpChannelParent as well, otherwise not.  I'll add a comment in the bug.

But that issue doesn't need to hold this code from landing.  For now we return NULL for auth prompts no matter what.

Comment 5

[[e-mail]]

Comment on attachment 441430 [details] [diff] [review]
Infrastructure for callbacks

>+  if (aIID.Equals(NS_GET_IID(nsIAuthPromptProvider))) {
>+     
>+  } else if (aIID.Equals(NS_GET_IID(nsIAuthPrompt2))) {
>+    
>+  } else if (aIID.Equals(NS_GET_IID(nsIAuthPrompt))) {

I'd like to see this as a list of || conditions, since empty 'if' blocks seem weird:

  if (aIID.Equals(NS_GET_IID(nsIAuthPromptProvider)) ||

      // comment
      aIID.Equals(NS_GET_IID(nsIAuthPrompt2)) ||
      ...

the body of which could then be the QI, and the DROP_DEAD part wouldn't need to be enclosed in an 'else'.

r=dwitte with that.

Comment 6

[Jason Duell]

> we have some clients that want HTTP auth to silently fail 

Ah, good to know. I've added a note to bug 537782 about this.

That issue doesn't need to hold this code from landing.  For now we return NULL for auth prompts no matter what.

Comment 7

[Jason Duell]

http://hg.mozilla.org/projects/electrolysis/rev/11682e4090a9

converted to use of || rather than lots of if..else if, per dwitte's request.

Comment 8

[Jason Duell]

Comment on attachment 441430 [details] [diff] [review]
Infrastructure for callbacks

clearing obsolete review request