761157 - Sometimes network requests do not display all information

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Mihai Sucan [:msucan]]

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]]

Attachment: 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]]

Asking for tracking-firefox15 because I'd like this bug fixed in Aurora.

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 629814 [details] [diff] [review]
proposed fix

Re-assigning the review request to Dave.

Comment 4

[Dave Camp (:dcamp)]

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]]

(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]]

Attachment: [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]]

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]]

Thanks for the review!

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/9fff1479db88

Comment 10

[Mihai Sucan [:msucan]]

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]

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]]

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