The window registry shouldn't be keeping strong references to content windows, as this is brittle. We now have window IDs that we can use instead, so that leaks won't be as catastrophic.

mass change: filter on PRIORITYSETTING

Proposed patch. Please see bug 594523 comment #14. This patch does the following: - removes various registries from the HUDService: _headsUpDisplays, displayRegistry, windowRegistry and uriRegistry. - we still keep: activatedContext (to keep track for which notification box IDs we are open) and hudReferences (hudIds mapped to HUD objects). - adds a new windowIds object which maps outer top window IDs to hudIds. - removes the deleteHeadsUpDisplay() and displays() methods. - simplifies the registerDisplay(), unregisterDisplay(), getHudIdByWindow() methods. - keeps the getHeadsUpDisplay(), getOutputNodeById() and displaysIndex() methods *only* for compatibility with older tests. changed them to use the new internals, such that they no longer pose technical issues. If you want I can go ahead and remove these methods entirely, but that would cause more "patch noise": I'll have to update all code that uses these three methods. - removed fallback code from getHudIdByWindow() ... which no longer applies (we removed uriRegistry). - removed the getHUDIdsForScriptError() and getParents() methods, because they are no longer relevant. All these changes seem big, but they seem to have minimal impact. Only three tests failed, and this patch updates those files as needed. This cleanup should fix bug 594523 and bug 592469 as well. I believe it is much less error prone, and clearer to maintain, less issues to mix up. Finally, I'd add that this patch now makes the HUDService entirely reliant on the new nsIScriptError2, and on the outerWindowID of such messages. Things are much cleaner because of this, but it also means we must first get bugs 603706, 603750 and 606498 ready and checked-in first. Otherwise, we'll loose error/warning messages. Looking forward for your feedback! Thanks!

Comment on attachment 493248 [details] [diff] [review] proposed patch this looks nice, mihai.

Comment on attachment 493248 [details] [diff] [review] proposed patch Thanks for your quick feedback+ Robert! Asking for review from Gavin.

Asking for blocking2.0+: this patch cleans up important parts of the Web Console internals, making it less error prone, while fixing other bugs as well (594523 and 592469). Bug 594523 is also blocking2.0+. Thanks!

Comment on attachment 493248 [details] [diff] [review] proposed patch Asking for review from Shawn.

