558622 - Rework HttpChannelChild::AsyncOpen

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

Attachment: Rework HttpChannelChild::AsyncOpen

Attached patch brings the logic in line with nsHttpChannel::AsyncOpen, and removes obsolete TODOs (dwitte: we don't need nsHttpChannel's cookie logic here, right?).  Also adds channel to loadgroup and calls OnModifyRequest().  I believe it fixes what would otherwise be a leaked AddRef if we wind up being canceled.

Does not appear to make the testXUL demo any wobblier :)

Comment 1

[[e-mail]]

Comment on attachment 438314 [details] [diff] [review]
Rework HttpChannelChild::AsyncOpen

>+  // notify "http-on-modify-request" observers
>+  gHttpHandler->OnModifyRequest(this);

This is the part we need the cookie bits for (and still do)... need to add the Cookie header before we send out OMR, so observers can... modify it. ;)

But you're not making anything worse, and it can wait 'til we have a story for how child/parent-side OMR observers are supposed to interoperate. Might want to leave the TODO in there, though, albeit with a better explanation.

>+  // FIXME: bug 536317: We may have been cancelled already, either by
>+  // on-modify-request listeners or by load group observers; in that case, 
>+  // don't create IPDL connection.  See nsHttpChannel::AsyncOpen(): I think
>+  // we'll need something like
>+  //
>+  // if (mCanceled) {
>+  //   LOG(("Calling AsyncAbort [rv=%x mCanceled=%i]\n", rv, mCanceled));
>+  //   AsyncAbort(rv);
>+  //   return NS_OK;
>+  // }

Hmm. But if we haven't created any IPDL connection yet, what do we need to abort? (I assume your AsyncAbort() here is an IPDL call, but maybe not?)

>   mState = HCC_OPENED;

Does this want to move up to the 'mIsOpened' et al setter expression, or stay here?

r=dwitte

Comment 2

[[e-mail]]

Also, the mCaps TODO still applies, right?

Comment 3

[Jason Duell]

http://hg.mozilla.org/projects/electrolysis/rev/2f30bbaa0912

> I assume your AsyncAbort() here is an IPDL call

AsyncAbort is not an IPDL function--it's boilerplate nsHttpChannel logic for calling OnStart/Stop when a channel is cancelled.

> >   mState = HCC_OPENED;
> Does this want to move up to the 'mIsOpened' et al setter expression, or stay
here?

I want to keep the mState = HCC_OPENED close to the SendAsyncOpen.  Eventually it will be replaced by IPDL's forthcoming state transition variable.

> need to add the Cookie header before we send out OMR

Added a bug (and comment) for the cookies logic.

> Also, the mCaps TODO still applies, right?

From my read of the code, nsHttpChannel should be getting an identical mCaps from the chrome process's nsHttpHandler::NewProxiedChannel, so we shouldn't need to do anything with the childs.  I left it in just so it can share the same Init() code.