Closed Bug 1260403 Opened 4 years ago Closed 3 years ago

Content compressed with brotli not recognized as actually compressed

Categories

(DevTools :: Netmonitor, defect)

defect
Not set

Tracking

(firefox52 fixed, firefox53 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: the.decryptor, Assigned: arai)

References

Details

Attachments

(5 files, 2 obsolete files)

When using brotli content-encoding, the network monitor doesn't understand that the content is compressed (Like with gzip), and shows the normal size of the resource to be the same as the compressed size. And viewing the specific details about a resource, the response pane (See attachment) only shows the raw compressed data, not the uncompressed version.
Here the 'highlight.pack.js' should show a transferred size of 16KB, and a real size of 45KB.
Having the same issue here with ff51.0a2 buildid 20161009004008

Found bug 1308778 while looking for similar bugs, seems to be a dupe of this one.
https://dxr.mozilla.org/mozilla-central/rev/8e8b146fcb8b268e3c09b646087c6b2ef9f0af6f/devtools/shared/webconsole/network-monitor.js#449
>       let acceptedEncodings = ["gzip", "deflate", "x-gzip", "x-deflate"];

apparently, adding "br" there fixed the issue (unless it's some intermittent)
taking
Assignee: nobody → arai.unmht
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
added "br" to acceptedEncodings in monitor.js.

also, added testcase that check "br" is handled properly in network monitor and decoded in response view.
I added dedicated testcase that uses HTTPS, since brotli is enabled only on HTTPS.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f89b05b340a5
Attachment #8808091 - Flags: review?(poirot.alex)
Comment on attachment 8808091 [details] [diff] [review]
Support brotli encoding in Developer Tools Netmonitor.

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

Note that instead of duplicating browser_net_content-type.js, you could have moved most of the add_task's content into a function and make it run all the tests against http and https. Then, there is no need to duplicate html_content-type-without-cache-test-page.html, you can load it from http and https as-is. You just have to add the call to `get("sjs_content-type-test-server.sjs?fmt=br")`.

That's not a big deal so feel free to submit that.

::: devtools/client/netmonitor/test/browser_net_https_content-type.js
@@ +3,5 @@
> +
> +"use strict";
> +
> +/**
> + * Tests if different response content types are handled correctly.

If you keep this test file, I would add "over https" in this comment.
Also it would be great to mention somewhere that brotli works only over https.

@@ +12,5 @@
> +
> +  let { tab, monitor } = yield initNetMonitor(HTTPS_CONTENT_TYPE_WITHOUT_CACHE_URL);
> +  info("Starting test... ");
> +
> +  let { document, Editor, NetMonitorView } = monitor.panelWin;

`Editor` doesn't seem to be used.
Attachment #8808091 - Flags: review?(poirot.alex) → review+
Thanks.
merged http and https testcases.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a1cddd232b6
Attachment #8808091 - Attachment is obsolete: true
Attachment #8808230 - Flags: review?(poirot.alex)
Comment on attachment 8808230 [details] [diff] [review]
Support brotli encoding in Developer Tools Netmonitor.

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

Thanks a lot for merging the tests!
Attachment #8808230 - Flags: review?(poirot.alex) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cceb2a87580748749fb0932fa4f553b1e60ee8c4
Bug 1260403 - Support brotli encoding in Developer Tools Netmonitor. r=ochameau
https://hg.mozilla.org/mozilla-central/rev/cceb2a875807
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Depends on: 1282788
After this patch, the browser_net_content-type.js takes twice as much time to complete, and sometimes times out in debug builds on try. So I'm thinking about splitting it up.

Arai, I have some questions:

- before this patch, the test checked only HTTP page, not HTTPS. Why did you add HTTPS? Only to test brotli on it and nothing else? There's an option to test only HTTP in this test, and move the brotli/HTTPS testing to another test.

- why is there a difference between status text on HTTP and HTTPS? "OK" vs "Connected"?

- the test case here:

http://searchfox.org/mozilla-central/rev/efcb1644e49c36445e75d89b434fdf4c84832c84/devtools/client/netmonitor/test/browser_net_content-type.js#104-114

should check item at index 7, not 6, the URL should have query string "fmt=br" and the transferred/content sizes should be updated accordingly. Am I right?
Flags: needinfo?(arai.unmht)
(In reply to Jarda Snajdr [:jsnajdr] from comment #12)
> After this patch, the browser_net_content-type.js takes twice as much time
> to complete, and sometimes times out in debug builds on try. So I'm thinking
> about splitting it up.
> 
> Arai, I have some questions:
> 
> - before this patch, the test checked only HTTP page, not HTTPS. Why did you
> add HTTPS? Only to test brotli on it and nothing else? There's an option to
> test only HTTP in this test, and move the brotli/HTTPS testing to another
> test.

I can split brotli test into dedicated test (it was so in draft patch)


> - why is there a difference between status text on HTTP and HTTPS? "OK" vs
> "Connected"?

It's because of SSL test infra.


> - the test case here:
> 
> http://searchfox.org/mozilla-central/rev/
> efcb1644e49c36445e75d89b434fdf4c84832c84/devtools/client/netmonitor/test/
> browser_net_content-type.js#104-114
> 
> should check item at index 7, not 6, the URL should have query string
> "fmt=br" and the transferred/content sizes should be updated accordingly. Am
> I right?

oops, apparently I copied wrong chunk.
will fix shortly.
Status: RESOLVED → REOPENED
Flags: needinfo?(arai.unmht)
Resolution: FIXED → ---
Attached patch followup: Fix brotli test. (obsolete) — Splinter Review
Separated into dedicated test for HTTPS, with correct testcode.
Attachment #8811392 - Flags: review?(poirot.alex)
Comment on attachment 8811392 [details] [diff] [review]
followup: Fix brotli test.

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

Thanks for updating the test!

::: devtools/client/netmonitor/test/browser_net_https_content-type.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +const HTTPS_CONTENT_TYPE_WITHOUT_CACHE_URL = HTTPS_EXAMPLE_URL + "html_https_content-type-without-cache-test-page.html";

I'd rather call the test something like "browser_net_brotli.js" and the test file could be "html_brotli-test-page.html". It describes the test much better.
okay, updated filename and some comment
Attachment #8811392 - Attachment is obsolete: true
Attachment #8811392 - Flags: review?(poirot.alex)
Attachment #8811403 - Flags: review?(poirot.alex)
Comment on attachment 8811403 [details] [diff] [review]
followup: Fix brotli test.

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

Works great, thanks for fixing this so quickly! There's just one eslint warning. r=me if that's fixed.

::: devtools/client/netmonitor/test/browser_net_brotli.js
@@ +84,5 @@
> +      }
> +    }
> +  }
> +
> +  function selectIndexAndWaitForTabUpdated(index) {

This function is unused and eslint issues a warning.
Attachment #8811403 - Flags: review?(poirot.alex) → review+
https://hg.mozilla.org/mozilla-central/rev/0a8aa6175b4c
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
So, this part missed firefox52 train.

Approval Request Comment
> [Feature/regressing bug #]
this bug and bug 1260403.

> [User impact if declined]
no user impact.
intermittent test failure

> [Describe test coverage new/current, TreeHerder]
tested on m-c

> [Risks and why]
No, test only.

> [String/UUID change made/needed]
None.
Attachment #8813428 - Flags: review+
Attachment #8813428 - Flags: approval-mozilla-aurora?
See Also: → 1316237
Comment on attachment 8813428 [details] [diff] [review]
(mozilla-aurora) followup: Fix brotli test. r=jsnajdr

It's usually okay to push test-only fixes to aurora without approval.
Attachment #8813428 - Flags: approval-mozilla-aurora?
It works as it must work and push test only case in Nighty version.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.