The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 23

Status

()

Firefox
Developer Tools: Netmonitor
P1
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

({verifyme})

unspecified
Firefox 25
verifyme
Points:
---

Firefox Tracking Flags

(firefox23 verified, firefox24 verified, firefox25 verified)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

4 years ago
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?
Flags: needinfo?(mihai.sucan)
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.
Flags: needinfo?(mihai.sucan)
(Assignee)

Comment 3

4 years ago
Created attachment 766410 [details] [diff] [review]
v1

Well this was fun.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #766410 - Flags: review?(mihai.sucan)
(Assignee)

Comment 4

4 years ago
Created attachment 766413 [details] [diff] [review]
v1.1

Fixed a typo.
Attachment #766410 - Attachment is obsolete: true
Attachment #766410 - Flags: review?(mihai.sucan)
Attachment #766413 - Flags: review?(mihai.sucan)
(Assignee)

Updated

4 years ago
Priority: -- → P1
(Assignee)

Comment 5

4 years ago
Created attachment 767710 [details] [diff] [review]
v1.2

Rebased.
Attachment #767710 - Flags: review?(mihai.sucan)
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|?
Attachment #767710 - Flags: review?(mihai.sucan) → review+

Updated

4 years ago
Attachment #766413 - Attachment is obsolete: true
Attachment #766413 - Flags: review?(mihai.sucan)
(Assignee)

Comment 7

4 years ago
Created attachment 767931 [details] [diff] [review]
v1.3

Addressed comments.
Attachment #767931 - Flags: review+
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/7dce0d4c0eff
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 9

4 years ago
Gonna have to document this...
(Assignee)

Comment 10

4 years ago
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
Attachment #768249 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7dce0d4c0eff
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25
(Assignee)

Comment 12

4 years ago
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
Attachment #768249 - Attachment description: patch for aurora → patch for aurora and beta
Attachment #768249 - Flags: approval-mozilla-beta?
(Assignee)

Comment 13

4 years ago
Aurora try: https://tbpl.mozilla.org/?tree=Try&rev=67d5b4e47fcf
(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 ?
(Assignee)

Comment 15

4 years ago
(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 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.
Attachment #768249 - Flags: approval-mozilla-beta?
Attachment #768249 - Flags: approval-mozilla-beta+
Attachment #768249 - Flags: approval-mozilla-aurora?
Attachment #768249 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 17

4 years ago
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #16)

Thank you!
(Assignee)

Comment 18

4 years ago
Landed on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/c57f86ca65ed
status-firefox24: --- → fixed
(Assignee)

Comment 19

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

Updated

4 years ago
status-firefox24: fixed → ---
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox25: --- → fixed
(Assignee)

Comment 20

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

Comment 21

4 years ago
Created attachment 769771 [details] [diff] [review]
[landed] rebased for beta
(Assignee)

Comment 22

4 years ago
Try runs:
Aurora: https://tbpl.mozilla.org/?tree=Try&rev=57d94a51e224
Beta: https://tbpl.mozilla.org/?tree=Try&rev=43079857aca4
(Assignee)

Comment 23

4 years ago
Try runs look good (again).
(Assignee)

Comment 24

4 years ago
Landed again on Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/5309228d21a6
status-firefox24: affected → fixed
(Assignee)

Comment 25

4 years ago
Landed on Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/428adf9c17a7
status-firefox23: affected → fixed
(Assignee)

Updated

4 years ago
Attachment #769770 - Attachment description: rebased for aurora → [landed] rebased for aurora
(Assignee)

Updated

4 years ago
Attachment #769771 - Attachment description: rebased for beta → [landed] rebased for beta
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
status-firefox23: fixed → verified
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
status-firefox24: fixed → verified
QA Contact: simona.marcu
Keywords: verifyme
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
Status: RESOLVED → VERIFIED
status-firefox25: fixed → verified
You need to log in before you can comment on or make changes to this bug.