985230 - mStatus is not correct in HttpChannelChild::OnStartRequest

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm debugging a test failure on e10s. The problem is that there's some code [1] that checks the status of an HTTP request from an OnStartRequest handler. It looks like we're not setting this field correctly in HttpChannelChild. When I look at mStatus for the nsHttpChannel in the parent process, it's some non-zero error code. But when I look at mStatus in the child, it's NS_OK.

It looks like we set mStatus in HttpChannelChild::OnStopRequest [2], but not in OnStartRequest. Should we just pass the status in as an additional parameter over IPDL to OnStartRequest?

[1] http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#321
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#506

Comment 1

[Jason Duell]

Attachment: v1

Yup, we should be updating the status of the channel for OnStart (and OnData), unless the channel has been canceled (or mStatus otherwise modified) in the child.

Bill, take this for a spin and let me know if it fixes things for you.

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8394501 [details] [diff] [review]
v1

Review of attachment 8394501 [details] [diff] [review]:
-----------------------------------------------------------------

Looks logical to me. FTPChannel too? WebSocketChannel? Any others needed?

r=me.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +488,2 @@
>    : mChild(child)
> +  , mStatusCode(channelStatus) {}

Nit: Since you use mChannelStatus in the other two events, please use it here too.

@@ +519,5 @@
>    if (mDivertingToParent) {
>      MOZ_RELEASE_ASSERT(!mFlushedForDiversion,
>        "Should not be processing any more callbacks from parent!");
>  
> +    SendDivertOnStopRequest(channelStatus);

We don't need to pass status back in SendDivertOnData, right?

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yes, this works. Thanks! I probably should have mentioned the test: it's content/base/crashtests/851353-1.html and you have to run it with reftest-ipc.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Jason, is this ready to land?

Comment 5

[Jason Duell]

Yes we need to do this for FTP too.  Not websockets though. If we can't do that ASAP then let's file a followup for FTP since we're blocking billm on this.

> We don't need to pass status back in SendDivertOnData, right?

Well, we were also passing it before this patch.  Looks like it's possible for a client in the child to Cancel the channel and then Divert it back to the parent? 

Steve, can you land this (do the rename s/mStatusCode/mChannelStatus if you like--that doesn't need re-review) and maybe knock off FTP while I'm gone next week?

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5475d87513b4

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5475d87513b4