Closed Bug 985230 Opened 6 years ago Closed 6 years ago

mStatus is not correct in HttpChannelChild::OnStartRequest

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: billm, Assigned: jduell.mcbugs)

References

(Blocks 1 open bug)

Details

Attachments

(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?

[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
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
Status: NEW → ASSIGNED
Attachment #8394501 - Flags: review?(sworkman)
Flags: needinfo?(jduell.mcbugs) → needinfo?(wmccloskey)
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?
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)
https://hg.mozilla.org/mozilla-central/rev/5475d87513b4
Status: ASSIGNED → RESOLVED
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.