1481002 - Developer tools network panel transferred total should ignore cached data

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Robert Marcano]

Attachment: Screenshot showing the issue

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180723085942

Steps to reproduce:

Visit a page that properly do static assets caching with the developers tool network panel open


Actual results:

Large cached assets sizes are added to the transferred total, when that data was not really transferred


Expected results:

The transferred bytes total should not include the size of the already cached assets, it should include the size of the HTTP 304 response, but not the size of the cached data.

This is important for development and debugging purposes, were the developer want to know if proper caching is reducing transfer sizes. If the developer want to know the full size, they can reload the entire page, or disable caching

See attached image

Comment 1

[Kanchan Kumari QA]

I couldn't reproduce this issue on
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0

Based on bug screenshot I am placing this under core:networking so someone can look into this. Thanks!

Comment 2

[Robert Marcano]

Tried with Firefox Developer Edition (Mozilla provided build), just in case this was an specific Fedora build issue. Still happen.

I have now a good test case, Visit https://www.wikipedia.org/ then reload the page (Control+R), all assets are properly cached and the browser triy validating them with If-Modified-Since headers and the server respond to all with 304 responses. The footer still says, 146.96 KB / 62.02 KB transferred, too large for a few requests and all 304 responses

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Confirmed this is a bug. Not sure how the netmonitor is adding things up, but the resource timing values for the entries are correct. Less than 1Kb for each loaded resource, yet the total is a lot higher. Either we are counting the wrong thing, or we're also counting things that don't show up in the resource list.
```javascript
for (let e of performance.getEntries()) { console.log(e.name, e.transferSize); }
```

Comment 4

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

Info for anyone interested in this issue:

1) This is the place where the summary (transferred) data is rendered:
https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/devtools/client/netmonitor/src/components/StatusBar.js#43

The `summary` props is calculated using: `getDisplayedRequestsSummary` selector (see bottom of the file)

2) Here is where `getDisplayedRequestsSummary` is implemented:
https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/devtools/client/netmonitor/src/selectors/requests.js#98

The investigation should start in this method. This code is supposed to ignore cached items:
https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/devtools/client/netmonitor/src/selectors/requests.js#111-113

Perhaps the `item.fromCache` is not set properly?

Let me know if you have more questions!

Honza

Comment 5

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

Bug 1484209 might be related

Honza

Comment 6

[Hemakshi Sachdev [:hemakshis]]

Attachment: Bug1481002.patch

Added a patch that fixes the bug.

@Honza, could you please review it?

Thanks
Hemakshi

Comment 7

[Hemakshi Sachdev [:hemakshis]]

Comment on attachment 9004974 [details] [diff] [review]
Bug1481002.patch

