Closed Bug 1208114 Opened 4 years ago Closed 4 years ago

Firefox sends RST_STREAM for HTTPS URI stream with HTTP/2 proxy

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 --- unaffected
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: tatsuhiro.t, Assigned: mcmanus)

References

Details

(Keywords: regression, Whiteboard: [spdy])

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150826023504

Steps to reproduce:

Browse HTTPS site via HTTP/2 secure proxy (e.g., nghttpx) using Firefox Nightly.


Actual results:

When HTTPS URI is requested, Firefox sends CONNECT request. After getting 200 response from proxy, Firefox sends RST_STREAM with error code CANCEL.
Therefore, HTTPS page cannot be loaded anymore.


Expected results:

HTTPS URI should be loaded as before.
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
so this is somehow tied as a regression to 1204614 which is on firefox 43. Thanks for noting it. (flow control issue, somehow).

I'll work on it.
Whiteboard: [spdy]
Keywords: regression
Attachment #8671046 - Flags: review?(hurley)
Assignee: nobody → mcmanus
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8671046 [details] [diff] [review]
fix h2 connect tunnels

Review of attachment 8671046 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/TunnelUtils.h
@@ +200,5 @@
>                           uint32_t count, uint32_t *countWritten) override final;
>    nsHttpRequestHead *RequestHead() override final;
>    void Close(nsresult reason) override final;
>  
> +  bool ConnectedReadyForInput();

I'm not wild about this name - when first reading the patch, it seemed to me like "well, if it's ready for input, why does it need to be buffered?"

Actually, I think the name itself is fine (because the stream in general is ready for input, it's just a little full right now and needs a moment), but there might need to be a clarifying comment that the whole "ready for input" part is more general, but the stream could still return WOULD_BLOCK.
Attachment #8671046 - Flags: review?(hurley) → review+
so the mediatest fail was in my try run - sorry about that. 

This is h2 only code, so I only actually looked at the xpcshell results as that's the only server in our suite that knows h2.

turns out the mediatest is contacting youtube (h2). I thought offsite tests were something we didn't do..
Flags: needinfo?(mcmanus)
The new assert included in this patch set did indeed catch a bug - but its a latent unrelated one. I'll reland without the assert (it will have the same old legacy runtime behavior) and file a separate bug about the new issue.

Http2Session::OnWriteSegment has a state machine and for any state not covered in there it returns NS_ERROR_UNEXPECTED. I added the assert() when that happens.

The state generated in https://www.youtube.com/watch?v=1visYpIREUM was DISCARDING_DATA_FRAME_PADDING which is probably ok to whitelist as a nop - but lets do it separately.
https://hg.mozilla.org/mozilla-central/rev/c66bf1d6890b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
I confirmed this bug has been fixed in current nightly.
Thank you very much!
You need to log in before you can comment on or make changes to this bug.