Closed Bug 569044 Opened 11 years ago Closed 11 years ago

e10s HTTP: handle requests with no responseHead

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jduell.mcbugs, Assigned: mayhemer)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: fennecko
Summary: e10s HTTP: handle requests with no reponseHead → e10s HTTP: handle requests with no responseHead
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.
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?
Sounds good.
Duplicate of this bug: 570704
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #450384 - Flags: review?(jduell.mcbugs)
Attached patch v1 - merged (obsolete) — Splinter Review
Just merged patch.
Attachment #450384 - Attachment is obsolete: true
Attachment #450756 - Flags: review?(jduell.mcbugs)
Attachment #450384 - Flags: review?(jduell.mcbugs)
Blocks: 570003
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+
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.
(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.
Making it static is a no-no in any case, as it contains ns*Strings.  See bug 506348 for details.
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.
(In reply to comment #11)
> Well then can we either make it a global variable...

Sure.
Blocks: 536317
Attached patch v1.1Splinter Review
Unbitrotted, nsHttpResponseHead ctor called only when needed.
Attachment #450756 - Attachment is obsolete: true
Attachment #452229 - Flags: review+
Pushed http://hg.mozilla.org/projects/electrolysis/rev/e9367578b98a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.