># HG changeset patch
># User Hemakshi Sachdev <[e-mail]>
>Bug 1481002 - Developer tools network panel transferred total should ignore cached data. r=Honza
>
>
>diff --git a/devtools/client/netmonitor/src/selectors/requests.js b/devtools/client/netmonitor/src/selectors/requests.js
>index 735cdfe7a438..5902948e7d98 100644
>--- a/devtools/client/netmonitor/src/selectors/requests.js
>+++ b/devtools/client/netmonitor/src/selectors/requests.js
>@@ -103,17 +103,17 @@ const getDisplayedRequestsSummary = createSelector(
>       return { count: 0, bytes: 0, millis: 0 };
>     }
> 
>     const totalBytes = requests.reduce((totals, item) => {
>       if (typeof item.contentSize == "number") {
>         totals.contentSize += item.contentSize;
>       }
> 
>-      if (typeof item.transferredSize == "number" && !item.fromCache) {
>+      if (typeof item.transferredSize == "number" && !(item.fromCache || item.status === "304")) {
>         totals.transferredSize += item.transferredSize;
>       }
> 
>       return totals;
>     }, { contentSize: 0, transferredSize: 0 });
> 
>     return {
>       count: requests.size,

Comment 8

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

Comment on attachment 9004974 [details] [diff] [review]
Bug1481002.patch

Review of attachment 9004974 [details] [diff] [review]:
-----------------------------------------------------------------

Fixes the problem for me, thanks!

Can you please yet add a test for this to avoid regression?

You might look at this one (to see how to generate cache requests):
devtools\client\netmonitor\test\browser_net_cached-status.js:

and this one (to see how to check summary):
devtools\client\netmonitor\test\browser_net_footer-summary.js

Honza

Comment 9

[Hemakshi Sachdev [:hemakshis]]

Attachment: Added a fix for the problem and also a test

Hi @Honza,

I have added the test for this. Also, merged the previous patch into this one.
Please review it.

Thank you
Hemakshi

Comment 10

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

Comment on attachment 9006180 [details] [diff] [review]
Added a fix for the problem and also a test

Review of attachment 9006180 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch, this is great progress!

Please resolve my review comments and upload new version.

Thanks!
Honza

::: devtools/client/netmonitor/test/browser.ini
@@ +183,5 @@
>  [browser_net_sort-02.js]
>  [browser_net_statistics-01.js]
>  skip-if = true # Bug 1373558
>  [browser_net_statistics-02.js]
> +[browser_net_status-bar-transferredSize.js]

The name of the file should be browser_net_status-bar-transferred-size.js

::: devtools/client/netmonitor/test/browser_net_status-bar-transferredSize.js
@@ +3,5 @@
> +/**
> + * Test whether the value of total data transferred is displayed correctly in the Status Bar.
> + * Test for Bug 1481002
> + */
> +

Remove this empty line (between the comment and the function name)

@@ +5,5 @@
> + * Test for Bug 1481002
> + */
> +
> +add_task(async () => {
> +

Remove this empty line (between the function name and const)

@@ +6,5 @@
> + */
> +
> +add_task(async () => {
> +
> +    const {

Fix tab indentation. Should be 2 spaces.

@@ +10,5 @@
> +    const {
> +      getFormattedSize,
> +    } = require("devtools/client/netmonitor/src/utils/format-utils");
> +
> +

Remove one empty line (there are two empty lines)

@@ +26,5 @@
> +    info("Performing requests #1...");
> +    await performRequestsAndWait();
> +
> +    info("Performing requests #2...");
> +    await performRequestsAndWait();

I think that we should call the `performRequestsAndWait` just once.
It generates two requests: one not cached and one cached.
That should be enough for this test

@@ +31,5 @@
> +
> +    let cachedItemsInUI = 0;
> +    for (const requestItem of document.querySelectorAll(".request-list-item")) {
> +      const requestTransferStatus = requestItem.querySelector('.requests-list-transferred')
> +                                      .textContent;

This line is too long (limit is 90 chars per line)

The formatting should be as follows:

  const requestTransferStatus = requestItem.querySelector(
    '.requests-list-transferred').textContent;

@@ +37,5 @@
> +        cachedItemsInUI++;
> +      }
> +    }
> +
> +    is(cachedItemsInUI, 3, "Number of cached requests displayed is correct");

So, this should check against 1 cached reqeuest

@@ +59,5 @@
> +
> +    async function performRequestsAndWait() {
> +      const wait = waitForNetworkEvents(monitor, 2);
> +      await ContentTask.spawn(tab.linkedBrowser, {}, async function() {
> +        content.wrappedJSObject.performThreeCachedRequests();

And yeah, this should be again `performTwoRequests` or something better.

@@ +63,5 @@
> +        content.wrappedJSObject.performThreeCachedRequests();
> +      });
> +      await wait;
> +    }
> +

Remove this empty line.

Comment 11

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

Comment on attachment 9006180 [details] [diff] [review]
Added a fix for the problem and also a test

Review of attachment 9006180 [details] [diff] [review]:
-----------------------------------------------------------------

Couple more comments.

::: devtools/client/netmonitor/test/browser.ini
@@ +183,5 @@
>  [browser_net_sort-02.js]
>  [browser_net_statistics-01.js]
>  skip-if = true # Bug 1373558
>  [browser_net_statistics-02.js]
> +[browser_net_status-bar-transferredSize.js]

The name of the file should be browser_net_status-bar-transferred-size.js

::: devtools/client/netmonitor/test/browser_net_status-bar-transferredSize.js
@@ +3,5 @@
> +/**
> + * Test whether the value of total data transferred is displayed correctly in the Status Bar.
> + * Test for Bug 1481002
> + */
> +

Remove this empty line (between the comment and the function name)

@@ +5,5 @@
> + * Test for Bug 1481002
> + */
> +
> +add_task(async () => {
> +

Remove this empty line (between the function name and const)

@@ +6,5 @@
> + */
> +
> +add_task(async () => {
> +
> +    const {

Fix tab indentation. Should be 2 spaces.

@@ +10,5 @@
> +    const {
> +      getFormattedSize,
> +    } = require("devtools/client/netmonitor/src/utils/format-utils");
> +
> +

Remove one empty line (there are two empty lines)

@@ +26,5 @@
> +    info("Performing requests #1...");
> +    await performRequestsAndWait();
> +
> +    info("Performing requests #2...");
> +    await performRequestsAndWait();

I think that we should call the `performRequestsAndWait` just once.
It generates two requests: one not cached and one cached.
That should be enough for this test

@@ +31,5 @@
> +
> +    let cachedItemsInUI = 0;
> +    for (const requestItem of document.querySelectorAll(".request-list-item")) {
> +      const requestTransferStatus = requestItem.querySelector('.requests-list-transferred')
> +                                      .textContent;

This line is too long (limit is 90 chars per line)

The formatting should be as follows:

  const requestTransferStatus = requestItem.querySelector(
    '.requests-list-transferred').textContent;

@@ +37,5 @@
> +        cachedItemsInUI++;
> +      }
> +    }
> +
> +    is(cachedItemsInUI, 3, "Number of cached requests displayed is correct");

So, this should check against 1 cached reqeuest

@@ +59,5 @@
> +
> +    async function performRequestsAndWait() {
> +      const wait = waitForNetworkEvents(monitor, 2);
> +      await ContentTask.spawn(tab.linkedBrowser, {}, async function() {
> +        content.wrappedJSObject.performThreeCachedRequests();

And yeah, this should be again `performTwoRequests` or something better.

@@ +60,5 @@
> +    async function performRequestsAndWait() {
> +      const wait = waitForNetworkEvents(monitor, 2);
> +      await ContentTask.spawn(tab.linkedBrowser, {}, async function() {
> +        content.wrappedJSObject.performThreeCachedRequests();
> +      });

Remove this unnecessary semicolon

@@ +62,5 @@
> +      await ContentTask.spawn(tab.linkedBrowser, {}, async function() {
> +        content.wrappedJSObject.performThreeCachedRequests();
> +      });
> +      await wait;
> +    }

missing semicolon after `}`

@@ +63,5 @@
> +        content.wrappedJSObject.performThreeCachedRequests();
> +      });
> +      await wait;
> +    }
> +

Remove this empty line.

Comment 12

[Hemakshi Sachdev [:hemakshis]]

Attachment: Fixed according to the review comments

Hi,
I have made the changes according to the review comments. Please let me know any further changes

Thanks
Hemakshi

Comment 13

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

Comment on attachment 9006299 [details] [diff] [review]
Fixed according to the review comments

Review of attachment 9006299 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the update!

Some more inline comment:

1) I am seeing an error when running the test two times as follows:
mach test devtools/client/netmonitor/test/browser_net_status-bar-transferred-size.js --repeat 2

devtools/client/netmonitor/test/browser_net_status-bar-transferred-size.js
  FAIL Number of cached requests displayed is correct - Got 2, expected 1
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:1304
chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_status-bar-transferred-size.js:null:35
chrome://mochikit/content/browser-test.js:Tester_execTest/<:1102
chrome://mochikit/content/browser-test.js:Tester_execTest:1093
chrome://mochikit/content/browser-test.js:nextTest/<:990
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795

It looks like the request is cached the first time and the second time both requests are cached.
Perhaps you could set the 'Expires' header to '0'?

See this file for some inspiration:
devtools\client\netmonitor\test\sjs_content-type-test-server.sjs

2) Here is try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1e157970af33f35b4e858301a837b48d867e6e7

Also, some tips how to run your test:

# Simple run
mach test devtools/client/netmonitor/test/browser_net_status-bar-transferred-size.js

# Running 10 times
mach test devtools/client/netmonitor/test/browser_net_status-bar-transferred-size.js --repeat 10

# Verifying the tests, runs multiple times in different modes:
mach test devtools/client/netmonitor/test/browser_net_status-bar-transferred-size.js --verify

# Running with no UI (so, you can continue using your machine)
mach test devtools/client/netmonitor/test/browser_net_status-bar-transferred-size.js --headless

# This is very useful: Verify & Headless:
mach test devtools/client/netmonitor/test/browser_net_status-bar-transferred-size.js --headless --verify

Honza

::: devtools/client/netmonitor/test/browser_net_status-bar-transferred-size.js
@@ +1,4 @@
> +"use strict";
> +
> +/**
> + * Test whether the value of total data transferred is displayed correctly in the Status Bar.

This line is too long

@@ +54,5 @@
> +  async function performRequestsAndWait() {
> +    const wait = waitForNetworkEvents(monitor, 2);
> +    await ContentTask.spawn(tab.linkedBrowser, {}, async function() {
> +      content.wrappedJSObject.performOneCachedRequest();
> +    })

missing semicolon

@@ +56,5 @@
> +    await ContentTask.spawn(tab.linkedBrowser, {}, async function() {
> +      content.wrappedJSObject.performOneCachedRequest();
> +    })
> +    await wait;
> +  };

unnecessary semicolon

@@ +57,5 @@
> +      content.wrappedJSObject.performOneCachedRequest();
> +    })
> +    await wait;
> +  };
> +});
\ No newline at end of file

new line required at the end of file

::: devtools/client/netmonitor/test/html_status-codes-test-page.html
@@ +53,5 @@
>            });
>          });
>        }
>  
> +      function performOneCachedRequest() {

The function needs to be exported at the top of this file:

/* exported performRequests, performCachedRequests, performOneCachedRequest */

@@ +60,5 @@
> +            // Done.
> +          });
> +        });
> +      }
> +

Remove this empty line

Comment 14

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

Oh, I also think that the following should be at the beginning of the test:

// Disable rcwn to make cache behavior deterministic.
await pushPref("network.http.rcwn.enabled", false);


Honza

Comment 15

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

The try run nicely shows the Eslint failures here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1e157970af33f35b4e858301a837b48d867e6e7&selectedJob=197558305

Honza

Comment 16

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

(In reply to Jan Honza Odvarko [:Honza] from comment #14)
> Oh, I also think that the following should be at the beginning of the test:
> 
> // Disable rcwn to make cache behavior deterministic.
> await pushPref("network.http.rcwn.enabled", false);

And you also need to clear cache (at the beginning of the test) as follows:

// Clear cache, so we see expected number of cached requests.
Services.cache2.clear();

Honza

Comment 17

[Hemakshi Sachdev [:hemakshis]]

(In reply to Jan Honza Odvarko [:Honza] from comment #16)

> And you also need to clear cache (at the beginning of the test) as follows:
> 
> // Clear cache, so we see expected number of cached requests.
> Services.cache2.clear();
>

This works as a charm! Thanks a lot.

(In reply to Jan Honza Odvarko [:Honza] from comment #13) 
> Also, some tips how to run your test:
> 
> # Simple run
> mach test
> devtools/client/netmonitor/test/browser_net_status-bar-transferred-size.js
> 
> # Running 10 times
> mach test
> devtools/client/netmonitor/test/browser_net_status-bar-transferred-size.js
> --repeat 10
> 
> # Verifying the tests, runs multiple times in different modes:
> mach test
> devtools/client/netmonitor/test/browser_net_status-bar-transferred-size.js
> --verify
> 
> # Running with no UI (so, you can continue using your machine)
> mach test
> devtools/client/netmonitor/test/browser_net_status-bar-transferred-size.js
> --headless
> 
> # This is very useful: Verify & Headless:
> mach test
> devtools/client/netmonitor/test/browser_net_status-bar-transferred-size.js
> --headless --verify
> 
> Honza
> 

I have tested in all these modes and now with the update you gave they all are working perfectly!

Thanks a lot

Hemakshi

Comment 18

[Hemakshi Sachdev [:hemakshis]]

Attachment: Bug1481002.patch

Fixed according to the review comments.

Hemakshi

Comment 19

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

Comment on attachment 9006797 [details] [diff] [review]
Bug1481002.patch

Review of attachment 9006797 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks!

R+ assuming try is green

Here is try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a388ba1391f09db5f761f27d12f828a754648d1

Honza

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed60b04e7eb
Developer tools network panel transferred total should ignore cached data. r=Honza

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/0ed60b04e7eb