886067 - Netmonitor displays request sizes as "0 KB" after opening Console

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Victor Porof [:vporof][:vp]]

STR:

1. Go to http://htmlpad.org/debugger/
2. Open NetMonitor
3. Refresh

Observe that request sizes are displayed correctly

4. Open Console
5. Refresh

Everything is at 0 KB

I think that we should always re-enable saveRequestAndResponseBodies when switching to the netmonitor.

Comment 1

[Victor Porof [:vporof][:vp]]

Mihai, do we have a pref for "Log request and response bodies" toggled by the webconsole context menu? Should we always disable this when switching away from the netmonitor?

Comment 2

[Mihai Sucan [:msucan]]

Based on the report here it seems to me the problem is caused by the fact the user first opens netmonitor, then the web console. When the web console opens it turns off log response bodies. The fix I suggest here is for the web console to query the preference from the web console actor, not the other way around. If the console actor already has the option enabled, that will be reflected in the UI correctly.

We should keep the context menu option working until we have a bug open about it causing problems.

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: v1

Well this was fun.

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: v1.1

Fixed a typo.

Comment 5

[Victor Porof [:vporof][:vp]]

Attachment: v1.2

Rebased.

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 767710 [details] [diff] [review]
v1.2

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

Patch looks good. Thank you!

Minor comments below. Please address them before landing.

::: browser/devtools/netmonitor/test/browser_net_req-resp-bodies.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Test for bug 886067.

Can you please include a summary of the bug?

