1401901 - DAMP should wait for netmonitor updates during reload test

Status: NEW

(DevTools :: General, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

Bug 1401187 is going to improve "reload" test for inspector, console and debugger.
But it would be great to also apply same improvement to net monitor and maybe also style editor.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1401901 - Wait for netmonitor updates during its DAMP reload test.

Review commit: https://reviewboard.mozilla.org/r/184630/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184630/

Comment 2

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=b3062f2d8bb03ebbc94e7829224058f35ac02115&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=172800
I think it makes netmonitor.requestsFinished test useless, as now, reload should better take netmonitor update into account.
Otherwise, you may see difference on debugger, but I think that is related to bug 1398576's patch that I included in this try...

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8913225 [details]
Bug 1401901 - Wait for netmonitor updates during its DAMP reload test.

https://reviewboard.mozilla.org/r/184630/#review189808

::: testing/talos/talos/tests/devtools/addon/content/damp.js:417
(Diff revision 1)
>        }),
>  
>        netmonitorOpen: Task.async(function* () {
>          yield this.testSetup(url);
>          const toolbox = yield this.openToolboxAndLog(label + ".netmonitor", "netmonitor");
>          const requestsDone = this.waitForNetworkRequests(label + ".netmonitor", toolbox);

Shouldn't we delete the waitForNetworkRequests function, or reuse it as a callback for reloadPageAndLog instead of this new function?

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8913225 [details]
Bug 1401901 - Wait for netmonitor updates during its DAMP reload test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184630/diff/1-2/

Comment 5

[Alexandre Poirot [:ochameau]]

I went for deleting it as the new function is more similar to the other ones used in other react tools.
But I would like to start asking feedback to each tool owner after bug 1401207, which would hopefully make DAMP more accessible to everyone.

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8913225 [details]
Bug 1401901 - Wait for netmonitor updates during its DAMP reload test.

Actually, It may be a great time asking for more review on DAMP.

Honza, this patch is tweaking DAMP to better track netmonitor performance.
Before this serie of patches, DAMP was measuring page reload only partially.
It was only looking at out much time it took to reload the web site.
i.e. the time between document load start and its "load" event.
This was DAMP result called "netmonitor.reload".

Now, in bug 1401187 I made it so that these "reload" tests measure the time it takes to update each panel during a website reload. In that bug I made it for inspector, console and debugger.

For the network monitor, it was already special. We were kind of tracking netmon panel update via DAMP result called "netmonitor.requestsFinished", which was including "netmonitor.reload". This result was implemented by "waitForAllRequestsFinished" function that I'm removing in this patch.
Instead, I introduced a "onReload" method which is going to delay the "netmonitor.reload" test until the panel is fully updated.
At the end, onReload and waitForAllRequestsFinished are both quite similar as they both try to listen for panel update completion after document reload.

Now, my main question is: is onReload implementation correct? is it any better/worse than waitForAllRequestsFinished?

My second question is: no matter which is best of these two, do you think it really covers full panel update, or are we missing some more asynchronous steps?

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8913225 [details]
Bug 1401901 - Wait for netmonitor updates during its DAMP reload test.

https://reviewboard.mozilla.org/r/184630/#review189908

DAMP-wise this looks good. Will let Honza decide if the reload detection is right

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8913225 [details]
Bug 1401901 - Wait for netmonitor updates during its DAMP reload test.

https://reviewboard.mozilla.org/r/184630/#review191326

> Now, my main question is: is onReload implementation correct?

I don't think so. You are checking `responseContent` field and existence of this field doesn't ensure that all requested data (for a HTTP request) has been fetched from the backend.

I've been also working on this in bug 1395759, bug then blocked by Bug 1371721 and consequently by Bug 1404138. These are fixed now and I'll hopefully get back to it again.

We could check all the fields (not only `responseContent`) to determine whether all data has been fetched from the backend.

Here is the list:
requestCookies
requestHeaders
responseCookies
responseHeaders
responseContent
securityInfo
postDataRequested

But of course, it's not that simple. Some fields don't have to be set. For example:

postDataRequested - it's only there if the HTTP request is POST and there are some POSTed data
responseCookies - only set if there are some response cookies
etc.

The `FirefoxDataProvider` could always set the fields to indicate that it's been fetched from the backend (no matter if it's actually empty or not) and consequently we can depend on simple condition: 'if all fields are set, fetching data from the backend has finished'.

However, we need to be careful. To improve performance any data should be fetched from the backend lazily (e.g. response body - bug 1404917) and we are using these fields to determine whether they have been already fetched or not.

Another appraoch (not based on existence of particular fields in the data structure) is monitoring all RDP requests for HTTP data and watching all expected responses. Such logic could give correct answer whether all requests for data has been resolved and it would also sent a final event saying 'yep the RDP communication is done'. Btw. this was the primary goal of the "network-request-payload-ready" event.

I don't know what the best approach is, but I am working on this in bug 1395759.

 
> is it any better/worse than waitForAllRequestsFinished?

I think that they are both equaly wrong. `waitForAllRequestsFinished` was unreliable because it was based on a premise that 'eventTimings' is the last event, which isn't true.


> My second question is: no matter which is best of these two, do you think it really covers full panel update, or are we missing some more asynchronous steps?

We need to do two things:
1) Wait till all requested RDP data are resolved (fetched from the backend)
2) Wait till all related (redux) actions are processd (i.e. net uses debouncing/batching just like the Console panel, see http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/devtools/client/netmonitor/src/middleware/batching.js#18)

add #2) we could disable action-batching for the test, but I think it's better if the test follows the reality.

You might mark this bug as blocked by bug 1395759.

Honza

::: testing/talos/talos/tests/devtools/addon/content/damp.js:417
(Diff revision 2)
> +            function countSources() {
> +              let count = 0;
> +              store.getState().requests.requests.forEach(r => {
> +                // This store is update many times for the same request
> +                // Only consider the last update for each where `responseContent` is set
> +                if (r.responseContent) {

Existence of the `responseContent` field doesn't ensure that all data (for the particular request) has been properly fetched from the backend.