Closed Bug 1404928 Opened 7 years ago Closed 7 years ago

Request Post DATA should be loaded lazily

Categories

(DevTools :: Netmonitor, enhancement, P2)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox57 fix-optional, firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox57 --- fix-optional
firefox59 --- fixed

People

(Reporter: ochameau, Assigned: rickychien)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug 1404928 - Request Post DATA should be loaded lazily
(deleted), text/x-review-board-request
Honza
: review+
ochameau
: review+
Details
Component: Menus → Developer Tools: Netmonitor
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Depends on: 1336382
It seems that not really helpful based on latest DAMP result. It's also weird that there is no subtests data in DAMP result.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=5d2094b3edb6&framework=1&filter=damp&selectedTimeRange=86400
Insignificant result from comment 3 may be because requestPostData doesn't appear in high frequency in our damp test. I believe it's also not a normal case that every browsing experience will cause such large amounts of requestPostData. However, loading requestPostData lazily is still worth doing and bring optimization for some websites.
(In reply to Ricky Chien [:rickychien] from comment #3)
> It seems that not really helpful based on latest DAMP result. It's also
> weird that there is no subtests data in DAMP result.
> 
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> central&newProject=try&newRevision=5d2094b3edb6&framework=1&filter=damp&selec
> tedTimeRange=86400

Yes, it looks like something is broken on PerfHerder...

Joel, any idea what is going on here?
The "subtests" link no longer exists...


Otherwise, DAMP may not report any win here because the tests don't have any request with POST data.
May be we should add a test document with at least one...
So yes, this patch is still really worth doing, I would just feel more confident by seing the subtests results still.
Flags: needinfo?(jmaher)
Comment on attachment 8929026 [details]
Bug 1404928 - Request Post DATA should be loaded lazily

https://reviewboard.mozilla.org/r/200342/#review205948

Please see my inline comments.

One more thing, clicking on a request doesn't select it. It looks like there is a selected request ID internally, but the row isn't highlighted.

Thanks Ricky for working on this!
Honza

::: devtools/client/netmonitor/src/components/CustomRequestPanel.js:34
(Diff revision 3)
>  const CUSTOM_NEW_REQUEST = L10N.getStr("netmonitor.custom.newRequest");
>  const CUSTOM_POSTDATA = L10N.getStr("netmonitor.custom.postData");
>  const CUSTOM_QUERY = L10N.getStr("netmonitor.custom.query");
>  const CUSTOM_SEND = L10N.getStr("netmonitor.custom.send");
>  
> -function CustomRequestPanel({
> +class CustomRequestPanel extends Component {

Please create a comment describing what this panel is responsible for.

/**
 * This panel is used for resending an existing ...
 */
 
 
(we should do this for every component)

::: devtools/client/netmonitor/src/request-list-context-menu.js:62
(Diff revision 3)
>     */
> -  open(event = {}) {
> +  async open(event = {}) {
>      let selectedRequest = this.selectedRequest;
> +    if (!selectedRequest) {
> +      return;
> +    }

Why si the menu disabled when clicking outside of the list? Items like "Start Performance Analysis...", "Save All as HAR", etc. still make sense.

Of course, in case of saving to har, post data (as well as response content) should be fetched automatically.

::: devtools/client/netmonitor/src/request-list-context-menu.js:72
(Diff revision 3)
> +      requestPostData,
> +      responseContent,
> +    ] = await Promise.all([
> +      this.requestData(selectedRequest.id, "requestPostData"),
> +      this.requestData(selectedRequest.id, "responseContent"),
> +    ]);

Can we fetch the data when the user actually executes related action? (e.g. Copy Post Data).
Attachment #8929026 - Flags: review?(odvarko) → review-
the subtests are broken as of bug 1418670 to perfherder:
https://github.com/mozilla/treeherder/commit/aa270ca5d974fe2207304d6c9dcdd8b8cbf24005
Flags: needinfo?(jmaher)
Comment on attachment 8929384 [details]
Bug 1404928 - Avoid passing dispatch props and keep RequestListContnet interface clearly

https://reviewboard.mozilla.org/r/200712/#review205966

I like the patch, makes the code cleaner!

R+, assuming try is green

If you have troubles to keep both patches in sync you might merge (the set of changes touches the same code as the previous patch)

Honza
Attachment #8929384 - Flags: review?(odvarko) → review+
sorry it is bug 1416870, we are working on fixing this and getting it deployed today so the perfherder subtest view will work.
Comment on attachment 8929026 [details]
Bug 1404928 - Request Post DATA should be loaded lazily

https://reviewboard.mozilla.org/r/200342/#review205948

> Can we fetch the data when the user actually executes related action? (e.g. Copy Post Data).

This is a good question. I'd prefer to fetch all needed data as much as I can once contextmenu is invoked, even if it doesn't hit any performance issue.

Moreover, line 94 is accessing `requestPostData` inevitably. Thus, I think moving those `requestData` calls at the beginning of `open` fucntion will make more sense.
Comment on attachment 8930002 [details]
Bug 1404928 - Fix test failures

https://reviewboard.mozilla.org/r/201192/#review206334

Looks good,

R+ assuming try is green

Thanks Ricky!
Honza

::: devtools/client/netmonitor/src/components/TabboxPanel.js:62
(Diff revision 3)
>        TabPanel({
>          id: PANELS.HEADERS,
>          title: HEADERS_TITLE,
>        },
> -        HeadersPanel({ request, cloneSelectedRequest, openLink }),
> +        HeadersPanel({ connector, request, cloneSelectedRequest, openLink }),
>        ),

nit: please put arguments on separate lines

::: devtools/client/netmonitor/test/browser_net_curl-utils.js:21
(Diff revision 3)
>    let { store, windowRequire, connector } = monitor.panelWin;
>    let Actions = windowRequire("devtools/client/netmonitor/src/actions/index");
>    let {
>      getSortedRequests,
>    } = windowRequire("devtools/client/netmonitor/src/selectors/index");
> -  let { getLongString } = connector;
> +  let { getLongString, requestData } = connector;

nit: separate lines
Attachment #8930002 - Flags: review?(odvarko) → review+
Comment on attachment 8929026 [details]
Bug 1404928 - Request Post DATA should be loaded lazily

https://reviewboard.mozilla.org/r/200342/#review206336

Thanks Ricky,

Re: context menu, I've been testing this and fetching post-data at menu-open seems to be quite ok in case the data are not 'too big'. 
The sub-menu opens relatively quickly. Only in a test where I posted 8MB, it took several seconds to get the sub-menu and it felt
as a bug. But, 8MB feels like corner case, so let's ignore it for now.

Honza
Attachment #8929026 - Flags: review?(odvarko) → review+
Comment on attachment 8929026 [details]
Bug 1404928 - Request Post DATA should be loaded lazily

https://reviewboard.mozilla.org/r/200342/#review206350

Sorry to bypass the review, but you are undoing my recent work from bug 1404917.

(In reply to Ricky Chien [:rickychien] from comment #13)
> Comment on attachment 8929026 [details]
> Bug 1404928 - Request Post DATA should be loaded lazily
> 
> https://reviewboard.mozilla.org/r/200342/#review205948
> 
> > Can we fetch the data when the user actually executes related action? (e.g. Copy Post Data).
> 
> This is a good question. I'd prefer to fetch all needed data as much as I
> can once contextmenu is invoked, even if it doesn't hit any performance
> issue.
> 
> Moreover, line 94 is accessing `requestPostData` inevitably. Thus, I think
> moving those `requestData` calls at the beginning of `open` fucntion will
> make more sense.

Can't you introduce requestPostDataAvailable, like responseContentAvailable?
  https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-data-provider.js#410-412
I think you could, from here:
  https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-data-provider.js#380

::: devtools/client/netmonitor/src/request-list-context-menu.js:70
(Diff revision 5)
> +      requestPostData,
> +      responseContent,
> +    ] = await Promise.all([
> +      this.requestData(selectedRequest.id, "requestPostData"),
> +      this.requestData(selectedRequest.id, "responseContent"),
> +    ]);

Please, don't force loading `responseContent` from the context menu.
You are undoing the efforts I did in bug 1404917!
There is no good reason to force loading it here.
Attachment #8929026 - Flags: review-
Attachment #8930002 - Attachment is obsolete: true
Attachment #8929384 - Attachment is obsolete: true
Attachment #8929384 - Flags: review?(poirot.alex)
Merge into one commit due to too many merging conflicts when rebasing on top of bug 1408182.
Alex, I've addressed all issues on comment 24, would you like to do me a favor to do a sanity check? Thanks
Comment on attachment 8929026 [details]
Bug 1404928 - Request Post DATA should be loaded lazily

https://reviewboard.mozilla.org/r/200342/#review207356

I would like to know why we need the `if (request) {` check in firefox-data-provider.js before proceeding.

::: devtools/client/netmonitor/src/components/CustomRequestPanel.js:65
(Diff revision 7)
> +  }
> +
> +  maybeFetchPostData(props) {
> +    if (!props.request.requestPostData ||
> +        !props.request.requestPostData.postData.text) {
> +      let actorId = props.request.id.replace("-clone", "");

You should add a comment about that.

::: devtools/client/netmonitor/src/components/RequestListContent.js:201
(Diff revision 7)
>      }
>    }
>  
>    onContextMenu(evt) {
>      evt.preventDefault();
> -    this.contextMenu.open(evt);
> +    this.contextMenu.open(evt.nativeEvent);

This change looks totally unrelated to this bug and not really used/justified?

If that happen to address something I would suggest to attach this to its own bug with a description of what you are addressing here.

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:529
(Diff revision 7)
>        // Remove the request from the cache, any new call to requestData will fetch the
>        // data again.
>        this.lazyRequestData.delete(key, promise);
>  
> -      let payloadFromQueue = this.getRequestFromQueue(actor).payload;
> +      let request = this.getRequestFromQueue(actor);
> +      if (request) {

This check is surprising. You should not need it.
There should always be a related request in queue.
Do you have any STR/test where you reproduce that?

In theory, `_requestData` is going to call onResponseContent/onRequestPostData, which call updateRequest and register an entry in the queue.

So if that doesn't fail anywhere I would prefer to not have this check as it hides some issue.
If that fails, may be there is something else to fix instead.

::: devtools/client/netmonitor/src/har/har-builder.js:170
(Diff revision 7)
> -    }
> -
>      request.headersSize = file.requestHeaders.headersSize;
> +    request.postData = await this.buildPostData(file);
>  
> +    if (file.requestPostData && file.requestPostData.postData.text) {

Now file.RequestPostData is only set when we go through HarAutomation codepath. I won't be set when running through the netmonitor usecase.

So I think you should do that now:
if (request.postData && request.postData.text) {
  request.bodySize = request.postData.text.length
}

Also, I think there is no need to re-do the fetchData call. We could have done that before your patch.
Attachment #8929026 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #31)
> ::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:529
> (Diff revision 7)
> >        // Remove the request from the cache, any new call to requestData will fetch the
> >        // data again.
> >        this.lazyRequestData.delete(key, promise);
> >  
> > -      let payloadFromQueue = this.getRequestFromQueue(actor).payload;
> > +      let request = this.getRequestFromQueue(actor);
> > +      if (request) {
> 
> This check is surprising. You should not need it.
> There should always be a related request in queue.
> Do you have any STR/test where you reproduce that?
> 
> In theory, `_requestData` is going to call
> onResponseContent/onRequestPostData, which call updateRequest and register
> an entry in the queue.
> 
> So if that doesn't fail anywhere I would prefer to not have this check as it
> hides some issue.
> If that fails, may be there is something else to fix instead.

Good catch! I'm stuck in test failure due to like actor connection closed error at [1] after removing `if (request)` check


89 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_accessibility-01.js | A promise chain failed to handle a rejection: 'getRequestPostData' active request packet to 'server1.conn0.child1/netEvent30' can't be sent as the connection just closed. - stack: null

Alex, I didn't see you modified any actor part in bug 1404917 but your patch still works well. Not sure why we get this kind of connection error when running tests. Any thoughts about this error?

[1] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-data-provider.js#558
Flags: needinfo?(poirot.alex)
Error message comes from [1]

[1] https://searchfox.org/mozilla-central/source/devtools/shared/client/debugger-client.js#1051-1053

Honza, do you have any idea what root cause of this?
Flags: needinfo?(odvarko)
(In reply to Ricky Chien [:rickychien] from comment #33)
> 89 INFO TEST-UNEXPECTED-FAIL |
> devtools/client/netmonitor/test/browser_net_accessibility-01.js | A promise
> chain failed to handle a rejection: 'getRequestPostData' active request
> packet to 'server1.conn0.child1/netEvent30' can't be sent as the connection
> just closed. - stack: null

This happened to me before and the problem was that date were requested from the back-end after the test finished. I forgot to wait for async code before finishing the test.

Honza
Flags: needinfo?(odvarko)
Comment on attachment 8929026 [details]
Bug 1404928 - Request Post DATA should be loaded lazily

https://reviewboard.mozilla.org/r/200342/#review207724

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:611
(Diff revision 8)
> -    return this.updateRequest(response.from, {
> +    let payload = await this.updateRequest(response.from, {
>        requestPostData: response
> -    }).then(() => {
> -      emit(EVENTS.RECEIVED_REQUEST_POST_DATA, response.from);
>      });
> +    emit(EVENTS.RECEIVED_REQUEST_POST_DATA, response.from);

You can wait for this event in order to ensure the request is done.

I used that for lazy response content over here for example:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_brotli.js#54

One even better way to wait would be to wait for React component to be updated with the expected Post DATA in the headers panel (or wherever we try loading Post DATA).
Attachment #8929026 - Flags: review?(poirot.alex)
Flags: needinfo?(poirot.alex)
In order to get `requestHeadersFromUploadStream`, we need to fetch post data in HeaderPanel. Thus, every single request click will open HeaderPanel, and then try to fetch post data via getRequestPostData(). Our tests try to invoke teardown while back-end is still sending packets via getRequestPostData(), so leading the actor connection closed error. 

We can add `monitor.panelWin.once(EVENTS.RECEIVED_REQUEST_POST_DATA)` around every select request action [1], but there are plenty of tests [2] are doing this. It could be cumbersome and test would be fragile (Writing new test which is going to select a request will need to wait for EVENTS.RECEIVED_REQUEST_POST_DATA).

However, those failed tests don't really check the content of `requestPostData`. So it's unnecessary to wait for `monitor.panelWin.once(EVENTS.RECEIVED_REQUEST_POST_DATA)` in tests.

I believe this kind of connection closed errors might be unharmed, so I'd propose to just skip this failure by wrapping try catch in _requestData() and make a comment to explanation.

Alex, do you think that make sense to you?

[1] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_accessibility-01.js#41
[2] https://treeherder.mozilla.org/#/jobs?repo=try&author=rchien@mozilla.com&selectedJob=147002779
Flags: needinfo?(poirot.alex)
(In reply to Ricky Chien [:rickychien] from comment #37)
> I believe this kind of connection closed errors might be unharmed, so I'd
> propose to just skip this failure by wrapping try catch in _requestData()
> and make a comment to explanation.

ah, try catch might not help in this case. So the previous if (request) check might be a good workaround for this case.
(In reply to Ricky Chien [:rickychien] from comment #37)
> In order to get `requestHeadersFromUploadStream`, we need to fetch post data
> in HeaderPanel. Thus, every single request click will open HeaderPanel, and
> then try to fetch post data via getRequestPostData(). Our tests try to
> invoke teardown while back-end is still sending packets via
> getRequestPostData(), so leading the actor connection closed error. 
> 
> We can add `monitor.panelWin.once(EVENTS.RECEIVED_REQUEST_POST_DATA)` around
> every select request action [1], but there are plenty of tests [2] are doing
> this. It could be cumbersome and test would be fragile (Writing new test
> which is going to select a request will need to wait for
> EVENTS.RECEIVED_REQUEST_POST_DATA).
> 
> However, those failed tests don't really check the content of
> `requestPostData`. So it's unnecessary to wait for
> `monitor.panelWin.once(EVENTS.RECEIVED_REQUEST_POST_DATA)` in tests.
> 
> I believe this kind of connection closed errors might be unharmed, so I'd
> propose to just skip this failure by wrapping try catch in _requestData()
> and make a comment to explanation.

I disagree. Tests should wait for full completion of any async code it dispatches.
Otherwise there may be overlap between two tests. The first test would still run
while the second starts running and lead to hard to debug issues!

I would suggest adding a "select request" helper doing both correct event waiting and request selection.

Also, I see you added some code to fetch requestPostData from HeadersPanel.js.
Why? This component doesn't seem to use that field?
If that's really needed, you should add a comment saying why.

And finally, in ParamsPanel.js, I think you should check for requestPostDataAvailable before trying to fetch POST Data field.
(this comment may also apply to CustomRequestPanel)

I have a very different view on this story. Not waiting correctly would make the test fragile.
A first action, selecting a request, would still run request/code/redux/react while another one already starts running!
Okay, I'm going to create a new "select request" helper function and replace all tests with it.
Flags: needinfo?(poirot.alex)
Test failures have been fixed! \o/

These packet connection problems have gone away without using any helper function after adding requestPostDataAvailable check across HeaderPanel, ParamsPanel and CustomRequestPanel. browser_net_raw_headers.js is the only one test which needs to wait for post data event arrived, apart from that, all tests are not need to wait for additional event.

Comment of why we need to call requestPostData from HeadersPanel.js has added, other open issues were addressed as well. Please have a look :)
Comment on attachment 8929026 [details]
Bug 1404928 - Request Post DATA should be loaded lazily

https://reviewboard.mozilla.org/r/200342/#review208086

Thanks for addressing all my comments.

I still have concerns about CustomRequestPanel and requestHeadersFromUploadStream in Har.

::: devtools/client/netmonitor/src/components/CustomRequestPanel.js:70
(Diff revision 13)
> +   */
> +  maybeFetchPostData(props) {
> +    if (props.request.requestPostDataAvailable &&
> +        (!props.request.requestPostData ||
> +        !props.request.requestPostData.postData.text)) {
> +      let actorId = props.request.id.replace("-clone", "");

Please add a comment to explain why you remove "-clone".

Because I'm lost here. Is it any useful to fetch the PostData here?
My comprehension of CustomRequestPanel is that we instanciate this React component with the *cloned request*. So `request` object everywhere in this component will refer to the cloned one, right?
But here you will make it so that the *original request* will get its `requestPostData` filled.
It looks useless as it will update the *original request*, which we do not use here. Do we?

::: devtools/client/netmonitor/src/components/ResponsePanel.js
(Diff revision 13)
>    }
>  
> -  /**
> -   * `componentWillReceiveProps` is the only method called when switching between two
> -   * requests while the response panel is displayed.
> -   */

Why do you remove these comments? (same comment applies to SecurityPanel.js)
It helps understanding why we have them both.

::: devtools/client/netmonitor/src/har/har-builder.js:261
(Diff revision 13)
>        postData.comment = L10N.getStr("har.requestBodyNotIncluded");
>        return postData;
>      }
> -
>      // Load request body from the backend.
> -    this.fetchData(file.requestPostData.postData.text).then(postDataText => {
> +    postData = await this.fetchData(requestPostData.postData.text).then(postDataText => {

You can simplify this function now that you use async/await:
  let postDataText = await this.fetchData(requestPostData.postData.text);
  postData.tet = postDataText;

::: devtools/client/netmonitor/src/har/har-builder.js:272
(Diff revision 13)
>          postData.mimeType = "application/x-www-form-urlencoded";
>  
>          // Extract form parameters and produce nice HAR array.
>          getFormDataSections(
>            file.requestHeaders,
>            file.requestHeadersFromUploadStream,

I don't think file.requestHeadersFromUploadStream is going to be set by `requestData(file.id, "requestPostData");` call.
The story behind `requestHeadersFromUploadStream` is complex... I completely missed that. May be requestData should return both Post Data and Headers from uploadstream? May be `fetchPostData` should be inlined into `onRequestPostData`?

::: devtools/client/webconsole/webconsole-connection-proxy.js:247
(Diff revision 13)
>    dispatchMessageUpdate: function (networkInfo, response) {
>      this.webConsoleFrame.newConsoleOutput.dispatchMessageUpdate(networkInfo, response);
>    },
>  
>    dispatchRequestUpdate: function (id, data) {
> +    if (this.webConsoleFrame) {

Do you still need this check?
If not, better remove it to prevent hiding potential issues.
Attachment #8929026 - Flags: review?(poirot.alex)
Comment on attachment 8929026 [details]
Bug 1404928 - Request Post DATA should be loaded lazily

https://reviewboard.mozilla.org/r/200342/#review208086

> Please add a comment to explain why you remove "-clone".
> 
> Because I'm lost here. Is it any useful to fetch the PostData here?
> My comprehension of CustomRequestPanel is that we instanciate this React component with the *cloned request*. So `request` object everywhere in this component will refer to the cloned one, right?
> But here you will make it so that the *original request* will get its `requestPostData` filled.
> It looks useless as it will update the *original request*, which we do not use here. Do we?

Yep, nice catch! Fetching PostData is useless here. I've removed them.

> Why do you remove these comments? (same comment applies to SecurityPanel.js)
> It helps understanding why we have them both.

componentDidMount and componentWillReceiveProps are react built-in APIs which behoviors are details documented on their own. I don't think it's necessary to explain all call paths which will trigger those life cycle functions. All Panels' scenarios are simple, if we look at Column like CookiesColumn, every show/hide or lazy requestCookies update can invoke these functions.

> I don't think file.requestHeadersFromUploadStream is going to be set by `requestData(file.id, "requestPostData");` call.
> The story behind `requestHeadersFromUploadStream` is complex... I completely missed that. May be requestData should return both Post Data and Headers from uploadstream? May be `fetchPostData` should be inlined into `onRequestPostData`?

I'm losting here. I guess that you're suggesting

```
payload: {
  uploadstream: {
    postData: ...,
    headers: ...,
  }
}
```
I know the structure of netmonitor's request payload is a mess... However, I don't prefer to do refactor within this bug. It's risk and might cause other regressions IMO.
(In reply to Ricky Chien [:rickychien] from comment #48)
> > I don't think file.requestHeadersFromUploadStream is going to be set by `requestData(file.id, "requestPostData");` call.
> > The story behind `requestHeadersFromUploadStream` is complex... I completely missed that. May be requestData should return both Post Data and Headers from uploadstream? May be `fetchPostData` should be inlined into `onRequestPostData`?
> 
> I'm losting here. I guess that you're suggesting
> 
> ```
> payload: {
>   uploadstream: {
>     postData: ...,
>     headers: ...,
>   }
> }
> ```
> I know the structure of netmonitor's request payload is a mess... However, I
> don't prefer to do refactor within this bug. It's risk and might cause other
> regressions IMO.

Unfortunately, I think HAR is broken with your patch, so, something has to be done.

Do you confirm that `file.requestHeadersFromUploadStream` is not going to be set when using HAR from the toolbox?
My understanding is that `requestHeadersFromUploadStream` is computed in FirefoxDataProvider, in `fetchData`,
but it won't be returned by the call to `requestData(file.id, "requestPostData");`.
`requestHeadersFromUploadStream` is set on the `payload` object, but it isn't returned by requestData.
So you would need to introduce a way for HAR to get access to this.

My proposal was to modify onRequestPostData to return both fields:
 async onRequestPostData(response) {
    let payload = await this.updateRequest(response.from, {
      requestPostData: response
    });
    emit(EVENTS.RECEIVED_REQUEST_POST_DATA, response.from);
    return {
      requestPostData: payload.requestPostData,
      requestHeadersFromUploadStream: payload.requestHeadersFromUploadStream,
    };
 }
But I'm open to alternatives as soon as it addresses HAR issue.
(In reply to Alexandre Poirot [:ochameau] from comment #52)
> Do you confirm that `file.requestHeadersFromUploadStream` is not going to be
> set when using HAR from the toolbox?
> My understanding is that `requestHeadersFromUploadStream` is computed in
> FirefoxDataProvider, in `fetchData`,
> but it won't be returned by the call to `requestData(file.id,
> "requestPostData");`.
> `requestHeadersFromUploadStream` is set on the `payload` object, but it
> isn't returned by requestData.
> So you would need to introduce a way for HAR to get access to this.

You're right! Even though requestHeadersFromUploadStream will be set in request object when calling dataProvider's updateReqeust(), there is no guarantee that requestHeadersFromUploadStream will be set immediately once we run requestData(). Return a payload from requestData() is more reliable. Patch has updated, please check again.
Comment on attachment 8929026 [details]
Bug 1404928 - Request Post DATA should be loaded lazily

https://reviewboard.mozilla.org/r/200342/#review208946

Thanks for addressing all my comments Ricky!

Would need another round, one test is still failing.

::: devtools/client/netmonitor/test/browser_net_resend.js:59
(Diff revision 18)
>    wait = waitForNetworkEvents(monitor, 0, 1);
>    store.dispatch(Actions.sendCustomRequest(connector));
>    yield wait;
>  
>    let sentItem = getSelectedRequest(store.getState());
> -  testSentRequest(sentItem, origItem);
> +  testSentRequest(sentItem, origItem, requestData);

The test wasn't failing because you missed calling testSentRequest with `yield` here.
With these fixes, the test fails because `requestPostData` is:
`{"from":"server1.conn0.child1/netEvent29-clone","error":"noSuchActor","message":"No such actor for ID: server1.conn0.child1/netEvent29-clone"}`

::: devtools/client/netmonitor/test/browser_net_resend.js:157
(Diff revision 18)
>      ok(hasHeader, "new header added to sent request");
>  
>      let hasUAHeader = headers.some(h => `${h.name}: ${h.value}` == ADD_UA_HEADER);
>      ok(hasUAHeader, "User-Agent header added to sent request");
>  
> -    is(data.requestPostData.postData.text,
> +    let requestPostData = yield requestData(data.id, "requestPostData");

This test needs to be updated.
Attachment #8929026 - Flags: review?(poirot.alex)
Comment on attachment 8929026 [details]
Bug 1404928 - Request Post DATA should be loaded lazily

https://reviewboard.mozilla.org/r/200342/#review209308

Thanks again, looks good!

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:553
(Diff revisions 18 - 19)
>      emit(EVENTS[updatingEventName], actor);
>  
>      let response = await new Promise((resolve, reject) => {
>        // Do a RDP request to fetch data from the actor.
>        if (typeof this.webConsoleClient[clientMethodName] === "function") {
> -        this.webConsoleClient[clientMethodName](actor, (res) => {
> +        // Make sure we fetch the real actor data instead of cloned actor

This may help if you mention a typical callsite.
I imagine `CustomRequestPanel` relies on that?

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:566
(Diff revisions 18 - 19)
>          reject(new Error(`Error: No such client method '${clientMethodName}'!`));
>        }
>      });
>  
> +    // Because response's properties are read-only, so we create a new response
> +    let newResponse = { ...response };

Please clone the `response` only when it is strictly required, i.e. if `actor.includes("-clone")`.

::: devtools/client/netmonitor/src/reducers/requests.js:159
(Diff revision 19)
>      case SEND_CUSTOM_REQUEST: {
>        // When a new request with a given id is added in future, select it immediately.
>        // where we know in advance the ID of the request, at a time when it
>        // wasn't sent yet.
> -      return closeCustomRequest(state.set("preselectedId", action.id));
> +      state.preselectedId = action.id;
> +      return closeCustomRequest(state);

Good catch!

::: devtools/client/netmonitor/src/reducers/requests.js:243
(Diff revision 19)
>  /**
>   * Remove an item from existing map and return new map.
>   */
>  function mapDelete(map, key) {
>    let newMap = mapNew(map);
> -  newMap.requests.delete(key);
> +  newMap.delete(key);

This is bug 1420711, you should remove that from your patch.
Attachment #8929026 - Flags: review?(poirot.alex) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5fc7d9fa4764
Request Post DATA should be loaded lazily r=Honza,ochameau
https://hg.mozilla.org/mozilla-central/rev/5fc7d9fa4764
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
https://hg.mozilla.org/projects/oak/rev/5fc7d9fa47645b236baa9bd18e3483e03eb60f12
Bug 1404928 - Request Post DATA should be loaded lazily r=Honza,ochameau
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: