1064458 - Remove 'Log request and response bodies' preference

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Brian Grinstead [:bgrins]]

STR:
Open Console
Right click in output and check "Log Request and Response Bodies"
Close and reopen toolbox
Right click in console output

Expected:
"Log Request and Response Bodies" is already checked

Actual:
"Log Request and Response Bodies" is unchecked

This should be stored as a user preference and possibly also shown in the options panel.

Comment 1

[Luc]

I too would prefer it to save this state when closing and re-opening the developer console, or better yet make it a setting in about:config that toggles whenever you enable/disable it from the developer console (thereby making the tickmark permanently set or unset).

Back when the option was always enabled I never noticed any issues with RAM usage or anything, which may be because I have a fairly high-end laptop. Being able to permanently enable it would be a convenience for me, it was really annoying when it first started to not keep request/response bodies by default. Can't count the number of times I've had to redo an action because it was, again, not logging the request/response bodies. By now I'm more or less used to doing this extra step every time, but I'd still like a permanent tickmark setting.

Comment 2

[Brian Grinstead [:bgrins]]

Looks like maybe there could be a pref for the frontend that calls setSaveRequestAndResponseBodies(true) once the webConsoleClient is attached if it is true.  Then we can also just toggle that pref anytime the option is changed (in reverseSaveBodiesPref).

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: webconsole-save-request-response-WIP.patch

(no tests, just a WIP).  Matt, can you check and see if this does what you need it to?

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8517040 [details] [diff] [review]
webconsole-save-request-response-WIP.patch

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

