1452442 - "Copy as cURL" fails to copy requests once response tab is opened (TypeError: requestPostData.postData is undefined)

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

[Robert Xiao]

Attachment: copy-as-curl.patch

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180323154952

Steps to reproduce:

1. Open a new window
2. Browse anywhere (e.g. https://google.com)
3. Open developer tools (Toggle Tools), view network tab
4. Browse somewhere else (e.g. https://bing.com), wait for the requests to settle down
5. Select any request (e.g. /), click on Response tab, wait for that to load
6. Right click on the request and select Copy > Copy as cURL...


Actual results:

1. Nothing was copied to the clipboard (clipboard is unchanged)
2. In the browser console, the following error is seen:
TypeError: requestPostData.postData is undefined  RequestListContextMenu.js:315:7



Expected results:

1. The cURL command should be copied to the clipboard.

I believe I know the cause of this bug. In resource://devtools/client/netmonitor/src/widgets/RequestListContextMenu.js, starting at line 80, is the code

    copySubmenu.push({
      id: "request-list-context-copy-as-curl",
      label: L10N.getStr("netmonitor.context.copyAsCurl"),
      accesskey: L10N.getStr("netmonitor.context.copyAsCurl.accesskey"),
      // Menu item will be visible even if data hasn't arrived, so we need to check
      // *Available property and then fetch data lazily once user triggers the action.
      visible: !!(selectedRequest &&
        (requestHeadersAvailable || requestHeaders) &&
        (responseContentAvailable || responseContent)),
      click: () =>
        this.copyAsCurl(id, url, method, httpVersion, requestHeaders, responseContent),
    });

This incorrectly passes "responseContent" to copyAsCurl, which is expecting requestPostData. Thus, a patch to this would be simply

diff --git a/devtools/client/netmonitor/src/widgets/RequestListContextMenu.js b/devtools/client/netmonitor/src/widgets/RequestListContextMenu.js
index 7d0d926..ed5f65b 100644
--- a/devtools/client/netmonitor/src/widgets/RequestListContextMenu.js
+++ b/devtools/client/netmonitor/src/widgets/RequestListContextMenu.js
@@ -85,9 +85,9 @@ class RequestListContextMenu {
       // *Available property and then fetch data lazily once user triggers the action.
       visible: !!(selectedRequest &&
         (requestHeadersAvailable || requestHeaders) &&
-        (responseContentAvailable || responseContent)),
+        (requestPostDataAvailable || requestPostData)),
       click: () =>
-        this.copyAsCurl(id, url, method, httpVersion, requestHeaders, responseContent),
+        this.copyAsCurl(id, url, method, httpVersion, requestHeaders, requestPostData),
     });
 
     copySubmenu.push({

This proposed patch is attached.

Comment 1

[Robert Xiao]

Attachment: copy-as-curl-v2.patch

Sorry, that was a clearly bogus patch because it made the cURL context menu dependent on POST data. Here's a new patch that just removes the check altogether.

This also has the side-benefit of making the cURL option available even if the request isn't complete - which might fix bug#1378464 and bug#1420513.

Comment 2

[Robert Xiao]

The patch applies cleanly to https://hg.mozilla.org/mozilla-central/file/856a2a04097f/devtools/client/netmonitor/src/widgets/RequestListContextMenu.js (current as of five hours ago, with changes from bug#1269468), but the actual bug (passing responseContent instead of requestPostData) is present in my copy of Firefox 59 too.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8966015 [details] [diff] [review]
copy-as-curl-v2.patch

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

Thanks for working on this, the patch looks good to me!

We also need a test for this to avoid regression.

Perhaps we could extend the existing test?

devtools/client/netmonitor/test/browser_net_copy_as_curl.js

Honza

Comment 4

[Robert Xiao]

Attachment: copy-as-curl-v3.patch

New patch with tests. This patch should apply cleanly to tip (83de58ddda20).

The main change to the code is removing the request header conditional entirely, because the request header is lazy-loaded by the copyAsCurl method anyway. I think the copy as cURL option should always be available - if there's a case when the request headers might not be present, we can add an extra line to conditional requestHeaders.headers and a corresponding test for whatever triggers that unusual case.

This patch also fixes bug#1420513 and bug#1378464 (and there's a corresponding test for this).

Comment 5

[Robert Xiao]

(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> Comment on attachment 8966015 [details] [diff] [review]
> copy-as-curl-v2.patch
> 
> Review of attachment 8966015 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for working on this, the patch looks good to me!
> 
> We also need a test for this to avoid regression.
> 
> Perhaps we could extend the existing test?
> 
> devtools/client/netmonitor/test/browser_net_copy_as_curl.js
> 
> Honza

Anyone want to look at the new patch?

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8966512 [details] [diff] [review]
copy-as-curl-v3.patch

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

Loos good to me and green when testing locally.

R+

Thanks for the patch!

Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Note that the patch needs to be rebased on the current head before landing.

Honza

Comment 8

[Robert Xiao]

Attachment: copy-as-curl-v3-rebased_2d95d70298e2.patch

Rebased to 2d95d70298e2 (tip as of 2018-05-07 16:29 +0000).

Comment 9

[Robert Xiao]

Thanks for the review, Honza. I just rebased it - please take a look and merge it if it's all good.

Pleasure working with you through this process.

Robert

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8973797 [details] [diff] [review]
copy-as-curl-v3-rebased_2d95d70298e2.patch

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

Thanks for the rebase!

All you need to do now is to mark this bug as `checkin-needed` and one of the sheriffs will land it for you.  
You might want to read:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

1) Click the 'Edit Bug' blue button at the top of this page
2) Look for the Tracking section and Keywords input bot
3) Add 'checkin-needed' keyword into the box and click 'Save changes'

