1128038 - HTTP PATCH requests over SSL fail 40% of the time in FF 35 with HTTP2.0 flag set

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[pamela]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.91 Safari/537.36

Steps to reproduce:

(Note: We are going to move PATCH to PUT to fix this soon, so this may not work for you soon)

1. Visit https://www.khanacademy.org/computing/computer-programming/programming/drawing-basics/p/challenge-crazy-face
2. Open your network monitor.
3. Type a /. Wait 3 seconds. See PATCH request.
4. Repeat until you see a 400. It will look like it sent the request payload, but actually, it is turned into an empty string by the time it arrives at our server.

We did some experimentation and found that:

1) It does not happen over HTTP, just HTTPS

2) It does not happen if we set network.http.spdy.enabled.http2draft to false.

3) It does not happen if we proxy through Charles:

SSL connection from FF 35.0.1 to khanacademy.org:
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, 128 bit keys

SSL connection when proxying through Charles:
TLS_DHE_RSA_WITH_AES_128_CBC_SHA, 128 bit keys


We now see 50,000 incidents of these a day, and they have been steadily increasing since the release of Firefox 35. You can see that graph here:
https://twitter.com/pamelafox/status/560885009758687232


Actual results:

Server received an empty request payload.



Expected results:

Server should have received the full JSON payload.

Comment 1

[Patrick McManus [:mcmanus]]

hi Pamela, thanks for taking the time for the bug report. It seems this may be the same issue as bug 1072525 which we haven't been able to build a good repro for - so I'm actually glad to see this report.

It seems your testcase has already been transitioned off PATCH - I don't see a PATCH request when I load the resource.

Do you think you could make a failing version of the resource (that uses PATCH) available under a different URL so it doesn't interfere with your production traffic? Oh - and can you be clear which subresource is being carried with the PATH method? Those things would really help.

Lastly, and I know this is asking a lot - so if you can't do this part but can do the above things please don't think you have to do it all - an http log would help a lot if you can reproduce the issue. To generate the log see the directions here:

https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging

Comment 2

[Patrick McManus [:mcmanus]]

interesting - I see that you're using the google front end just as in 1072525 .. its possible (but unclear) the error is on their side (I presume they are gatewaying to http/1 to your service).. so the information in comment 1 would be very useful in getting whatever implementation that is having trouble up to speed.

Comment 3

[pamela]

Okay, I've deployed the PATCH version to a non-default version (that's an App Engine thing where it shares same datastore but different code):
https://znd-deleteme-dot-khan-academy.appspot.com/computing/computer-programming/programming/drawing-basics/p/challenge-crazy-face

I was able to get a 400 after 20 tries, so it can take a few minutes, since you have to wait a few seconds after typing before we send off the PATCH (it's debounced).
Hopefully you can get the 400 as well, otherwise I can try to set up the logging.

Comment 4

[Patrick McManus [:mcmanus]]

Pamela - this is my bug, thank you I was able to find it thanks to your test case.

with DAV methods sometimes we would put the END_STREAM bit on both the HEADERS frame and the DATA frame that carries the payload (in this case the json).

fix forthcoming

Comment 6

[Patrick McManus [:mcmanus]]

Attachment: h2 DAV methods set end_stream bit twice

Comment 7

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb1084756196

Comment 8

[u408661]

Comment on attachment 8558020 [details] [diff] [review]
h2 DAV methods set end_stream bit twice

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

Removing effectively duplicate code *and* fixing a semi-long-standing bug? NICE

Comment 9

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da85315b09a5

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da85315b09a5

Comment 11

[Patrick McManus [:mcmanus]]

Comment on attachment 8558020 [details] [diff] [review]
h2 DAV methods set end_stream bit twice

Approval Request Comment
[Feature/regressing bug #]:http2 original
[User impact if declined]: problem with http2 based DAV services.. h2 is also disabled on b2g due to this bug and fixing it means we can enable it there. new h2 services are being rolled out every day as http/2 finishes standardization
[Describe test coverage new/current, TreeHerder]: new tbpl test
[Risks and why]: very low risk
[String/UUID change made/needed]: none

Comment 12

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/17acc8fbe6b4
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/13fa209bd0b0