::: browser/devtools/webconsole/webconsole.js
@@ +477,5 @@
>      }).then(() => {
>        let id = WebConsoleUtils.supportsString(this.hudId);
> +
> +      if (Services.prefs.getBoolPref("devtools.webconsole.filter.network.bodies")) {
> +        this.setSaveRequestAndResponseBodies(true);

This doesn't seem to work for the Browser Console.

Comment 5

[Brian Grinstead [:bgrins]]

Also see: https://github.com/firebug/firebug.next/issues/406

Comment 6

[Florent Fayolle]

Brian, would you have time to work on this?

Florent

Comment 7

[Daniele "Mte90" Scasciafratte]

This is very annoying, I use for work the FFDev Edition and this is a flag that can be enabled on that release as default nad in the other version disabled for the problem explained on the comment #1.

Comment 8

[Brian Grinstead [:bgrins]]

I believe this isn't needed anymore since clicking on requests now goes to the netmonitor, so we can remove the pref and the UI for controlling it

Comment 9

[Lin Clark [:linclark]]

Attachment: Bug1064458.patch

This is WIP (to run against try).

Comment 10

[Lin Clark [:linclark]]

The patch above just removed the option from the UI. This leaves a lot of cruft... for example, the preference is sent between client and actor. 

After a discussion with bgrins, this is the current plan:

- Expose the pref in about:config
- Currently, the pref is sent between client and server using WebConsoleClient.setPreference. Instead, just have the actor access the pref directly.
- In our code, WebConsoleClient.setPreference is only used for this preference. It should be removed. However, because there is a small chance addons use WebConsoleClient.setPreference to manage their own preferences, we won't remove it in this issue. It will either be deprecated or we'll remove it after discussing on the mailing list.

Comment 11

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

Just to note that there is bug 1211525, that should allow to expand HTTP events in the Console panel (and even in the Browser Console) - in order to see the details (including the response body). Not sure how that matches the plan here.

Honza

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> Just to note that there is bug 1211525, that should allow to expand HTTP
> events in the Console panel (and even in the Browser Console) - in order to
> see the details (including the response body). Not sure how that matches the
> plan here.

For that case we will need to make sure that saveRequestAndResponseBodies is true by default (and not only when the netmonitor panel is opened), which fits in perfectly with the plan listed in Comment 10.

I thought over this plan some more and realized that by changing the default to true we would be storing bodies for the Browser Console which is wasteful since there's no way to view them from there (and we also are logging all requests that happen for the entire browser).  So at least for now I think we need to keep the setPreferences machinery around and set it to false from the client side in the Browser Console case.  Probably once the connection is initialized if this.owner._browserConsole around here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.js#5096.  If we later add the ability to view response bodies from the browser console directly (bug 1211525) then we can revisit this part.

Comment 13

[Lin Clark [:linclark]]

It turns out that this.owner._browserConsole doesn't exist in WebConsoleConnectionProxy._onAttachConsole even when it is opened by the Browser Console.

The function signature for the constructor is WebConsoleConnectionProxy(aWebConsole, aTarget). The comment says that the first param should be a WebConsole instance. However, when it is created, the parameter passed in for aWebConsole is a WebConsoleFrame instance.

https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.js#494

This WebConsoleFrame instance is then assigned to this.owner.

There seems to be a mismatch between the expectation and what the code is doing. Is this possibly technical debt?

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Lin Clark from comment #13)
> The function signature for the constructor is
> WebConsoleConnectionProxy(aWebConsole, aTarget). The comment says that the
> first param should be a WebConsole instance. However, when it is created,
> the parameter passed in for aWebConsole is a WebConsoleFrame instance.
> 
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/
> webconsole.js#494
> 
> This WebConsoleFrame instance is then assigned to this.owner.
> 
> There seems to be a mismatch between the expectation and what the code is
> doing. Is this possibly technical debt?

For future reference, we discussed this on irc and figured out that the header comment / arg name for WebConsoleConnectionProxy is wrong

Comment 15

[Lin Clark [:linclark]]

Attachment: Bug1064458.patch

Here's a WIP patch. I am currently debugging the test that it adds.

Comment 16

[Lin Clark [:linclark]]

Attachment: Bug1064458.patch

This patch sets netlogging to true by default. If the console is a browser console, it turns off netlogging. 

It adds a test for the browser console case. There is another issue to transition the test to add_task(), Bug #1234287.

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8700707 [details] [diff] [review]
Bug1064458.patch

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

First pass - it looks good to me.  I'll take another look at it tomorrow.

::: devtools/client/netmonitor/netmonitor-controller.js
@@ +222,5 @@
>        this.tabClient = this._target.activeTab;
>      }
>  
>      let connectWebConsole = () => {
>        let deferred = promise.defer();

The 3 lines dealing with `deferred` in this function can be removed so this can be a one-liner (I guess may as well leave it as a named function since that helps explain what it's doing)

::: devtools/client/webconsole/webconsole.js
@@ +366,5 @@
>    },
>  
>    _destroyer: null,
>  
>    // Used in tests.

This comment isn't really accurate anymore - no tests use this, it's just used to keep some state that isn't queried anywhere.

@@ +882,4 @@
>          let prefKey = target.getAttribute("prefKey");
>          this.setFilterState(prefKey, state);
>  
>          // Disable the log response and request body if network logging is off.

Looks like this entire if block can be removed

@@ +5039,5 @@
> +    // There is no way to view response bodies from the Browser Console, so do
> +    // not waste the memory.
> +    let saveBodies = !this.webConsoleFrame.owner._browserConsole;
> +    this.webConsoleFrame.setSaveRequestAndResponseBodies(saveBodies).then(() => {
> +      this.webConsoleClient.on("networkEvent", this._onNetworkEvent);

Does everything here need to happen inside the resolution of setSaveRequestAndResponseBodies?  It looks independent to me, like it could all happen before the call (I could be missing something here).

Comment 18

[Lin Clark [:linclark]]

Attachment: Bug1064458.patch

Thanks for the review and for catching all of that. After stepping through it more, I'm pretty sure that you're correct about not needing the promise chain after setSaveRequestAndResponseBodies.

This patch fixes everything from the previous review.

Comment 19

[Brian Grinstead [:bgrins]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=88f7d7d3a638
Including a talos push to see if this regresses perf: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=88f7d7d3a638

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8700905 [details] [diff] [review]
Bug1064458.patch

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

This all looks good to me.  Let's see what talos says once that build finishes (hopefully not much)

::: devtools/client/webconsole/test/browser_console_netlogging.js
@@ +6,5 @@
> +// Tests that network log messages bring up the network panel.
> +
> +"use strict";
> +
> +const TEST_URI = "data:text/html;charset=utf-8,Web Console network " +

Were you planning to update this to use add_task as a separate patch on this bug or a follow up bug?

Comment 21

[Lin Clark [:linclark]]

Thanks for catching that comment, I'll update it.

I'm going to switch to add_task in Bug #1234287.

Comment 22

[Brian Grinstead [:bgrins]]

There is a failure in devtools/shared/webconsole/test/test_network_get.html ('oth' in https://treeherder.mozilla.org/#/jobs?repo=try&revision=88f7d7d3a638)

Comment 23

[Lin Clark [:linclark]]

Attachment: Bug1064458.patch

I fixed the failures. But I think I caught one that passed which should fail.


This is what it is currently, which passes:

> ok(!aResponse.postData.text, "no request POST data");

But I think the NOT should be removed. When I do remove the NOT, then it fails because there is no postData. 

I don't think this signals a bug in the code. The browser_webconsole_netlogging.js test checks postData and passes. I think it might be a flaw in the test.

Brian, do you have any ideas?

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Lin Clark from comment #23)
> Created attachment 8704868 [details] [diff] [review]
> Bug1064458.patch
> 
> I fixed the failures. But I think I caught one that passed which should fail.
> 
> 
> This is what it is currently, which passes:
> 
> > ok(!aResponse.postData.text, "no request POST data");
> 
> But I think the NOT should be removed. When I do remove the NOT, then it
> fails because there is no postData. 
> 
> I don't think this signals a bug in the code. The
> browser_webconsole_netlogging.js test checks postData and passes. I think it
> might be a flaw in the test.
> 
> Brian, do you have any ideas?

The fact that the changed !aResponse.postDataDiscarded is working makes me think that there isn't actually any POST data (so aResponse.postData.text is false).  I believe that's expected, since this test is checking GET requests.  So I think that the test should assert:

  ok(!aResponse.postData.text, "no request POST data");
  ok(!aResponse.postDataDiscarded, "request POST data was not discarded");

Comment 25

[Lin Clark [:linclark]]

Attachment: Bug1064458.patch

Good call. You're right, I wasn't looking at the method. I was just looking at the .postDataDiscarded flag. It shouldn't have POST data on a GET. We test for that in the other test too.

The test passes with this patch.

Comment 26

[Brian Grinstead [:bgrins]]

Comment on attachment 8705665 [details] [diff] [review]
Bug1064458.patch

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

Alright, looks good to me

::: devtools/client/netmonitor/netmonitor-controller.js
@@ +226,2 @@
>        this.webConsoleClient = this._target.activeConsole;
> +      return promise.defer().resolve();

I think we don't need to return anything here.. or if we do then promise.resolve() should work fine

Comment 27

[Lin Clark [:linclark]]

Attachment: Bug1064458.patch

Thanks for the review! You're right about that return. I removed it.

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

Comment 28

[Brian Grinstead [:bgrins]]

(In reply to Lin Clark from comment #27)
> Created attachment 8706720 [details] [diff] [review]
> Bug1064458.patch
> 
> Thanks for the review! You're right about that return. I removed it.
> 
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=68aade8426c6

I have one last talos push: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=5acc2a44834c&newProject=try&newRevision=8f3fe80cfd00&framework=1&showOnlyImportant=0.  Looks like a minor regression on linux (small enough to not worry about it), let's see how Windows pans out.

Comment 29

[Brian Grinstead [:bgrins]]

DAMP from the try push looks fine - I'd say this is good to land

Comment 30

[Lin Clark [:linclark]]

\o/

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8d816bd77d80

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d816bd77d80

Comment 33

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED --> VERIFIED

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Actual Results: 
As expected

Expected Results: 
No option is there like "log request and response bodies" while right click in output area.