"Copy Response" context menu is not shown when open "Response" tab

VERIFIED FIXED in Firefox 59

Status

defect
P2
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: magicp.jp, Assigned: rickychien)

Tracking

Trunk
Firefox 59
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 unaffected, firefox59 verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

2 years ago
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0 ID:20171221220104

Steps to reproduce:
1. Launch Nightly
2. Open Netmonitor (Ctrl+Shift+E)
3. Go to any sites (e.g. https://www.mozilla.org)
4. Select any requests and right clicking > context menu will be shown
5. Navigate context menu to Copy > Copy Response
6. Switch sidebar tab to "Response" (or select Copy Response in the context menu)
7. Confirm context menu again

Actual results:
In step 7, "Copy Response" menu is not shown.

Expected results:
"Copy Response" menu should be shown.

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=db713228c8e317cbc8e549edce7019b97ebd1b0b&tochange=6111cfdd4f938c15e8f8e165b40e31bcac5c849c
Reporter

Updated

2 years ago
Blocks: 1418927
Has Regression Range: --- → yes
Has STR: --- → yes
The fix is to ensure that lockdown *Available property right after requestData call finished.
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Summary: "Copy Response" context menu in not shown when open "Response" tab → "Copy Response" context menu is not shown when open "Response" tab
Comment on attachment 8938701 [details]
Bug 1426809 - Prevent fetching network update packet again after packet arrived

https://reviewboard.mozilla.org/r/209288/#review215364

::: devtools/client/netmonitor/src/utils/request-utils.js:94
(Diff revision 1)
>  
>      if (request[`${updateType}Available`] && !request[updateType]) {
> -      requestData(request.id, updateType);
> +      await requestData(request.id, updateType);
> +      // Lockdown *Available property once we fetch data from back-end.
> +      // Using this as a flag to prevent fetching arrived data again.
> +      request[`${updateType}Available`] = false;

I don't understand why the 'Copy Response' item is visible when the 'responseContentAvailable' is set to false here.

The condition for the menu item visibility is as follows:

`visible: !!(selectedRequest && responseContentAvailable)`

?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 6

2 years ago
mozreview-review
Comment on attachment 8938701 [details]
Bug 1426809 - Prevent fetching network update packet again after packet arrived

https://reviewboard.mozilla.org/r/209288/#review215460

::: devtools/client/netmonitor/src/utils/request-utils.js:94
(Diff revision 1)
>  
>      if (request[`${updateType}Available`] && !request[updateType]) {
> -      requestData(request.id, updateType);
> +      await requestData(request.id, updateType);
> +      // Lockdown *Available property once we fetch data from back-end.
> +      // Using this as a flag to prevent fetching arrived data again.
> +      request[`${updateType}Available`] = false;

You're right! What we did here is to lockdown *Available property when calling fetchNetworkUpdatePacket and do the check right here to prevent fetching twice, but this constrain doesn' apply in requestData call itself.

So if someone wants to fetch data from backend, just explict calling requestData.

I've submitted a separate patch to fixing real issue here. As a result, the first patch will be a small code improvement for reducing duplicated code in firefox-data-provider.js. I hope that makes sense to you!

Comment 7

2 years ago
hg error in cmd: hg pull upstream: pulling from https://hg.mozilla.org/integration/autoland
searching for changes
abort: HTTP Error 500: Internal Server Error

Comment 8

2 years ago
hg error in cmd: hg pull upstream: pulling from https://hg.mozilla.org/integration/autoland
searching for changes
abort: HTTP Error 500: Internal Server Error
Comment hidden (mozreview-request)
Attachment #8939571 - Attachment is obsolete: true
Attachment #8939571 - Flags: review?(odvarko)
Comment hidden (mozreview-request)
Patch has updated again to address the lockdown *Available property issue. Solutions in comment 6 have been changed, please read below:

* Lockdown *Available property has moved to requestData() instead of fetchNetworkUpdatePacket(). Because all fetched data will store in redux request state property and requestData() call is redux agnostic, we need to check request[`${updateType}Available`] before invoking requestData.
* Issue in RequestListContextMenu.js - lazily fetched data in RequestListContextMenu.js is supposed to be reusable if they have already fetched in request state. Therefore, we can prevent fetching them once they're ready.
* Fixing all incorrect 'visible' & 'click' properties in RequestListContextMenu's submenu item (including fixing the issue of Copy Response submenu.
Comment hidden (mozreview-request)
Comment on attachment 8938701 [details]
Bug 1426809 - Prevent fetching network update packet again after packet arrived

https://reviewboard.mozilla.org/r/209288/#review215764

Thanks for the update Ricky!
Please see my inline questions.

Honza

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:421
(Diff revision 4)
> +          // Using this as a flag to prevent fetching arrived data again.
> +          [`${method}Available`]: false,
> +        }, true);
>        }
>  
>        return payload;

