1223726 - Large response bodies (over 1MiB) are truncated, even in HAR

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[daniel.peder]

What did you do?
================
1. browser game forge of empires
2. open network monitor tool to analyze network data
3. look at biggest json data response - over 1,5MB


What happened?
==============
json data corrupted, truncated at end
there are many json responses, but only the biggest got corrupted

What should have happened?
==========================
should have returned nontruncated data, the whole json as returned from server

Is there anything else we should know?
======================================
its obvious, that because the game is working good, there is not serverside error, but more likely data size calculation problem, probably limited buffer size or unicode vs ascii data length calculation

Comment 1

[daniel.peder]

Attachment: network monitor window showing the corrupted json data

Comment 2

[daniel.peder]

additionaly:

1) save HAR to file has the same problem, the response->content->text string data are truncated at the end

2) similar feature (save HAR to file) works good with google chrome browser

Comment 3

[daniel.peder]

probably same issue as https://bugzilla.mozilla.org/show_bug.cgi?id=1214703

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Bug 1214703 is different I think.

But yes, we do currently truncate to 1 MiB.  I realize that's not ideal.

Normally I'd have duped this to bug 1159078, but you mentioned HAR as well, and I think that's an important, separate issue.

Honza, should we do anything special with large files for HAR export?  It's probably tricky, since I believe you're examine the data Net Monitor already saved, which would be truncated.

Comment 5

[daniel.peder]

- what about to simply asking users by adding an checkbox option, "truncate large bodies"

- chrome simply omits the large bodies, however it seems the limit is higher than 1MB

- according to bug https://bugzilla.mozilla.org/show_bug.cgi?id=1159078
what about to add an extra property next to size, size-truncated, or just an large-text-omitted attribute

Comment 6

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> Honza, should we do anything special with large files for HAR export?  It's
> probably tricky, since I believe you're examine the data Net Monitor already
> saved, which would be truncated.

I think we can solve it in several steps:

(A) Automated HAR Export
The RESPONSE_BODY_LIMIT (hard-coded constants defined in shared/webconsole/network-monitor.js) should be replaced by a preference. If the value is set to -1 the response body has no limit, if it's 0 the response body is discarded, otherwise it represents max number of bytes stored.

Additionally HAR can introduce custom property "_sizeTruncated" as suggested in comment #5