Honza

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Thanks for the patch, Robert! One request, please attach a patch which includes the necessary commit information needed for checkin (name/email/descriptive commit message) and re-request after doing so.

Comment 12

[Robert Xiao]

Attachment: formatted_copy-as-curl-v3-rebased_0cd106a2eb78.patch

Added HG header, rebased to latest tip (0cd106a2eb78) just in case (no changes to the diff).

Comment 13

[Robert Xiao]

Attachment: formatted_copy-as-curl-v3-rebased_0cd106a2eb78.patch

Fixed r=, remove spurious [PATCH] from commit notes.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4608ac6ced9
Fix Copy as cURL and add tests. r=Honza

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset c4608ac6ced9 (bug 1452442) for ES Lint failure on /builds/worker/checkouts/gecko/devtools/client/netmonitor/test/browser_net_copy_as_curl.js

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/de518c23c3c7a6d0779c874a9fbaff3cbb4df00c

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c4608ac6ced97d28916a38aa8830a1aee14d97bc

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=177576253&repo=mozilla-inbound&lineNumber=310

Comment 16

[Robert Xiao]

Attachment: copy-as-curl-v4.patch

Fixed the ESLint problems, ran ESLint to verify there are no more issues.

Comment 17

[Robert Xiao]

(In reply to Bogdan Tara[:bogdan_tara] from comment #15)
> Backed out changeset c4608ac6ced9 (bug 1452442) for ES Lint failure on
> /builds/worker/checkouts/gecko/devtools/client/netmonitor/test/
> browser_net_copy_as_curl.js
> 
> Backout link:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> de518c23c3c7a6d0779c874a9fbaff3cbb4df00c
> 
> Push with failure:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=c4608ac6ced97d28916a38aa8830a1aee14d97bc
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=177576253&repo=mozilla-
> inbound&lineNumber=310

Sorry about that - did not realize there was an ESLint pass (this is my first patch). Should be fixed now.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed269252f57
Fix Copy as cURL and add tests. r=Honza

Comment 19

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/0ed269252f57