Closed Bug 1426809 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Netmonitor, defect, P2)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

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

Status:
VERIFIED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- verified

People

(Reporter: magicp.jp, Assigned: rickychien)

References

Details

Attachments

(2 files, 1 obsolete file)

steps-to-reproduce_20171222_141612.mp4
845.21 KB, video/mp4
Details
Bug 1426809 - Prevent fetching network update packet again after packet arrived
59 bytes, text/x-review-board-request
Honza
: review+
Details 
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
Blocks: 1418927
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox57: --- → unaffected
status-firefox58: --- → unaffected
status-firefox-esr52: --- → unaffected
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 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!
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
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
Attachment #8939571 - Attachment is obsolete: true
Attachment #8939571 - Flags: review?(odvarko)
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 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)
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+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ddd27352065
Prevent fetching network update packet again after packet arrived r=Honza
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/6b76f6a5ca63
Prevent fetching network update packet again after packet arrived r=Honza
https://hg.mozilla.org/mozilla-central/rev/6b76f6a5ca63
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
This bug fix has verified in the latest Nightly build (20180104220114). Thanks!
Status: RESOLVED → VERIFIED
status-firefox59: fixed → verified
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: