Closed Bug 1418928 Opened 7 years ago Closed 7 years ago

requestCookies and responseCookies should be loaded lazily

Categories

(DevTools :: Netmonitor, enhancement, P2)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox59 fixed)

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

People

(Reporter: rickychien, Assigned: rickychien)

References

Details

Attachments

(1 file)

Bug 1418928 - requestCookies and responseCookies should be loaded lazily
59 bytes, text/x-review-board-request
Honza
: review+
Details 
According to https://bugzilla.mozilla.org/show_bug.cgi?id=1404913#c9, 

Lazy load requestCookies and responseCookies looks promising for improving netmonitor perf.
Summary: Request Post DATA should be loaded lazily → requestCookies and responseCookies should be loaded lazily
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Comment on attachment 8931194 [details]
Bug 1418928 - requestCookies and responseCookies should be loaded lazily

https://reviewboard.mozilla.org/r/202284/#review208052

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:387
(Diff revision 2)
>          break;
> +      case "requestCookies":
> +      case "responseCookies":
>        case "requestPostData":
> -        this.updateRequest(actor, {
> -          // This field helps knowing when/if requestPostData property is available
> +        // This field helps knowing when/if requestPostData property is available

nit: now this is not only for the requestPostData
This patch needs to apply on top of bug 1404928.
Comment on attachment 8931194 [details]
Bug 1418928 - requestCookies and responseCookies should be loaded lazily

https://reviewboard.mozilla.org/r/202284/#review208140

Looks nice and works for me.

R+ assuming my inline comments are resolved and try is green.

Thanks Ricky!

Honza

::: devtools/client/netmonitor/src/components/CookiesPanel.js:42
(Diff revision 5)
> +    };
> +  }
> +
> +  componentDidMount() {
> +    this.maybeFetchRequestCookies(this.props);
> +    this.maybeFetchResponseCookies(this.props);

merge these two methods into one: `maybeFetchCookies`

::: devtools/client/netmonitor/src/components/RequestListColumnCookies.js:40
(Diff revision 5)
>    }
>  
> +  /**
> +   * When switching to another request, lazily fetch request cookies
> +   * from the backend. The panel will first be empty and then display the content.
> +   */

The comment should be updated

::: devtools/client/netmonitor/src/components/RequestListColumnSetCookies.js:40
(Diff revision 5)
>    }
>  
> +  /**
> +   * When switching to another request, lazily fetch response cookies
> +   * from the backend. The panel will first be empty and then display the content.
> +   */

The comment should be updated
Attachment #8931194 - Flags: review?(odvarko) → review+
Comment on attachment 8931194 [details]
Bug 1418928 - requestCookies and responseCookies should be loaded lazily

https://reviewboard.mozilla.org/r/202284/#review208428

Sorry to interfere once again, but this patch is breaking filtering by response cookies:
https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/devtools/client/netmonitor/src/utils/filter-autocomplete-provider.js#44-53

We briefly discussed about that in netmonitor meeting, but didn't come to a conclusion.
We should either strip this filtering feature or force fetching all response cookies when user uses these cookies filters.

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:232
(Diff revision 6)
>            payload.requestCookies = reqCookies;
>          }
>        }
> +
> +      // Lock down requestCookiesAvailable once we fetch data from back-end.
> +      payload.requestCookiesAvailable = false;

Why do you do that? Is it really used?
Why only for cookies and not for responseContent, securityInfo nor requestPostData?
Attachment #8931194 - Flags: review-
A quick comment about what I said regarding waiting for async events.

Today, we are using `waitForNetworkEvents` almost everywhere:
  https://searchfox.org/mozilla-central/search?q=waitForNetworkEvents&case=false&regexp=false&path=
If we are introducing async requests related to cookies, we should do the same,
and wait for these cookies requests in each action that dispatch some.

We should not only wait for them in teardown.
Waiting for them only in teardown will most likely make netmonitor test prone to intermittents.
Because these cookies async requests may collide. Two distinct actions may happen to run at the same time by overlapping.
This is as bad as two tests overlapping.

So I would suggest to tune waitForNetworkEvents to accept an argument specific to cookies,
or introduce another helper specific to cookies requests.
This helper would wait for EVENTS.RECEIVED_REQUEST_COOKIES.
Alex,

