Remove 'Log request and response bodies' preference

RESOLVED FIXED in Firefox 46

Status

defect
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: bgrins, Assigned: linclark)

Tracking

(Blocks 1 bug)

Trunk
Firefox 46
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
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.
(Reporter)

Comment 2

5 years ago
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).
(Reporter)

Comment 3

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

Updated

5 years ago
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
(Reporter)

Comment 5

4 years ago
Also see: https://github.com/firebug/firebug.next/issues/406
Whiteboard: [firebug-gap]
(Reporter)

Updated

4 years ago
Blocks: firebug-gaps
Whiteboard: [firebug-gap]

Comment 6

4 years ago
Brian, would you have time to work on this?

Florent
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.
(Reporter)

Comment 8

3 years ago
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
Summary: Log request and response bodies preference is not remembered between sessions → Remove 'Log request and response bodies' preference
(Assignee)

Updated

3 years ago
Assignee: nobody → lclark
(Assignee)

Comment 9

3 years ago
Posted patch Bug1064458.patch (obsolete) — Splinter Review
This is WIP (to run against try).
(Reporter)

Updated

3 years ago
Depends on: 861335
(Assignee)

Comment 10

3 years ago
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.
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
(Reporter)

Comment 12

3 years ago
(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.
(Assignee)

Comment 13

3 years ago
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?
(Reporter)

Comment 14

3 years ago
(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
(Assignee)

Updated

3 years ago
Depends on: 1232727
(Assignee)

Comment 15

3 years ago
Posted patch Bug1064458.patch (obsolete) — Splinter Review
Here's a WIP patch. I am currently debugging the test that it adds.
Attachment #8517040 - Attachment is obsolete: true
Attachment #8694421 - Attachment is obsolete: true
Flags: needinfo?(bgrinstead)
(Reporter)

Updated

3 years ago
Flags: needinfo?(bgrinstead)
(Assignee)

Updated

3 years ago
Blocks: 1234287
(Assignee)

Comment 16

3 years ago
Posted patch Bug1064458.patch (obsolete) — Splinter Review
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.
Attachment #8700685 - Attachment is obsolete: true
Attachment #8700707 - Flags: review?(bgrinstead)
(Reporter)

Comment 17

3 years ago
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).
Attachment #8700707 - Flags: review?(bgrinstead) → feedback+
(Assignee)

Comment 18

3 years ago
Posted patch Bug1064458.patch (obsolete) — Splinter Review
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.
Attachment #8700707 - Attachment is obsolete: true
Attachment #8700905 - Flags: review?(bgrinstead)
(Reporter)

Updated

3 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 20

3 years ago
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?
Attachment #8700905 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 21

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

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

Comment 22

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

Comment 23

3 years ago
Posted patch Bug1064458.patch (obsolete) — Splinter Review
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?
Flags: needinfo?(bgrinstead)
(Reporter)

Comment 24

3 years ago
(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");
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 25

3 years ago
Posted patch Bug1064458.patch (obsolete) — Splinter Review
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.
Attachment #8700905 - Attachment is obsolete: true
Attachment #8704868 - Attachment is obsolete: true
Attachment #8705665 - Flags: review?(bgrinstead)
(Reporter)

Comment 26

3 years ago
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
Attachment #8705665 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 27

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

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68aade8426c6
Attachment #8705665 - Attachment is obsolete: true
(Reporter)

Comment 28

3 years ago
(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.
(Reporter)

Comment 29

3 years ago
DAMP from the try push looks fine - I'd say this is good to land
(Assignee)

Comment 30

3 years ago
\o/
Keywords: checkin-needed

Comment 32

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8d816bd77d80
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
(Reporter)

Updated

3 years ago
Depends on: 1240070
(Assignee)

Updated

3 years ago
No longer depends on: 1240070
(Reporter)

Updated

3 years ago
See Also: → 1239920

Comment 33

3 years ago
[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.

Updated

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