568634 - Update networking log entries with subsequent http transactions

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[David Dahl :ddahl]

Each network log entry is at the outset only a record of the initial http request, and has no subsequent transaction data.

Each original log entry needs to be updated with additional nodes that can be displayed via an "arrow button" or something like that.

Comment 1

[David Dahl :ddahl]

A lot of this functionality is already in the patch on bug 534398, we mostly need tests and UI implementation.

Comment 2

[Julian Viereck]

(In reply to comment #0)
> Each original log entry needs to be updated with additional nodes that can be
> displayed via an "arrow button" or something like that.

Maybe instead of using an arrow button, use a panel for the output?

Comment 3

[Julian Viereck]

Attachment: Patch 1, part 1

First part: Implements the underlaying infrastructure to log more then just the beginning of a network request. Also includes unit tests.
There is not to much that gets displayed of this extra data that is logged expect the total duration for the request and the response status.

Patch applies cleanly on trunk.

Comment 4

[Julian Viereck]

Attachment: Patch 1, part 2

This adds a very simple way to inspect the networking activity using a PropertyPanel.

This patch requires the patches from bug 573102 (JS/DOM Object Introspector). Also, the HUD.jsterm is often null. To fix this, the patch 459449 from bug 578658 is required.

Comment 5

[David Dahl :ddahl]

Comment on attachment 460563 [details] [diff] [review]
Patch 1, part 1

># HG changeset patch
># Parent c2fa4377a7994542e1a77910375ca3c78f4462b5
>
>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
>--- a/toolkit/components/console/hudservice/HUDService.jsm
>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>@@ -83,16 +83,293 @@ XPCOMUtils.defineLazyGetter(this, "strin
> 
> const ERRORS = { LOG_MESSAGE_MISSING_ARGS:
>                  "Missing arguments: aMessage, aConsoleNode and aMessageNode are required.",
>                  CANNOT_GET_HUD: "Cannot getHeads Up Display with provided ID",
>                  MISSING_ARGS: "Missing arguments",
>                  LOG_OUTPUT_FAILED: "Log Failure: Could not append messageNode to outputNode",
> };
> 
>+/**
>+ * Implements the nsIStreamListener and nsIRequestObserver interface. Used
>+ * within the HS_httpObserverFactory function to get the response body of
>+ * requests.
>+ *
>+ * The code is mostly based on code listings from:
>+ *
>+ *   http://www.softwareishard.com/blog/firebug/
>+ *      nsitraceablechannel-intercept-http-traffic/
>+ */

This is looking good. The first thing you need to do when using source from other projects is add the project's license and contributors to this source file. There may be an example in the inspector's DOMPlate code.

I have to start working on a blocker, but will keep working through this patch each day.  I'm thinking we should target it for beta 4

Comment 6

[David Dahl :ddahl]

oh wait. I meant the parts pulled from firebug itself.

Comment 7

[Julian Viereck]

This bug is part of improving the logging of networking messages in the WebConsole. Requesting blocking status for this bug.

Comment 8

[David Dahl :ddahl]

Comment on attachment 460565 [details] [diff] [review]
Patch 1, part 2


>+                    header: httpActivity.response.header,
>+                    status: httpActivity.response.status
>+                  },
>+                  "Timing":   formatedTiming
>+                };
>+
>+                let link = msgObject.elementFactory("span");
I think gavin likes these as <a> tags instead of  <spans>

Comment 9

[David Dahl :ddahl]

Comment on attachment 460563 [details] [diff] [review]
Patch 1, part 1


>+ * The code is mostly based on code listings from:
>+ *
>+ *   http://www.softwareishard.com/blog/firebug/
>+ *      nsitraceablechannel-intercept-http-traffic/
>+ */
anything that came out of firebug should also include their license and authors

>+  CCIN: function (aCName, aIfaceName)
All functions should be named: CCIN: function HS_CCIN() [or whatever...]


>+          // The following try { } body always throughs an exp because hudId
>+          // is undefined in the end. Just use the catch { } body.
>+          // DISCUSS: What's the downside in using only the catch { } body?
>+          //

That is correct, perhaps for now, you leave this code in here knowing it always fails, there is no downside to the catch always executing. We need to file a followup bug on moving this code into a method that logs image requests regardless, to either all WebConsoles or after looking deeper into firebug stealing their code for matching up requests with contentWindows. I have a feeling the use of the httpActivityObserver is what is killing this connection after all, as they are not using it, IIRC.

>         let _hud = new HeadsUpDisplay(config);
>+        _hud.gBrowser = gBrowser;
I am not sure if this is needed anymore, or if it is should be a part of this patch

