1253487 - Copy as curl generate incorrect --2.0

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[phuongnd08]

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

Steps to reproduce:

1. Open Firefox on Mac OSX
2. Go to www.facebook.com
3. Try to post a comment
4. Use copy as curl to copy the comment
5. Run on mac Terminal


Actual results:

curl: option --2.0: is unknown


Expected results:

A comment posted on facebook

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I don't have a Facebook account, so this will be hard to reproduce...  Do you have any more details about the request?  Can you include more of the curl command?

Comment 2

[phuongnd08]

On which version of curl that --2.0 is supported you know? Because with curl 7.43.0 (x86_64-apple-darwin14.0) libcurl/7.43.0 SecureTransport zlib/1.2.5 it's apparently not supported.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to phuongnd08 from comment #2)
> On which version of curl that --2.0 is supported you know? Because with curl
> 7.43.0 (x86_64-apple-darwin14.0) libcurl/7.43.0 SecureTransport zlib/1.2.5
> it's apparently not supported.

I am guessing it's probably nonsense that was never supported by any curl version, so we need to figure out what caused it to appear.  Can you help with the questions from comment 1?

Comment 4

[phuongnd08]

I think you can register a facebook account which take 5 seconds, then make your own post and comment to see the request which take another 5 seconds then use the Copy as cURL.

Comment 5

[Andreas Jung]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) (at work week until June 21) from comment #3)
> I am guessing it's probably nonsense that was never supported by any curl
> version, so we need to figure out what caused it to appear.  Can you help
> with the questions from comment 1?

This (incorrect) command line option is generated for all HTTP/2.0 requests (also can be reproduced on e.g. google.com).

curl apparently uses HTTP/1.1 by default, correct values to change that are:
 -0, --http1.0       Use HTTP 1.0 (H)
     --http1.1       Use HTTP 1.1 (H)
     --http2.0       Use HTTP 2.0 (H)

Comment 6

[ph4nt0m055]

Same for me, even curl 7.50 with http2 support does not understand that --2.0 option

And prints:
curl: option --2.0: is unknown
curl: try 'curl --help' or 'curl --manual' for more information

curl -V
curl 7.50.1 (x86_64-apple-darwin15.6.0) libcurl/7.50.1 OpenSSL/1.0.2h zlib/1.2.5 nghttp2/1.14.0

Comment 7

[Nickolay_Ponomarev]

