Closed Bug 985230 Opened 6 years ago Closed 6 years ago

mStatus is not correct in HttpChannelChild::OnStartRequest


(Core :: Networking, defect)

Not set





(Reporter: billm, Assigned: jduell.mcbugs)


(Blocks 1 open bug)



(1 file)

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?

Flags: needinfo?(jduell.mcbugs)
Attached patch v1Splinter Review
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.
Assignee: nobody → jduell.mcbugs
Attachment #8394501 - Flags: review?(sworkman)
Flags: needinfo?(jduell.mcbugs) → needinfo?(wmccloskey)
Comment on attachment 8394501 [details] [diff] [review]

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

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


::: 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?
Attachment #8394501 - Flags: review?(sworkman) → review+
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.
Flags: needinfo?(wmccloskey)
Jason, is this ready to land?
Flags: needinfo?(jduell.mcbugs)
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?
Flags: needinfo?(jduell.mcbugs) → needinfo?(sworkman)
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.