Closed
Bug 1404928
Opened 7 years ago
Closed 7 years ago
Request Post DATA should be loaded lazily
Categories
(DevTools :: Netmonitor, enhancement, P2)
DevTools
Netmonitor
Tracking
(firefox57 fix-optional, firefox59 fixed)
RESOLVED
FIXED
Firefox 59
Tracking | Status | |
---|---|---|
firefox57 | --- | fix-optional |
firefox59 | --- | fixed |
People
(Reporter: ochameau, Assigned: rickychien)
References
Details
Attachments
(1 file, 2 obsolete files)
Per bug 1404913, we should try to lazy load POST Data. It is currently used from: * CustomRequestPanel http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/custom-request-panel.js#45 * ParamsPanel http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/params-panel.js#44 * MonitorPanel http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/monitor-panel.js#66 * RequestListContextMenu http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/request-list-context-menu.js#275
Updated•7 years ago
|
Component: Menus → Developer Tools: Netmonitor
Updated•7 years ago
|
status-firefox57:
--- → fix-optional
Priority: -- → P2
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
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.
Reporter | ||
Comment 7•7 years ago
|
||
(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 8•7 years ago
|
||
mozreview-review |
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-
Comment 9•7 years ago
|
||
the subtests are broken as of bug 1418670 to perfherder: https://github.com/mozilla/treeherder/commit/aa270ca5d974fe2207304d6c9dcdd8b8cbf24005
Flags: needinfo?(jmaher)
Comment 10•7 years ago
|
||
mozreview-review |
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+
Comment 11•7 years ago
|
||
sorry it is bug 1416870, we are working on fixing this and getting it deployed today so the perfherder subtest view will work.
Comment 12•7 years ago
|
||
here is a link to the subtests: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=5d2094b3edb6&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=86400
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 22•7 years ago
|
||
mozreview-review |
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 23•7 years ago
|
||
mozreview-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+
Reporter | ||
Comment 24•7 years ago
|
||
mozreview-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-
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8930002 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8929384 -
Attachment is obsolete: true
Attachment #8929384 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 28•7 years ago
|
||
Merge into one commit due to too many merging conflicts when rebasing on top of bug 1408182.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
Alex, I've addressed all issues on comment 24, would you like to do me a favor to do a sanity check? Thanks
Reporter | ||
Comment 31•7 years ago
|
||
mozreview-review |
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 33•7 years ago
|
||
(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)
Assignee | ||
Comment 34•7 years ago
|
||
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)
Comment 35•7 years ago
|
||
(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)
Reporter | ||
Comment 36•7 years ago
|
||
mozreview-review |
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)
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 37•7 years ago
|
||
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)
Assignee | ||
Comment 38•7 years ago
|
||
(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.
Reporter | ||
Comment 39•7 years ago
|
||
(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!
Assignee | ||
Comment 40•7 years ago
|
||
Okay, I'm going to create a new "select request" helper function and replace all tests with it.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(poirot.alex)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 43•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 47•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 48•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 52•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 54•7 years ago
|
||
(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 hidden (mozreview-request) |
Reporter | ||
Comment 56•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Reporter | ||
Comment 58•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 59•7 years ago
|
||
I ran DAMP but not particular win as test pages on DAMP do not involve any Post Data: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=a557b20ab478f7fcbb7fb60ad3d2ed93c6aea053&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 65•7 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5fc7d9fa4764 Request Post DATA should be loaded lazily r=Honza,ochameau
Comment 66•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5fc7d9fa4764
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Comment 67•7 years ago
|
||
noise |
https://hg.mozilla.org/projects/oak/rev/5fc7d9fa47645b236baa9bd18e3483e03eb60f12 Bug 1404928 - Request Post DATA should be loaded lazily r=Honza,ochameau
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•