We need to make sure that the Web Console code is e10s-ready. The plan: https://wiki.mozilla.org/DevTools/e10s#Web_Console To try the Web Console in e10s mode add browser.tabs.remote=true in about:config.

Taking this bug. I already have work underway to make the Web Console open/close properly in e10s mode, and more work is coming.

First WIP patch: cleanup hudservice code and trim it to make the Web Consoles open and close in e10s mode. Also included the first attempt to make jsterm code evaluation work with content processes. It's all very WIPish. I have commented out parts of code to avoid exceptions until I make those parts work in e10s mode (for example autocomplete is commented out). Broken stuff is intentionally marked as FIXME. TODO: - improve/finalize jsterm evaluation. - fix minor bits in init/uninit code. - make nsIScriptError2 messages show. - fix the object inspector. - fix the network logging code. - fix the tests.

At this point jsterm evaluation works in e10s mode as it does in non e10s-mode. Same goes for object inspection. For the user, things are fine. How object inspection works: - the object is inspected in the content process. Information that needs to be displayed is sent to the main process on demand. The entire object tree is not sent, and this avoids problems with cyclic refs. - each object property that points to another object is given an objectId that can be used from the main process to get the actual object content, when the user wants to expand the property view for that object. - all of this works really nice, but we get into the problem of garbage collection. for how long do we keep the evaluated object references in the content process? this quickly becomes a hairy question to answer. - an attempt to avoid the GC problem would be to move the object inspector into an iframe that runs in the content process, but that's really the same in the end: we would have to GC the eval results from the content process when you clear the web console, or when messages are purged when the log limit is reached, and so on. - and given that the object inspector in its current incarnation is going to be short-lived - we'll soon replace it with a better UI, there's always the need to keep track of which objects we use in the main process from the content process. This need applies to DOM and any other JS object. - the need we expressed for the DOM Inspector, that we would like a unique identifier for each dom node, so we can communicate between processes about it, is really a need that extends to JS objects as well. see the current code implementation: the main process needs a way to track which object it wants from the content process. All in all: the current code works, but there's no GC. I can add some, but it will be prone to errors. Thoughts? Comments?

A note about GC of eval result objects in the content process: it's NOT too hard. It's really a matter of "what is sufficient" for us to do? I can clear the cached result objects when the web console is closed, or when the console output is cleared. It's simple/trivial. It only gets complicated if we want to take into consideration fine-grained approaches to GC: per eval result (when each output message is pruned from the output), or when we have an object inspector opened while we want to clear the output. We need refcounting...... Or not, and we can skip over fancier GC and ref counting, and so on.

the correct patch version. (the previous attachment had an experiment with weak refs that did not work as intended, since objects are GC'ed by gecko far too early, so we can't use weak refs.)

drive-by: >diff --git a/toolkit/content/HUDService-content.js b/toolkit/content/HUDService-content.js >+XPCOMUtils.defineLazyGetter(this, "ConsoleUtils", function () { >+ let obj = {}; >+ Cu.import("resource:///modules/HUDService.jsm", obj); >+ return obj.ConsoleUtils; >+}) Might be nice to pull out ConsoleUtils into it's own file so we don't load the full HUDService.jsm file here.

(In reply to comment #6) > drive-by: > > >diff --git a/toolkit/content/HUDService-content.js b/toolkit/content/HUDService-content.js > > >+XPCOMUtils.defineLazyGetter(this, "ConsoleUtils", function () { > >+ let obj = {}; > >+ Cu.import("resource:///modules/HUDService.jsm", obj); > >+ return obj.ConsoleUtils; > >+}) > > Might be nice to pull out ConsoleUtils into it's own file so we don't load > the full HUDService.jsm file here. Entirely agreed. File/code organization is also something I wanted to discuss with Robert. The code in the patch is a bit convoluted right now, because the PropertyPanel needs parts of it to run in chrome process and other parts in content process. ConsoleUtils are methods are needed in both chrome and content processes, and so on. I did not do the code reorganization yet ... because that makes the patch even bigger. :) The FIXME I noted there is a reminded for me to that, a bit later in the process. Thanks for your comment! Any further comments are very much welcome!

Comment on attachment 548433 [details] [diff] [review] wip 2 fixed I'm not really sure what you want feedback on for this in-progress early-days patch. If you're asking for feedback on the approach, I think it's fine. It makes sense. You've separated the HUD into UI and content sections. There's obviously a lot to figure out still. If you have specific questions about parts of it, I'm happy to answer them if I can.

Rob: thanks for looking into the patch. The feedback request was about the general direction and about the way I am working with the jsterm evaluation result objects coming from the content processes. I also talked to dcamp about handling the GC of result objects from the content processes. The idea we settled on was that we will implement "buckets" of objects that get collected/cached in the content processes and they will be cleared/emptied when certain conditions are met in the main chrome process. For example, jsterm result objects are going to be kept, together with all of their associated objects, until the result is pruned from the Web Console output. Similarly, we will keep all the cached objects inspected in a Property Panel, and release the "bucket" of objects once the panel is closed. This keeps us away from getting into hairy lock/release mechanisms for *each and every* object we hold to from the content processes. This approach is already implemented in the wip2 patch posted here - objects are cached by a unique jsterm evaluation ID. I just didn't do any sort of GC - I was waiting for feedback from dcamp/robcee/etc, for the acceptable approach to do GC. Dave Camp has also noted that the JS debugger is going to use a similar approach. Any comments and thoughts about this topic are very much appreciated. Thank you!

Updated patch. Changes: - lots of rebasing. - added a new WebConsoleUtils.jsm that provides functions we need both in content and chrome processes. This provides clearer code separation and much lesser confusion than in the previous patch. - added a mechanism for correct initialization and uninitialization of the HUDService content process code. Properly destroy mechanism that tries to ensure no memleaks happen. - added "garbage collection" for objects evaluated and cached in the content process. - property panels work fine and automatically free objects. - JSTerm output also frees objects when they go away from the console. Things seem fine now to go forward: need to do autocomplete support, window.console API support, nsIScriptErrors and network logging. (Posted this WIP because it's usable. When I move to other parts of the code, I'll break the Web Console again :) )

Changes: - added support for autocomplete, as-you-type, in e10s mode. - finished the last thing that wasn't working in the Object Inspector: the Update button. Now it works in e10s mode. - fixed bug 583816.

Added e10s code to support window.console API messages. All messages are now displayed, including console.dir() and trace() - these two being most demanding - both required special-casing for object marshaling from the content processes to the chrome process. Luckily, the e10s changes for the Object Inspector were easily reusable here.

Fixed all of the JSTerm helper functions. They now work in e10s mode. Notable exceptions: - $0 requires that the InspectorUI has e10s mode support. - inspectstyle(element) also requires e10s support from the Style Inspector. Remaining work: - show nsIScriptErrors. - network logging for e10s. Perhaps a rewrite is needed, for the relevant code. - code cleanups. currently there's lots of debug output (dump() ftw!), lack of code comments, and there are a bunch of obsolete comments. - fix all the tests.

Added support for displaying the nsIScriptErrors from content processes, in e10s. This also brings a bug fix for the window.console API messages - better handling when different xul:browsers share the same process.

We're doing developer tool prioritization, filter on 'brontozaur' to ignore the spam.

Does this patch/feature still wanted? I spoke to Mihai quickly on IRC and he told me that there is still some work involved to finish this patch but not that much. I have started a dirty similar work in http://github.com/vingtetun/remote-web-console to be able to use the web console through a socket for debugging mobile devices and I have done some similar (but very dirty) code to support that (moving the sandbox on the content script side, serializing/deserializing DOM nodes, etc...) Having this code landed will allow B2G, Fennec and JetPack to build on top of the web console instead of having to rewrite their own. That's would be really cool.

This is the latest WIP patch. It has *many* changes since wip 7. Mainly lots of bug fixes and almost-complete network logging support. Remaining work: - complete the code update for the network panel. It only needs some changes related to the data structure that it receives from the logging code (from the content process). - deal with some of the new console.* API methods that have been added. (see TODOs/FIXMEs in the patch) - update all of the web console tests. (this is supposed to be in a separate patch) Please note that this code uses HAR-like data structures for network logging. This will allow us to easily create a function to export the network log to a HAR file (see bug 708717). This patch needs to be rebased. Last update was on the 1st of November 2011.