> 
>         let hudWeakRef = Cu.getWeakReference(_hud);
>         HUDService.registerHUDWeakReference(hudWeakRef, hudId);
>       }
>       else {
>         // only need to attach a console object to the window object
>         let config = { hudNode: hudNode,
>                        consoleOnly: true,
>                        contentWindow: aContentWindow
>                      };
> 
>         let _hud = new HeadsUpDisplay(config);
>+        _hud.gBrowser = gBrowser;
ditto

+ with changes

Comment 10

[Jeff Balogh (:jbalogh)]

Will this include detailed timing data?  Webkit shows me time spent for Connection, SSL, Receiving, etc. http://grab.by/5PGC

Comment 11

[Julian Viereck]

Jeff, there is bug 573103 which is about to implement a network panel. This bug is more about implementing the necessary infrastructure to collect all the necessary data. Can you move over to that bug? I've attached some markups there and would be glad to get feedback.

Comment 12

[Rob Campbell [:rc] (:robcee)]

That's something we definitely want to do, but not sure if it'll make it into the first version or not.

Comment 13

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #12)
> That's something we definitely want to do, but not sure if it'll make it into
> the first version or not.

to clarify, I meant I'm not sure we'll be getting detailed timing information in the first version.

Comment 14

[Julian Viereck]

(In reply to comment #9)
> Comment on attachment 460563 [details] [diff] [review]
> Patch 1, part 1
> 
> >+  CCIN: function (aCName, aIfaceName)
> All functions should be named: CCIN: function HS_CCIN() [or whatever...]
 
Do you want them to be called HS_CCIN or NH_CCIN (NH = NetworkManager). The code is not defined within HUD_SERVICE, that's why I'm asking.

> That is correct, perhaps for now, you leave this code in here knowing it always
> fails, there is no downside to the catch always executing. We need to file a
> followup bug on moving this code into a method that logs image requests
> regardless, to either all WebConsoles or after looking deeper into firebug
> stealing their code for matching up requests with contentWindows. I have a
> feeling the use of the httpActivityObserver is what is killing this connection
> after all, as they are not using it, IIRC.
> 
> >         let _hud = new HeadsUpDisplay(config);
> >+        _hud.gBrowser = gBrowser;
> I am not sure if this is needed anymore, or if it is should be a part of this
> patch

I need to have a reference to gBrowser for the functions readPostTextFromRequest() and readPostTextFromPage(). This gBrowser is the same object for all windows/tabs? Can we store/query it in one central place and store it on the HUDService object?

Comment 15

[David Dahl :ddahl]

(In reply to comment #14)
> (In reply to comment #9)
> > Comment on attachment 460563 [details] [diff] [review] [details]
> > Patch 1, part 1
> > 
> > >+  CCIN: function (aCName, aIfaceName)
> > All functions should be named: CCIN: function HS_CCIN() [or whatever...]
> 
> Do you want them to be called HS_CCIN or NH_CCIN (NH = NetworkManager). The
> code is not defined within HUD_SERVICE, that's why I'm asking.
> 
NH_CCIN

> 
> I need to have a reference to gBrowser for the functions
> readPostTextFromRequest() and readPostTextFromPage(). This gBrowser is the same
> object for all windows/tabs? Can we store/query it in one central place and
> store it on the HUDService object?

you have a gBrowser for each xulWindow that is open, that is why we need a lazy way to figure out what the gBrowser is for each window. Right now, we are using HUDService.currentContext()

However, gavin does not like how this method works - that is another story. You can use it for now though.

Comment 16

[David Dahl :ddahl]

Actually, in this case, I am not sure if HS.currentContext will work for you.

Comment 17

[David Dahl :ddahl]

<ddahl> jviereck: you may want to add to your patch a line below this one where we set this._gBrowser = Cu.getWeakReference(gBrowser)
<ddahl> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/HUDService.jsm#1627
<ddahl> then write a getter that returns gBrowser in each HeadsUpDisplay object. this may already exist though...
<ddahl> i think we never did that
<ddahl> we just let it go out of scope
<ddahl> so we need a gBrowser getter that calls get() on _gBrowser in each HeadsUpDisplay object
<jviereck> ddahl: is that the "right" gBrowser then?
<ddahl> jviereck: yes
<jviereck> currenlty I use the gBrowser in the initializerWindow function
<ddahl> yeah, that may NOT be the right one in some cases since this code will run even if the tab is not focused

Comment 18

[Julian Viereck]

Attachment: Patch 2

Improved patch based on David's feedback. The former part 2 patch is no longer included as there is a Network Panel on the way.

Comment 19

[Julian Viereck]

Attachment: Patch 3

While looking over this patch within my queue I realized that 

+            // Try to get a displayNode for this loadGroup. If there is none,
+            // then we try to load images in most cases.
+            displayNode = self.getDisplayByLoadGroup(loadGroup);
+            if (!displayNode) {
               return;
+            }
+
+            // Get the hudId.
+            outputNode = displayNode.querySelectorAll(".hud-output-node")[0];
+            hudId = displayNode.getAttribute("id");


`displayNode` is not initialized (no let or var!) and `outputNode` is never used. The patch attached adds a "let" in front of `displayNode =`. Future more

 browser = gBrowser.getBrowserForTab(tab);
+let testAsync;
 
Adding the `testAsync;` variable is not needed (was in the first draft of this patch, but not in this version anymore).

Comment 20

[Julian Viereck]

Remove the "checkin-needed" due to new patch.

Comment 21

[Julian Viereck]

Attachment: Patch v4

This is an overall improved patch. It doesn't leak anymore (at least in my tests - got to do another try-server run).

This patch depends on the patch attached to bug 568034.

Comment 22

[Julian Viereck]

Attachment: patch v5

Updated patch that has i10n support + some small improvements.

Comment 23

[Julian Viereck]

Attachment: Patch v5.1

This is a rebased patch + change the l10n notes + cleaned some small stuff up.

Comment 24

[Julian Viereck]

Attachment: Patch v5.1 rebased

This is a rebased patch. Note that while running the unit tests I get the info:

TEST-INFO | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | Console message: [JavaScript Error: "ev.target.classList is undefined" {file: "resource://gre/modules/HUDService.jsm" line: 2265}]

The code in this patch adds some textNodes to an existing xul:label, which doesn't seem to make a new function by Patrick fail. I will open up a bug on this.

Comment 25

[Dietrich Ayala (:dietrich)]

Comment on attachment 467368 [details] [diff] [review]
Patch v5.1 rebased


> var NetworkHelper =
> {
>   /**
>+   *
>+   * @param object aObj
>+   * @param nsIIDRef aIID
>+   */
>+  QI: function NH_QI(aObj, aIface)
>+  {
>+    return aObj.QueryInterface(aIface);
>+  },
>+
>+  /**
>+   *
>+   * @param string aCName
>+   * @param string aIfaceName
>+   */
>+  CCSV: function NH_CCSV(aCName, aIfaceName)

above the params, add a summary comment for these

>   /**
>+   * Parses aHeader string.
>+   *
>+   * @param string  aHeader Header string to parse.
>+   * @returns array First item holds the status string, the second item holds
>+   *                the parsed header as an object <value>: <key>
>+   */
>+  parseHttpHeader: function HS_parseHttpHeader(aHeader)
>+  {
>+    let lines = aHeader.split("\n");

http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.7.1

>   startHTTPObservation: function HS_httpObserverFactory()
>   {

this is a gargantuan function. i'd prefer if it was broken up somehow, but i guess that doesn't need to happen in this patch.

>diff --git a/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties
>--- a/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties
>+++ b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties
>@@ -59,8 +59,9 @@ jsPropertyTitle=Object Inspector
> # Example: The user executed `window.document` in the WebConsole. The `document`
> # object is written to the output. If the user clicks on the `document` output
> # in the console, a PropertyPanel will show up. The title of the PropertyPanel
> # is set to `Inspect: window.document` because the clicked `document` object was
> # evaluated based on the `window.document` string.
> jsPropertyInspectTitle=Inspect: %S
> copyCmd.label=Copy
> copyCmd.accesskey=C
>+durationMS=%Sms

Hm, not sure this is right for RTL. Cc [e-mail] on the bug and ask about the ideal way to do this?

r=me with comments fixed.

Comment 26

[Julian Viereck]

(In reply to comment #25)
> Comment on attachment 467368 [details] [diff] [review]
> Patch v5.1 rebased
> >diff --git a/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties
> >--- a/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties
> >+++ b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties
> >@@ -59,8 +59,9 @@ jsPropertyTitle=Object Inspector
> > # Example: The user executed `window.document` in the WebConsole. The `document`
> > # object is written to the output. If the user clicks on the `document` output
> > # in the console, a PropertyPanel will show up. The title of the PropertyPanel
> > # is set to `Inspect: window.document` because the clicked `document` object was
> > # evaluated based on the `window.document` string.
> > jsPropertyInspectTitle=Inspect: %S
> > copyCmd.label=Copy
> > copyCmd.accesskey=C
> >+durationMS=%Sms
> 
> Hm, not sure this is right for RTL. Cc [e-mail] on the bug and ask
> about the ideal way to do this?

Can someone of the l10n team please comment on this?

Comment 27

[Axel Hecht [:Pike]]

Comment on attachment 467368 [details] [diff] [review]
Patch v5.1 rebased

+                msgObject.messageNode.appendChild(
+                  msgObject.textFactory(" " +
+                    self.getFormatStr("durationMS", [requestDuration]) + "]"));

looks wrong. It's hard to tell how without completely understanding the code, what's the expected result?

Also, I've heard from ehsan that at least Persian doesn't have that much of a concept of milliseconds.

Comment 28

[Julian Viereck]

(In reply to comment #27)
> Comment on attachment 467368 [details] [diff] [review]
> Patch v5.1 rebased
> 
> +                msgObject.messageNode.appendChild(
> +                  msgObject.textFactory(" " +
> +                    self.getFormatStr("durationMS", [requestDuration]) +
> "]"));
> 
> looks wrong. It's hard to tell how without completely understanding the code,
> what's the expected result?

When the network request starts, the URL of the request is logged to the WebConsole. Once the response header is there, a string like "[HTTP/1.1 200 OK" is added. When the request is done an additional string is added that contains the complete duration for the request tha tlooks like " 200ms]".

All in all the logged network request looks like this when finished (just an example):

  https://bugzilla.mozilla.org/show_bug.cgi?id=568634 [HTTP/1.1 200 OK 202ms]

 
> Also, I've heard from ehsan that at least Persian doesn't have that much of a
> concept of milliseconds.

I'm not sure what you want to say with this. Does it need a special function to display the duration for the Persian l10n support in seconds using a calculation?

Comment 29

[Axel Hecht [:Pike]]

I'd like to give some time for the l10n community to comment, a bunch of them watch :l10n. This sounds scary.

I wonder if going for two strings, one with the url, and another with the url, http, status, ok, time, would give best flexibility. Then you'd replace the content on load instead of appending.

Comment 30

[Julian Viereck]

(In reply to comment #29)
> I'd like to give some time for the l10n community to comment, a bunch of them
> watch :l10n. This sounds scary.
> 
> I wonder if going for two strings, one with the url, and another with the url,
> http, status, ok, time, would give best flexibility. Then you'd replace the
> content on load instead of appending.

> http, status, ok

The status string is returned by the server. It's a combination of the HTTP protocol used (version 1.1 in this example) and the status code itself (there are a few possible values, see here: http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html).

That's why I think there are only 3 components that are displayed: the URL, the status code (HTTP/1.1 200 OK) and the duration (202ms). This information is not available at once but appear one after the other (which might result in 3 different l10n string?).

Comment 31

[Julian Viereck]

Attachment: patch v6

Updating the patch based on Dietrich's review. Waiting for feedback from the l10n team and review on bug 587573 to set the needs-checking flag.

Comment 32

[Axel Hecht [:Pike]]

Yeah, sounds like three strings is the best fit. It might also be better for the load phase, as you'd not end up with "http://....foo.html [HTTP/1.1 200 OK" without the closing brace (which you do right now, AFAICT?)

Comment 33

[Julian Viereck]

(In reply to comment #32)
> Yeah, sounds like three strings is the best fit. It might also be better for
> the load phase, as you'd not end up with "http://....foo.html [HTTP/1.1 200 OK"
> without the closing brace (which you do right now, AFAICT?)

If there is no closing brace this indicates that the request is still going on and is not finished yet. That's a feature ;)

Comment 34

[Julian Viereck]

(In reply to comment #32)
> Yeah, sounds like three strings is the best fit. It might also be better for
> the load phase, as you'd not end up with "http://....foo.html [HTTP/1.1 200 OK"
> without the closing brace (which you do right now, AFAICT?)

@pwalton: When I update the content of a xul:label in the WebConsole, do I have to call something special to make filtering work?

Comment 35

[Julian Viereck]

Attachment: Patch v6.1

This patch includes some changes as requested by Axel to integrate a better i10n support. There is now a `networkUrlWithStatus` and `networkUrlWithStatusAndDuration` i10n string + i10n notes.

Axel, can give me a hint if this is okay now?

Comment 36

[Axel Hecht [:Pike]]

Comment on attachment 468330 [details] [diff] [review]
Patch v6.1

feedback+ with comments.

<...>
>diff --git a/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties
<...>
>+# LOCALIZATION NOTE (networkUrlWithStatus):
>+#
>+# When the HTTP request is started only the URL of the request is printed to the
>+# WebConsole. As the response status of the HTTP request arrives, the URL string
>+# is replaced by this string (the response status can look like `HTTP/1.1 200 OK).

like `HTTP/1.1 200 OK`.

>+# The braket is not closed to mark that this request is not done by now. As the

bracket

>+# request is finished (the HTTP connection is closed) this string is replaced
>+# by `networkUrlWithStatusAndDuration` which has a closing the braket.
>+#
# %1$S = URL of network request
# %2$S = response status code from the server (e.g. `HTTP/1.1 200 OK`)
networkUrlWithStatus=%1$S [%2$S

>+# LOCALIZATION NOTE (networkUrlWithStatusAndDuration):
>+#
>+# When the HTTP request is finished (the HTTP connection is closed) this string
>+# replaces the former `networkUrlWithStatus` string in the WebConsole.
>+#
# %1$S = URL of network request
# %2$S = response status code from the server (e.g. `HTTP/1.1 200 OK`)
# %3$S = duration for the complete network request in milliseconds
networkUrlWithStatusAndDuration=%1$S [%2$S %3$Sms]

Comment 37

[Julian Viereck]

Attachment: patch v6.2

Improved based on Axel's feedback. Thanks for that!

@Shawn: Dietrich did a review on this bug already. Maybe you can make an iterdiff and review only the change in the meantime?

Comment 38

[Julian Viereck]

Attachment: patch v6.3

Rebased patch + small improvements.

Comment 39

[Shawn Wilsher :sdwilsh]

Comment on attachment 468673 [details] [diff] [review]
patch v6.3

>+  /**
>+   * Stores the received data.
>+   */
>+  receivedData: null,
The type would be useful in the comment (it's an Array in your current code, but I think you actually want it to be a String).

>+  /**
>+   * Helper function for XPCOM instanciation (from Firebug)
>+   *
>+   * @param string aCName
>+   * @param string aIfaceName
>+   */
>+  CCIN: function RL_CCIN(aCName, aIfaceName)
>+  {
>+    return Cc[aCName].createInstance(Ci[aIfaceName]);
>+  },
So, this is really awkward and totally foreign to the code base.  This is trying to solve the problem that Components.Constructor was designed to solve, but in a less elegant way.  See https://developer.mozilla.org/en/Components.constructor for more details.

>+  onDataAvailable: function RL_onDataAvailable(aRequest, aContext, aInputStream,
>+                                                aOffset, aCount)
>+  {
A comment stating that this only grabs a copy of the data, and passes it on to the original listener would be handy here.

>+    // Copy received data as they come.
awkward sentence.

>+    let data = binaryInputStream.readBytes(aCount);
>+    this.receivedData.push(data);
I think, given the only consumer in this patch, that you really want to treat this as a string:
this.receivedData += binaryInputStream.readBytes(aCount);

>+  onStartRequest: function RL_onStartRequest(aRequest, aContext)
>+  {
>+    try {
>+      this.originalListener.onStartRequest(aRequest, aContext);
>+    } catch (ex) {
>+      aRequest.cancel(ex.result);
>+    }
This changes the behavior here - you should just let the exception propagate up the stack, right?

>+  QueryInterface: function RL_QueryInterface(aIID)
Use XPCOMUtils please

>   /**
>+   * Returns the queried interface aIface from the aObj object.
>+   *
>+   * @param object aObj
>+   * @param nsIIDRef aIID
>+   */
>+  QI: function NH_QI(aObj, aIface)
>+  {
>+    return aObj.QueryInterface(aIface);
>+  },
We should just call QueryInterface, this wrapper doesn't get us anything other than indirection.

>+  /**
>+   * Returns a Components.classes service.
>+   *
>+   * @param string aCName
>+   * @param string aIfaceName
>+   */
>+  CCSV: function NH_CCSV(aCName, aIfaceName)
>+  {
>+    try {
>+      // If fail to load, the error can be Cc[cName] has no properties.
>+      return Cc[aCName].getService(Ci[aIfaceName]);
>+    }
>+    catch(exc) {
>+      Cu.reportError(aCName + "@" + aIfaceName + " FAILED " + exc);
>+      if (!Cc[aCName]) {
>+        Cu.reportError("No Components.classes entry for " + aCName);
>+      }
>+      else if (!Ci[aIfaceName]) {
>+        Cu.reportError("No Components.interfaces entry for " + aIfaceName);
>+      }
>+    }
>+  },
You only use this for one service.  That doesn't constitute the need of a helper, and this makes things slower by wrapping in a try-catch needlessly.  Please remove this.

>+  /**
>+   * Converts aText with a given aCharset to unicode.
>+   *
>+   * @param string aText Text to convert.
>+   * @param string aCharset Charset to convert the text to.
>+   * @returns string Converted text.
>+   */
nit: as I've commented in other patches, the documentation format is inconsistent

>+  convertToUnicode: function NH_convertToUnicode(aText, aCharset)
>+  {
>+    if (!aText) {
>+      return "";
>+    }
This feels like a premature optimization, yeah?

>+    let conv;
>+    try {
>+      conv = this.CCSV("@mozilla.org/intl/scriptableunicodeconverter",
>+                        "nsIScriptableUnicodeConverter");
Need to createInstance on this, not createService

>+      conv.charset = aCharset ? aCharset : "UTF-8";
conv.charset = aCharset | "UTF-8";

>+    }
>+    catch (exc) {
>+      // the exception is worthless, make up a new one
>+      throw new Error(
>+        "Failed to convert to unicode using charset: " +
>+        conv.charset + " in @mozilla.org/intl/scriptableunicodeconverter");
it's possible this will just throw again of the charset is not supported.  Not sure it's even worth having the try-catch here.


>+  /**
>+   * Reads all available bytes from aStream and converts them to aCharset.
>+   * If aNoClose is true, the internal used binaryInputStream is not closed.
>+   *
>+   * @param nsISeekableStream aStream
You are actually passing an nsIInputStream here

>+   * @param boolean aNoClose
not clear what this means.  Also, it's generally frowned upon to have NoSometing as a boolean argument (it gets confusing).  Just make it the Something, so in this case, aClose.

>+   * @returns string
string of what?

>+  readFromStream: function NH_readFromStream(aStream, aCharset, aNoClose)
This is really "readAndConcertFromStream", right?

>+  {
>+    let sis = this.CCSV("@mozilla.org/binaryinputstream;1",
>+                        "nsIBinaryInputStream");
>+    sis.setInputStream(aStream);
>+
>+    let segments = [];
>+    var availableBytes;
>+    while (availableBytes = aStream.available()) {
>+      segments.push(sis.readBytes(availableBytes));
>+    }
So, you have code here, and in RL_onDataAvailable that consume the input stream and get a string out of it.  You should create a helper method that both use.  In both cases, you have an nsIInputStream, so you can share code:
function readInputStreamToString(aInputStream, /* optional */ aCount)
{
  let bis = Cc["@mozilla.org/binaryinputstream;1"].createInstance(Ci.nsIBinaryInputStream);
  bis.setInputStream(aInputStream);

  let size = aCount | aInputStream.available();
  return bis.readBytes(size);
}
Note, however, that JS is limited in what it can do here.  We really want to do what NS_ReadInputStreamToString (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1149) does, but we don't have all the APIs available to us.  This means that this can trow (and the looping it code in NH_readFromStream doesn't protect against this).

>+  /**
>+   * Reads the posted text from aRequest.
>+   *
>+   * @param nsIHttpChannel aRequest
>+   * @param nsIDOMNode aBrowser
>+   * @returns string or null
string or null in what cases?  being more elaborate in java-doc comments is helpful

>+  readPostTextFromRequest: function NH_readPostTextFromRequest(aRequest, aBrowser)
>+  {
>+    try {
>+      let is = this.QI(aRequest, Ci.nsIUploadChannel).uploadStream
>+      if (is) {
better to do:
if (aRequest instanceof Ci.nsIUploadChannel) {
  let is = aRequest.uploadStream;
XPConnect will do the QI for you, and you don't need to try-catch.

>+        let ss = this.QI(is, Ci.nsISeekableStream);
>+        let prevOffset;
>+        if (ss) {
let ss;
if (is instanceof Ci.nsISeekableStream) {
with that, you don't need the try-catch anymore

>+  readPostTextFromPage: function NH_readPostTextFromPage(aUrl, aBrowser)
ditto on instanceof stuff instead of doing explict QI's here.

I don't see tests for the various line endings possible for HTTP headers, which would be good to have.

Comment 40

[Julian Viereck]

Attachment: Patch v6.4

This is an improved patch based on Shawn's review (thanks a lot for it!). I've also moved the unit test into a separate test file.

Some questions:
- There is now a readInputStreamToString() function as pointed out. Should every caller of this function add a try{}cath(){} around it as it might throws an error?
- You said:
> I don't see tests for the various line endings possible for HTTP headers, which
> would be good to have.

I don't exactly what to test. The HS_parseHttpHeader function?

Comment 41

[Shawn Wilsher :sdwilsh]

(In reply to comment #40)
> - There is now a readInputStreamToString() function as pointed out. Should
> every caller of this function add a try{}cath(){} around it as it might throws
> an error?
You didn't have it before, so you might be lucky enough to not have any input streams that you have to worry about this for.  You won't get anything useful anyway if it throws, so it's probably OK to not catch it.

> I don't exactly what to test. The HS_parseHttpHeader function?
Yup.  Basically, want to see that we handle the different cases of line endings there.

Comment 42

[Shawn Wilsher :sdwilsh]

(In reply to comment #39)
> Note, however, that JS is limited in what it can do here.  We really want to do
> what NS_ReadInputStreamToString
> (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1149)
> does, but we don't have all the APIs available to us.  This means that this can
> trow (and the looping it code in NH_readFromStream doesn't protect against
> this).
Which will be fixed by using NetUtil.readInputStreamToString in bug 590654.  You should use that.

Comment 43

[Julian Viereck]

Attachment: Patch v7

Shawn, this patch uses the new NetUtil.readInputStreamToString function. Most of the rest should have stayed the same beside rebasing.

Comment 44

[Shawn Wilsher :sdwilsh]

Comment on attachment 469707 [details] [diff] [review]
Patch v7

>+  convertToUnicode: function NH_convertToUnicode(aText, aCharset)
>+  {
>+    let conv;
nit: declare at first use

>+    conv = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
>+              createInstance(Ci["nsIScriptableUnicodeConverter"]);
nit: createInstance should line up with Cc, and just do Ci.nsIScript....

>+   * @returns string
>+   *        UTF-16 encoded string based on the content of aStream and aCharset.
global-nit: think you want this lined up with "string"

>+      if (is instanceof Ci.nsISeekableStream) {
>+        let ss = is.QueryInterface(Ci.nsISeekableStream);
you can just use is here - the instanceof check will automagically do the QI for you.

>+        let prevOffset;
>+        if (ss) {
and this would be true if |is| is an instaceof nsISeekableStream

>+          prevOffset = ss.tell();
>+          ss.seek(Ci.nsISeekableStream.NS_SEEK_SET, 0);
>+        }
>+
>+        // Read data from the stream.
>+        let charset = aBrowser.contentWindow.document.characterSet;
>+        let text = this.readAndConvertFromStream(is, charset);
but given this, I think you want to support cases where is isn't a seekable stream?

will look at the rest in a bit

Comment 45

[Shawn Wilsher :sdwilsh]

Comment on attachment 469707 [details] [diff] [review]
Patch v7

>+  readPostTextFromPage: function NH_readPostTextFromPage(aUrl, aBrowser)
>+  {
>+    if (aUrl == aBrowser.contentWindow.location.href) {
It's not clear why you have to check this.  Can you please add a comment explaining why.

>+      let webNav = aBrowser.webNavigation;
>+      if (webNav instanceof Ci.nsIWebPageDescriptor) {
>+        let descriptor =
>+          webNav.QueryInterface(Ci.nsIWebPageDescriptor).currentDescriptor;
let descriptor = webNav.currentDescriptor;

>+        if (descriptor instanceof Ci.nsISHEntry) {
>+          let entry = descriptor.QueryInterface(Ci.nsISHEntry);
>+          if (entry && entry.postData) {
>+            if (entry instanceof Ci.nsISeekableStream) {
>+              let postStream = entry.postData.QueryInterface(Ci.nsISeekableStream);
don't need to do explicit QI's here

>   /**
>+   * Parses aHeader string.
>+   *
>+   * @param string  aHeader Header string to parse.
>+   * @returns array First item holds the status string, the second item holds
>+   *                the parsed header as an object <value>: <key>
>+   */
nit: inconsistent comment styling here with how you do other functions

>+  parseHttpHeader: function HS_parseHttpHeader(aHeader)
>+  {
>+    let lines = aHeader.split(/\r\n|\n|\r/);
>+    let status = lines[0];
>+    let headerContent = {};
>+
>+    // Skip the first line and parse the rest.
It's clear that you are skipping the first line.  What would be more useful in the comment is why you skip the first line.

>+    lines.slice(1).forEach(function (line) {
>+      // Skip empty lines.
comment isn't needed; code is obvious

>           var transCodes = this.httpTransactionCodes;
>+          var hudId;
why var for these?

>+            // In some cases loggedNode can be undefined (e.g. if an image was
>+            // requested). Don't continue in such a case.
>+            if (loggedNode === undefined) {
just |if (!loggedNode) {| right?
>+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_netlogging.js
Tests can be public domain (which we should use) per http://www.mozilla.org/MPL/license-policy.html.  Please change the license block accordingly.
>+XPCOMUtils.defineLazyGetter(this, "HUDService", function () {
>+  Cu.import("resource://gre/modules/HUDService.jsm");
>+  try {
>+    return HUDService;
>+  }
>+  catch (ex) {
>+    dump(ex + "\n");
>+  }
>+});
No reason to make this lazy in the test.  Just import it.

r- for the stuff in comment 44.  This is super close!

Comment 46

[Julian Viereck]

Attachment: Patch v8

This bug is improved based on Shawn's latest feedback. The self written HTTP-header-parse is gone and instead the nsIHttpChannel.visitResponseHeaders|visitRequestHeaders functions are used. Calling the nsIHttpChannel.visitResponseHeaders from inside of the HS_httpObserverFactory function caused errors. It seems like the header is not ready to get accessed even when the transaction is closed (!!!). To get around this problem I set the httpActivity object now on the responseListener and set the response information (header, status, body) once the response is done (RL_onStopRequest). I hope this way bug 588533 might be fixed now (will need to test that later).

Comment 47

[Shawn Wilsher :sdwilsh]

I would expect the earliest you could safely get the headers would be at OnDataAvailable, fwiw.

Comment 48

[Shawn Wilsher :sdwilsh]

Comment on attachment 470081 [details] [diff] [review]
Patch v8

>+      // Seek locks the file so, seek to the beginning only if necko hasn't
nit: comma before so, not after

>+  readPostTextFromPage: function NH_readPostTextFromPage(aBrowser)
>+  {
>+    let webNav = aBrowser.webNavigation;
>+    if (webNav instanceof Ci.nsIWebPageDescriptor) {
>+      let descriptor = webNav.currentDescriptor;
>+
>+      if (descriptor instanceof Ci.nsISHEntry) {
>+        if (descriptor.postData) {
>+          if (descriptor instanceof Ci.nsISeekableStream) {
so, the nesting is really deep here.  I think it'd be better to do something like this:
if (descriptor instanceof Ci.nsISHEntry && descriptor.postData &&
    descriptor instanceof Ci.nsISeekableStream) {
  ...
}

>+            try {
>+              // Copy the request header data.
>+              aChannel.visitRequestHeaders({
>+                visitHeader: function(aName, aValue) {
>+                  httpActivity.request.header[aName] = aValue;
>+                }
>+              });
>+            } catch (ex) { }
please add a comment as to why this might throw and why we don't care if it does

>+              case activityDistributor.ACTIVITY_SUBTYPE_RESPONSE_HEADER:
>+                msgObject = httpActivity.messageObject;
>+
>+                // aExtraStringData contains the response header. The first line
>+                // contains the response status (e.g. HTTP/1.1 200 OK).
>+                //
>+                // Note: The response header is not saved here. Calling the
>+                //       aChannel.visitResponseHeaders at this point sometimes
>+                //       causes an NS_ERROR_NOT_AVAILABLE exception. Therefore,
>+                //       the response header and response body is stored on the
>+                //       httpActivity object within the RL_onStopRequest function.
>+                httpActivity.response.status =
>+                  aExtraStringData.split(/\r\n|\n|\r/)[0];
aExtraStringData.trim() should work here and make more sense

It looks like you removed the test that tested the header with different types of line endings.  I think that test was useful to keep even if you don't parse it yourself anymore, so please add it back.

r=sdwilsh

Comment 49

[Shawn Wilsher :sdwilsh]

(In reply to comment #48)
> aExtraStringData.trim() should work here and make more sense
disregard this.  I misunderstood the comment.

Comment 50

[Julian Viereck]

Attachment: Patch v8.1

Thanks to Shawn's tip the response header is now calculated in the onDataAvailable as well. It's not enough to do it there only as if there is no response body onDataAvailable is never called for this request but the response header must be read somewhere.

Comment 51

[Julian Viereck]

Attachment: [backed-out] Patch v8.1.1

Patch as it should be (missed the hg qrefresh).

Comment 52

[Shawn Wilsher :sdwilsh]

Comment on attachment 470099 [details] [diff] [review]
Patch v8.1

r=sdwilsh

Comment 53

[Shawn Wilsher :sdwilsh]

Comment on attachment 470101 [details] [diff] [review]
[backed-out] Patch v8.1.1

r=sdwilsh on this one too, while I'm here.

Comment 54

[David Dahl :ddahl]

By the time I was ready with my patch queue, I pulled one last time and ended up with many bad hunks. I do not know how this could happen. I created a new repo even, and applied everything, still I had hg failures.

Will try to look into this over the weekend, but do not bet on it.:(

Comment 55

[David Dahl :ddahl]

So these are the changesets I pulled when this hg failure happend:

anarchy ~/code/moz/mozilla-central/mozilla-central % hg incoming
comparing with https://hg.mozilla.org/mozilla-central/
searching for changes
changeset:   51666:be857a136ffd
parent:      51659:2715808a3010
user:        Oleg Romashin <[e-mail]>
date:        Fri Jul 16 06:57:00 2010 -0500
summary:     Bug 591559 and bug 575567 - "Drop X[Parent/Child] widget embedding from remote browser" [r=cjones] [a=bustage-fix+blocking]

changeset:   51667:b49737f77ad2
user:        Gavin Sharp <[e-mail]>
date:        Sat Aug 28 00:48:17 2010 -0400
summary:     Bug 591566: disable browser_bug562797.js because it times out a lot, a=orange

changeset:   51668:8c3b57225056
user:        Brad Lassey <[e-mail]>
date:        Fri Aug 27 16:22:57 2010 -0400
summary:     bug 591153 - Show progress dialog on start up for android r=mwu a=blocking-fennec

changeset:   51669:e051ccf2361a
tag:         tip
user:        Brad Lassey <[e-mail]>
date:        Fri Aug 27 16:24:42 2010 -0400
summary:     bug 591412 - Android make android package step failing due to error in AndroidManifest.xml r=mwu a=blocking-fennec

Does not make any sense

Comment 56

[David Dahl :ddahl]

anarchy ~/code/moz/mozilla-central/push-only/mozilla % hg qpush
applying update-log-entries
now at: update-log-entries
anarchy ~/code/moz/mozilla-central/push-only/mozilla % hg qpush
applying click-handle-log-details
patching file toolkit/components/console/hudservice/HUDService.jsm
Hunk #1 FAILED at 235
Hunk #2 FAILED at 451
Hunk #3 FAILED at 1002
Hunk #4 FAILED at 1361
Hunk #5 FAILED at 1407
Hunk #6 FAILED at 1444
Hunk #7 FAILED at 1461
Hunk #8 FAILED at 1543
8 out of 8 hunks FAILED -- saving rejects to file toolkit/components/console/hudservice/HUDService.jsm.rej
patching file toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js
Hunk #1 FAILED at 64
Hunk #2 FAILED at 563
Hunk #3 FAILED at 823
3 out of 3 hunks FAILED -- saving rejects to file toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js.rej
patching file toolkit/components/console/hudservice/tests/browser/browser_webconsole_netlogging.js
Hunk #1 FAILED at 11
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh click-handle-log-details

I'll attach the .rej

Comment 57

[David Dahl :ddahl]

Attachment: HUDService.jsm.rej

Comment 58

[Julian Viereck]

(In reply to comment #57)
> Created attachment 470153 [details]
> HUDService.jsm.rej

David, the reject you attached is the patch for bug 573103 ("Click event handler and display of log entry details") and NOT the patch for this bug. Have you mixed them up a little bit?

Comment 59

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 470101 [details] [diff] [review]
[backed-out] Patch v8.1.1

http://hg.mozilla.org/mozilla-central/rev/2d1aebfcf1ef

Comment 60

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 470101 [details] [diff] [review]
[backed-out] Patch v8.1.1

http://hg.mozilla.org/mozilla-central/rev/289e6da90526

leaks

Comment 61

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/rev/1ac4f2b24a91

Comment 62

[David Dahl :ddahl]

(In reply to comment #61)
> http://hg.mozilla.org/mozilla-central/rev/1ac4f2b24a91

I should have mentioned that after tryserver success, we are pushing again

Comment 63

[Tim Maks van den Broek [:mad_maks]]

+networkUrlWithStatus=%1$S [%2$S

shouldn't this have a closing ] ?

Comment 64

[Axel Hecht [:Pike]]

(In reply to comment #63)
> +networkUrlWithStatus=%1$S [%2$S
> 
> shouldn't this have a closing ] ?

That's considered to be a feature, see comment 33.

Comment 65

[Tim Maks van den Broek [:mad_maks]]

sorry didn't read all of the LOCALIZATION NOTE, only what the variable stands for :-(

but i thinks this will create a lot of comments from the community....