I'm going to introduce a helper waitForLazyLoadEvents() in head.js for addressing lazy fetching payload in mochitest [1]. All failed tests will need to insert this helper between the place which will cause async events. I'd like to ask your opinions first before going to fix the rest of failed tests.


[1] https://hg.mozilla.org/try/diff/420ccb4bcf99/devtools/client/netmonitor/test/head.js#l1.112
[2] https://hg.mozilla.org/try/diff/420ccb4bcf99/devtools/client/netmonitor/test/browser_net_column_headers_tooltips.js#l1.12
(In reply to Ricky Chien [:rickychien] from comment #15)
> Alex,
> 
> I'm going to introduce a helper waitForLazyLoadEvents() in head.js for
> addressing lazy fetching payload in mochitest [1]. All failed tests will
> need to insert this helper between the place which will cause async events.
> I'd like to ask your opinions first before going to fix the rest of failed
> tests.
> 
> 
> [1]
> https://hg.mozilla.org/try/diff/420ccb4bcf99/devtools/client/netmonitor/test/
> head.js#l1.112
> [2]
> https://hg.mozilla.org/try/diff/420ccb4bcf99/devtools/client/netmonitor/test/
> browser_net_column_headers_tooltips.js#l1.12

It looks good overall, with that, tests should be solid!

May be the helper can be slightly more complex to help listening at multiple events at once.
With something like this:
  let waitForEvents = waitForLazyLoadEvents(monitor, [ EVENTS.RECEIVED_REQUEST_COOKIES, EVENTS.RECEIVED_RESPONSE_COOKIES ], 1);
  ...
  yield waitForEvents;

Also, so do not forget to call panel.off when you are done listening.
Comment on attachment 8931194 [details]
Bug 1418928 - requestCookies and responseCookies should be loaded lazily

https://reviewboard.mozilla.org/r/202284/#review209250

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:183
(Diff revision 7)
>        }, 0);
>  
>        requestPostData.postData.text = postData;
>        payload.requestPostData = Object.assign({}, requestPostData);
>        payload.requestHeadersFromUploadStream = { headers, headersSize };
>      }

Should we add `payload.fetchPostDataAvailable = false;` here for consistency?
Comment on attachment 8931194 [details]
Bug 1418928 - requestCookies and responseCookies should be loaded lazily

https://reviewboard.mozilla.org/r/202284/#review209250

> Should we add `payload.fetchPostDataAvailable = false;` here for consistency?

this flag mechanism has removed, see above comment.
Comment on attachment 8931194 [details]
Bug 1418928 - requestCookies and responseCookies should be loaded lazily

https://reviewboard.mozilla.org/r/202284/#review208428

> Why do you do that? Is it really used?
> Why only for cookies and not for responseContent, securityInfo nor requestPostData?

Every time a props change will trigger maybeFetchRequestCookies() [1] and try fetching data via requestData(). In order to prevent fetching existed data again, I set `requestCookiesAvailable = false` as a flag to skip redundant data fetching.

After investigating, I'm going to remove the code [2] to avoid unnecessary data fetching. Hopefully that makes sense to you.

[1] https://hg.mozilla.org/try/rev/9509a99bafa15eeee65bd6e85dfeda5da222ab52#l2.22

[2] https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/devtools/client/netmonitor/src/connector/firefox-data-provider.js#511-513
Some test cases might start fetching network update payloads while test page is loading. It's hard for catching all request events by using onResponseConent similar approach to wait for specific events. I'm going to introduce a lazy events collector when a test start [1] to capture all lazily fetched events, and then wait for all captured events at restartNetMonitor [2] & teardown [3], 

Surprisingly, it works as expected! It means the robust harness can detect and catch all lazy events to avoid connection exceptions.