(In reply to Vivien Nicolas (:vingtetun) from comment #16) > Does this patch/feature still wanted? Yes. > I spoke to Mihai quickly on IRC and he told me that there is still some work > involved to finish this patch but not that much. I just attached the latest patch I have. > I have started a dirty similar work in > http://github.com/vingtetun/remote-web-console to be able to use the web > console through a socket for debugging mobile devices and I have done some > similar (but very dirty) code to support that (moving the sandbox on the > content script side, serializing/deserializing DOM nodes, etc...) That's cool stuff! > Having this code landed will allow B2G, Fennec and JetPack to build on top > of the web console instead of having to rewrite their own. That's would be > really cool. We want to finish the patch we have and land it. We are aware of the benefits of allowing the Web Console to receive async messages, to have parts of it separated from the UI. Thanks for your push for this feature!

Sonny: assigning this bug to you. Thank you very much for your interest to help us with this bug! Very much appreciated! I would like us to closely collaborate on the evolution of this patch, here on. Please rebase the patch and attach it here for a quick feedback round. Thanks! Please always ask me on IRC for any questions you might have, do not hesitate!

Very few fixes on Mihai's patch to get it working and testable. Also, I took the time to understand the code. I'm starting working on the rebasing. /me summons rebasing gods

(In reply to Sonny Piers [:sonny] from comment #20) > I'm starting working on the rebasing. The process of rebasing can be very complex. If you need any help, feel free to ping me on IRC.

wip8 rebased Only handles net activity at the moment.

Is it possible for a web page to throw CSS errors? If yes, how?

Needs to be done: Async net panels Fix several easy //TODO and //FIXME unit tests

Comment on attachment 588674 [details] [diff] [review] wip8 - very few fixes Sonny: first of all thank you very much for your work here! This is really great to see you taking this patch! I am going through the patches you've attached to the patch, to follow the code changes you did. You get f+ for this update. Thanks for your fixes!

Comment on attachment 589616 [details] [diff] [review] rebasing - WIP Review of attachment 589616 [details] [diff] [review]: ----------------------------------------------------------------- Please note that I am going through these patches in order, to remind myself what I did, and to properly follow your work. The comments below do not need to be addressed, *only* if they are still valid comments for the latest patch you have. General comments: - you are removing trailing commas when you have arrays or objects. We do not remove them to keep churn to a minimum when new items are added in subsequent patches. Overall the patch is awesome! Thanks a lot for your work! Don't worry about the comments below. I am saving them here, so I can recheck in the newer patches. ::: browser/devtools/webconsole/HUDService.jsm @@ +75,5 @@ > +XPCOMUtils.defineLazyGetter(this, "StyleInspector", function () { > + var obj = {}; > + Cu.import("resource:///modules/devtools/StyleInspector.jsm", obj); > + return obj.StyleInspector; > +}); Why do you add this here? @@ +149,1 @@ > XPCOMUtils.defineLazyGetter(this, "namesAndValuesOf", function () { Why do you keep this? @@ +249,5 @@ > "cssparser", > "exception", > "console", > "input", > + "output" Is this change needed? @@ -1551,5 @@ > if (!aAnimated || hudRef.consolePanel) { > this.disableAnimation(hudId); > } > > - chromeDocument.getElementById("Tools:WebConsole").setAttribute("checked", "true"); Why do you remove this? IIANM, this should work. @@ -1604,3 @@ > } > > - chromeDocument.getElementById("Tools:WebConsole").setAttribute("checked", "false"); Does this need to be removed? @@ +1547,5 @@ > delete this.lastFinishedRequestCallback; > > HUDWindowObserver.uninit(); > HUDConsoleObserver.uninit(); > ConsoleAPIObserver.shutdown(); This is stuff that it seems you forgot to rebase (in this patch at least). In the previous patch there are a number of changes in HS_suspend() which are not in this patch. @@ +1852,5 @@ > (aScriptError.flags & aScriptError.strictFlag)) { > severity = SEVERITY_WARNING; > } > > + let outputNode = this.hudReferences[aHUDId].outputNode; This is the HS_reportPageError() method which seems to have been incorrectly rebased. There are some issues, like undefined aHUDId. @@ -2960,5 @@ > - > - _browser.webProgress.addProgressListener(hud.progressListener, > - Ci.nsIWebProgress.NOTIFY_STATE_ALL); > - > - hud.displayCachedConsoleMessages(); This is in the HS_WindowInitializer() method. You are removing the call to hud.displayCachedConsoleMessages(). Please add a TODO to HS_activateHUDForContext() which mentions the need to rebase the displaying of cached console message. This is work we need to do before we can land the async patch. Basically what this means: when the Web Console opens the content script already receives a notification. You con do post-initialization a send of all the cached error and console messages to the main process. (either one by one, or a single message with an array of all the cached stuff) Some of the code logic related to cached messages from HUDService needs to move into the content script. Please ping me on IRC if you need further help/details. Thanks! @@ +2271,5 @@ > + let usegcli = false; > + > +//SONNY > +#ifdef MOZ_E10S_COMPAT > + // Bug 685526 - GCLI should allow basic async types (this is in the constructor of the HeadsUpDisplay objects) This code was based on a different scope of the patch. I think you can remove the ifdefs and we should make sure that gcli works on Firefox desktop. When we bring the Web Console closer to being usable with Fennec we will disable GCLI when the Web Console is connected to the remote browser. @@ -3635,5 @@ > - * displayed. > - * > - * @returns void > - */ > - displayCachedConsoleMessages: function HUD_displayCachedConsoleMessages() Don't remove this code yet. Add a FIXME so we can remember to deal with the cached console messages, before we land the async patch. @@ +3835,5 @@ > + if (this.autocompletePopup.isOpen) { > + inputUpdated = this.complete(this.COMPLETE_FORWARD); > + } > + else if (this.canCaretGoNext()) { > + inputUpdated = this.historyPeruse(HISTORY_FORWARD); I'm not sure of these changes. Will double check in your subsequent patches.

Comment on attachment 590500 [details] [diff] [review] patch rebased v0.1 Review of attachment 590500 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. I see you fixed HS_reportPageError() and the ConsoleListener. Neat! Thanks again!

Comment on attachment 590503 [details] [diff] [review] patch rebased v0.2 Review of attachment 590503 [details] [diff] [review]: ----------------------------------------------------------------- I see more fixes/cleanup wrt. GCLI and reportPageError. Good progress, thanks!

Suppose bug 685526 was fixed (GCLI should allow basic async types) would you write this code any differently? Or put it another way, do I need to raise the profile of this bug at all (it has fairly low priority right now)

(In reply to Joe Walker from comment #31) > Or put it another way, do I need to raise the profile of this bug at all (it > has fairly low priority right now) This bug would let JetPack and B2G (and potentially pdf.js in the future) to build on top of it to have a web console. Sounds important to me. Furthermore it clean up the webconsole logic by splitting processing/display and this sounds good to me too considering the size of HUDService.jsm.

(In reply to Vivien Nicolas (:vingtetun) from comment #32) > (In reply to Joe Walker from comment #31) > > Or put it another way, do I need to raise the profile of this bug at all (it > > has fairly low priority right now) > > This bug would let JetPack and B2G (and potentially pdf.js in the future) to > build on top of it to have a web console. Sounds important to me. > > Furthermore it clean up the webconsole logic by splitting processing/display > and this sounds good to me too considering the size of HUDService.jsm. I think Joe was talking about bug 685526. This bug (bug 673148) is already P1.

(In reply to Paul Rouget [:paul] from comment #33) > (In reply to Vivien Nicolas (:vingtetun) from comment #32) > > (In reply to Joe Walker from comment #31) > > > Or put it another way, do I need to raise the profile of this bug at all (it > > > has fairly low priority right now) > > > > This bug would let JetPack and B2G (and potentially pdf.js in the future) to > > build on top of it to have a web console. Sounds important to me. > > > > Furthermore it clean up the webconsole logic by splitting processing/display > > and this sounds good to me too considering the size of HUDService.jsm. > > I think Joe was talking about bug 685526. This bug (bug 673148) is already > P1. I was - thanks for the clarification Paul.

I think this patch is really big now that we'd like to keep its growth to a minimum. Making GCLI remotable should be a separate bug/patch, now that e10s is no longer a pressing issue for us. Joe, what do you think?

(In reply to Mihai Sucan [:msucan] from comment #35) > I think this patch is really big now that we'd like to keep its growth to a > minimum. > > Making GCLI remotable should be a separate bug/patch, now that e10s is no > longer a pressing issue for us. Joe, what do you think? I'm not suggesting we do all that work right now - I'm asking how much of a pain it is that GCLI doesn't do async, and offering to raise the priority of the work.

(In reply to Joe Walker from comment #36) > (In reply to Mihai Sucan [:msucan] from comment #35) > > I think this patch is really big now that we'd like to keep its growth to a > > minimum. > > > > Making GCLI remotable should be a separate bug/patch, now that e10s is no > > longer a pressing issue for us. Joe, what do you think? > > I'm not suggesting we do all that work right now - I'm asking how much of a > pain it is that GCLI doesn't do async, and offering to raise the priority of > the work. Hopefully it's not going to be painful at all to keep GCLI as is, while the rest of the Web Console becomes async. The GCLI integration work was well done (thank you!) - in the sense it did not make strong changes to existing code - it only added a separate code path for input (GCLI itself). Output can still come as it does from GCLI.

I realize I haven't posted any update for the last 2 weeks, sorry about that, I got myself more busy than I wanted to. I'll post a new revision of the patch tomorrow evening (currently rebasing it). I'm expecting to have a fully working patch at the end of the weekend. Then I'll have to review Mihai's feedback and work on the tests.

Rebased (again). Some cleanup / fixes.

Comment on attachment 596808 [details] [diff] [review] patch rebase v0.3 Review of attachment 596808 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your updated patch Sonny! Much appreciated! I looked through the code changes and I have the following comments: - Thanks for addressing some of my previous comments above! - In HUDService.jsm in the NetworkPanel code, in NP_update() you remove a FIXME from case this._DISPLAYED_REQUEST_HEADER. Is the case fixed? As far as I know that code still needs to be updated to work properly. - In makeHUDNodes() the "let self = this;" line was removed in the previous patch - correctly, because it was no longer needed. This new patch no longer makes that change. Any specific reason? - In HUDService.jsm, in HS_suspend() there are still some changes missing. I get "activityDistributor" undefined. See comment 28: the HS_suspend() changes got lost in attachment 589616 [details] [diff] [review]. - In general I am not sure of the GCLI changes you did. You have currently removed quite some code and I fear those changes will break things. Please explain them. Can we setup a meeting on TeamViewer/Skype to discuss this? Thanks a lot for your work! ::: browser/devtools/webconsole/HUDService.jsm @@ +141,3 @@ > }); > > XPCOMUtils.defineLazyGetter(this, "gConsoleStorage", function () { This is no longer used in HUDService.jsm, it will be used in HUDService-content.js. The previous patch you submitted correctly removed gConsoleStorage. @@ +5081,4 @@ > }; > > this.opts = { > + environment: { hudId: this.hudId }, Please explain these changes here. I think for GCLI you still need to pass the complete environment information (chromeDocument, contentDocument and so on), because for GCLI we are not currently aiming to do any async-related work. Please correct me if I am wrong (this is why I am asking you to explain these changes).

This patch need to be applied on top of the patch 0.3. It addresses Mihai's comments.

(In reply to Mihai Sucan [:msucan] from comment #28) > Comment on attachment 589616 [details] [diff] [review] > rebasing - WIP > > Review of attachment 589616 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please note that I am going through these patches in order, to remind myself > what I did, and to properly follow your work. The comments below do not > need to be addressed, *only* if they are still valid comments for the latest > patch you have. > > General comments: > - you are removing trailing commas when you have arrays or objects. We do > not remove them to keep churn to a minimum when new items are added in > subsequent patches. Noted! > > Overall the patch is awesome! Thanks a lot for your work! Don't worry about > the comments below. I am saving them here, so I can recheck in the newer > patches. > > ::: browser/devtools/webconsole/HUDService.jsm > @@ +75,5 @@ > > +XPCOMUtils.defineLazyGetter(this, "StyleInspector", function () { > > + var obj = {}; > > + Cu.import("resource:///modules/devtools/StyleInspector.jsm", obj); > > + return obj.StyleInspector; > > +}); > > Why do you add this here? Fixed. > > @@ +149,1 @@ > > XPCOMUtils.defineLazyGetter(this, "namesAndValuesOf", function () { > > Why do you keep this? Fixed. > > @@ +249,5 @@ > > "cssparser", > > "exception", > > "console", > > "input", > > + "output" > > Is this change needed? No, I re-added the comma. > > @@ -1551,5 @@ > > if (!aAnimated || hudRef.consolePanel) { > > this.disableAnimation(hudId); > > } > > > > - chromeDocument.getElementById("Tools:WebConsole").setAttribute("checked", "true"); > > Why do you remove this? IIANM, this should work. It has been deleted since anyway. > > @@ -1604,3 @@ > > } > > > > - chromeDocument.getElementById("Tools:WebConsole").setAttribute("checked", "false"); > > Does this need to be removed? It has been deleted since anyway. > > @@ +1547,5 @@ > > delete this.lastFinishedRequestCallback; > > > > HUDWindowObserver.uninit(); > > HUDConsoleObserver.uninit(); > > ConsoleAPIObserver.shutdown(); > > This is stuff that it seems you forgot to rebase (in this patch at least). > In the previous patch there are a number of changes in HS_suspend() which > are not in this patch. Fixed. > > @@ +1852,5 @@ > > (aScriptError.flags & aScriptError.strictFlag)) { > > severity = SEVERITY_WARNING; > > } > > > > + let outputNode = this.hudReferences[aHUDId].outputNode; > > This is the HS_reportPageError() method which seems to have been incorrectly > rebased. There are some issues, like undefined aHUDId. Fixed. > > @@ -2960,5 @@ > > - > > - _browser.webProgress.addProgressListener(hud.progressListener, > > - Ci.nsIWebProgress.NOTIFY_STATE_ALL); > > - > > - hud.displayCachedConsoleMessages(); > > This is in the HS_WindowInitializer() method. You are removing the call to > hud.displayCachedConsoleMessages(). Please add a TODO to > HS_activateHUDForContext() which mentions the need to rebase the displaying > of cached console message. This is work we need to do before we can land the > async patch. Done. > > Basically what this means: when the Web Console opens the content script > already receives a notification. You con do post-initialization a send of > all the cached error and console messages to the main process. (either one > by one, or a single message with an array of all the cached stuff) > > Some of the code logic related to cached messages from HUDService needs to > move into the content script. Please ping me on IRC if you need further > help/details. Thanks! > > @@ +2271,5 @@ > > + let usegcli = false; > > + > > +//SONNY > > +#ifdef MOZ_E10S_COMPAT > > + // Bug 685526 - GCLI should allow basic async types > > (this is in the constructor of the HeadsUpDisplay objects) > > This code was based on a different scope of the patch. I think you can > remove the ifdefs and we should make sure that gcli works on Firefox > desktop. When we bring the Web Console closer to being usable with Fennec we > will disable GCLI when the Web Console is connected to the remote browser. Done. > > @@ -3635,5 @@ > > - * displayed. > > - * > > - * @returns void > > - */ > > - displayCachedConsoleMessages: function HUD_displayCachedConsoleMessages() > > Don't remove this code yet. Add a FIXME so we can remember to deal with the > cached console messages, before we land the async patch. Done. > > @@ +3835,5 @@ > > + if (this.autocompletePopup.isOpen) { > > + inputUpdated = this.complete(this.COMPLETE_FORWARD); > > + } > > + else if (this.canCaretGoNext()) { > > + inputUpdated = this.historyPeruse(HISTORY_FORWARD); > > I'm not sure of these changes. Will double check in your subsequent patches. It was a rebasing mistake, I have reverted it.

(In reply to Mihai Sucan [:msucan] from comment #40) > Comment on attachment 596808 [details] [diff] [review] > patch rebase v0.3 > > Review of attachment 596808 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for your updated patch Sonny! Much appreciated! > > I looked through the code changes and I have the following comments: > > - Thanks for addressing some of my previous comments above! > - In HUDService.jsm in the NetworkPanel code, in NP_update() you remove a > FIXME from case this._DISPLAYED_REQUEST_HEADER. Is the case fixed? As far as > I know that code still needs to be updated to work properly. You right, fixed. > - In makeHUDNodes() the "let self = this;" line was removed in the previous > patch - correctly, because it was no longer needed. This new patch no longer > makes that change. Any specific reason? No :-) fixed. > - In HUDService.jsm, in HS_suspend() there are still some changes missing. I > get "activityDistributor" undefined. See comment 28: the HS_suspend() > changes got lost in attachment 589616 [details] [diff] [review]. Fixed. > - In general I am not sure of the GCLI changes you did. You have currently > removed quite some code and I fear those changes will break things. Please > explain them. Can we setup a meeting on TeamViewer/Skype to discuss this? > I didn't take a look at this yet, I must have messed up with rebasing. I add this comment to my todo list and will take a look at it when I'll work on GCLI. > Thanks a lot for your work! > > ::: browser/devtools/webconsole/HUDService.jsm > @@ +141,3 @@ > > }); > > > > XPCOMUtils.defineLazyGetter(this, "gConsoleStorage", function () { > > This is no longer used in HUDService.jsm, it will be used in > HUDService-content.js. The previous patch you submitted correctly removed > gConsoleStorage. Right fixed. > > @@ +5081,4 @@ > > }; > > > > this.opts = { > > + environment: { hudId: this.hudId }, > > Please explain these changes here. I think for GCLI you still need to pass > the complete environment information (chromeDocument, contentDocument and so > on), because for GCLI we are not currently aiming to do any async-related > work. Please correct me if I am wrong (this is why I am asking you to > explain these changes). You right, fixed. Thanks for your reviews!

Comment on attachment 596808 [details] [diff] [review] patch rebase v0.3 Sonny: this patch is not obsoleted by patch 0.4. You need to keep v0.3 as active, because it's needed by people when they want to apply the whole patch queue: the big patch + the smaller patches with more fixes. Thanks for your fixes and for addressing my comments!

Comment on attachment 599331 [details] [diff] [review] patch 0.4 Patch looks fine. Not much to comment on, for now.

(In reply to Mihai Sucan [:msucan] from comment #44) > Comment on attachment 596808 [details] [diff] [review] > patch rebase v0.3 > > Sonny: this patch is not obsoleted by patch 0.4. You need to keep v0.3 as > active, because it's needed by people when they want to apply the whole > patch queue: the big patch + the smaller patches with more fixes. > > Thanks for your fixes and for addressing my comments! Yes, I've made it obsolete by mistake and didn't found how to revert.

sonny, you can change the flags on a patch by going into the patch's "Details" link, clicking "Edit Details" by the patch description and then unchecking the flags.

Resuming work on this patch.

Applies on top of attachment 605083 [details] [diff] [review]. Changes: went through the network logging code to address the remaining TODOs and FIXMEs - cleanups, bug fixes, etc. Submitting incremental patches to make progress tracking easier. Will submit updated folded patches at semi-arbitrary points (eg. when I rebase, or move on to other chunks of work). (Please note that comments are always welcome!)

Made a Github repo where I am pushing all of the daily incremental changes for this patch. Follow progress at: https://github.com/mihaisucan/Web-Console-async-patch-queue

Submitting The Big Web Console async patch - for tracking/historic purposes. On March 15 I did the last update to the big patch. Compared to the previous patch this one includes more networking logging work, l10n-related work and general bug fixes. I also moved the Network Panel into its own JSM.

Proposed patch for making the window.console API async in the Web Console. Details: - This patch takes a good part of the work I did in the big patch. I did work to polish that code, add comments, address FIXMEs/TODOs and overall improve the quality. - Kept the number of changes to a minimum - to make it easier to review and land this patch soon. - Compared to the big patch: I added support for the newer window.console methods (group, groupCollapsed, groupEnd, time and timeEnd). Also added support for the cached console API messages. Beyond that, I fixed all the tests that were broken by these changes (the big patch has no work on fixing tests). - Introduced several files: - HUDService-content.js is the script that is injected into the content process. This script holds no UI, but it holds all the listeners we want to have in the Web Console content instance. For now I only included the window.console API-related code. Later on we will take the rest of listeners we have in the big async-webconsole patch. - WebConsoleUtils.jsm holds a good number of methods we use in HUDService.jsm and HUDService-content.js. It also includes methods we sometimes use in our test sute. I needed this to avoid to Cu.import() HUDService.jsm in HUDService-content.js or in other JSMs. This new JSM is meant to be shared by any local and/or remote Web Console instance. - PropertyPanelAsync.jsm holds the "new" Property Panel that works with sync and async object inspection. This wasn't the case in the big patch, but here I avoided making more changes than needed: I am not touching the code that works with the older property panel. Later, when we migrate the whole code to use the async property panel we can remove the old jsm. If you want, I can just update this patch to do that. Please let me know if you have any preferences here. (there's code in the big patch that does all this) - In HUDService.jsm I updated the code that calls helper functions to directly make the calls to the same helpers that are now in WebConsoleUtils.jsm. I also removed most of the helpers from HUDService.jsm - these changes are easy to review/skim through. There will be more stuff like this in subsequent patches - the big patch has more helpers moved into the new WebConsoleUtils.jsm. - More code cleanup/removals will come in subsequent patches - as we touch relevant parts and as soon as more parts of the existing code become obsolete (again, more stuff is waiting to be split into smaller patches from the big one). - HUDService-content.js has a "Manager" that handles which features are enabled and it handles communication between the main and content processes. We can, if we want, split "features" into smaller files if we decide that having a single big file is too much to hold all the code. - The JSTerm stuff in HUDService-content.js was required to make console.dir() work. This is actually part of the code that allows async jsterm execution. The rest of the code will come in a subsequent patch. - The tests have been updated in a minimal approach: they are not entirely async. For example jsterm execution is still sync - I only made changes for window.console calls. Later on I can do the rest. - I tried to make a "simple workflow" to update each test and came up with waitForSuccess() and using generators when things get more complicated. Please let me know if you have suggestions on that. (I don't like the function name but I did not have a better idea at that time.) That's all for now. Please let me know if you have any questions and concerns. Looking forward to your review. Thank you!

Added support for async page errors (js/css and other errors that come from the nsIConsoleService as nsIScriptErrors). All tests pass. I am looking forward to your reviews!

Comment on attachment 614414 [details] [diff] [review] part 1 - async window.console API Review of attachment 614414 [details] [diff] [review]: ----------------------------------------------------------------- Very solid overall. I want to test this in a build locally before committing to it, but I feel pretty good about it so far. Great work! Do we need or want any additional eyes on this? ::: browser/devtools/webconsole/HUDService-content.js @@ +43,5 @@ > + * ***** END LICENSE BLOCK ***** */ > + > +"use strict"; > + > +(function() { this gets appended to the browser-content script. Should probably declare that explicitly in a comment. @@ +46,5 @@ > + > +(function() { > +let Cc = Components.classes; > +let Ci = Components.interfaces; > +let Cu = Components.utils; shouldn't these be consts? @@ +51,5 @@ > + > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); > + > +let _global = Cu.getGlobalForObject(Cu); // can't use this in strict mode. So what is the outcome of this? Does it work? Fail? Either lose strict mode or figure out an alternative, I guess. @@ +69,5 @@ > +XPCOMUtils.defineLazyGetter(_global, "l10n", function () { > + return WebConsoleUtils.l10n; > +}); > + > +_global = null; Pretty strange! What are you doing? This voodoo should be explained in comments. @@ +171,5 @@ > + > + if (aMessage.cachedMessages) { > + this._sendCachedMessages(aMessage.cachedMessages); > + } > + }, this is nice. ::: browser/devtools/webconsole/HUDService.jsm @@ +4099,5 @@ > + > + // Make sure the cached evaluated object will be purged when the node is > + // removed. > + node._evalCacheId = aMessage.objectsCacheId; > + } nice! ::: browser/devtools/webconsole/PropertyPanelAsync.jsm @@ +160,5 @@ > + * The level you want to give to each property in the remote object. > + */ > + _updateRemoteObject: function PTV__updateRemoteObject(aObject, aLevel) > + { > + aObject.forEach(function(aElement) { bug 734464 ::: browser/devtools/webconsole/WebConsoleUtils.jsm @@ +184,5 @@ > + * The window object with the given outer ID. > + */ > + getWindowByOuterId: function WCU_getWindowByOuterId(aOuterId, aHintWindow) > + { > + let someWindow = aHintWindow || Services.wm.getMostRecentWindow(null); do we need a window type here? e.g., navigator:browser? @@ +209,5 @@ > + * The window object with the given inner ID. > + */ > + getWindowByInnerId: function WCU_getWindowByInnerId(aInnerId, aHintWindow) > + { > + let someWindow = aHintWindow || Services.wm.getMostRecentWindow(null); and here? navigator:browser?

Comment on attachment 614785 [details] [diff] [review] part 2 - async page errors Review of attachment 614785 [details] [diff] [review]: ----------------------------------------------------------------- Not much to say here. Does what it says on the tin. Do you want additional eyes on this? I feel pretty good about these changes. ::: browser/devtools/webconsole/HUDService-content.js @@ +752,2 @@ > Manager.init(); > })(); nice and compact changes to this file. Very clean.

(In reply to Rob Campbell [:rc] (:robcee) from comment #54) > Comment on attachment 614414 [details] [diff] [review] > part 1 - async window.console API > > Review of attachment 614414 [details] [diff] [review]: > ----------------------------------------------------------------- > > Very solid overall. I want to test this in a build locally before committing > to it, but I feel pretty good about it so far. Great work! Thank you! > Do we need or want any additional eyes on this? As discussed on IRC, I will ask Felipe for a review on this as well. > ::: browser/devtools/webconsole/HUDService-content.js > @@ +43,5 @@ > > + * ***** END LICENSE BLOCK ***** */ > > + > > +"use strict"; > > + > > +(function() { > > this gets appended to the browser-content script. Should probably declare > that explicitly in a comment. Will do! > @@ +46,5 @@ > > + > > +(function() { > > +let Cc = Components.classes; > > +let Ci = Components.interfaces; > > +let Cu = Components.utils; > > shouldn't these be consts? Good point. Will do! > @@ +51,5 @@ > > + > > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > > +Cu.import("resource://gre/modules/Services.jsm"); > > + > > +let _global = Cu.getGlobalForObject(Cu); // can't use this in strict mode. > > So what is the outcome of this? Does it work? Fail? > > Either lose strict mode or figure out an alternative, I guess. Cu.getGlobalForObject() works, but my wording in that comment is confusing. "can't use |this| in strict mode" so I had to use getGlobalForObject(). Will make this code less confusing. > ::: browser/devtools/webconsole/WebConsoleUtils.jsm > @@ +184,5 @@ > > + * The window object with the given outer ID. > > + */ > > + getWindowByOuterId: function WCU_getWindowByOuterId(aOuterId, aHintWindow) > > + { > > + let someWindow = aHintWindow || Services.wm.getMostRecentWindow(null); > > do we need a window type here? e.g., navigator:browser? Afaik it doesn't matter. The window object is just needed to get to the nsIDOMWindowUtils interface.

Rob's comments addressed. Felipe: we are looking into making the entire Web Console async and we started out with e10s support back in autumn last year. We will switch over to the remote debug protocol later. Still, we would appreciate any comments you have on this work. Can you please take a look over this patch? Thank you!

Rebased the patch.

When memory leaks occur in content scripts the nsFrameMessageManager causes a whole browser crash on exit. This patch prevents the crash. Thanks to Ms2ger for telling me how to fix this!

Rebased patch and memleaks fixed.

Rebased patch and included quite some work on fixing memleaks.

This is the interdiff of all the changes I did for parts 1 and 2, to fix memory leaks. Summary: - HUDService-content.js: - Made it no longer put any lazy getters in the global scope of content scripts. Avoid leaking any object. - In Manager.destroy() null most of the objects we have in the content script. - Track owner XUL window close and TabClose events. When these events happen the message manager no longer transmits any messages from the "main process" to the content process. The content script can't otherwise cleanup after itself, so I am breaking the "e10s" boundary here by listening for those events from the content script itself. This is a temporary solution until we switch to the remote debug protocol - and it doesn't negatively impact us at this point because we will not truly enable the content process separation (browser remote!=true). - Track private browsing changes to avoid memory leaks. - Test fixes. - Some tests leaked objects and many tests did not wait for the Web Console to open / close. - Re-enabled the web workers test in message_categories.js.

All green on try server: https://tbpl.mozilla.org/?tree=Try&rev=575efde13203

Comment on attachment 621398 [details] [diff] [review] part 1 - async window.console API - memleak fixes Review of attachment 621398 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by nits: ::: browser/devtools/webconsole/HUDService-content.js @@ +1,4 @@ > +/* -*- Mode: js2; js2-basic-offset: 2; indent-tabs-mode: nil; -*- */ > +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ > +/* ***** BEGIN LICENSE BLOCK ***** > + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 Please use the new headers: http://www.mozilla.org/MPL/headers/ @@ +74,5 @@ > +/** > + * The Web Console content instance manager. > + */ > +let Manager = { > + get "window"() content, Are those quotes necessary? @@ +129,5 @@ > + this.destroy(); > + break; > + default: { > + let handler = this._messageHandlers[aMessage.name]; > + handler && handler(aMessage.json); Is this better than an if? ::: browser/devtools/webconsole/HUDService.jsm @@ +5209,5 @@ > hud.cssNodes = {}; > > + let outputNode = hud.outputNode; > + let node; > + while (node = outputNode.firstChild) { Extra parens around assignment in a condition ::: browser/devtools/webconsole/PropertyPanelAsync.jsm @@ +250,5 @@ > + { > + if (this.getLevel(idx) == 0) { > + return -1; > + } > + for (var t = idx - 1; t >= 0; t--) { --t

Comment on attachment 621398 [details] [diff] [review] part 1 - async window.console API - memleak fixes Review of attachment 621398 [details] [diff] [review]: ----------------------------------------------------------------- I took a look at all the message manager related changes that I could find. All the comments are nits/things to keep in mind. The patch is good as is! r=me ::: browser/devtools/webconsole/HUDService-content.js @@ +52,5 @@ > + > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); > + > +XPCOMUtils.defineLazyGetter(_global, "gConsoleStorage", function () { no need to change, but if you want XPCOMUtils.defineLazyModuleGetter does this boilerplate for you @@ +68,5 @@ > +XPCOMUtils.defineLazyGetter(_global, "l10n", function () { > + return WebConsoleUtils.l10n; > +}); > + > +_global = null; is it necessary to clear _global here to avoid leaks? If yes please add a comment to that effect, and it will make it easier to remember to remove this once you move to using debugger protocol. I do remember running into weird message manager leaks in the past so I wouldn't be surprised if it's necessary. It's kinda weird that the function arg ("_global") won't be available to be used beyond this line. Maybe clearing it at the end of this function scope also works? I don't want you to spend much sweat though in figuring this out, so only go for it if it ends up being a simple change. @@ +123,5 @@ > + this.enableFeature(aMessage.json.feature, aMessage.json); > + break; > + case "WebConsole:DisableFeature": > + this.disableFeature(aMessage.json.feature); > + break; are WebConsole:EnableFeature and WebConsole:DisableFeature unused? I don't see anything setting up listeners for these two messages @@ +148,5 @@ > + * the start. For each feature you enable you can pass feature-specific > + * options in a property on the JSON object you send with the same name > + * as the feature. > + * - cachedMessages - (optional) an array of cached messages you want > + * to receive: only "ConsoleAPI" is available for now. I don't think it's necessary to mention ConsoleAPI as being the only one available. If more messages are added in the future I can see this comment going out of sync as none of the code around this function needs to change for more messages @@ +187,5 @@ > + */ > + addMessageHandler: function Manager_addMessageHandler(aName, aCallback) > + { > + if (aName in this._messageHandlers) { > + return; I think it's worth throwing or at least reporting an error here to avoid accidental callback overriding ::: browser/devtools/webconsole/HUDService.jsm @@ +3017,5 @@ > > // A cache for tracking repeated CSS Nodes. > this.cssNodes = {}; > + > + this.setupMessageManager(); I assume HeadsUpDisplay is lazily created only when the console is actually used, right? To avoid loading this frame script in pages where the console is not going to be used @@ +4085,5 @@ > + * and initializes the connection to the content script. > + * > + * @private > + */ > + setupMessageManager: function HUD_setupMessageManager() private function, should be prefixed with _ ?

Comment on attachment 621399 [details] [diff] [review] part 2 - async page error - memleak fixes Review of attachment 621399 [details] [diff] [review]: ----------------------------------------------------------------- Disregard comment about _global on previous patch, I see it was removed here ::: browser/devtools/webconsole/HUDService-content.js @@ +105,5 @@ > + xulWindow.addEventListener("unload", this._onXULWindowClose, false); > + > + let tabContainer = xulWindow.gBrowser.tabContainer; > + tabContainer.addEventListener("TabClose", this._onTabClose, false); > + i wonder if listening for both unload/TabClose could be replaced by listening to either inner-window-destroyed or outer-window-destroyed. I think this is worth investigating. Smaug: is there any clean way to clean up the frame script references when a tab is closed? @@ +410,5 @@ > + xulWindow.removeEventListener("unload", this._onXULWindowClose, false); > + let tabContainer = xulWindow.gBrowser.tabContainer; > + tabContainer.removeEventListener("TabClose", this._onTabClose, false); > + > + let hudId = this.hudId; "let hudId = this.hudId;" leftover? @@ +422,5 @@ > this.hudId = null; > this._messageHandlers = null; > + Manager = ConsoleAPIObserver = JSTerm = ConsoleListener = null; > + Cc = Ci = Cu = XPCOMUtils = Services = gConsoleStorage = > + WebConsoleUtils = l10n = null; do you think cleaning up each feature (ConsoleAPI, JSTerm, ConsoleListener) should be done on disable feature (after .destroy() is called for them) instead of here?

This makes the entire JSTerm async: JS evaluation and object inspection. All tests pass in dbg and opt builds (Mac OS and Linux). No obvious memleaks - but I'll let the try server have the final say on this. Try results: https://tbpl.mozilla.org/?tree=Try&rev=6203f61992f1

(In reply to Ms2ger from comment #64) > Comment on attachment 621398 [details] [diff] [review] > part 1 - async window.console API - memleak fixes > > Review of attachment 621398 [details] [diff] [review]: > ----------------------------------------------------------------- > > Drive-by nits: Thanks for your drive-by comments! > ::: browser/devtools/webconsole/HUDService-content.js > @@ +1,4 @@ > > +/* -*- Mode: js2; js2-basic-offset: 2; indent-tabs-mode: nil; -*- */ > > +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ > > +/* ***** BEGIN LICENSE BLOCK ***** > > + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 > > Please use the new headers: http://www.mozilla.org/MPL/headers/ Will do! > @@ +74,5 @@ > > +/** > > + * The Web Console content instance manager. > > + */ > > +let Manager = { > > + get "window"() content, > > Are those quotes necessary? No longer needed. A few months ago when I wrote the code they were needed. > @@ +129,5 @@ > > + this.destroy(); > > + break; > > + default: { > > + let handler = this._messageHandlers[aMessage.name]; > > + handler && handler(aMessage.json); > > Is this better than an if? I'm inclined to say yes for the purpose I am using it, but if you want me to change this, I can do it. Please let me know if you consider this important. > ::: browser/devtools/webconsole/HUDService.jsm > @@ +5209,5 @@ > > hud.cssNodes = {}; > > > > + let outputNode = hud.outputNode; > > + let node; > > + while (node = outputNode.firstChild) { > > Extra parens around assignment in a condition Done. Thanks again!

(In reply to Felipe Gomes (:felipe) from comment #65) > Comment on attachment 621398 [details] [diff] [review] > part 1 - async window.console API - memleak fixes > > Review of attachment 621398 [details] [diff] [review]: > ----------------------------------------------------------------- > > I took a look at all the message manager related changes that I could find. > All the comments are nits/things to keep in mind. The patch is good as is! > r=me Thank you! > ::: browser/devtools/webconsole/HUDService-content.js > @@ +52,5 @@ > > + > > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > > +Cu.import("resource://gre/modules/Services.jsm"); > > + > > +XPCOMUtils.defineLazyGetter(_global, "gConsoleStorage", function () { > > no need to change, but if you want XPCOMUtils.defineLazyModuleGetter does > this boilerplate for you Thanks for the tip! > @@ +68,5 @@ > > +XPCOMUtils.defineLazyGetter(_global, "l10n", function () { > > + return WebConsoleUtils.l10n; > > +}); > > + > > +_global = null; > > is it necessary to clear _global here to avoid leaks? If yes please add a > comment to that effect, and it will make it easier to remember to remove > this once you move to using debugger protocol. > > I do remember running into weird message manager leaks in the past so I > wouldn't be surprised if it's necessary. It's kinda weird that the function > arg ("_global") won't be available to be used beyond this line. Maybe > clearing it at the end of this function scope also works? I don't want you > to spend much sweat though in figuring this out, so only go for it if it > ends up being a simple change. Good points. Yep, I clear _global to avoid memleaks, but as you saw in the second patch I even avoid adding the JSMs to the global - I just put the objects I need in my content script scope. > @@ +123,5 @@ > > + this.enableFeature(aMessage.json.feature, aMessage.json); > > + break; > > + case "WebConsole:DisableFeature": > > + this.disableFeature(aMessage.json.feature); > > + break; > > are WebConsole:EnableFeature and WebConsole:DisableFeature unused? I don't > see anything setting up listeners for these two messages Oops, good catch. I forgot to add the listeners. Will fix. > @@ +148,5 @@ > > + * the start. For each feature you enable you can pass feature-specific > > + * options in a property on the JSON object you send with the same name > > + * as the feature. > > + * - cachedMessages - (optional) an array of cached messages you want > > + * to receive: only "ConsoleAPI" is available for now. > > I don't think it's necessary to mention ConsoleAPI as being the only one > available. If more messages are added in the future I can see this comment > going out of sync as none of the code around this function needs to change > for more messages This comment is fixed in the second patch. Please let me know if that is sufficient. > @@ +187,5 @@ > > + */ > > + addMessageHandler: function Manager_addMessageHandler(aName, aCallback) > > + { > > + if (aName in this._messageHandlers) { > > + return; > > I think it's worth throwing or at least reporting an error here to avoid > accidental callback overriding Good point. > ::: browser/devtools/webconsole/HUDService.jsm > @@ +3017,5 @@ > > > > // A cache for tracking repeated CSS Nodes. > > this.cssNodes = {}; > > + > > + this.setupMessageManager(); > > I assume HeadsUpDisplay is lazily created only when the console is actually > used, right? To avoid loading this frame script in pages where the console > is not going to be used That is correct. > @@ +4085,5 @@ > > + * and initializes the connection to the content script. > > + * > > + * @private > > + */ > > + setupMessageManager: function HUD_setupMessageManager() > > private function, should be prefixed with _ ? Sure. Thanks again for your review!

(In reply to Felipe Gomes (:felipe) from comment #66) > Comment on attachment 621399 [details] [diff] [review] > part 2 - async page error - memleak fixes > > Review of attachment 621399 [details] [diff] [review]: > ----------------------------------------------------------------- > > Disregard comment about _global on previous patch, I see it was removed here > > ::: browser/devtools/webconsole/HUDService-content.js > @@ +105,5 @@ > > + xulWindow.addEventListener("unload", this._onXULWindowClose, false); > > + > > + let tabContainer = xulWindow.gBrowser.tabContainer; > > + tabContainer.addEventListener("TabClose", this._onTabClose, false); > > + > > i wonder if listening for both unload/TabClose could be replaced by > listening to either inner-window-destroyed or outer-window-destroyed. I > think this is worth investigating. This worked well for me and it's really something we want to switch away from. We want to move the code to remote debug protocol. Do you want me to try outer-window-destroyed? > @@ +410,5 @@ > > + xulWindow.removeEventListener("unload", this._onXULWindowClose, false); > > + let tabContainer = xulWindow.gBrowser.tabContainer; > > + tabContainer.removeEventListener("TabClose", this._onTabClose, false); > > + > > + let hudId = this.hudId; > > "let hudId = this.hudId;" > leftover? Yes, good catch! > @@ +422,5 @@ > > this.hudId = null; > > this._messageHandlers = null; > > + Manager = ConsoleAPIObserver = JSTerm = ConsoleListener = null; > > + Cc = Ci = Cu = XPCOMUtils = Services = gConsoleStorage = > > + WebConsoleUtils = l10n = null; > > do you think cleaning up each feature (ConsoleAPI, JSTerm, ConsoleListener) > should be done on disable feature (after .destroy() is called for them) > instead of here? I did think of this. I do not want to remove the actual objects on DisableFeature, because I want to keep the possibility to later re-enable features. However, when the entire content script is destroyed, then it makes sense to remove all objects to avoid memleaks. Thanks again for your comments. I will update the patches ASAP.

Comment on attachment 621400 [details] [diff] [review] parts 1 and 2 - fixes for memleaks (interdiff) -const Cc = Components.classes; -const Ci = Components.interfaces; -const Cu = Components.utils; +(function _HUDServiceContent() { +let Cc = Components.classes; +let Ci = Components.interfaces; +let Cu = Components.utils; why no const? + // Need to track the owner XUL window to listen to the unload and TabClose + // events, to avoid memory leaks. + let xulWindow = this.window.QueryInterface(Ci.nsIInterfaceRequestor) + .getInterface(Ci.nsIWebNavigation) + .QueryInterface(Ci.nsIDocShell) + .chromeEventHandler.ownerDocument.defaultView; + whew, that's a lotta QI.gI.QI.cEH.oD.dV. :) and again in destroy(). You *could* make a getter for that, but I won't block you on it. + + let hudId = this.hudId; + no longer needed (thanks felipe!) beautiful work, mihai.

(In reply to Rob Campbell [:rc] (:robcee) from comment #71) > Comment on attachment 621400 [details] [diff] [review] > parts 1 and 2 - fixes for memleaks (interdiff) > > -const Cc = Components.classes; > -const Ci = Components.interfaces; > -const Cu = Components.utils; > +(function _HUDServiceContent() { > +let Cc = Components.classes; > +let Ci = Components.interfaces; > +let Cu = Components.utils; > > why no const? So I can null them out as well. Maybe I am overzealous about this... memleaks are horror. ;) > + // Need to track the owner XUL window to listen to the unload and > TabClose > + // events, to avoid memory leaks. > + let xulWindow = this.window.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIWebNavigation) > + .QueryInterface(Ci.nsIDocShell) > + .chromeEventHandler.ownerDocument.defaultView; > + > > whew, that's a lotta QI.gI.QI.cEH.oD.dV. > > :) > > and again in destroy(). You *could* make a getter for that, but I won't > block you on it. True. This is fixed in part 3. > + > + let hudId = this.hudId; > + > > no longer needed (thanks felipe!) > > beautiful work, mihai. Thank you!

Addressed review comments. Thank you Ms2ger and Felipe!

Addressed review comments. Looking forward for the complete review. Thank you!

Test fixes based on try server results and a patch rebase. Try results are green: https://tbpl.mozilla.org/?tree=Try&rev=a860faecb41f

Comment on attachment 621397 [details] [diff] [review] [in-fx-team] part 0 - fix nsFrameMessageManager crasher Landed: https://hg.mozilla.org/integration/fx-team/rev/5df471539342 Thanks for the r+!

Comment on attachment 622889 [details] [diff] [review] [in-fx-team] part 1 - async window.console API - addressed review comments https://hg.mozilla.org/integration/fx-team/rev/53d47b23009b

Comment on attachment 622890 [details] [diff] [review] [in-fx-team] part 2 - async page errors - addressed review comments https://hg.mozilla.org/integration/fx-team/rev/6d1fa0441830

https://hg.mozilla.org/mozilla-central/rev/5df471539342 https://hg.mozilla.org/mozilla-central/rev/53d47b23009b https://hg.mozilla.org/mozilla-central/rev/6d1fa0441830

There are still patches in this bug to land. It'll get RESO FIXED when they're in. Thanks Tim!

actually calling this ASSIGNED since it's still in progress.

This patch takes out the network logging code from HUDService.jsm and refactors parts of it - hopefully made it cleaner, clearer (less confusing than it was). See the reorganized code in HUDService-content.js. I also took out the network panel code and put it into its own jsm. I have a couple of concerns in the patch - please review and see comments marked as "XXX for reviewer". I am looking forward to your reviews and comments. Thanks a lot! All tests pass in opt and dbg builds (Linux and Mac OS). First try push: https://tbpl.mozilla.org/?tree=Try&rev=5e7476216895

Updated to include test fixes based on a number of try server runs. Latest try run results: https://tbpl.mozilla.org/?tree=Try&rev=18ebe789d648

As discussed, the last part for this bug. This patch includes a number of initialization changes, cleanups and fixes. I removed the need for window IDs, windowInitializer and more of the crufty code. I also removed the GCLI integration code. To this effect I removed several tests that have become in one way or another obsolete. Please let me know if you have any concerns. Looking forward to your review. Thank you! Try run results: https://tbpl.mozilla.org/?tree=Try&rev=3aa84993b51c

Comment on attachment 626408 [details] [diff] [review] part 3 - async jsterm - rebase - let xulWindow = this.window.QueryInterface(Ci.nsIInterfaceRequestor) - .getInterface(Ci.nsIWebNavigation) - .QueryInterface(Ci.nsIDocShell) - .chromeEventHandler.ownerDocument.defaultView; - + let xulWindow = this._xulWindow(); :) HUDService-content.js looks good. in HUDService.jsm: XPCOMUtils.defineLazyGetter(this, "PropertyPanel", function () { - var obj = {}; - try { - Cu.import("resource:///modules/PropertyPanel.jsm", obj); - } catch (err) { - Cu.reportError(err); - } + let obj = {}; + Cu.import("resource:///modules/PropertyPanel.jsm", obj); return obj.PropertyPanel; }); I wonder why that try was there? Oh well, glad it's leaving. /** * JSTerm * * JavaScript Terminal: creates input nodes for console code interpretation * and 'JS Workspaces' */ this comment bugs me. We should describe it as a JavaScript command line and not "JS Workspaces". probably doesn't bug me enough to change it in this patch, however. + _executeResultCallback: + function JST__executeResultCallback(aResponse, aRequest) + // Hide undefined results coming from JSTerm helper functions. + if (!errorMessage && resultString == "undefined" && + aResponse.helperResult && !aResponse.inspectable && + !aResponse.helperRawOutput) { + return; + } I think some extra line-breaks might help readability on this condition. if (!errorMessage && resultString == "undefined" && + aResponse.helperResult && !aResponse.inspectable && + !aResponse.helperRawOutput) { ... + let callback = this._receiveAutocompleteProperties.bind(this, aCallback); + _receiveAutocompleteProperties: + function JST__receiveAutocompleteProperties(aCallback, aMessage) not sure if that should be Autocomplete or AutoComplete. Don't really care, but thought I'd mention it. :) ... and it's got historical precedent (I think I commented on it before during earlier reviews). in PropertyPanel.jsm: + /** * Use this setter to update the content of the tree. * * @param object aObject no longer matches: ... */ - set data(aObject) { + set data(aData) { also, in the above comment: + * An object that holds information about the object you want to might want to say An "object descriptor" or "meta object" just to further clarify that this is not the actual object you're dealing with, but some object describing it. I guess object descriptor would be misleading as it's not actually that. in WebConsoleUtils.jsm: + let deprecated = ["width", "height", "inputEncoding"]; thank you! in namesAndValuesOf: + if (isDOMDocument && deprecated.indexOf(propName) > -1) { indexOf(propName) should probably == 0. Could you have an nsIDOMDocument that also contains arbitrary, user set properties with "width", "height" somewhere in their contents? e.g., foowidth: "barvalue". - value = aObject[propName]; - presentable = this.presentableValueFor(value); + try { + value = aObject[propName]; + presentable = this.presentableValueFor(value); + } + catch (ex) { + continue; + } looks like you got a tab in there on the catch line. in findCompletionBeginning: + // Single quoate state > ' < + case STATE_QUOTE: typo in your comment. This is great. Can land with above nits addressed.

(In reply to Rob Campbell [:rc] (:robcee) from comment #86) > Comment on attachment 626408 [details] [diff] [review] > part 3 - async jsterm - rebase > in HUDService.jsm: > > > XPCOMUtils.defineLazyGetter(this, "PropertyPanel", function () { > - var obj = {}; > - try { > - Cu.import("resource:///modules/PropertyPanel.jsm", obj); > - } catch (err) { > - Cu.reportError(err); > - } > + let obj = {}; > + Cu.import("resource:///modules/PropertyPanel.jsm", obj); > return obj.PropertyPanel; > }); > > I wonder why that try was there? Oh well, glad it's leaving. Yeah, we have a number of random try-catch "wrappers" for no obvious reasons. I'm removing them because if we have failures, we need to know about them. > /** > * JSTerm > * > * JavaScript Terminal: creates input nodes for console code interpretation > * and 'JS Workspaces' > */ > > this comment bugs me. We should describe it as a JavaScript command line and > not "JS Workspaces". > > probably doesn't bug me enough to change it in this patch, however. Will change. > + _executeResultCallback: > + function JST__executeResultCallback(aResponse, aRequest) > > + // Hide undefined results coming from JSTerm helper functions. > + if (!errorMessage && resultString == "undefined" && > + aResponse.helperResult && !aResponse.inspectable && > + !aResponse.helperRawOutput) { > + return; > + } > > I think some extra line-breaks might help readability on this condition. > > if (!errorMessage && > resultString == "undefined" && > + aResponse.helperResult && > !aResponse.inspectable && > + !aResponse.helperRawOutput) { > > ... Good point. Will change. > in PropertyPanel.jsm: > > + /** > * Use this setter to update the content of the tree. > * > * @param object aObject > > no longer matches: > ... > */ > - set data(aObject) { > + set data(aData) { Hah, good catch! Updating comments was fun! > also, in the above comment: > > + * An object that holds information about the object you want to > > might want to say An "object descriptor" or "meta object" just to further > clarify that this is not the actual object you're dealing with, but some > object describing it. > > I guess object descriptor would be misleading as it's not actually that. Going with "meta object". > in WebConsoleUtils.jsm: > > + let deprecated = ["width", "height", "inputEncoding"]; > > thank you! > > in namesAndValuesOf: > > + if (isDOMDocument && deprecated.indexOf(propName) > -1) { > > indexOf(propName) should probably == 0. Could you have an nsIDOMDocument > that also contains arbitrary, user set properties with "width", "height" > somewhere in their contents? e.g., foowidth: "barvalue". indexOf() here is not within a string, it's indexOf() within an array - I just search for the property name in the array items. 'foowidth' won't be matched. > - value = aObject[propName]; > - presentable = this.presentableValueFor(value); > + try { > + value = aObject[propName]; > + presentable = this.presentableValueFor(value); > + } > + catch (ex) { > + continue; > + } > > looks like you got a tab in there on the catch line. Thanks! > in findCompletionBeginning: > > + // Single quoate state > ' < > + case STATE_QUOTE: > > typo in your comment. +1! > This is great. Can land with above nits addressed. Thanks again for your review! Going to update the patch and land it.

Landed: https://hg.mozilla.org/integration/fx-team/rev/4eafad042c36

Comment on attachment 627161 [details] [diff] [review] [in-fx-team] part 3 - review comments addressed Sorry, backed out for turning browser_dbg_stack-04.js permaorange: https://tbpl.mozilla.org/?tree=Fx-Team&rev=4eafad042c36 { TEST-START | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_stack-04.js TEST-INFO | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_stack-04.js | Console message: [JavaScript Warning: "Use of Mutation Events is deprecated. Use MutationObserver instead." {file: "chrome://browser/content/orion.js" line: 3408}] TEST-INFO | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_stack-04.js | Console message: [JavaScript Warning: "Use of Mutation Events is deprecated. Use MutationObserver instead." {file: "chrome://browser/content/orion.js" line: 3408}] TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_stack-04.js | Should only be getting stack frames while paused. TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_stack-04.js | Should have two frames. TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_stack-04.js | All children should be frames. TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_stack-04.js | Should have no frames after resume - Got 2, expected 0 Stack trace: JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 428 JS frame :: chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_stack-04.js :: listener :: line 44 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 JS frame :: chrome://browser/content/debugger-controller.js :: DC_dispatchEvent :: line 279 JS frame :: chrome://browser/content/debugger-controller.js :: SF__afterFramesCleared :: line 457 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_stack-04.js | Should only have one child. - Got 2, expected 1 Stack trace: JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 428 JS frame :: chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_stack-04.js :: listener :: line 47 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 JS frame :: chrome://browser/content/debugger-controller.js :: DC_dispatchEvent :: line 279 JS frame :: chrome://browser/content/debugger-controller.js :: SF__afterFramesCleared :: line 457 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_stack-04.js | Should have the empty list explanation. - Got 0, expected 1 Stack trace: JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 428 JS frame :: chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_stack-04.js :: listener :: line 50 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 JS frame :: chrome://browser/content/debugger-controller.js :: DC_dispatchEvent :: line 279 JS frame :: chrome://browser/content/debugger-controller.js :: SF__afterFramesCleared :: line 457 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 INFO TEST-END | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_stack-04.js | finished in 712ms } https://hg.mozilla.org/integration/fx-team/rev/1c896e9f1949

Rebased the patch. One minor change: I added one MIME type for json. See bug 756972.

Rebased and included fixes to avoid test failures on the try servers. Green results: https://tbpl.mozilla.org/?tree=Try&rev=73b436820f12 Looking forward to your reviews! Thank you!

Comment on attachment 627161 [details] [diff] [review] [in-fx-team] part 3 - review comments addressed Re-landed together with a fix for bug 758208: https://hg.mozilla.org/integration/fx-team/rev/73a99c34a7f9

Comment on attachment 627274 [details] [diff] [review] part 5 - cleanups and fixes - rebase and test fixes Forgot to ask Joe for feedback on the GCLI removal. Joe: please take a look at the patch and see if I am not missing stuff. My main concern is with styling actually. I did not touch any of the Web Console styling code. Should I? Thank you!

Comment on attachment 627271 [details] [diff] [review] part 4 - async network logging - rebase Remove the XXX comments asking about ISO8601DateUtils.jsm as discussed in IRC.

Comment on attachment 627274 [details] [diff] [review] part 5 - cleanups and fixes - rebase and test fixes Review of attachment 627274 [details] [diff] [review]: ----------------------------------------------------------------- The twittersphere tells me robcee can't find anything wrong with your patches... Let me try :) ::: browser/devtools/webconsole/HUDService-content.js @@ +2251,4 @@ > */ > +let ConsoleProgressListener = { > + MONITOR_FILE_ACTIVITY: 1, > + MONITOR_LOCATION_CHANGE: 2, Document here what those constants are used for @@ +2266,5 @@ > + * @type boolean > + */ > + _locationChange: false, > + > + _initialized: false, No javadoc? @@ +2342,5 @@ > + > + onStateChange: > + function CPL_onStateChange(aProgress, aRequest, aState, aStatus) > + { > + if (!_alive) { Hmm, where does this variable come from? (Pre-existing) ::: browser/devtools/webconsole/HUDService.jsm @@ +2508,5 @@ > + let popupset = this.chromeDocument.getElementById("mainPopupSet"); > + let panels = popupset.querySelectorAll("panel[hudId=" + this.hudId + "]"); > + for (let i = 0; i < panels.length; i++) { > + if (panels[i] != this.consolePanel) { > + panels[i].hidePopup(); for (let panel of panels) { if (panel != this.consolePanel) { panel.hidePopup(); ? @@ +2647,5 @@ > + inputStack.appendChild(this.inputNode); > + > + let term = this.hud.makeXULNode("hbox"); > + term.setAttribute("class", "jsterm-input-container"); > + term.setAttribute("style", "direction: ltr;"); In general, the 'dir' DOM attribute is preferred over the 'direction' CSS property.

haha. Nice. :) > + > + onStateChange: > + function CPL_onStateChange(aProgress, aRequest, aState, aStatus) > + { > + if (!_alive) { > Hmm, where does this variable come from? (Pre-existing) Yes. you have to look at this in the context of the other patches. ::: browser/devtools/webconsole/HUDService.jsm @@ +2508,5 @@ > + let popupset = this.chromeDocument.getElementById("mainPopupSet"); > + let panels = popupset.querySelectorAll("panel[hudId=" + this.hudId + "]"); > + for (let i = 0; i < panels.length; i++) { > + if (panels[i] != this.consolePanel) { > + panels[i].hidePopup(); for (let panel of panels) { if (panel != this.consolePanel) { panel.hidePopup(); ? That could benefit from a little extra clarification, I think. @@ +2647,5 @@ > + inputStack.appendChild(this.inputNode); > + > + let term = this.hud.makeXULNode("hbox"); > + term.setAttribute("class", "jsterm-input-container"); > + term.setAttribute("style", "direction: ltr;"); > In general, the 'dir' DOM attribute is preferred over the 'direction' CSS property. yes! These are good suggestions. Thanks!

Updated patch to address review comments. Thank you Rob for the r+! Green try run: https://tbpl.mozilla.org/?tree=Try&rev=05048b00960e Ms2ger: thank you for your comments on the part 5 patch! I will address them once I get the review from Rob as well.

Comment on attachment 628106 [details] [diff] [review] [in-fx-team] part 4 - async network logging - address review comments Landed: https://hg.mozilla.org/integration/fx-team/rev/ebe7a997ac58

Comment on attachment 627274 [details] [diff] [review] part 5 - cleanups and fixes - rebase and test fixes Review of attachment 627274 [details] [diff] [review]: ----------------------------------------------------------------- I don't have any additions to suggest for this. If all tests pass, I'm fine with these changes. Thanks!

Joe, you're still tagged for feedback. Could you make a review pass on this as well to make sure I didn't miss anything? Thanks!

Comment on attachment 627274 [details] [diff] [review] part 5 - cleanups and fixes - rebase and test fixes Rob, Joe has already f+ed the patch today. Thank you Joe! Any comments are always welcome!

(In reply to Mihai Sucan [:msucan] from comment #102) > Comment on attachment 627274 [details] [diff] [review] > part 5 - cleanups and fixes - rebase and test fixes > > Rob, Joe has already f+ed the patch today. > > Thank you Joe! Any comments are always welcome! Just taken another pass. Nothing to add. Thanks.

(In reply to :Ms2ger from comment #96) > Comment on attachment 627274 [details] [diff] [review] > part 5 - cleanups and fixes - rebase and test fixes > > Review of attachment 627274 [details] [diff] [review]: > ----------------------------------------------------------------- > > The twittersphere tells me robcee can't find anything wrong with your > patches... Let me try :) Hehe, thanks for your comments! Much appreciated. > ::: browser/devtools/webconsole/HUDService-content.js > @@ +2251,4 @@ > > */ > > +let ConsoleProgressListener = { > > + MONITOR_FILE_ACTIVITY: 1, > > + MONITOR_LOCATION_CHANGE: 2, > > Document here what those constants are used for Will do! > @@ +2266,5 @@ > > + * @type boolean > > + */ > > + _locationChange: false, > > + > > + _initialized: false, > > No javadoc? Will add! > @@ +2342,5 @@ > > + > > + onStateChange: > > + function CPL_onStateChange(aProgress, aRequest, aState, aStatus) > > + { > > + if (!_alive) { > > Hmm, where does this variable come from? (Pre-existing) Yep, this comes from previous work related to avoiding errors after the content script unloads and memory leaks "fun". > ::: browser/devtools/webconsole/HUDService.jsm > @@ +2508,5 @@ > > + let popupset = this.chromeDocument.getElementById("mainPopupSet"); > > + let panels = popupset.querySelectorAll("panel[hudId=" + this.hudId + "]"); > > + for (let i = 0; i < panels.length; i++) { > > + if (panels[i] != this.consolePanel) { > > + panels[i].hidePopup(); > > for (let panel of panels) { > if (panel != this.consolePanel) { > panel.hidePopup(); Will do. > @@ +2647,5 @@ > > + inputStack.appendChild(this.inputNode); > > + > > + let term = this.hud.makeXULNode("hbox"); > > + term.setAttribute("class", "jsterm-input-container"); > > + term.setAttribute("style", "direction: ltr;"); > > In general, the 'dir' DOM attribute is preferred over the 'direction' CSS > property. Tried this and it doesn't work. Only style direction: ltr works. Thanks! Will update the patch and land it.

Landed: https://hg.mozilla.org/integration/fx-team/rev/1f2733c36b67 Thank you Rob, Felipe, Joe and Ms2ger for your reviews and comments!

Comment on attachment 628674 [details] [diff] [review] [in-fx-team] part 5 - cleanups and fixes - address review comments Landed in m-c as well: https://hg.mozilla.org/mozilla-central/rev/1f2733c36b67

w00t!

(In reply to Mihai Sucan [:msucan] from comment #108) > w00t! <3 Thanks for the hard work!

FWIW: I've been debugging a web console related issue (bug 748766), and the code in the tree now (not just this bug, but whatever landed the past day or two: it also seems better before this code landed today) is much more efficient at garbage collection. I was seeing up to 451 websockets created before the 1st was collected: now it's down to about 60, and the average is much less--about 20. This is true for HTTP channels, too. I have no idea if there was some GC-related work that caused this, or the recent web console work, but either way, yay!