Last Comment Bug 761157 - Sometimes network requests do not display all information
: Sometimes network requests do not display all information
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 16
Assigned To: Mihai Sucan [:msucan]
:
: Brian Grinstead [:bgrins]
Mentors:
Depends on:
Blocks: 761257
  Show dependency treegraph
 
Reported: 2012-06-04 07:55 PDT by Mihai Sucan [:msucan]
Modified: 2012-06-19 03:08 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
proposed fix (4.48 KB, patch)
2012-06-04 09:34 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
[in-fx-team] address review comment (7.32 KB, patch)
2012-06-11 12:00 PDT, Mihai Sucan [:msucan]
dcamp: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Mihai Sucan [:msucan] 2012-06-04 07:55:28 PDT
Under certain conditions network requests only display the request headers - no response information.

STR: Load a page with lots of network requests.


I have a fix almost ready. Will submit a patch with fix and explanation of the problem.
Comment 1 Mihai Sucan [:msucan] 2012-06-04 09:34:40 PDT
Created attachment 629814 [details] [diff] [review]
proposed fix

Problem: when many requests happen at once the code that delays output causes handleNetworkActivity() to not find previous messages to update their network request information.

Fix: use a networkRequests object to hold information about requests that are displayed or about to be displayed. Using the DOM node as the object to attach request information to was not ideal in the first place. Plus, the querySelector() was potentially slow with a lot of output in the Web Console.

Potential of negative impact: minimal to none.

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=c0255fe22a09

Once the try run is green and I get r+ I would like to ask for aurora approval. Thanks!
Comment 2 Mihai Sucan [:msucan] 2012-06-05 03:38:26 PDT
Asking for tracking-firefox15 because I'd like this bug fixed in Aurora.
Comment 3 Mihai Sucan [:msucan] 2012-06-06 09:44:15 PDT
Comment on attachment 629814 [details] [diff] [review]
proposed fix

Re-assigning the review request to Dave.
Comment 4 Dave Camp (:dcamp) 2012-06-07 15:37:54 PDT
Comment on attachment 629814 [details] [diff] [review]
proposed fix

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

::: browser/devtools/webconsole/HUDService.jsm
@@ +2743,5 @@
>          location = aNode.childNodes[4].getAttribute("title");
>        }
>        delete this.cssNodes[desc + location];
>      }
> +    else if (aNode.classList.contains("webconsole-msg-network")) {

Is removeOutputMessage() always guaranteed to be called for every outputMessageNode() call?  Is there any case where a message might get queued and never displayed?  For example, if 1000 nodes are added while the scrollback limit is 200, do you throw away 800 nodes without every displaying them?

If we're not guaranteed that, we'll leak network requests here.
Comment 5 Mihai Sucan [:msucan] 2012-06-08 01:19:46 PDT
(In reply to Dave Camp (:dcamp) from comment #4)
> Comment on attachment 629814 [details] [diff] [review]
> proposed fix
> 
> Review of attachment 629814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webconsole/HUDService.jsm
> @@ +2743,5 @@
> >          location = aNode.childNodes[4].getAttribute("title");
> >        }
> >        delete this.cssNodes[desc + location];
> >      }
> > +    else if (aNode.classList.contains("webconsole-msg-network")) {
> 
> Is removeOutputMessage() always guaranteed to be called for every
> outputMessageNode() call?  Is there any case where a message might get
> queued and never displayed?  For example, if 1000 nodes are added while the
> scrollback limit is 200, do you throw away 800 nodes without every
> displaying them?

Yes, messages that are queued are not guaranteed to be displayed - they could get thrown away.


> If we're not guaranteed that, we'll leak network requests here.

The HUDService-content script should not leak network requests - their processing and cleanup is independent of UI. The UI can't leak the actual network requests as such (no access to nsIRequests, nsIHttpChannels, etc). However you do rightly point out that we can leak JS objects that hold network information. We do clear them when clearOutput() is called and when the Web Console is closed. (just checked and I see I forgot to add _networkRequests = {} in clearOutput()).

To avoid such cases I can make sure that when the queue pruning happens we clean up the associated network info object.

Do you want me to make these changes here? I kept this patch minimal so we can land it in Aurora. To me it's fine to do this here.

Do note that bug 761257 makes more invasive changes and JS objects with network information in the HUD object are not even created if the messages are pruned before being displayed. So those objects no longer leak when pruning happens.

Thank you!
Comment 6 Mihai Sucan [:msucan] 2012-06-11 12:00:45 PDT
Created attachment 631973 [details] [diff] [review]
[in-fx-team] address review comment

Made the code to reuse the removeOutputMessage() function when output queue pruning happens. The removeOutputMessage() handles the clearing of any recorded information for each message. This hopefully addresses your comment.

Looking forward to your review. Thank you!
Comment 7 Mihai Sucan [:msucan] 2012-06-14 05:03:34 PDT
Comment on attachment 631973 [details] [diff] [review]
[in-fx-team] address review comment

https://hg.mozilla.org/integration/fx-team/rev/9fff1479db88
Comment 8 Mihai Sucan [:msucan] 2012-06-14 05:04:14 PDT
Thanks for the review!
Comment 9 Tim Taubert [:ttaubert] 2012-06-16 03:45:12 PDT
https://hg.mozilla.org/mozilla-central/rev/9fff1479db88
Comment 10 Mihai Sucan [:msucan] 2012-06-18 02:27:40 PDT
Comment on attachment 631973 [details] [diff] [review]
[in-fx-team] address review comment

[Approval Request Comment]
Bug caused by (feature/regressing bug #): caused by bug 722685 - the quick succession of many network requests combined with delayed output of messages triggers the issue.
User impact if declined: some network requests in the Web Console will have no UI updates / no details about the request and response.
Testing completed (on m-c, etc.): several try pushes, testing in fx-team and m-c.
Risk to taking this patch (and alternatives if risky): Should be no risk - this code only makes sure that network requests being logged have their details and UI properly updated.
String or UUID changes made by this patch: None.
Comment 11 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-18 16:28:38 PDT
Comment on attachment 631973 [details] [diff] [review]
[in-fx-team] address review comment

Approving for aurora landing, will set tracking as well in case we need to backout or followup.
Comment 12 Mihai Sucan [:msucan] 2012-06-19 03:07:42 PDT
Comment on attachment 631973 [details] [diff] [review]
[in-fx-team] address review comment

Thank you!

Landed:
https://hg.mozilla.org/releases/mozilla-aurora/rev/72c27b21702e

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