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

RESOLVED FIXED in Firefox 61

Status

defect
P3
normal
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: daniel.peder, Assigned: Honza, NeedInfo)

Tracking

(Blocks 1 bug)

unspecified
Firefox 61
All
Other
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [specification][type:bug])

Attachments

(4 attachments)

Reporter

Description

4 years ago
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
Component: General → Developer Tools: Netmonitor
Product: Mozilla Developer Network → Firefox
Reporter

Updated

4 years ago
Summary: currupted data dump - Network Monitor - Firefox Developer Tools → corrupted data dump - Network Monitor - Firefox Developer Tools
Reporter

Comment 2

4 years ago
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
Reporter

Comment 3

4 years ago
probably same issue as https://bugzilla.mozilla.org/show_bug.cgi?id=1214703
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(odvarko)
See Also: → 1159078
Summary: corrupted data dump - Network Monitor - Firefox Developer Tools → Large response bodies (over 1MiB) are truncated, even in HAR
Reporter

Comment 5

4 years ago
- 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
(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/
Flags: needinfo?(odvarko)
Blocks: 1430765
Priority: -- → P3

Updated

a year ago
Duplicate of this bug: 1436477

Comment 8

a year ago
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.
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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
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.
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)

Comment 14

a year ago
mozreview-review
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);`.
Attachment #8957279 - Flags: review?(poirot.alex) → review+

Comment 15

a year ago
mozreview-review
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?
Attachment #8957280 - Flags: review?(poirot.alex) → review+
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(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
@vivek: please see comment #7 does that work for you?

Honza
Flags: needinfo?(vivek.nedyavila)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 26

a year ago
mozreview-review
Comment on attachment 8958052 [details]
Bug 1223726 - Update tests;

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

Works for me, thanks!
Attachment #8958052 - Flags: review?(jdescottes) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
The issue should be solved now, trying to land again.

Honza
Flags: needinfo?(odvarko)

Comment 34

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e90a8fa79dcb
https://hg.mozilla.org/mozilla-central/rev/fe5c013a93ae
https://hg.mozilla.org/mozilla-central/rev/2d0f0690c4a4
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61

Updated

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