Netmonitor total size gets very confused by maps.google.com

RESOLVED FIXED in Firefox 47

Status

RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: bgrins, Assigned: Honza)

Tracking

unspecified
Firefox 47

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8709726 [details]
maps.google-size-netmonitor.png

STR: 
Open netmonitor
Load https://www.google.com/maps/@37.3565066,-122.0322134,13z

Expected:
Something like 1MB is shown as transferred

Actual:
While requests are streaming in you can see the size jump around from 0 to 1000kb to 0 to 4000kb etc.  Once it's done the number is way too small (see screenshot)
(Reporter)

Comment 1

3 years ago
Created attachment 8709727 [details]
netmon-maps.google.gif

Also, the 'size' value jumps around randomly when I sort by size in the table
(Reporter)

Comment 2

3 years ago
This is pretty bad.  I've suspected for a while that the 'size' column isn't always telling me the truth and this is the most clear example I've been able to find.

Honza, can you reproduce this and help get it prioritized if so?
Flags: needinfo?(odvarko)
I actually noticed this the other day on a complex site.
(In reply to Brian Grinstead [:bgrins] from comment #2)
> This is pretty bad.  I've suspected for a while that the 'size' column isn't
> always telling me the truth and this is the most clear example I've been
> able to find.
> 
> Honza, can you reproduce this and help get it prioritized if so?
Yes, I'll investigate it.

Brian, you are talking about the total size displayed
at the bottom/right of the panel, right?

Honza
Flags: needinfo?(odvarko) → needinfo?(bgrinstead)
(Reporter)

Comment 5

3 years ago
(In reply to Jan Honza Odvarko [:Honza] from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > This is pretty bad.  I've suspected for a while that the 'size' column isn't
> > always telling me the truth and this is the most clear example I've been
> > able to find.
> > 
> > Honza, can you reproduce this and help get it prioritized if so?
> Yes, I'll investigate it.
> 
> Brian, you are talking about the total size displayed
> at the bottom/right of the panel, right?

Yes, exactly.  That whole 'summary' section is weird since all of the fields continually bounce between 0 and some other value as requests stream in, but this bug is specifically about that total size being wrong.
Flags: needinfo?(bgrinstead)
Created attachment 8722444 [details] [diff] [review]
bug1240959.patch

This patch should fix the problem
- Summary calculation check the contentSize field before adding.
- Convert response.text to avoid an exception

Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Attachment #8722444 - Flags: review?(vporof)
Comment on attachment 8722444 [details] [diff] [review]
bug1240959.patch

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

r+ with comment addressed

::: devtools/client/netmonitor/netmonitor-view.js
@@ +2226,5 @@
> +
> +    let result = 0;
> +    aItemsArray.forEach(item => {
> +      let size = item.attachment.contentSize;
> +      result += (typeof size == "number") ? size : 0;

What does this change exactly? The forEach and iterative addition works exactly as before. However, the typeof conditional suggests something wrong with `contentSize`. When is that not a number? And if it isn't a number, why?
Attachment #8722444 - Flags: review?(vporof)
(In reply to Victor Porof [:vporof][:vp] from comment #8)
> What does this change exactly? The forEach and iterative addition works
> exactly as before. However, the typeof conditional suggests something wrong
> with `contentSize`.
Yes

> When is that not a number? And if it isn't a number, why?
It's undefined since it isn't set yet.

Honza
Comment on attachment 8722444 [details] [diff] [review]
bug1240959.patch

Comment addressed so, r+

Honza
Attachment #8722444 - Flags: review+
Created attachment 8722484 [details] [diff] [review]
bug1240959.patch

Yet updating the commit message.

Honza
Attachment #8722444 - Attachment is obsolete: true
Attachment #8722484 - Flags: review+
Keywords: checkin-needed
There's some netmonitor failures in the try push.
Flags: needinfo?(odvarko)
Backed out for failures in browser_net_content-type.js (dt tests).

Backout: https://hg.mozilla.org/integration/fx-team/rev/53864eca4327

Push with failure (newer pushes also show failures in Tier 1 jobs): https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=e0cc363043ad
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=7516494&repo=fx-team

16:48:25     INFO -  134 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_content-type.js | Test timed out -
Flags: needinfo?(odvarko)
This generated some performance regressions before it was backed out:

https://treeherder.allizom.org/perf.html#/alerts?id=267
Created attachment 8723513 [details] [diff] [review]
bug1240959.patch

Patch updated. I put back the original response.text conversion (using btoa), but kept the 'catch' clause for an exception.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a51625ade054

Honza
Attachment #8722484 - Attachment is obsolete: true
Flags: needinfo?(odvarko)
Comment on attachment 8723513 [details] [diff] [review]
bug1240959.patch

Victor, the tests are passing now. I reverted the response decoding (it breaks images), but kept the try-catch block. Does that sound ok to you?
Size summary works well for me.

Honza
Attachment #8723513 - Flags: review?(vporof)
Attachment #8723513 - Flags: review?(vporof) → review+
Keywords: checkin-needed

Comment 20

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/37ca88b45a72
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
I have reproduced this bug according to (2016-1-19)

It's fixed on Latest Developer Edition -- Build ID (20160408004012), User Agent: Mozilla/5.0 (Windows NT 6.3; rv:47.0) Gecko/20100101 Firefox/47.0

Tested OS-- Windows8.1 32bit
QA Whiteboard: [bugday-20160406]

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.