Closed
Bug 569044
Opened 14 years ago
Closed 14 years ago
e10s HTTP: handle requests with no responseHead
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
People
(Reporter: jduell.mcbugs, Assigned: mayhemer)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
Right now trying to load "http://mdc" will cause the test-ipc demo to abort in HttpChannelParent::OnStartRequest, because the current code expects mResponseHead to be set, which it's not.
Reporter | ||
Updated•14 years ago
|
Blocks: fennecko
Summary: e10s HTTP: handle requests with no reponseHead → e10s HTTP: handle requests with no responseHead
Assignee | ||
Comment 1•14 years ago
|
||
I don't think we need to specially 'handle' such requests at all. It is a normal situation the consumer of nsHttpChannel (HttpChannelParent and Child in this case) gets OnStartRequest while the channel doesn't have the response head. E.g. when the DNS request failed or the server were down. By contract, when there is no way to resume, nsHttpChannel has to report OnStartRequest and OnStopRequest(failure status) in reaction to AsyncOpen call. There is no need to break this.
So, I think the limitation to DROP_DEAD when there's no response head is not necessary.
Assignee | ||
Comment 2•14 years ago
|
||
To fix this I would add a parameter aUseResponseHead (= !!responseHead) and when there is no response head provided just pass empty one (reference to a variable on stack). Jason, do you agree?
Reporter | ||
Comment 3•14 years ago
|
||
Sounds good.
Assignee | ||
Comment 5•14 years ago
|
||
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #450384 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Comment 6•14 years ago
|
||
Just merged patch.
Attachment #450384 -
Attachment is obsolete: true
Attachment #450756 -
Flags: review?(jduell.mcbugs)
Attachment #450384 -
Flags: review?(jduell.mcbugs)
Reporter | ||
Comment 7•14 years ago
|
||
Comment on attachment 450756 [details] [diff] [review]
v1 - merged
Looks good. Will need merging--bitrotted. I can do it when I check it in.
+ nsHttpResponseHead emptyResponseHead
I will make this static unless there's a reason not to.
Attachment #450756 -
Flags: review?(jduell.mcbugs) → review+
Assignee | ||
Comment 8•14 years ago
|
||
Jason, I'm used to land my patches my self.. This one particular lies among other patches in a queue and landing it sooner would lead to a more complicated merging. Thanks.
I'll do changes you propose.
Assignee | ||
Comment 9•14 years ago
|
||
(In reply to comment #7)
> I will make this static unless there's a reason not to.
We should probably first get an answer to bug 569629 comment 10.
Comment 10•14 years ago
|
||
Making it static is a no-no in any case, as it contains ns*Strings. See bug 506348 for details.
Reporter | ||
Comment 11•14 years ago
|
||
Well then can we either make it a global variable, or a temp on the stack that only gets created if needed? Seems silly to incur the constructor cost each time even when it's not generally needed.
Assignee | ||
Comment 12•14 years ago
|
||
(In reply to comment #11)
> Well then can we either make it a global variable...
Sure.
Assignee | ||
Comment 13•14 years ago
|
||
Unbitrotted, nsHttpResponseHead ctor called only when needed.
Attachment #450756 -
Attachment is obsolete: true
Attachment #452229 -
Flags: review+
Comment 14•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•