::: browser/devtools/webconsole/test/browser_webconsole_bug_600183_charset.js
@@ +41,5 @@
>      browser.removeEventListener("load", onLoad, true);
>  
>      openConsole(null, function(hud) {
> +      hud.ui.setSaveRequestAndResponseBodies(true).then(() => {
> +        waitForSuccess({

setSaveRequestAndResponseBodies() updates hud.ui._saveRequestAndResponseBodies before resolving the promise. We no longer need waitForSuccess() here.

Just add an ok(hud.ui._saveRequestAndResponseBodies, ...); here and continue the test.

::: browser/devtools/webconsole/test/browser_webconsole_bug_602572_log_bodies_checkbox.js
@@ +74,4 @@
>  
>    // Enable body logging.
> +  huds[1].ui.setSaveRequestAndResponseBodies(true).then(() => {
> +    menupopups[1].addEventListener("popuphidden", function _onhidden(aEvent) {

I'm not sure why you add the popuphidden event listener only after setSaveRequestAndResponseBodies().

@@ +89,2 @@
>  
>    waitForSuccess({

I think you can let the popuphidden event listener as it was, just remove the call to waitForSuccess() and change the setSaveRequestAndResponseBodies() call to something like:

hud.ui.setSaveRequestAndResponseBodies(true).then(() => menupopup.hidePopup());

@@ +165,2 @@
>  
>    waitForSuccess({

Same as above.

::: browser/devtools/webconsole/test/browser_webconsole_bug_630733_response_redirect_headers.js
@@ +21,5 @@
>  function consoleOpened(hud)
>  {
>    webConsoleClient = hud.ui.webConsoleClient;
> +  hud.ui.setSaveRequestAndResponseBodies(true).then(() => {
> +    waitForSuccess({

See comment for browser_webconsole_bug_600183_charset.js

::: browser/devtools/webconsole/test/browser_webconsole_netlogging.js
@@ +84,5 @@
>  
>  function testPageLoadBody()
>  {
>    // Turn on logging of request bodies and check again.
> +  hud.ui.setSaveRequestAndResponseBodies(true).then(() => {

Ditto. This becomes |...then(testPageLoadBodyAfterSettingUpdate);|

::: browser/devtools/webconsole/webconsole.js
@@ +363,5 @@
>     * Disabled by default to save memory.
>     * @type boolean
>     */
> +  getSaveRequestAndResponseBodies:
> +  function WCF_saveRequestAndResponseBodies() {

WCF_getSave...

Please also update the comment by adding @return information.

@@ +370,5 @@
> +      "NetworkMonitor.saveRequestAndResponseBodies"
> +    ];
> +
> +    // Make sure the web console client connection is established first.
> +    this._initConnection().then(() => {

Is the call to _initConnection() needed? I would like users to always first call the web console init() method. We can't ensure the correct state before that, for every method we have.

@@ +375,5 @@
> +      this.webConsoleClient.getPreferences(preferences, aResponse => {
> +        if (!aResponse.error) {
> +          this._saveRequestAndResponseBodies = aResponse.retrieved[preferences[0]];
> +          deferred.resolve(this._saveRequestAndResponseBodies);
> +        } else {

nit: }\nelse {

@@ +391,5 @@
>     * @param boolean aValue
>     *        The new value you want to set.
>     */
> +  setSaveRequestAndResponseBodies:
> +  function WCF_setSaveRequestAndResponseBodies(aValue) {

nit: \n before {

Please also update the method comment by adding the @return information.

@@ +399,5 @@
>        "NetworkMonitor.saveRequestAndResponseBodies": newValue,
>      };
>  
> +    // Make sure the web console client connection is established first.
> +    this._initConnection().then(() => {

Same as above.

@@ +533,5 @@
> +    }
> +
> +    let reverseSaveBodiesPref = () => {
> +      this.getSaveRequestAndResponseBodies().then(aValue => {
> +        this.setSaveRequestAndResponseBodies(!aValue);

I worry this can go out of sync: UI needs to always have the same state as the server. It can happen if the pref changes on the server after the user opens the popup.

Can you also update the menu items based on |aValue|? To set their "checked" attribute.

::: toolkit/devtools/server/actors/webconsole.js
@@ +674,5 @@
> +   *
> +   * @param object aRequest
> +   *        The request message - which preferences need to be retrieved.
> +   */
> +  onGetPreferences: function WCA_onGetPreferences(aRequest)

Please add @return description in the comment.

@@ +676,5 @@
> +   *        The request message - which preferences need to be retrieved.
> +   */
> +  onGetPreferences: function WCA_onGetPreferences(aRequest)
> +  {
> +    let prefs = {};

nit: Object.create(null);

@@ +678,5 @@
> +  onGetPreferences: function WCA_onGetPreferences(aRequest)
> +  {
> +    let prefs = {};
> +    for (let key of aRequest.preferences) {
> +      prefs[key] = !!this._prefs[key];

In the future I do not expect all prefs to be booleans.

@@ +680,5 @@
> +    let prefs = {};
> +    for (let key of aRequest.preferences) {
> +      prefs[key] = !!this._prefs[key];
> +    }
> +    return { retrieved: prefs };

How about |preferences| instead of |retrieved|?

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: v1.3

Addressed comments.

Comment 8

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/7dce0d4c0eff

Comment 9

[Victor Porof [:vporof][:vp]]

Gonna have to document this...

Comment 10

[Victor Porof [:vporof][:vp]]

Attachment: patch for aurora and beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Data loss when using the Network Monitor after opening the Web Console
User impact if declined: Users will be terribly confused to see that network requests appear empty in the Network Monitor
Testing completed (on m-c, etc.): locally, fx-team, probably m-c as well next day
Risk to taking this patch (and alternatives if risky): None, straightforward changes
String or IDL/UUID changes made by this patch: None

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/7dce0d4c0eff

Comment 12

[Victor Porof [:vporof][:vp]]

Comment on attachment 768249 [details] [diff] [review]
patch for aurora and beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Data loss when using the Network Monitor after opening the Web Console
User impact if declined: Users will be terribly confused to see that network requests appear empty in the Network Monitor
Testing completed (on m-c, etc.): locally, fx-team, m-c
Risk to taking this patch (and alternatives if risky): None, straightforward changes
String or IDL/UUID changes made by this patch: None

Comment 13

[Victor Porof [:vporof][:vp]]

Aurora try: https://tbpl.mozilla.org/?tree=Try&rev=67d5b4e47fcf

Comment 14

[bhavana bajaj [:bajaj]]

(In reply to Victor Porof [:vp] from comment #12)
> Comment on attachment 768249 [details] [diff] [review]
> patch for aurora and beta
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Data loss when using the Network
> Monitor after opening the Web Console
> User impact if declined: Users will be terribly confused to see that network
> requests appear empty in the Network Monitor
> Testing completed (on m-c, etc.): locally, fx-team, m-c
> Risk to taking this patch (and alternatives if risky): None, straightforward
> changes
> String or IDL/UUID changes made by this patch: None

Sounds like a regression ? Which bug is this regressed from and what branches are affected here ?

Comment 15

[Victor Porof [:vporof][:vp]]

(In reply to bhavana bajaj [:bajaj] from comment #14)
> 
> Sounds like a regression ? Which bug is this regressed from and what
> branches are affected here ?

Unfortunately, this is not a regression. This was the case since the netmonitor's inception, mostly due to us not being careful enough to catch such things. The process is as follows:

1. Open network monitor, everything works as expected
2. Opening the webconsole silently turns off a pref in the backend, thus
3. Network monitor now functions with that pref off

That pref is request and response body logging. Some details here: https://developer.mozilla.org/en-US/docs/Tools/Web_Console/remoting

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 768249 [details] [diff] [review]
patch for aurora and beta

Since we're release-noting Network Monitor for FF23 we should definitely avoid confusing the user with data loss, approving for Aurora & Beta, it's early enough to make sure this fixes the issue correctly.

Comment 17

[Victor Porof [:vporof][:vp]]

(In reply to [e-mail] [:lsblakk] from comment #16)

Thank you!

Comment 18

[Victor Porof [:vporof][:vp]]

Landed on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/c57f86ca65ed

Comment 19

[Victor Porof [:vporof][:vp]]

Backed out for oranges in browser_webconsole_bug_600183_charset.js: https://hg.mozilla.org/releases/mozilla-aurora/rev/8e5d6f719af1

Probably a bad rebase, however try run in comment #13 was green. Investigating.

Comment 20

[Victor Porof [:vporof][:vp]]

Attachment: [landed] rebased for aurora

This seems to really have been a rebase issue. An arrow function expression became a plain old function, thus |this| wasn't |this| anymore, thus death and suffering.

Comment 21

[Victor Porof [:vporof][:vp]]

Attachment: [landed] rebased for beta

Comment 22

[Victor Porof [:vporof][:vp]]

Try runs:
Aurora: https://tbpl.mozilla.org/?tree=Try&rev=57d94a51e224
Beta: https://tbpl.mozilla.org/?tree=Try&rev=43079857aca4

Comment 23

[Victor Porof [:vporof][:vp]]

Try runs look good (again).

Comment 24

[Victor Porof [:vporof][:vp]]

Landed again on Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/5309228d21a6

Comment 25

[Victor Porof [:vporof][:vp]]

Landed on Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/428adf9c17a7

Comment 26

[Simona Badau, Desktop QA]

Verified as fixed on Firefox 23 beta 3 - the request sizes are displayed correctly after opening the web console on Ubuntu 12.10, Windows 7 and Mac OS X 10.8:

Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0
Build ID: 20130703181823

Comment 27

[Simona Badau, Desktop QA]

Verified as fixed on Firefox 24 beta 10.

Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20100101 Firefox/24.0
Build ID: 20130909203154

Comment 28

[Simona Badau, Desktop QA]

Verified as fixed on Firefox 25.0a2:

Mozilla/5.0 (Windows NT 6.1; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0
Build ID: 20130912004004