Last Comment Bug 886067 - Netmonitor displays request sizes as "0 KB" after opening Console
: Netmonitor displays request sizes as "0 KB" after opening Console
Status: VERIFIED FIXED
: verifyme
Product: Firefox
Classification: Client Software
Component: Developer Tools: Netmonitor (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 25
Assigned To: Victor Porof [:vporof][:vp]
: Simona B [:simonab ] -PTO- back Sept 5th
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-22 14:36 PDT by Victor Porof [:vporof][:vp]
Modified: 2013-09-12 07:23 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified


Attachments
v1 (23.79 KB, patch)
2013-06-23 04:07 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v1.1 (23.83 KB, patch)
2013-06-23 04:17 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v1.2 (23.85 KB, patch)
2013-06-26 05:10 PDT, Victor Porof [:vporof][:vp]
mihai.sucan: review+
Details | Diff | Splinter Review
v1.3 (22.96 KB, patch)
2013-06-26 12:36 PDT, Victor Porof [:vporof][:vp]
vporof: review+
Details | Diff | Splinter Review
patch for aurora and beta (23.08 KB, patch)
2013-06-27 04:01 PDT, Victor Porof [:vporof][:vp]
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
[landed] rebased for aurora (23.08 KB, patch)
2013-07-01 11:28 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
[landed] rebased for beta (21.55 KB, patch)
2013-07-01 11:28 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review

Description Victor Porof [:vporof][:vp] 2013-06-22 14:36:52 PDT
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] 2013-06-22 14:38:01 PDT
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] 2013-06-23 01:36:23 PDT
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] 2013-06-23 04:07:58 PDT
Created attachment 766410 [details] [diff] [review]
v1

Well this was fun.
Comment 4 Victor Porof [:vporof][:vp] 2013-06-23 04:17:19 PDT
Created attachment 766413 [details] [diff] [review]
v1.1

Fixed a typo.
Comment 5 Victor Porof [:vporof][:vp] 2013-06-26 05:10:49 PDT
Created attachment 767710 [details] [diff] [review]
v1.2

Rebased.
Comment 6 Mihai Sucan [:msucan] 2013-06-26 11:35:04 PDT
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] 2013-06-26 12:36:47 PDT
Created attachment 767931 [details] [diff] [review]
v1.3

Addressed comments.
Comment 8 Victor Porof [:vporof][:vp] 2013-06-26 13:37:02 PDT
https://hg.mozilla.org/integration/fx-team/rev/7dce0d4c0eff
Comment 9 Victor Porof [:vporof][:vp] 2013-06-26 13:37:20 PDT
Gonna have to document this...
Comment 10 Victor Porof [:vporof][:vp] 2013-06-27 04:01:20 PDT
Created 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, 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] (on PTO, back Aug 29th) 2013-06-27 07:34:15 PDT
https://hg.mozilla.org/mozilla-central/rev/7dce0d4c0eff
Comment 12 Victor Porof [:vporof][:vp] 2013-06-27 09:50:26 PDT
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] 2013-06-28 13:49:55 PDT
Aurora try: https://tbpl.mozilla.org/?tree=Try&rev=67d5b4e47fcf
Comment 14 bhavana bajaj [:bajaj] 2013-06-28 14:03:16 PDT
(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] 2013-06-28 14:20:05 PDT
(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 2013-06-30 18:54:04 PDT
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] 2013-07-01 06:21:27 PDT
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #16)

Thank you!
Comment 18 Victor Porof [:vporof][:vp] 2013-07-01 06:42:12 PDT
Landed on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/c57f86ca65ed
Comment 19 Victor Porof [:vporof][:vp] 2013-07-01 08:13:06 PDT
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] 2013-07-01 11:28:15 PDT
Created attachment 769770 [details] [diff] [review]
[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] 2013-07-01 11:28:40 PDT
Created attachment 769771 [details] [diff] [review]
[landed] rebased for beta
Comment 22 Victor Porof [:vporof][:vp] 2013-07-01 11:30:31 PDT
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] 2013-07-01 14:56:08 PDT
Try runs look good (again).
Comment 24 Victor Porof [:vporof][:vp] 2013-07-02 03:34:22 PDT
Landed again on Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/5309228d21a6
Comment 25 Victor Porof [:vporof][:vp] 2013-07-02 06:55:41 PDT
Landed on Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/428adf9c17a7
Comment 26 Simona B [:simonab ] -PTO- back Sept 5th 2013-07-04 01:41:20 PDT
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 B [:simonab ] -PTO- back Sept 5th 2013-09-10 09:18:06 PDT
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 B [:simonab ] -PTO- back Sept 5th 2013-09-12 07:23:27 PDT
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

Note You need to log in before you can comment on or make changes to this bug.