569044 - e10s HTTP: handle requests with no responseHead

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

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.

Comment 1

[Honza Bambas (:mayhemer)]

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.

Comment 2

[Honza Bambas (:mayhemer)]

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?

Comment 3

[Jason Duell]

Sounds good.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v1 - merged

Just merged patch.

Comment 7

[Jason Duell]

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.

Comment 8

[Honza Bambas (:mayhemer)]

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.

Comment 9

[Honza Bambas (:mayhemer)]

(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

[Josh Matthews [:jdm]]

Making it static is a no-no in any case, as it contains ns*Strings.  See bug 506348 for details.

Comment 11

[Jason Duell]

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.

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to comment #11)
> Well then can we either make it a global variable...

Sure.

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Unbitrotted, nsHttpResponseHead ctor called only when needed.

Comment 14

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/projects/electrolysis/rev/e9367578b98a