(In reply to Andreas Jung from comment #5)
> curl apparently uses HTTP/1.1 by default, correct values to change that are:
>  -0, --http1.0       Use HTTP 1.0 (H)
>      --http1.1       Use HTTP 1.1 (H)
>      --http2.0       Use HTTP 2.0 (H)

Interestingly, the curl installed with macports (port install curl +http2) doesn't accept '--http2.0' and insists that '--http2' is correct.

> curl 7.50.3 (x86_64-apple-darwin13.4.0) libcurl/7.50.3 OpenSSL/1.0.2i zlib/1.2.8 nghttp2/1.14.1
> Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
> Features: IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets

Comment 8

[Andreas Jung]

curl 7.35.0 (x86_64-pc-linux-gnu) libcurl/7.35.0 says available options are:
 -0, --http1.0       Use HTTP 1.0 (H)
     --http1.1       Use HTTP 1.1 (H)
     --http2.0       Use HTTP 2.0 (H)

curl 7.50.1 (x86_64-pc-linux-gnu) libcurl/7.50.1 says available options are:
 -0, --http1.0       Use HTTP 1.0 (H)
     --http1.1       Use HTTP 1.1 (H)
     --http2         Use HTTP 2 (H)
     --http2-prior-knowledge  Use HTTP 2 without HTTP/1.1 Upgrade (H)

So the exact command line option apparently changed at some point. (A quick glance on the curl changelog doesn't reveal when exactly though).

Comment 9

[Tim McCormack]

Andreas, can you double-check that --http2.0 vs. --http2 on your system? What version do you have?

On Debian Jessie I see:

curl 7.38.0 (x86_64-pc-linux-gnu) libcurl/7.38.0 OpenSSL/1.0.1t zlib/1.2.8 libidn/1.29 libssh2/1.4.3 librtmp/2.3
       --http2
              (HTTP)  Tells  curl  to  issue  its  requests using HTTP 2. This
              requires that the underlying libcurl was built  to  support  it.
              (Added in 7.33.0)

Comment 10

[Tim McCormack]

Ugh, sorry, just saw your latest comment. Well. Any idea if 7.35.0 is still in common use or how to find out? Because Debian Stable tends to trail, and even it has the newer flag.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Seems like we'd need to make a change around here:

http://searchfox.org/mozilla-central/source/devtools/client/shared/curl.js#108

I would guess it needs the "http" prefix, and if the value is "2.0", trim to just "2".

Take a look at https://wiki.mozilla.org/DevTools/Hacking to get started with the code base.  Set the need info field at the bottom of this bug to mentor if you have questions.

Comment 12

[Tim McCormack]

Attachment: http2-untested.patch - No tests, manually checked; proof of concept

I gave it a shot, but won't be able to do this bug justice. Attached is a proof of concept, but I had the following difficulties:

1. curl 7.38.0, as packaged for Debian Jessie (and apparently recent Ubuntu?), is not compiled with HTTP/2 support even though it knows the --http2 flag. See this Ubuntu ticket for repackaging: https://bugs.launchpad.net/ubuntu/+source/curl/+bug/1567697
2. I don't know how to get the mochitests to make HTTP/2 calls that I can then copy to clipboard. (It would also be nice to confirm that HTTP/1.0 calls are unaltered by this change, but can Firefox even make them?)

I'm also just plain out of time for this task. Hopefully someone will take this patch and make a better one with tests. :-)

Comment 13

[joshua.d.horwitz]

I would like to work on this with the mentor if possible and this is still available.

Comment 14

[--]

This bug is still present in Firefox 58 and it causes curl users grief. The correct option should be "--http2" if you insist on enforcing HTTP/2.

But... not all curl versions have HTTP/2 enabled (hello Mac OS users) and those that *have* HTTP/2 enabled, already try to use HTTP/2 by default on HTTPS sites. So there's really no reason for Firefox to ever generate this option.

I propose to just drop it.

Comment 15

[Boris Zbarsky [:bzbarsky]]

jryans, did you ever respond to comment 13?

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to joshua.d.horwitz from comment #13)
> I would like to work on this with the mentor if possible and this is still
> available.

Sorry for missing this message earlier!  Looks like it got lost in my inbox.  Please use the needinfo field at the bottom of the bug and set it to mentor if you have questions.  That will ensure I don't miss them next time.

In any case, yes, I am still happy to help you or others with this change.  Please see my earlier tips in comment 11.  With :bagder's advice in comment 14, it seems reasonable to just remove the option.

Comment 17

[nchambers]

I ran into someone on IRC who had this issue. This https://spectre.spookyinternet.com/dl/curl.sh shell script might be useful as a temporary work around for those that use sh/bash/maybe some others. If anyone wants to use it, feel free. (This is by no means a real solution, and as was pointed out to me, has an edge case if you do curl -o --2.0 ...).

Comment 18

[--]

Attachment: 0001-bug-1253487-remove-the-explicit-HTTP-version-r.patch

Here's my take at a patch that simply removes the selection of explicit HTTP version. Based on the motivation explained above.

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8911671 [details] [diff] [review]
0001-bug-1253487-remove-the-explicit-HTTP-version-r.patch

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

Thanks for the patch, I am happy to review it!  (I don't Jarda is actively involved lately, we should probably update the suggested reviewers...  I sent a mail internally to have someone update it.)

This change makes sense to me!  There is a test at http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_copy_as_curl.js, but it doesn't seem like it ever checked the HTTP version, so it probably fine as-is.

Comment 20

[--]

oops, let me refresh it first

Comment 21

[--]

Attachment: v2-0001-bug-1253487-remove-the-explicit-HTTP-version-r-jr.patch

fixed commit message

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79a2f1bec652
Remove the explicit HTTP version from curl.js. r=jryans

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c00155154bad
Remove the explicit HTTP version from curl.js: Remove unused const. r=eslint-fix

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/79a2f1bec652
https://hg.mozilla.org/mozilla-central/rev/c00155154bad