This would solve mainly automated HAR export. In this case (and it's actually recommended) the Net panel doesn't have to be initialized/selected or (with help of HARExportTrigger[1]) the Toolbox doesn't have to be opened at all. So, no problem with the UI

(B) Net panel UI
This is related to bug 1159078

The way how HTTP requests and all the details are displayed in the Console works as follows:
(currently implemented in Firebug.next, but will be moved to m-c)

1) All details about HTTP request (including response body) is fetched from the backend actor as soon as it's needed to render it (i.e. when the user expands an HTTP entry and selects the Response tab)
2) When the response body is fetched, it's returned as a long-string. The initial length is hard-coded as LONG_STRING_INITIAL_LENGTH (in DebuggerServer, 1000 B) and should also be replaced by a pref.
3) If the user wants to see more, there is a message at the end of the response body saying: "Response size limit has been reached. Click here to load more."
4) If the user clicks "here", the long-string is resolved and more data returned.
5) The resolved string is still limited by RESPONSE_BODY_LIMIT (the actor just doesn't store more). But, if the user wants more, it's always possible to right click on the response and pick "Open URL in New Tab". In this case the data will be re-fetched from the (HTTP) server. If the user doesn't like the "refetch", it should be possible to increase the RESPONSE_BODY_LIMIT pref (not existing yet).

Of course manual HAR export, represented by 'Copy All as HAR' and 'Save All as HAR' context menu items, always use data collected by the backend actor (no page reload) and if the user wants to see more/all data of a response body, the RESPONSE_BODY_LIMIT pref needs to be increased or set to -1.

---

So, I suggest introducing the RESPONSE_BODY_LIMIT pref (and LONG_STRING_INITIAL_LENGTH at the same time) for now (it should fix this bug) and fix the UI as soon as I am porting the Console panel XHR previews in m-c (it should fix bug 1159078).

Honza

[1] https://github.com/firebug/har-export-trigger/

Comment 8

[vivek.nedyavila]

I wanted to suggest a very minimal feature that would immediately unblock many of us using NetMonitor and facing issues with "Copy Response" with responses > 1 MiB:

Add a preference that users can change in about:config, call it "devtools.netmonitor.netDisplayedResponseLimit" (mirroring the old Firebug's "extensions.firebug.netDisplayedResponseLimit") and have it behave how Jan described: "If the value is set to -1 the response body has no limit, if it's 0 the response body is discarded, otherwise it represents max number of bytes stored."

Is there an ETA for this? I'm currently being forced to switch to Chrome every time I need to inspect a large response.

Comment 9

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

Attachment: Bug 1223726 - Customize response body interception;

* Introduce pref for custom response body limit
* Introduce pref for saving request/response bodies

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

Comment 10

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

Attachment: Bug 1223726 - Update tests;

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

Comment 11

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

(In reply to vivek.nedyavila from comment #8)
> I wanted to suggest a very minimal feature that would immediately unblock
> many of us using NetMonitor and facing issues with "Copy Response" with
> responses > 1 MiB:
> 
> Add a preference that users can change in about:config, call it
> "devtools.netmonitor.netDisplayedResponseLimit" (mirroring the old Firebug's
> "extensions.firebug.netDisplayedResponseLimit") and have it behave how Jan
> described: "If the value is set to -1 the response body has no limit, if
> it's 0 the response body is discarded, otherwise it represents max number of
> bytes stored."

The patch implements the following:

* Introduces a new pref: devtools.netmonitor.responseBodyLimit
- 1 MB by default
- if set to 0 then no limit

* Introduces a new pref: devtools.netmonitor.responseBodyLimit
- if true, request/response bodies are stored
- if false, they are not stored

* Changing the pref doesn't require browser restart


> Is there an ETA for this? I'm currently being forced to switch to Chrome
> every time I need to inspect a large response.
Let's see how the review goes ;-)

Honza

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Could we make this situation more discoverable be showing something in the UI as well once we hit the limit?  Maybe an icon next to large requests with a tooltip that explains what has happened?

Maybe it's better for a follow up bug, but overall I think it's important to surface it somehow to the user once we have these prefs, so that they know there's an action they can take, instead of being confused for a while until they (maybe) find this bug.

Comment 13

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)
> Could we make this situation more discoverable be showing something in the
> UI as well once we hit the limit?  Maybe an icon next to large requests with
> a tooltip that explains what has happened?
> 
> Maybe it's better for a follow up bug, but overall I think it's important to
> surface it somehow to the user once we have these prefs, so that they know
> there's an action they can take, instead of being confused for a while until
> they (maybe) find this bug.
Absolutely agree, bug 1444179 reported to cover this.

Honza

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8957279 [details]
Bug 1223726 - Customize response body interception;

https://reviewboard.mozilla.org/r/226206/#review232300

Thanks Honza, it looks good. I think you can proceed with all comments addressed without another review.

Also, ideally, response body limit size would be implemented like `setSaveRequestAndResponseBodies`.
i.e. the client has control over server/actors behavior. It wouldn't be based on a pref set on the server side (think about Fennec/Firefox for android usecase).
But given the current state of netmonitor actor, I can understand why you went for a pref.

::: devtools/client/preferences/devtools.js:172
(Diff revision 1)
>  
> +// Limit for intercepted response bodies (1 MB)
> +// Possible values:
> +// 0 => the response body has no limit
> +// n => represents max number of bytes stored
> +pref("devtools.netmonitor.responseBodyLimit", 1048576);

This pref is server specific and should be moved to `modules/libpref/init/all.js` file. Otherwise it won't be present on Fennec, where we don't ship client folder.

::: devtools/client/webconsole/webconsole-connection-proxy.js:212
(Diff revision 1)
>  
>      this.webConsoleClient = webConsoleClient;
>      this._hasNativeConsoleAPI = response.nativeConsoleAPI;
>  
> +    let saveBodies = Services.prefs.getBoolPref(
> +      "devtools.netmonitor.saveRequestAndResponseBodies");

It isn't clear why you are introducing this pref.
Is this for some other bug/followup?

If this is no plan to use it, I would suggest removing it, unless there is a hidden usecase where users would actually tweak this pref manually.

::: devtools/shared/webconsole/network-monitor.js:413
(Diff revision 1)
>      let data = NetUtil.readInputStreamToString(inputStream, count);
>  
>      this.bodySize += count;
>  
>      if (!this.httpActivity.discardResponseBody) {
> -      if (this.receivedData.length < RESPONSE_BODY_LIMIT) {
> +      let limit = Services.prefs.getIntPref("devtools.netmonitor.responseBodyLimit");

We shouldn't check the pref on each onDataAvailable call as I'm expecting a perf overhead.
I imagine you wouldn't want to retrieve is in module header as it would make it harder to use it from tests?
Otherwise you could move it to NetworkResponseListener contructor.
Another option is to use a pref listener...
https://searchfox.org/mozilla-central/source/toolkit/components/search/SearchSuggestionController.jsm#27-30

::: devtools/shared/webconsole/network-monitor.js:421
(Diff revision 1)
>            NetworkHelper.convertToUnicode(data, request.contentCharset);
> -      } else {
> +      }
> +      if (this.receivedData.length > limit && limit > 0) {
> +        this.receivedData = this.receivedData.substr(0, limit);
>          this.truncated = true;
>        }

The whole logic of this method should be tweaked but that's not the goal of this bug/patch.

`let data = NetUtil.readInputStreamToString(inputStream, count);`
should be moved into `if (!this.httpActivity.discardResponseBody) {` block and `count` argument should be set according to `limit` rather than stripping `receivedData` after the fact like you do in `this.receivedData = this.receivedData.substr(0, limit);`.

Comment 15

[Alexandre Poirot [:ochameau]]

Comment on attachment 8957280 [details]
Bug 1223726 - Update tests;

https://reviewboard.mozilla.org/r/226208/#review232304

Thanks for the test!

::: devtools/client/netmonitor/src/har/test/browser_net_har_copy_all_as_har.js:40
(Diff revision 1)
>    is(entry.response.status, 200, "Check response status");
>    is(entry.response.statusText, "OK", "Check response status text");
>    is(entry.response.headers.length, 6, "Check number of response headers");
>    is(entry.response.content.mimeType, // eslint-disable-line
>      "text/html", "Check response content type"); // eslint-disable-line
>    isnot(entry.response.content.text, undefined, // eslint-disable-line

You may want to update this to check the right length.

::: devtools/client/netmonitor/src/har/test/browser_net_har_copy_all_as_har.js:55
(Diff revision 1)
> +
> +  // Test response body limit (zero).
> +  await pushPref("devtools.netmonitor.responseBodyLimit", 0);
> +  har = await reloadAndCopyAllAsHar(tab, monitor);
> +  entry = har.log.entries[0];
> +  ok(entry.response.content.text.length > 0, // eslint-disable-line

I think it would be safer to asser the precise value you are expecting to have rather than `> 0`.

::: devtools/client/netmonitor/test/browser_net_truncate.js
(Diff revision 1)
>  "use strict";
>  
>  /**
>   * Verifies that truncated response bodies still have the correct reported size.
>   */
> -

It looks like there is a leftover empty modification here?

Comment 16

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

(In reply to Alexandre Poirot [:ochameau] from comment #14)
> This pref is server specific and should be moved to
> `modules/libpref/init/all.js` file. Otherwise it won't be present on Fennec,
> where we don't ship client folder.
Done

> > +      "devtools.netmonitor.saveRequestAndResponseBodies");
> 
> It isn't clear why you are introducing this pref.
> Is this for some other bug/followup?
> 
> If this is no plan to use it, I would suggest removing it, unless there is a
> hidden usecase where users would actually tweak this pref manually.
The reason is related to HAR export. The response body doesn't have
to be always exported and this pref allows disabling it. Plus,
this should also have positive impact on performance during export.

> We shouldn't check the pref on each onDataAvailable call as I'm expecting a
> perf overhead.
> I imagine you wouldn't want to retrieve is in module header as it would make
> it harder to use it from tests?
> Otherwise you could move it to NetworkResponseListener contructor.
Done

> ::: devtools/shared/webconsole/network-monitor.js:421
> (Diff revision 1)
> >            NetworkHelper.convertToUnicode(data, request.contentCharset);
> > -      } else {
> > +      }
> > +      if (this.receivedData.length > limit && limit > 0) {
> > +        this.receivedData = this.receivedData.substr(0, limit);
> >          this.truncated = true;
> >        }
> 
> The whole logic of this method should be tweaked but that's not the goal of
> this bug/patch.
> 
> `let data = NetUtil.readInputStreamToString(inputStream, count);`
> should be moved into `if (!this.httpActivity.discardResponseBody) {` block
> and `count` argument should be set according to `limit` rather than
> stripping `receivedData` after the fact like you do in `this.receivedData =
> this.receivedData.substr(0, limit);`.
Yeah this would be nice, but it looks like the inputStream must be consumed otherwise the Network panel is broken. We might do some analysis in a follow up.

Honza

Comment 17

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

Comment on attachment 8957279 [details]
Bug 1223726 - Customize response body interception;

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

Comment 18

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

Comment on attachment 8957280 [details]
Bug 1223726 - Update tests;

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

Comment 19

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

(In reply to Alexandre Poirot [:ochameau] from comment #15)
> devtools/client/netmonitor/src/har/test/browser_net_har_copy_all_as_har.js:55
> (Diff revision 1)
> > +
> > +  // Test response body limit (zero).
> > +  await pushPref("devtools.netmonitor.responseBodyLimit", 0);
> > +  har = await reloadAndCopyAllAsHar(tab, monitor);
> > +  entry = har.log.entries[0];
> > +  ok(entry.response.content.text.length > 0, // eslint-disable-line
> 
> I think it would be safer to asser the precise value you are expecting to
> have rather than `> 0`.
Done

> 
> ::: devtools/client/netmonitor/test/browser_net_truncate.js
> (Diff revision 1)
> >  "use strict";
> >  
> >  /**
> >   * Verifies that truncated response bodies still have the correct reported size.
> >   */
> > -
> 
> It looks like there is a leftover empty modification here?
Removed

Honza

Comment 20

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

@vivek: please see comment #7 does that work for you?

Honza

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6adb35cc708e
Customize response body interception; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/a95b0b28ca16
Update tests; r=ochameau

Comment 22

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 2 changesets (bug 1223726) for failing devtools/client/styleeditor/test/browser_styleeditor_fetch-from-netmonitor.js a=backout on a CLOSED TREE

Failed job: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a95b0b28ca16d84b92b3831c9ff12297254cd048&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=pending&filter-resultStatus=running&filter-classifiedState=unclassified&filter-searchStr=87718d3629e41a11d28525fabb47ce7c360d17f

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=167119583&repo=autoland

Backout push: https://hg.mozilla.org/integration/autoland/rev/1dfc5eba64f1b2ddb308da27206bd39de8a7f1a6

Comment 23

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

Attachment: Bug 1223726 - Update tests;

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

Comment 24

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

Comment on attachment 8957279 [details]
Bug 1223726 - Customize response body interception;

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

Comment 25

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

Comment on attachment 8957280 [details]
Bug 1223726 - Update tests;

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

Comment 26

[Julian Descottes [:jdescottes]]

Comment on attachment 8958052 [details]
Bug 1223726 - Update tests;

https://reviewboard.mozilla.org/r/226990/#review232734

Works for me, thanks!

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa0a1e88a238
Customize response body interception; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/41da2897f549
Update tests; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/1a09e6c3c0a3
Update tests; r=jdescottes

Comment 28

[Cosmin Sabou [:CosminS]]

Backed out for mochitest devtools failures on devtools/client/styleeditor/test/browser_styleeditor_fetch-from-netmonitor.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1a09e6c3c0a3c0109c32e49b164c7170fc3cfbe5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=167393247

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=167393247&repo=autoland&lineNumber=11252

Backout link: https://hg.mozilla.org/integration/autoland/rev/48400efc495805643cc4e8d00ecc0c43fee2cfec

Comment 29

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

Comment on attachment 8957279 [details]
Bug 1223726 - Customize response body interception;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226206/diff/3-4/

Comment 30

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

Comment on attachment 8957280 [details]
Bug 1223726 - Update tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226208/diff/3-4/

Comment 31

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

Comment on attachment 8958052 [details]
Bug 1223726 - Update tests;

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

Comment 32

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

The issue should be solved now, trying to land again.

Honza

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e90a8fa79dcb
Customize response body interception; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/fe5c013a93ae
Update tests; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/2d0f0690c4a4
Update tests; r=jdescottes

Comment 34

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e90a8fa79dcb
https://hg.mozilla.org/mozilla-central/rev/fe5c013a93ae
https://hg.mozilla.org/mozilla-central/rev/2d0f0690c4a4

Comment 35

[7cf148fd]

Hi
there seem to be a mini-bug when you set the devtools.netmonitor.responseBodyLimit value to 0.

When the value is set to 0, the response ceases to be truncated (which is expected).
However, in the Developers Tools / Request Details panel / Response tab, for each and every element that is not a picture, a "Response has been truncated" message (with a Red background) is (obviously erroneously) displayed on top of the Response payload whatever its size.

Cheers

Comment 36

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

(In reply to 7cf148fd from comment #35)

there seem to be a mini-bug when you set the devtools.netmonitor.responseBodyLimit value to 0.

When the value is set to 0, the response ceases to be truncated (which is expected).
However, in the Developers Tools / Request Details panel / Response tab, for each and every element that is not a picture, a "Response has been truncated" message (with a Red background) is (obviously erroneously) displayed on top of the Response payload whatever its size.

Thanks for the comment!

Please create a new report for it since this has been already closed.

Thanks,
Honza

Comment 37

[7cf148fd]

Done -> Bug 1571449