It's a little weird that this methods returns either a payload or promise. I think it should be promise at all times. But, I am not going to block the patch for it.

::: devtools/client/netmonitor/src/widgets/RequestListContextMenu.js:289
(Diff revision 4)
>        .join(Services.appinfo.OS === "WINNT" ? "\r\n" : "\n");
>  
>      // Fall back to raw payload.
>      if (!string) {
> -      let { requestPostData } = await this.props.connector
> -        .requestData(id, "requestPostData");
> +      requestPostData = requestPostData ||
> +        await this.props.connector.requestData(id, "requestPostData").requestPostData;

This is related to my previous comment. How is it ensured that the return value from `requestData` is not a promise in this case?

::: devtools/client/netmonitor/src/widgets/RequestListContextMenu.js:307
(Diff revision 4)
> -  async copyAsCurl(id, url, method, requestHeaders, httpVersion) {
> -    if (!requestHeaders) {
> -      requestHeaders = await this.props.connector.requestData(id, "requestHeaders");
> -    }
> -    let { requestPostData } = await this.props.connector
> -      .requestData(id, "requestPostData");
> +  async copyAsCurl(id, url, method, httpVersion, requestHeaders, requestPostData) {
> +    requestHeaders = requestHeaders ||
> +      await this.props.connector.requestData(id, "requestHeaders");
> +
> +    requestPostData = requestPostData ||
> +      await this.props.connector.requestData(id, "requestPostData");

The same, the return value can't be a promise in this case (and there are other places in this module)
Assignee

Comment 14

2 years ago
mozreview-review-reply
Comment on attachment 8938701 [details]
Bug 1426809 - Prevent fetching network update packet again after packet arrived

https://reviewboard.mozilla.org/r/209288/#review215764

> It's a little weird that this methods returns either a payload or promise. I think it should be promise at all times. But, I am not going to block the patch for it.

This requestData() method will always return a promise [1], so yes, it will return promise at all times. And then the resolve value from the fulfilled / resolved promise chain would be payload object [2]. Thus, we've used a lots of `await` to wait and read the resolved value from a promise object like [3].

If I misunderstand your question here, please let me know.


[1] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-data-provider.js#428,445
[2] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-data-provider.js#440
[3] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/widgets/RequestListContextMenu.js#297
Comment on attachment 8938701 [details]
Bug 1426809 - Prevent fetching network update packet again after packet arrived

https://reviewboard.mozilla.org/r/209288/#review215820

(In reply to Ricky Chien [:rickychien] from comment #14)
> This requestData() method will always return a promise [1], so yes, it will
> return promise at all times.

Ah, you are right, I wrongly read the code.

R+

Thanks!
Honza
Attachment #8938701 - Flags: review?(odvarko) → review+

Comment 16

2 years ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ddd27352065
Prevent fetching network update packet again after packet arrived r=Honza

Comment 17

2 years ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/6b76f6a5ca63
Prevent fetching network update packet again after packet arrived r=Honza

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6b76f6a5ca63
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Reporter

Comment 19

2 years ago
This bug fix has verified in the latest Nightly build (20180104220114). Thanks!
Status: RESOLVED → VERIFIED

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.