(In reply to comment #2) > - keeps the getHeadsUpDisplay(), getOutputNodeById() and displaysIndex() > methods *only* for compatibility with older tests. changed them to use the new > internals, such that they no longer pose technical issues. > > If you want I can go ahead and remove these methods entirely, but that would > cause more "patch noise": I'll have to update all code that uses these three > methods. Can you please do this in a part 2? No point in maintaining code that is only used by tests if we do not need to. > Finally, I'd add that this patch now makes the HUDService entirely reliant on > the new nsIScriptError2, and on the outerWindowID of such messages. Things are > much cleaner because of this, but it also means we must first get bugs 603706, > 603750 and 606498 ready and checked-in first. Otherwise, we'll loose > error/warning messages. Do we have an ETA on when those are going to land? You mentioned to me that this bug blocked other dev-tools work, but it seems like it's still blocked on those three bugs.

(In reply to comment #7) > (In reply to comment #2) > > - keeps the getHeadsUpDisplay(), getOutputNodeById() and displaysIndex() > > methods *only* for compatibility with older tests. changed them to use the new > > internals, such that they no longer pose technical issues. > > > > If you want I can go ahead and remove these methods entirely, but that would > > cause more "patch noise": I'll have to update all code that uses these three > > methods. > Can you please do this in a part 2? No point in maintaining code that is only > used by tests if we do not need to. Sure, I can. > > Finally, I'd add that this patch now makes the HUDService entirely reliant on > > the new nsIScriptError2, and on the outerWindowID of such messages. Things are > > much cleaner because of this, but it also means we must first get bugs 603706, > > 603750 and 606498 ready and checked-in first. Otherwise, we'll loose > > error/warning messages. > Do we have an ETA on when those are going to land? You mentioned to me that > this bug blocked other dev-tools work, but it seems like it's still blocked on > those three bugs. An ETA strongly depends on reviews bandwidth. Two of the three bugs are ready to land. The third has four patches, one has r+, and the others are very close to r+. I was given an estimate that this week I'll get the reviews.

As requested by Shawn, here's the patch that does further code cleanup. I removed the following methods: getHeadsUpDisplay(), getOutputNodeById(), displaysIndex(), getConsoleOutputNode(), getChromeWindowFromContentWindow() and getContentWindowFromHUDId().

Comment on attachment 496609 [details] [diff] [review] Part 2: cleanup Since we're doing cleanup anyway, can we fix all the methods that have overloaded argument types? E.g., clearDisplay(aHUD) where aHUD can either be a string (id) or a dom node. I would much rather see separate methods like clearDisplayForNode or similar. Maybe that's an over-refactoring and it will make this patch more complicated, but I think it'll make dealing with these methods more explicit and get rid of a lot of repeated | if (typeof aHUD... | tests around the the file. Same goes for register/unregisterDisplay and I'm sure others. - var outputNode = aHUD.querySelector(".hud-output-node"); + var outputNode = typeof aHUD == "string" ? + this.hudReferences[aHUD].outputNode : + aHUD.querySelector(".hud-output-node"); use let instead of var. (I know, we use var in a bunch of places, but we shouldn't be and I'd like to replace them) + let hudBox = outputNode; + while (!hudBox.classList.contains("hud-box")) { + hudBox = hudBox.parentNode; + } + + hudBox.lastTimestamp = 0; This seems like it should be a separate method. Maybe resetTimestamp(outputNode) ? Other than that, not much to say. I skimmed the modified tests and the changes look reasonable. Lots fewer lines! (oh, and presumably these patches will have to be rebased)

Comment on attachment 493248 [details] [diff] [review] proposed patch >+++ b/toolkit/components/console/hudservice/HUDService.jsm >+ registerDisplay: function HS_registerDisplay(aHUDId) > { >+ // register a display DOM node Id with the service nit: use complete sentences with punctuation please > observe: function HCO_observe(aSubject, aTopic, aData) > { > if (aTopic == "xpcom-shutdown") { > this.uninit(); > return; > } > >+ if (!(aSubject instanceof Ci.nsIScriptError) || >+ !(aSubject instanceof Ci.nsIScriptError2) || >+ !aSubject.outerWindowID) { Are we ever going to get something that is an nsIScriptError now? I think we should log an error to the error console if aSubject is not an nsIScriptError2 object at this point so we can track down any cases that come up. r=sdwilsh

(In reply to comment #11) > nit: use complete sentences with punctuation please Nit: use complete sentences with punctuation please.

(In reply to comment #12) > Nit: use complete sentences with punctuation please. I'm glad you noticed that I have a higher grammar standard for comments in code than I do for review comments. :D

(In reply to comment #11) > Comment on attachment 493248 [details] [diff] [review] > proposed patch > > >+++ b/toolkit/components/console/hudservice/HUDService.jsm > >+ registerDisplay: function HS_registerDisplay(aHUDId) > > { > >+ // register a display DOM node Id with the service > nit: use complete sentences with punctuation please Will update the patch. :) > > observe: function HCO_observe(aSubject, aTopic, aData) > > { > > if (aTopic == "xpcom-shutdown") { > > this.uninit(); > > return; > > } > > > >+ if (!(aSubject instanceof Ci.nsIScriptError) || > >+ !(aSubject instanceof Ci.nsIScriptError2) || > >+ !aSubject.outerWindowID) { > Are we ever going to get something that is an nsIScriptError now? I think we > should log an error to the error console if aSubject is not an nsIScriptError2 > object at this point so we can track down any cases that come up. You have a point, but I believe this would spam the error console too much. Whenever an nsIScriptError is shown, we'd also issue a warning about it. Not nice. We still have cases when we receive nsIScriptErrors (not 2s), when those cases are chrome-related, or when they are "not-so-interesting" messages. What I did in the patches that landed is an exhaustive search for cases we are interested into - web page content errors. I did include only a few cases related to chrome code. We also still have nsIConsoleMessages, which are not all converted to nsIScriptError2. At the moment, I believe we should leave that as is. Or is your suggestion a required change for this patch? I can do it, if you want. Thanks for your review+!

(In reply to comment #14) > At the moment, I believe we should leave that as is. Or is your suggestion a > required change for this patch? I can do it, if you want. Naw, I'm not sure it is worth it at this point.

Updated the patch to fix the comment. No rebase needed, still applies cleanup. This is ready for checkin.

Addressed the feedback comments from Robert. Thanks for the feedback+! Please note that the solution for register/clearDisplay() was a simple fix, not many lines of patches to clean that up. Also, the problem with reset timestamp is "wider": jsterm.clearOutput() is a duplicate of HUDService.clearDisplay(). I aliased HUDService.clearDisplay() to clearOutput().

Comment on attachment 499014 [details] [diff] [review] Part 2: more cleanup Canceling review request. We are going to move this code into a separate bug.

with above patch rebased to apply ontop of styling code patch. (attaching rebased patch after this)

Fixed the patch rebase. No test failures here.

Comment on attachment 499529 [details] [diff] [review] [checked-in] rebased patch http://hg.mozilla.org/mozilla-central/rev/babc86ced878