[1] https://hg.mozilla.org/try/diff/4d8b495c822f/devtools/client/netmonitor/test/head.js#l1.31
[2] https://hg.mozilla.org/try/diff/4d8b495c822f/devtools/client/netmonitor/test/head.js#l1.57
[3] https://hg.mozilla.org/try/diff/4d8b495c822f/devtools/client/netmonitor/test/head.js#l1.78
(In reply to Ricky Chien [:rickychien] from comment #22)
> Comment on attachment 8931194 [details]
> Bug 1418928 - requestCookies and responseCookies should be loaded lazily
> 
> https://reviewboard.mozilla.org/r/202284/#review208428
> 
> > Why do you do that? Is it really used?
> > Why only for cookies and not for responseContent, securityInfo nor requestPostData?
> 
> Every time a props change will trigger maybeFetchRequestCookies() [1] and
> try fetching data via requestData(). In order to prevent fetching existed
> data again, I set `requestCookiesAvailable = false` as a flag to skip
> redundant data fetching.
> 
> After investigating, I'm going to remove the code [2] to avoid unnecessary
> data fetching. Hopefully that makes sense to you.
> 
> [1]
> https://hg.mozilla.org/try/rev/9509a99bafa15eeee65bd6e85dfeda5da222ab52#l2.22
> 
> [2]
> https://searchfox.org/mozilla-central/rev/
> be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/devtools/client/netmonitor/src/
> connector/firefox-data-provider.js#511-513

You can't remove `this.lazyRequestData.delete(key, promise);` from [2],
otherwise you are going to leak a lot of data as this Map will never be freed!

May be you need to move the delete *after* the `updateRequest` call.
What should happen is that, either there is a pending promise in `lazyRequestData`
and so we should not dispatch duplicated RDP messages -or- request.requestCookies will be defined,
so that maybeFetchRequestCookies won't call connector.requestData anymore.
(In reply to Ricky Chien [:rickychien] from comment #23)
> Some test cases might start fetching network update payloads while test page
> is loading. It's hard for catching all request events by using
> onResponseConent similar approach to wait for specific events. 

This is not new. Many test use a "performRequests" function to wait before dispatching requests and use waitForRequests around the performRequests call.

> I'm going to introduce a lazy events collector when a test start [1] to capture all
> lazily fetched events, and then wait for all captured events at
> restartNetMonitor [2] & teardown [3], 

Your helper is fine. Hooking into initNetmonitor and using promises like that is clever.
The only issue is about calling it on restartNetmonitor/teardown.
Because the issue I'm highlighting in comment 12 remains:
>  We should not only wait for them in teardown.
>  Waiting for them only in teardown will most likely make netmonitor test prone to intermittents.
>  Because these cookies async requests may collide. Two distinct actions may happen to run at the same time by overlapping.
>  This is as bad as two tests overlapping.

So you should put `yield Promise.all(lazyLoadEvents);` in tests, after each action that introduces async requests.
(You probably want to call an helper instead of `yield Promise.all(lazyLoadEvents);`)
Exactly like our current usage of waitForRequests.

This is why I suggested to do something around waitForRequests, but may be it is easier to introduce another distinct helper.
(In reply to Alexandre Poirot [:ochameau] from comment #25)
> You can't remove `this.lazyRequestData.delete(key, promise);` from [2],
> otherwise you are going to leak a lot of data as this Map will never be
> freed!

Right. This might happen. we should prevent eating up memory in this case. 

> May be you need to move the delete *after* the `updateRequest` call.
> What should happen is that, either there is a pending promise in
> `lazyRequestData`
> and so we should not dispatch duplicated RDP messages -or-
> request.requestCookies will be defined,
> so that maybeFetchRequestCookies won't call connector.requestData anymore.

Moving it after updateRequest call doesn't work because requestData could be called in every component changes in high frequency even before updateRequest is done. As a result, I'm going back to use flag approach to ensure we won't get unnecessary data fetching.
Fixed autocomplete issues from comment 10 and lazyRequestData from comment 25.

For issue of comment 26, we had a conversion in IRC and understand that for the issue of async requests may collide (see comment 12), we can use existing wait code approach like [1] or waitUntil() to wait for specific react render state. There's no conflicts in using both approaches simultaneously. With using this event collector, we can guarantee that all lazy network request events will be finished properly before teardown. (currently I just watch request cookies and response cookies).

Let's wait for try green...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=610e6b1af5761a8d9d14ac0f705ae5429c4cba94


[1] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_brotli.js#54
Update patch for fixing browser_net_copy_params.js failure.
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee15ca302afd
requestCookies and responseCookies should be loaded lazily r=Honza
Thanks! it's under investigation...
Flags: needinfo?(rchien)
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8dca851ba43
requestCookies and responseCookies should be loaded lazily r=Honza
https://hg.mozilla.org/mozilla-central/rev/f8dca851ba43
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
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: