Developer tools network panel transferred total should ignore cached data

RESOLVED FIXED in Firefox 64

Status

defect
P2
normal
RESOLVED FIXED
Last year
10 months ago

People

(Reporter: robert, Assigned: hemakshis, Mentored)

Tracking

({good-first-bug})

61 Branch
Firefox 64

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: good-first-bug, )

Attachments

(2 attachments, 3 obsolete attachments)

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
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!
Component: Untriaged → Networking
Product: Firefox → Core
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
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); }
```
Status: UNCONFIRMED → NEW
Component: Networking → Netmonitor
Ever confirmed: true
Product: Core → DevTools
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
Mentor: odvarko
Keywords: good-first-bug
Priority: -- → P2
Whiteboard: good-first-bug,
Summary: Developer tools network panel tranferred total should ignore cached data → Developer tools network panel transferred total should ignore cached data
Posted patch Bug1481002.patch (obsolete) — Splinter Review
Added a patch that fixes the bug.

@Honza, could you please review it?

Thanks
Hemakshi
Flags: needinfo?(odvarko)
Attachment #9004974 - Flags: review+
Comment on attachment 9004974 [details] [diff] [review]
Bug1481002.patch

># HG changeset patch
># User Hemakshi Sachdev <sachdev.hemakshi@gmail.com>
>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,
Attachment #9004974 - Flags: review+ → review?(odvarko)
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
Attachment #9004974 - Flags: review?(odvarko) → review+
Assignee: nobody → sachdev.hemakshi
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Attachment #9004974 - Attachment is obsolete: true
Hi @Honza,

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

Thank you
Hemakshi
Attachment #9006180 - Flags: review?(odvarko)
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.
Attachment #9006180 - Flags: review?(odvarko)
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.
Attachment #9006180 - Attachment is obsolete: true
Hi,
I have made the changes according to the review comments. Please let me know any further changes

Thanks
Hemakshi
Attachment #9006299 - Flags: review?(odvarko)
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
Attachment #9006299 - Flags: review?(odvarko)
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
(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
(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
Fixed according to the review comments.

Hemakshi
Attachment #9006797 - Flags: review?(odvarko)
Attachment #9006299 - Attachment is obsolete: true
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
Attachment #9006797 - Flags: review?(odvarko) → review+
Keywords: checkin-needed
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed60b04e7eb
Developer tools network panel transferred total should ignore cached data. r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ed60b04e7eb
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.