Last Comment Bug 612658 - Implement ConsoleAPIStorage - add caching for the window.console API
: Implement ConsoleAPIStorage - add caching for the window.console API
Status: RESOLVED FIXED
[console-1][fixed-in-fx-team]
: dev-doc-needed
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 All
: P1 normal (vote)
: Firefox 9
Assigned To: Mihai Sucan [:msucan]
:
Mentors:
: 602199 (view as bug list)
Depends on: 728538
Blocks: devtools4 609890 611032 658368
  Show dependency treegraph
 
Reported: 2010-11-16 12:21 PST by David Dahl :ddahl
Modified: 2012-02-18 02:25 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
v 0.1 ConsoleStorageService (13.94 KB, patch)
2010-11-16 18:34 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.2 reduced noisyness (13.94 KB, patch)
2010-12-03 14:33 PST, David Dahl :ddahl
gavin.sharp: review-
Details | Diff | Splinter Review
v 0.3 Review comments adressed (17.55 KB, patch)
2010-12-10 13:57 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.4 comments addressed (17.88 KB, patch)
2010-12-10 14:11 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.4.1 comments addressed (17.73 KB, patch)
2010-12-10 15:16 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.5 Combined all privateBrowsing (22.29 KB, patch)
2010-12-15 19:29 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.5.1 moved test to toolkit, some tweaks (17.88 KB, patch)
2010-12-20 15:07 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.5.1 moved test to toolkit, some tweaks (23.22 KB, patch)
2010-12-20 15:09 PST, David Dahl :ddahl
gavin.sharp: review-
Details | Diff | Splinter Review
v 0.5.2 Comments adressed, nasty leak (22.29 KB, patch)
2010-12-21 20:15 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.6 de-xpcomified - still leaks - tests pass (21.17 KB, patch)
2010-12-22 12:00 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.6.1 forgot to remove idl (18.41 KB, patch)
2010-12-22 12:02 PST, David Dahl :ddahl
gavin.sharp: review-
Details | Diff | Splinter Review
v 0.7 comments addressed, leaks gone! (16.87 KB, patch)
2010-12-22 15:52 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.7.1 comments addressed, leaks gone! (16.81 KB, patch)
2010-12-22 15:59 PST, David Dahl :ddahl
gavin.sharp: review-
Details | Diff | Splinter Review
v 0.7.2 Comments addressed, new tests added (17.21 KB, patch)
2011-01-05 22:02 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
0.7.3 unbitrot (16.61 KB, patch)
2011-03-28 16:06 PDT, David Dahl :ddahl
gavin.sharp: review-
Details | Diff | Splinter Review
0.8 Addressed gavin's comments (17.32 KB, patch)
2011-05-11 14:15 PDT, David Dahl :ddahl
gavin.sharp: review-
Details | Diff | Splinter Review
v 0.9 Fixed test and observer fixes (16.84 KB, patch)
2011-05-12 15:40 PDT, David Dahl :ddahl
gavin.sharp: review-
Details | Diff | Splinter Review
v 0.9.1 comments addressed (16.76 KB, patch)
2011-05-25 13:49 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.10 changed window used by garbageCollect() (16.49 KB, patch)
2011-05-25 14:05 PDT, David Dahl :ddahl
gavin.sharp: review-
Details | Diff | Splinter Review
v 0.11 Strange behavior with PB mode (15.88 KB, patch)
2011-06-08 13:48 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
modified test (2.27 KB, text/plain)
2011-06-08 14:41 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details
v 0.12 comments addressed (16.03 KB, patch)
2011-06-08 16:13 PDT, David Dahl :ddahl
gavin.sharp: review-
Details | Diff | Splinter Review
v 0.13 comments addressed (16.00 KB, patch)
2011-06-09 20:34 PDT, David Dahl :ddahl
gavin.sharp: review+
Details | Diff | Splinter Review
v 0.14 Comments addressed (15.74 KB, patch)
2011-06-15 14:53 PDT, David Dahl :ddahl
bugzilla: review+
Details | Diff | Splinter Review
v 0.15 fix for iframes (16.67 KB, patch)
2011-07-06 12:59 PDT, Mihai Sucan [:msucan]
gavin.sharp: review-
Details | Diff | Splinter Review
v 0.14b revert (16.41 KB, patch)
2011-07-08 10:32 PDT, Mihai Sucan [:msucan]
bzbarsky: superreview-
Details | Diff | Splinter Review
v 0.15 improved comments (17.75 KB, patch)
2011-07-11 13:22 PDT, Mihai Sucan [:msucan]
bzbarsky: superreview+
Details | Diff | Splinter Review
v 0.16 (17.76 KB, patch)
2011-07-23 07:28 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
v 0.17 rebase (17.99 KB, patch)
2011-08-04 05:35 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
[checked-in] v 0.18 rebase (18.07 KB, patch)
2011-08-25 04:48 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review

Description David Dahl :ddahl 2010-11-16 12:21:37 PST
ConsoleStorageService caches all messages that eventually are displayed in each WebConsole. If the WebConsole UI is displayed, the messages will be cached and displayed. If not, the messages are cached - to be potentially displayed when the UI is opened.
Comment 1 Kevin Dangoor 2010-11-16 18:31:44 PST
I think this is already covered by bug 601260 and bug 609890?
Comment 2 David Dahl :ddahl 2010-11-16 18:34:41 PST
Created attachment 491074 [details] [diff] [review]
v 0.1 ConsoleStorageService
Comment 3 Kevin Dangoor 2010-11-22 11:14:33 PST
mass change: filter on PRIORITYSETTING
Comment 4 Kevin Dangoor 2010-11-22 11:32:17 PST
mass change: filter on PRIORITYSETTING
Comment 5 David Dahl :ddahl 2010-11-23 08:08:21 PST
needs to block as this patch will allow for caching errors and console api calls regardless of having a Web Console UI visible
Comment 6 Dietrich Ayala (:dietrich) 2010-11-30 08:21:56 PST
Blocking. There's another bug that's a dupe and blocker, that David is going to close.
Comment 7 David Dahl :ddahl 2010-12-01 14:38:29 PST
Gavin:

Would love to get the review process going on this and it's related bugs: bug 609890 and bug 602199. Let me know if you won't be able to get to it any time soon, so I can find another reviewer.
Comment 8 David Dahl :ddahl 2010-12-03 14:33:57 PST
Created attachment 495121 [details] [diff] [review]
v 0.2 reduced noisyness

removed where we throw when storage not found. too noisy.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-12-08 18:52:30 PST
Comment on attachment 495121 [details] [diff] [review]
v 0.2 reduced noisyness

>diff --git a/dom/base/ConsoleStorageService.idl b/dom/base/ConsoleStorageService.idl

>+interface nsIConsoleStorageService : nsISupports

>+  nsIVariant createConsoleStorage(in AString aId);
>+  void removeConsoleStorage(in AString aId);
>+  void recordEvent(in AString aId, in nsIVariant aEvent);
>+  void recordGlobalEvent(in nsIVariant aEvent);
>+  nsIVariant getConsoleStorage(in AString aId);  
>+  nsIVariant getGlobalConsoleStorage();

This looks more complicated than I was expecting... can we have something more like:

void recordEvent(aWindowID, aEvent);
nsIVariant getCachedEvents(aWindowID, aEvent);

I'm not sure what the separate global storage is intended to be used for, since I don't see any use of it in the blocked bug.

>diff --git a/dom/base/ConsoleStorageService.js b/dom/base/ConsoleStorageService.js

>+ConsoleStorageService.prototype = {

>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIConsoleStorageService]),

Missing nsIObserver?

>+  getInterfaces: function CS_getInterfaces(countRef)
>+  {
>+    var interfaces = [Ci.nsIConsoleStorageService, Ci.nsIObserver,];
>+    countRef.value = interfaces.length;
>+    return interfaces;
>+  },

This isn't necessary - this is a nsIDOMClassInfo method, and this component doesn't implement that interface (and doesn't need to).

>+  recordEvent: function CS_recordEvent(aId, aEvent)

Seems like this method should be a no-op if we're in private browsing mode.

>+  truncateConsoleStorage: function CS_truncateConsoleStorage(aId)

This is fine for preventing individual buckets from getting too big, but there's no limit on the number of buckets, right? Nor does there seem to be anything that evicts the contents of the cache when windows are destroyed, which seems problematic. Perhaps we can address that by adding a notification similar to content-document-global-created but for destruction, and use that to trigger the evicting.

Another potential issue is that the window IDs we use are outer window IDs, so entirely different documents/domains loaded in the same tab will share a cache. It seems like it would be an odd experience to open the console on a page and potentially end up seeing a bunch of messages from previous unrelated pages that have been loaded in that tab... For this use case it seems like we actually want inner window IDs, which I suppose we could do by having the ConsoleAPI object pass both in its event object (using nsIDOMWindowUtils::GetCurrentInnerWindowID).
Comment 10 Rob Campbell [:rc] (:robcee) 2010-12-09 09:59:15 PST
That's a really good point, re: outer window.IDs as our cache. Presumably we'll need both outer and inner to keep events stored across reloads. Or will we? I guess we only care in that case if the console is opened, and it should be able to keep the previous contents before the reload anyway.

content-document-global-destroyed sounds like a good idea for doing this. Would be cleaner than using a ProgressListener to watch the document.
Comment 11 David Dahl :ddahl 2010-12-09 10:18:42 PST
(In reply to comment #10)
> That's a really good point, re: outer window.IDs as our cache. Presumably we'll
> need both outer and inner to keep events stored across reloads. Or will we? I
> guess we only care in that case if the console is opened, and it should be able
> to keep the previous contents before the reload anyway.
> 

I'm not sure this matters at all. it really is all about the tab, not so much the window. Isn't it true that a new inner id will be generated for a new inner window at the same URL? Web devs will be reloading the same page over and over to debug stuff. Won't this just complicate the way we display logged data/events?

> content-document-global-destroyed sounds like a good idea for doing this. Would
> be cleaner than using a ProgressListener to watch the document.

indeed, this will be a clean way to jettison old storage data.
Comment 12 Rob Campbell [:rc] (:robcee) 2010-12-09 10:41:19 PST
(In reply to comment #11)
 
> I'm not sure this matters at all. it really is all about the tab, not so much
> the window. Isn't it true that a new inner id will be generated for a new inner
> window at the same URL? Web devs will be reloading the same page over and over
> to debug stuff. Won't this just complicate the way we display logged
> data/events?

Yeah, a reload replaces the inner window object with a new one as does any other type of navigation. The outer window associated with the tab seems to make sense, though it might be difficult to isolate chatter in the log across page load boundaries.

(maybe a side-note for a follow-up bug to add separators to the console on navigation or reload)

What do you think, Gavin? We could still store the inner window id, but collecting for the outer window still seems to make sense to me. The tab is the focus of the user, not the transient objects underneath it.

> > content-document-global-destroyed sounds like a good idea for doing this. Would
> > be cleaner than using a ProgressListener to watch the document.
> 
> indeed, this will be a clean way to jettison old storage data.
Comment 13 David Dahl :ddahl 2010-12-09 11:41:14 PST
(In reply to comment #9)
> Comment on attachment 495121 [details] [diff] [review]
> v 0.2 reduced noisyness
> 
> >diff --git a/dom/base/ConsoleStorageService.idl b/dom/base/ConsoleStorageService.idl
> 
> >+interface nsIConsoleStorageService : nsISupports
> 
> >+  nsIVariant createConsoleStorage(in AString aId);
> >+  void removeConsoleStorage(in AString aId);
> >+  void recordEvent(in AString aId, in nsIVariant aEvent);
> >+  void recordGlobalEvent(in nsIVariant aEvent);
> >+  nsIVariant getConsoleStorage(in AString aId);  
> >+  nsIVariant getGlobalConsoleStorage();
> 
> This looks more complicated than I was expecting... can we have something more
> like:
> 
> void recordEvent(aWindowID, aEvent);
> nsIVariant getCachedEvents(aWindowID, aEvent);
> 
OK. sounds good.

> I'm not sure what the separate global storage is intended to be used for, since
> I don't see any use of it in the blocked bug.


I can remove it - I was anticipating using it for what will be vaporware for some time to come:)
Comment 14 David Dahl :ddahl 2010-12-09 12:07:56 PST
(In reply to comment #9)
> 
> Seems like this method should be a no-op if we're in private browsing mode.
> 
I have another bug that deals with private browsing, can I attend to this change there? see bug 602199: 

> >+  truncateConsoleStorage: function CS_truncateConsoleStorage(aId)
> Perhaps we can address that by adding a notification
> similar to content-document-global-created but for destruction, and use that to
> trigger the evicting.
ok
Comment 15 David Dahl :ddahl 2010-12-10 13:57:52 PST
Created attachment 496917 [details] [diff] [review]
v 0.3 Review comments adressed

I am using a call to notify observers ("console-storage-cache-event") that should not be used, as the test is the only thing listening. What is the better way of doing this?
Comment 16 David Dahl :ddahl 2010-12-10 14:01:12 PST
(In reply to comment #15)
> Created attachment 496917 [details] [diff] [review]
> v 0.3 Review comments adressed
I removed the xpcshell test and am using a browser chrome test now because of the storage IDL changes - the test relied on previously public API calls. I still need one more test that exhaustively calls the API.
Comment 17 David Dahl :ddahl 2010-12-10 14:11:16 PST
Created attachment 496923 [details] [diff] [review]
v 0.4 comments addressed

added to test so all api methods are called
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-12-10 14:38:38 PST
The cache is only used to populate the console on opening. It seems to me that we really only want to do that for the current child inner windows, rather than all of the inner windows that have ever been associated with the outer window (tab). Storing using the inner window ID as the key and properly evicting the cached data for destroyed windows will do that automatically. You can still persist data across reloads while the console is open, because that doesn't depend on the cache.

Private browsing support isn't hard to add (single call to isPrivateBrowsingEnabled), so it shouldn't be deferred to a followup bug - let's just do it right the first time.
Comment 19 David Dahl :ddahl 2010-12-10 15:12:59 PST
(In reply to comment #18)
> The cache is only used to populate the console on opening. It seems to me that
> we really only want to do that for the current child inner windows, rather than
> all of the inner windows that have ever been associated with the outer window
> (tab).

I think this will break from what might be expected from many developers (me included), as firebug keeps old logging data until you clear it or you hit a "MAX" limit. Also the Error Console keeps old data around until you hit 300 log messages.

> 
> Private browsing support isn't hard to add (single call to
> isPrivateBrowsingEnabled), so it shouldn't be deferred to a followup bug -
> let's just do it right the first time.

Right. I added it after all. (as far as recordEvent being a noop). I have another patch that clears the entire storage object when leaving PB mode.
Comment 20 David Dahl :ddahl 2010-12-10 15:16:29 PST
Created attachment 496963 [details] [diff] [review]
v 0.4.1 comments addressed

removed unused IDL method
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-12-10 16:06:03 PST
(In reply to comment #19)
> I think this will break from what might be expected from many developers (me
> included), as firebug keeps old logging data until you clear it or you hit a
> "MAX" limit. Also the Error Console keeps old data around until you hit 300 log
> messages.

Why would you want the web console to show output from previous pages *when you open it*? I'm not suggesting we need to clear output while it's open, but only that we discard data generated by now-dead documents while the console is closed. If you're debugging a page and reloading it a lot, you're going to have the console open all the time, so our behavior here won't matter.

I don't think the old error console's behavior is particularly relevant. The nature of its output is slightly different (warnings generated by us running/loading the page rather than the page itself), and it's global, rather than tab specific, so it really has no other choice than to just show everything and act like a dumb buffer.

We currently don't even support showing exceptions thrown prior to the web console being opened for the *current page*, so my proposed behavior is still a net win over what we currently do for other types of console output.
Comment 22 David Dahl :ddahl 2010-12-10 16:23:17 PST
(In reply to comment #21)
> Why would you want the web console to show output from previous pages *when you
> open it*? I'm not suggesting we need to clear output while it's open, but only
> that we discard data generated by now-dead documents while the console is
> closed. If you're debugging a page and reloading it a lot, you're going to have
> the console open all the time, so our behavior here won't matter.

OK, I hear ya. Just playing devil's advocate. I can see a bug being filed over and over again for this, but it really is not that big a deal.
Comment 23 Rob Campbell [:rc] (:robcee) 2010-12-12 08:59:24 PST
it's a good advocate to play, David.

I was thinking the same thing regarding stashing of previous inner window messages, but on further persuasion by Gavin, I think making a clean break and just giving contents of the current inner window on console activation is a good idea, will give us cleaner results and probably make the console snappier to boot.
Comment 24 David Dahl :ddahl 2010-12-15 11:34:16 PST
(In reply to comment #23)
> it's a good advocate to play, David.
> 
> I was thinking the same thing regarding stashing of previous inner window
> messages, but on further persuasion by Gavin, I think making a clean break and
> just giving contents of the current inner window on console activation is a
> good idea, will give us cleaner results and probably make the console snappier
> to boot.

I just confirmed with Jonas that document-content-global-created is synchronous and a safe place to empty the storage array before the new window loads a document, script or css.

So, it will be simpler to empty the cache on window create, ignoring completely the innerWindow ids, etc...
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-12-15 13:07:10 PST
*** Bug 602199 has been marked as a duplicate of this bug. ***
Comment 26 David Dahl :ddahl 2010-12-15 19:29:47 PST
Created attachment 497997 [details] [diff] [review]
v 0.5 Combined all privateBrowsing

There is an additional observer topic that is used in this patch, which is the wrong way to handle the events in the tests. I would love some pointers on how to do it correctly.
Comment 27 David Dahl :ddahl 2010-12-20 15:07:49 PST
Created attachment 498872 [details] [diff] [review]
v 0.5.1 moved test to toolkit, some tweaks
Comment 28 David Dahl :ddahl 2010-12-20 15:09:38 PST
Created attachment 498873 [details] [diff] [review]
v 0.5.1 moved test to toolkit, some tweaks

forgot to hg add a file
Comment 29 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-12-21 14:42:10 PST
Comment on attachment 498873 [details] [diff] [review]
v 0.5.1 moved test to toolkit, some tweaks

>diff --git a/content/base/test/file_htmlserializer_2.html b/content/base/test/file_htmlserializer_2.html
>index 2156b1610c6a19d841d75059b31936f2237ac25e..99973da63bad131a6277d481a5eec484f3dc4a69
>GIT binary patch

Looks like this somehow got included by mistake.

>diff --git a/dom/base/ConsoleStorageService.js b/dom/base/ConsoleStorageService.js

>+XPCOMUtils.defineLazyServiceGetter(this, "gPrivBrowsing",
>+                                   "@mozilla.org/privatebrowsing;1",
>+                                   "nsIPrivateBrowsingService");

This needs to be a soft dependency (private browsing service isn't part of Gecko so might not exist), so I don't think you can use defineLazyServiceGetter. You probably need to use a normal defineLazyGetter that puts the getService call in a try/catch and returns null if it fails, and then null check gPrivBrowsing everywhere it's used.

>+function ConsoleStorageService()

>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIConsoleStorageService,
>+                                         Ci.nsIObserver,
>+                                         Ci.nsISupports]),

Don't need to include nsISupports in argument passed to generateQI.

>+  getInterfaces: function CS_getInterfaces(countRef)

Don't need this, this is a nsIClassInfo method and you don't implement that.

>+  observe: function CS_observe(aSubject, aTopic, aData)

>+    else if (aTopic == "private-browsing") {
>+      if (aData == "exit") {
>+        this.resetStorage();

I think this isn't needed, given that you don't record data while in PB mode.

>+  createConsoleStorage: function CS_createConsoleStorage(aId)

This doesn't seem worth splitting out - just inline it where it's currently called in recordEvent?

>+  removeConsoleStorage: function CS_removeConsoleStorage(aId)

This isn't on the interface and has no caller.

>+  /**
>+   * Record an event to the storage array specified

How about "Record an event associated with the given window ID" (same re-wording applies to the interface docs - "storage array" as a term seems like an implementation detail that's best avoided).

>+   * @param string aId
>+   *        The id of the storage array

"windowID, the ID of the outer window for which the event occurred."

>+  recordEvent: function CS_recordEvent(aId, aEvent)
>+  {
>+    if (gPrivBrowsing.privateBrowsingEnabled) {
>+      this.resetStorage();

I don't see why you'd want to resetStorage here.

>+    aEvent.timestamp = Date.now();

This doesn't seem to be used? Not sure it's a good idea in general to modify the passed-in event object.

>+  truncateConsoleStorage: function CS_truncateConsoleStorage(aId)

>+  deleteStorage: function CS_DeleteStorage()

These also seems like they would be simpler to just have inline.

>+  removeStorage: function CS_removeStorage(aId)

Hmm, I think we still need a better solution to clearing out data for closed windows that doesn't rely on the HUDService calling removeStorage. Perhaps we do need that outer-window-destroyed notification after all.

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>   uninit: function HWO_uninit()
>   {
>-    Services.obs.removeObserver(this, "content-document-global-created");

This looks unrelated?
Comment 30 David Dahl :ddahl 2010-12-21 17:00:00 PST
(In reply to comment #29)

> >diff --git a/content/base/test/file_htmlserializer_2.html b/content/base/test/file_htmlserializer_2.html
> >index 2156b1610c6a19d841d75059b31936f2237ac25e..99973da63bad131a6277d481a5eec484f3dc4a69
> >GIT binary patch
> 
> Looks like this somehow got included by mistake.

Yep. hg confused. will edit patch after qref

> >diff --git a/dom/base/ConsoleStorageService.js b/dom/base/ConsoleStorageService.js

> This needs to be a soft dependency (private browsing service isn't part of
> Gecko so might not exist), so I don't think you can use
> defineLazyServiceGetter. 
OK. 10 - 4.

> >+    else if (aTopic == "private-browsing") {
> >+      if (aData == "exit") {
> >+        this.resetStorage();
> 
> I think this isn't needed, given that you don't record data while in PB mode.

I was being too aggressive on that front.

> >+  recordEvent: function CS_recordEvent(aId, aEvent)
> >+  {
> >+    if (gPrivBrowsing.privateBrowsingEnabled) {
> >+      this.resetStorage();
> 
> I don't see why you'd want to resetStorage here.

right. too much data cleaning again.

> >+  truncateConsoleStorage: function CS_truncateConsoleStorage(aId)
> 
> >+  deleteStorage: function CS_DeleteStorage()
> 
> These also seems like they would be simpler to just have inline.

Check.

> 
> >+  removeStorage: function CS_removeStorage(aId)
> 
> Hmm, I think we still need a better solution to clearing out data for closed
> windows that doesn't rely on the HUDService calling removeStorage. Perhaps we
> do need that outer-window-destroyed notification after all.

Cool. so simple. 

> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >   uninit: function HWO_uninit()
> >   {
> >-    Services.obs.removeObserver(this, "content-document-global-created");
> 
> This looks unrelated?

i'll add it back if it does not cause anymore asserts - it seems like we were trying to remove observers that were already removed.
Comment 31 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-12-21 17:16:21 PST
(In reply to comment #30)
> i'll add it back if it does not cause anymore asserts - it seems like we were
> trying to remove observers that were already removed.

Looks like Josh filed that as bug 620832.
Comment 32 David Dahl :ddahl 2010-12-21 20:15:21 PST
Created attachment 499226 [details] [diff] [review]
v 0.5.2 Comments adressed, nasty leak

More of a feedback request... Trying to figure out a nasty leak of at least a DOMWindow, maybe you can pinpoint it? I'll keep looking at it as well.
Comment 33 David Dahl :ddahl 2010-12-22 12:00:42 PST
Created attachment 499348 [details] [diff] [review]
v 0.6 de-xpcomified - still leaks - tests pass
Comment 34 David Dahl :ddahl 2010-12-22 12:02:40 PST
Created attachment 499349 [details] [diff] [review]
v 0.6.1 forgot to remove idl
Comment 35 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-12-22 13:52:28 PST
Comment on attachment 499349 [details] [diff] [review]
v 0.6.1 forgot to remove idl

>diff --git a/dom/base/ConsoleAPI.js b/dom/base/ConsoleAPI.js

>+XPCOMUtils.defineLazyGetter(this, "gConsoleStorage", function (){
>+  Cu.import("resource://gre/modules/ConsoleStorageService.jsm");
>+  return consoleStorageService;
>+});

Need to use the object scoping variant of import() to avoid polluting the global scope:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#332

>diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm

>+var EXPORTED_SYMBOLS = ["consoleStorageService"];

ConsoleStorageService capitalization is more conventional for module symbols, I think, so switch them around?

>+function ConsoleStorageService()
>+{
>+  Services.obs.addObserver(this, "xpcom-shutdown", false);
>+  Services.obs.addObserver(this, "content-document-global-created", false);
>+  Services.obs.addObserver(this, "private-browsing", false);

no longer needed

>+  observe: function CS_observe(aSubject, aTopic, aData)

>+    else if (aTopic == "content-document-global-created") {
>+      // need to clear out previously-loaded window's storage:
>+      let windowID  = aSubject.QueryInterface(Ci.nsIInterfaceRequestor).
>+                        getInterface(Ci.nsIDOMWindowUtils).outerWindowID;
>+      this.consoleStorage[windowID] = [];

delete this.consoleStorage[windowID]? Might not exist, in which case you don't really want to set it.

>+    else if (aTopic == "outer-window-destroyed") {
>+      try {
>+        // aSubject will sometimes throw NS_NOINTERFACE here
>+        // They are windows we are uninterested in
>+        let windowID  = aSubject.QueryInterface(Ci.nsIInterfaceRequestor).
>+                          getInterface(Ci.nsIDOMWindowUtils).outerWindowID;
>+        this.removeStorage(windowID);

This topic actually only gets the ID, as a nsISupportsPRUint64 (see http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#6374 ).

So I believe you want just windowID = aSubject.QueryInterface(Ci.nsISupportsPRUint64)

>+  getCachedEvents: function CS_getCachedEvents(aId)

>+    // TODO: report info message here "No storage for Window ID blah"

Don't think this is needed - it's not really an exceptional case for there not to be anything stored for a given window ID.

>+  recordEvent: function CS_recordEvent(aWindowID, aEvent)
>+  {
>+    if (gPrivBrowsing) {
>+      if (gPrivBrowsing.privateBrowsingEnabled) {

Collapse these into one line

>+    var storage = this.getCachedEvents(aWindowID);
>+    storage.push(aEvent);
>+
>+    // truncate
>+    var maxLength, toRemove;
>+    var currentStorage = this.consoleStorage[aWindowID];
>+    var len = currentStorage.length;
>+
>+    if (len > STORAGE_MAX_EVENTS) {
>+      toRemove = (len - STORAGE_MAX_EVENTS);
>+      currentStorage.splice(0, toRemove);
>+    }

currentStorage and storage refer to the same array here, so this can be simplified. There's no real benefit to accessing consoleStorage via getCachedEvents, so AFAICT this could just be:

storage.push(aEvent);
if (storage.length > STORAGE_MAX_EVENTS)
  storage.shift();

>+  removeStorage: function CS_removeStorage(aId)
>+  {
>+    try {
>+      delete this.consoleStorage[aId];
>+    } catch (ex) {}

delete can't ever throw, AFAIK, so the try isn't needed.

>diff --git a/dom/base/ConsoleStorageService.manifest b/dom/base/ConsoleStorageService.manifest

No longer needed.

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>   closeConsoleOnTab: function HS_closeConsoleOnTab(aTab)

>+    // need to kill off the consoleStorageData
>+    let windowID = this.getWindowId(linkedBrowser.contentWindow);

Also no longer needed now that we evict data on window destruction.
Comment 36 David Dahl :ddahl 2010-12-22 15:45:17 PST
(In reply to comment #35)
> Comment on attachment 499349 [details] [diff] [review]

> >+function ConsoleStorageService()
> >+{
> >+  Services.obs.addObserver(this, "xpcom-shutdown", false);
> >+  Services.obs.addObserver(this, "content-document-global-created", false);
> >+  Services.obs.addObserver(this, "private-browsing", false);
> 
> no longer needed

I don't follow - why not?
Comment 37 David Dahl :ddahl 2010-12-22 15:52:49 PST
Created attachment 499414 [details] [diff] [review]
v 0.7 comments addressed, leaks gone!
Comment 38 David Dahl :ddahl 2010-12-22 15:59:39 PST
Created attachment 499417 [details] [diff] [review]
v 0.7.1 comments addressed, leaks gone!

removed addObserver call for private browsing. Gavin: I think that is what you meant.
Comment 39 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-12-23 08:16:56 PST
Yes, sorry I didn't trim enough - that is what I meant.
Comment 40 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-12-23 11:57:36 PST
Comment on attachment 499417 [details] [diff] [review]
v 0.7.1 comments addressed, leaks gone!

>diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm

>+function consoleStorageService()

>+  observe: function CS_observe(aSubject, aTopic, aData)

>+    else if (aTopic == "content-document-global-created") {
>+      // need to clear out previously-loaded window's storage:
>+      let windowID  = aSubject.QueryInterface(Ci.nsIInterfaceRequestor).
>+                        getInterface(Ci.nsIDOMWindowUtils).outerWindowID;

Missing a call to actually use windowID here. Should probably have a test that covers this? Just need to check that the cache for a given outer window gets cleared on navigation.

>+    else if (aTopic == "outer-window-destroyed") {
>+      try {
>+        let windowID  = aSubject.QueryInterface(Ci.nsISupportsPRUint64);
>+        this.removeStorage(windowID);

Test for this too would be good (close window, test that cache is cleared). Don't think the try{}/catch{} is necessary.

>+  getCachedEvents: function CS_getCachedEvents(aId)

This can just be:
return this.consoleStorage[aId] || [];

>+  recordEvent: function CS_recordEvent(aWindowID, aEvent)

It would probably be good to validate that aWindowID is an integer.

let ID = parseInt(aWindowID);
if (isNaN(ID))
  throw new Error("Invalid window ID argument");

>+    var storage = this.getCachedEvents(aWindowID);

let storage = this.consoleStorage[ID];

>+    storage.push(aEvent);
>+
>+    // truncate
>+    storage.push(aEvent);

Adding twice is wrong. Could use a test that would catch this too (check cache length before/after a cached event addition?)

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+XPCOMUtils.defineLazyGetter(this, "gConsoleStorage", function () {
>+  let obj = {};
>+  Cu.import("resource://gre/modules/ConsoleStorageService.jsm", obj);
>+  return obj.ConsoleStorageService;
>+});

Now unused

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_ConsoleStorageAPITests.js b/toolkit/components/console/hudservice/tests/browser/browser_ConsoleStorageAPITests.js

>+const PB_KEEP_SESSION_PREF = "browser.privatebrowsing.keep_current_session";

>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource://gre/modules/Services.jsm");

Again, no need to import these.
Comment 41 David Dahl :ddahl 2011-01-05 22:02:09 PST
Created attachment 501590 [details] [diff] [review]
v 0.7.2 Comments addressed, new tests added
Comment 42 David Dahl :ddahl 2011-02-08 11:16:04 PST
gavin:

just curious if you will have time to finish this review this week? Would LOVE to get this in for Beta 12.
Comment 43 Mike Beltzner [:beltzner, not reading bugmail] 2011-03-03 07:11:48 PST
** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 19 being moved from blocking2.0:betaN+ to blocking2.0:- as we reached the endgame of Firefox 4. The rationale for the move is:

 - the bug had been identified as a "soft" blocker which could be fixed in some follow up release
 - the bug had been identified as one requiring beta coverage, thus is not appropriate for a ".x" stability & security release

The owner of the bug may wish to renominate for .x consideration.
Comment 44 Mike Beltzner [:beltzner, not reading bugmail] 2011-03-03 07:18:35 PST
(er updating flag to "-" as per previous comment!)
Comment 45 David Dahl :ddahl 2011-03-28 16:06:32 PDT
Created attachment 522510 [details] [diff] [review]
0.7.3 unbitrot
Comment 46 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-10 13:22:10 PDT
Comment on attachment 522510 [details] [diff] [review]
0.7.3 unbitrot

>diff --git a/content/base/test/file_htmlserializer_2.html b/content/base/test/file_htmlserializer_2.html

I hope you sorted out why this keeps appearing in your diffs? :)

>diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm

>+consoleStorageService.prototype = {

>+    else if (aTopic == "content-document-global-created") {

>+    else if (aTopic == "outer-window-destroyed") {

Looks like we can actually store based on the inner window ID, and use the notification from bug 484710 for eviction.

That means:
  - edit ConsoleAPI to also pass the inner window ID
  - make this component store based on inner window IDs (and adjust documentation to make that clear)
  - use inner-window-destroyed to evict entries from the cache

We might also want to reset the storage on memory-pressure notifications.

>diff --git a/toolkit/components/console/hudservice/tests/browser/Makefile.in b/toolkit/components/console/hudservice/tests/browser/Makefile.in

>+	browser_ConsoleStorageAPITests.js \

Shouldn't this test live under dom/, next to the relevant code? (it might need to not depend on PrivateBrowsing service existing then, I suppose)

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_ConsoleStorageAPITests.js b/toolkit/components/console/hudservice/tests/browser/browser_ConsoleStorageAPITests.js

>+  observe: function CO_observe(aSubject, aTopic, aData)

>+        gWindow.wrappedJSObject.console.log("testing noop in storage");
>+
>+        let events = gConsoleStorage.getCachedEvents(gWindowID);
>+        let checkDone = false;
>+        for (let idx in events) {
>+          let msg = Array.join(events[idx].arguments, " ");
>+          if (msg == "testing noop in storage") {
>+            ok(false, "Found a logged message during Private Browsing");

Shouldn't this just check that event.length == 0 (or that the length didn't change from before the .log() call)
Comment 47 David Dahl :ddahl 2011-05-11 14:12:37 PDT
(In reply to comment #46)
> Comment on attachment 522510 [details] [diff] [review] [review]
> 0.7.3 unbitrot
> 
> >diff --git a/content/base/test/file_htmlserializer_2.html b/content/base/test/file_htmlserializer_2.html
> 
> I hope you sorted out why this keeps appearing in your diffs? :)
I really do not get it. It must be a linux thing. Some patches pick up "changed files" when I never have opened or touched them. I will eyeball the patch before attaching. 

> 
> >diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm
> 
> >+consoleStorageService.prototype = {
> 
> >+    else if (aTopic == "content-document-global-created") {
> 
> >+    else if (aTopic == "outer-window-destroyed") {
> 
> Looks like we can actually store based on the inner window ID, and use the
> notification from bug 484710 for eviction.
> 
Sounds good.

> We might also want to reset the storage on memory-pressure notifications.
>
check - did not know about that topic, very cool.
 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/Makefile.in b/toolkit/components/console/hudservice/tests/browser/Makefile.in
> 
> >+	browser_ConsoleStorageAPITests.js \
> 
> Shouldn't this test live under dom/, next to the relevant code? (it might
> need to not depend on PrivateBrowsing service existing then, I suppose)
> 
I thought we were being pretty adamant about testing new features with PB in mind. That is the only reason the test is in toolkit. I can make 2 tests, but is that really necessary?

> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_ConsoleStorageAPITests.js b/toolkit/components/console/hudservice/tests/browser/browser_ConsoleStorageAPITests.js
> 
> >+  observe: function CO_observe(aSubject, aTopic, aData)
> 
> >+        gWindow.wrappedJSObject.console.log("testing noop in storage");
> >+
> >+        let events = gConsoleStorage.getCachedEvents(gWindowID);
> >+        let checkDone = false;
> >+        for (let idx in events) {
> >+          let msg = Array.join(events[idx].arguments, " ");
> >+          if (msg == "testing noop in storage") {
> >+            ok(false, "Found a logged message during Private Browsing");
> 
> Shouldn't this just check that event.length == 0 (or that the length didn't
> change from before the .log() call)

right, nice shortcut. I think i needed to add a resetStorage() call before doing that check, that's why it was so convoluted.
Comment 48 David Dahl :ddahl 2011-05-11 14:15:10 PDT
Created attachment 531742 [details] [diff] [review]
0.8 Addressed gavin's comments
Comment 49 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-11 16:51:50 PDT
Comment on attachment 531742 [details] [diff] [review]
0.8 Addressed gavin's comments

>diff --git a/dom/base/ConsoleAPI.js b/dom/base/ConsoleAPI.js

>   init: function CA_init(aWindow) {

>     try {
>-      id = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+      outerID = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>                   .getInterface(Ci.nsIDOMWindowUtils)
>                   .outerWindowID;
>-    } catch (ex) {
>+      innerID = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                  .getInterface(Ci.nsIDOMWindowUtils).currentInnerWindowID;
>+    }

Seems to me like this should be inside the try{} too, rather than the catch.

>diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm

>+function consoleStorageService()
>+{
>+  Services.obs.addObserver(this, "xpcom-shutdown", false);
>+  Services.obs.addObserver(this, "content-document-global-created", false);

No need to watch for this anymore.

>+  Services.obs.addObserver(this, "outer-window-destroyed", false);

"inner", not "outer"
Comment 50 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-11 16:53:10 PDT
(In reply to comment #47)
> I thought we were being pretty adamant about testing new features with PB in
> mind. That is the only reason the test is in toolkit. I can make 2 tests,
> but is that really necessary?

Yes, the test should test the PB interaction (if the PB service exists). It just also needs to handle the PB service not existing (this is true regardless of whether it lives in toolkit/ or dom/). There's no need for two tests.
Comment 51 David Dahl :ddahl 2011-05-12 14:57:39 PDT
(In reply to comment #49)
> Comment on attachment 531742 [details] [diff] [review] [review]
> 0.8 Addressed gavin's comments
> 
> >diff --git a/dom/base/ConsoleAPI.js b/dom/base/ConsoleAPI.js
> 
> >   init: function CA_init(aWindow) {
> 
> >     try {
> >-      id = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> >+      outerID = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> >                   .getInterface(Ci.nsIDOMWindowUtils)
> >                   .outerWindowID;
> >-    } catch (ex) {
> >+      innerID = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> >+                  .getInterface(Ci.nsIDOMWindowUtils).currentInnerWindowID;
> >+    }
> 
> Seems to me like this should be inside the try{} too, rather than the catch.
>
indeed, it was: https://bugzilla.mozilla.org/attachment.cgi?id=531742&action=diff  -- weird, huh? hg freakout or something?
Comment 52 David Dahl :ddahl 2011-05-12 15:40:30 PDT
Created attachment 532050 [details] [diff] [review]
v 0.9 Fixed test and observer fixes
Comment 53 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-12 17:48:53 PDT
(In reply to comment #51)
> indeed, it was:
> https://bugzilla.mozilla.org/attachment.cgi?id=531742&action=diff  -- weird,
> huh? hg freakout or something?

oh no, I just misread the diff! my bad :)
Comment 54 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-18 15:31:13 PDT
Comment on attachment 532050 [details] [diff] [review]
v 0.9 Fixed test and observer fixes

>diff --git a/dom/base/ConsoleAPI.js b/dom/base/ConsoleAPI.js

> ConsoleAPI.prototype = {

>   init: function CA_init(aWindow) {

>+      outerID = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                  .getInterface(Ci.nsIDOMWindowUtils).outerWindowID;
>+      
>+      innerID = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                  .getInterface(Ci.nsIDOMWindowUtils).currentInnerWindowID;

nit: add a local var for "windowUtils" and (trim that unnecessary whitespace on the blank line :)

>diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm

>+  removeStorage: function CS_removeStorage(aId)

>+  resetStorage: function CS_resetStorage()

These are kind of confusingly similar. Perhaps removeStorage should be removeStorageForWindow, and resetStorage should be clearStorage? I suppose removeStorageForWindow doesn't even need to be "public".

>+var ConsoleStorageService = new consoleStorageService();

This is a bit confusing. I'd just have all the methods live on a ConsoleStorageService object directly, give it an init method, and then call that here. So:

var ConsoleStorageService = {
  // everything you put in the prototype above

  init: function () {
    // addObserver calls from consoleStorageService constructor
  }
}

ConsoleStorageService.init();

>diff --git a/dom/tests/browser/browser_ConsoleStorageAPITests.js b/dom/tests/browser/browser_ConsoleStorageAPITests.js

>+/* ***** BEGIN LICENSE BLOCK *****

You can use the PD license from https://www.mozilla.org/MPL/boilerplate-1.1/pd-c for tests, if you'd like.

>+XPCOMUtils.defineLazyServiceGetter(this, "pb",
>+                                   "@mozilla.org/privatebrowsing;1",
>+                                   "nsIPrivateBrowsingService");
>+
>+if (!pb) {
>+  let pb = null;
>+}

This won't work, since getService will throw. You need to use defineLazyGetter and handle fallback yourself.

>+var tab, gWindow, gWindowID, apiCallCount;

Please avoid using global variables wherever possible. browser_loadDisallowInherit.js is a good example of this. gWindowID doesn't need to be a global (can be retrieved when needed), tab can be local, etc.

>+var ConsoleObserver = {

>+  init: function CO_init()

>+    Services.prefs.setBoolPref(PB_KEEP_SESSION_PREF, true);

Looks like this pref doesn't get unset. Should clearUserPref it in a cleanup function (and set it in the test() function, rather than in this init() method).

>+  observe: function CO_observe(aSubject, aTopic, aData)

>+    if (pb) {
>+      if (aTopic == "private-browsing") {

You won't get private browsing notifications if the service doesn't exist, so these checks are redundant.

>+          gConsoleStorage.resetStorage();

You should have an explicit test for resetStorage() (probably in console-storage-cache-event), that checks length before/after. The PB test just needs to check that length before == length after (i.e. no additional entry was added). It would probably be cleaner to have the PB test in a seperate file, just to keep the tests simpler.

>+            // test to make sure the storage is cleared on navigation

>+                  ok(storageArray.length == 0, "storageArray is empty");

This should also check that the storage for the old window's ID is empty, shouldn't it? This could be tricky because the window could end up in bfcache and thus not be dead. Same applies to the tab close case (but delayed GC might be the issue there). Maybe you can use nsIDOMWindowUtils.garbageCollect() to ensure that the window dies.

>+                  // now test that the console cache is cleared on tab close:

This test doesn't seem to actually wait for TEST_URI to load, so I don't think it's testing what it wants to.

>+function test()

>+      let win = XPCNativeWrapper.unwrap(gWindow);

|win| is unused here (you should use it instead of "gWindow.wrappedJSObject")
Comment 55 David Dahl :ddahl 2011-05-24 13:07:15 PDT
(In reply to comment #54)

> This should also check that the storage for the old window's ID is empty,
> shouldn't it? This could be tricky because the window could end up in
> bfcache and thus not be dead. Same applies to the tab close case (but
> delayed GC might be the issue there). Maybe you can use
> nsIDOMWindowUtils.garbageCollect() to ensure that the window dies.

Yep. this is tricky. I log in the test when the window is closed and the inner-window-destroyed event fires. That appears to be taking to long to sweep out the messages from the storage array. executeSoon is not helping either.
Comment 56 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-25 11:52:50 PDT
So if you do these in order:
- close the tab (gBrowser.removeTab with {animate: false})
- call nsIDOMWindowUtils.garbageCollect
- check that the storage for the window ID is gone

you should be able to get consistent results. Is that not the case?
Comment 57 David Dahl :ddahl 2011-05-25 12:40:44 PDT
(In reply to comment #56)
> So if you do these in order:
> - close the tab (gBrowser.removeTab with {animate: false})
> - call nsIDOMWindowUtils.garbageCollect
> - check that the storage for the window ID is gone
> 
> you should be able to get consistent results. Is that not the case?

I keep getting an event in the cache, test:

        // make sure a closed window's events are in fact removed from the
        // storage cache
        window.console.log("adding a new event");

        // close the window - the storage cache should now be empty
        gBrowser.removeTab(tab, {animate: false});

        // need to get a new handle to a window in order to call garbageCollect()
        tab = gBrowser.addTab(TEST_URI);
        gBrowser.selectedTab = tab;
        browser = gBrowser.selectedBrowser;
        window = XPCNativeWrapper.unwrap(browser.contentWindow);
        window.QueryInterface(Ci.nsIInterfaceRequestor)
          .getInterface(Ci.nsIDOMWindowUtils).garbageCollect();

        // use the old windowID again to see if we have any stray cached messages
        messages = gConsoleStorage.getCachedEvents(windowID);

        executeSoon(function (){
          ok(messages.length == 0,
            "0 events found, tab close is clearing the cache");
        });

Not sure what I am doing wrong. In the inner-window-destroyed event I do this:

      let innerWindowID = aSubject.QueryInterface(Components.interfaces.nsISupportsPRUint64).data;
      this.removeStorageForWindow(innerWindowID);
Comment 58 David Dahl :ddahl 2011-05-25 13:37:51 PDT
Ok, moving the getConsoleStorage call into the executeSoon does the trick. Really don't like the executeSoon crutch.
Comment 59 David Dahl :ddahl 2011-05-25 13:49:04 PDT
Created attachment 535173 [details] [diff] [review]
v 0.9.1 comments addressed

split out private browsing test, added check for inner-window-destroyed
Comment 60 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-25 13:54:10 PDT
(In reply to comment #57)
>         // need to get a new handle to a window in order to call
> garbageCollect()

You want to use the chrome window for this (just |window|).
Comment 61 David Dahl :ddahl 2011-05-25 14:05:39 PDT
Created attachment 535176 [details] [diff] [review]
v 0.10 changed window used by garbageCollect()
Comment 62 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-01 17:11:49 PDT
Comment on attachment 535176 [details] [diff] [review]
v 0.10 changed window used by garbageCollect()

>diff --git a/dom/tests/browser/browser_ConsoleStorageAPITests.js b/dom/tests/browser/browser_ConsoleStorageAPITests.js

>+function log(aMsg)

Don't add this (in either test). Use info() if you really need debugging output in the tests.

>+XPCOMUtils.defineLazyGetter(this, "gConsoleStorage", function () {
>+  let obj = {};
>+  Cu.import("resource://gre/modules/ConsoleStorageService.jsm", obj);
>+  return obj.ConsoleStorageService;
>+});

Just Cu.import(ConsoleStorageService.jsm) directly (both tests).

>+  observe: function CO_observe(aSubject, aTopic, aData)

>+        executeSoon(function (){

This executeSoon shouldn't be here - if it's really needed, then the test is susceptible to intermittent failures.

>diff --git a/dom/tests/browser/browser_ConsoleStoragePBTest.js b/dom/tests/browser/browser_ConsoleStoragePBTest.js

>+  observe: function CO_observe(aSubject, aTopic, aData)

>+        executeSoon(function() {
>+          try {
>+            if (pb.privateBrowsingEnabled) {
>+              pb.privateBrowsingEnabled = false;
>+              finish();
>+            }
>+          }

Why try/catch?

>+    else {
>+      finish();

Why is this here? A test generally shouldn't have multiple calls to finish().
Comment 63 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-02 14:55:38 PDT
Comment on attachment 535176 [details] [diff] [review]
v 0.10 changed window used by garbageCollect()

>diff --git a/dom/base/ConsoleAPI.js b/dom/base/ConsoleAPI.js

>+XPCOMUtils.defineLazyGetter(this, "gConsoleStorage", function () {

Just import ConsoleStorageService directly.

>diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm

>+var ConsoleStorageService = {

>+  consoleStorage: {},

Just realized that this is actually exposed, so naming it _consoleStorage is probably best.

Sorry to flip-flop on the method names, but I think an API that looks like this would probably be best:

getEvents(id)
clearEvents(optional ID) (clears all events if no ID is passed)

recordEvent(id, event)

r- for the test issues, but this is very close.
Comment 64 David Dahl :ddahl 2011-06-07 14:15:01 PDT
(In reply to comment #62)
> Comment on attachment 535176 [details] [diff] [review] [review]
> Why try/catch?
> 
> >+    else {
> >+      finish();

Because this happens each time i try to shutdown pb mode:

TEST-PASS | chrome://mochitests/content/browser/dom/tests/browser/browser_ConsoleStoragePBTest.js | No logging entries found in PB mode
TEST-INFO | chrome://mochitests/content/browser/dom/tests/browser/browser_ConsoleStoragePBTest.js | [Exception... "'Failure' when calling method: [nsIPrivateBrowsingService::privateBrowsingEnabled]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://mochitests/content/browser/dom/tests/browser/browser_ConsoleStoragePBTest.js :: CO_observe :: line 48"  data: no]

It is very vague
Comment 65 David Dahl :ddahl 2011-06-07 14:21:07 PDT
Ok, it is throwing because PB mode is never entered into in the first place. All that happens is this: http://img717.imageshack.us/img717/103/selection008j.png

I thought PB mode would be automatically entered when you set pb.privateBrowsingMode = true;
Comment 66 David Dahl :ddahl 2011-06-07 14:22:00 PDT
(In reply to comment #65)

> pb.privateBrowsingMode = true;

or - rather:     pb.privateBrowsingEnabled = true;
Comment 67 :Ehsan Akhgari 2011-06-07 15:49:53 PDT
(In reply to comment #65)
> Ok, it is throwing because PB mode is never entered into in the first place.
> All that happens is this:
> http://img717.imageshack.us/img717/103/selection008j.png
> 
> I thought PB mode would be automatically entered when you set
> pb.privateBrowsingMode = true;

That is correct (provided that we're not already in PB mode, i.e., pb.privateBrowsingEnabled === false).  What exactly is failing for you?
Comment 68 David Dahl :ddahl 2011-06-08 13:36:43 PDT
latest madness: So when I am in PB mode, and I do 'pb.privateBrowsingEnabled = false;'

It is supposed to exit and notify observers with aData == exit - right? In my case it does not exit, it just throws an error which is swallowed by mochitest:

[Exception... "'Failure' when calling method: [nsIPrivateBrowsingService::privateBrowsingEnabled]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://mochitests/content/browser/dom/tests/browser/browser_ConsoleStoragePBTest.js :: CO_observe :: line 48"  data: no]

And the test times out, however all checks pass
Comment 69 David Dahl :ddahl 2011-06-08 13:48:57 PDT
Created attachment 538108 [details] [diff] [review]
v 0.11 Strange behavior with PB mode

Gavin and Ehsan:

In the test browser_ConsoleStorageServicePBTest.js I cannot get the test to finish() without calling finish() 2X. Its very weird. All checks pass, but cannot get things to quit cleanly and am unsure why.
Comment 70 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-08 14:41:42 PDT
Created attachment 538120 [details]
modified test

Here's a modified version of browser_ConsoleStoragePBTest.js that seems to work fine.
Comment 71 David Dahl :ddahl 2011-06-08 15:26:54 PDT
Comment on attachment 538120 [details]
modified test

nice. I was hoping one of you guys could tell me the error in my test, but i'll take this and run with it!
Comment 72 David Dahl :ddahl 2011-06-08 16:13:53 PDT
Created attachment 538138 [details] [diff] [review]
v 0.12 comments addressed

stole your test and made the last tweaks, I hope.
Comment 73 :Ehsan Akhgari 2011-06-08 19:17:05 PDT
Sorry for my delay here.  In your previous version of the test, you never quit the private browsing mode.  Also, doing stuff with the window on the enter notification is not safe, because the transition has not been completed.

Also, you don't need to check for the existence of the private browsing service, since this is a browser-chrome test which only runs in Firefox, which should always have this service.  So, please remove the check for the PB service and just assume that it's always available.
Comment 74 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-08 19:49:46 PDT
Browser chrome tests don't only run on Firefox. They also run in SeaMonkey, and could potentially run in other applications in the future. So please leave the private browsing check in.
Comment 75 :Ehsan Akhgari 2011-06-08 19:51:59 PDT
(In reply to comment #74)
> Browser chrome tests don't only run on Firefox. They also run in SeaMonkey, and
> could potentially run in other applications in the future. So please leave the
> private browsing check in.

If that's the case, there are many more browser chrome tests which should also change.  At least in those tests that I wrote myself, I've assumed the existence of the PB service.
Comment 76 David Dahl :ddahl 2011-06-08 20:25:32 PDT
(In reply to comment #75)
> (In reply to comment #74)
> > Browser chrome tests don't only run on Firefox. They also run in SeaMonkey, and
> > could potentially run in other applications in the future. So please leave the
> > private browsing check in.
> 
> If that's the case, there are many more browser chrome tests which should
> also change.  At least in those tests that I wrote myself, I've assumed the
> existence of the PB service.

Gavin's new version checks and quits if we get null, so that should be good. Thanks for looking at this.
Comment 77 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-09 15:23:13 PDT
Comment on attachment 538138 [details] [diff] [review]
v 0.12 comments addressed

>diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm

One (hopefully the last) naming nit: let's just rename this to ConsoleAPIStorage (filename and exported symbol), to avoid confusion with nsIConsoleService, and avoid using "Service" since this isn't a service in the XPCOM sense.

>+var ConsoleStorageService = {

>+  getEvents: function CS_getEvents(aId)
>+  {
>+    if (!aId)
>+      return this._consoleStorage;

I'm not sure this functionality is useful (who would want to get all events for all windows?), so let's omit it until there is a use case.

>+  removeStorageForWindow: function CS_removeStorageForWindow(aId)

>+  clearEvents: function CS_clearEvents()

Let's merge these (as suggested in comment 63)

>diff --git a/dom/tests/browser/browser_ConsoleStorageAPITests.js b/dom/tests/browser/browser_ConsoleStorageAPITests.js

>+        executeSoon(function (){

This executeSoon needs to go (comment 62).

>+registerCleanupFunction(tearDown);

Browser chrome tests shouldn't run code outside of test() - put this inside.
Comment 78 David Dahl :ddahl 2011-06-09 20:34:33 PDT
Created attachment 538428 [details] [diff] [review]
v 0.13 comments addressed

Comments addressed
Comment 79 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-15 11:38:18 PDT
Comment on attachment 538428 [details] [diff] [review]
v 0.13 comments addressed

>diff --git a/dom/base/ConsoleAPIStorage.jsm b/dom/base/ConsoleAPIStorage.jsm

>+var ConsoleAPIStorage = {

>+  /**
>+   * Object that contains all storage arrays
>+   */
>+  _consoleStorage: {},

Let's put this object outside of ConsoleAPIStorage (just a global in the file) so that it isn't exposed.

>+  /**
>+   * Get a storage array by ID
>+   *
>+   * @param string aId [optional]
>+   * @returns array
>+   *          (with optional ID, an object containing all events without an ID)

I don't understand this parenthetical.

>+  recordEvent: function CS_recordEvent(aWindowID, aEvent)

>+    if (gPrivBrowsing && gPrivBrowsing.privateBrowsingEnabled) {
>+      return;
>+    }

I think we'll probably want to revisit this - we should be able to store stuff in memory while in private browsing mode, we just need to clear it when we transition out. Let's do that in a followup.

>+   * Clear storage data
>+   *
>+   * @param string aId [optional]

I'm not a fan of these verbose comment blocks in general (particularly when they're larger than the self-explanatory functions they describe), but if they're going to be here you might as well document this properly, i.e. "ID of the window whose events should be cleared".

I don't think the "@returns void" comments are useful, I would just remove them.

>+  clearEvents: function CS_clearEvents(aId)
>+  {
>+    if (aId) {

Let's make this (aId != null) - clearEvents(0) should probably not clear all events.

>diff --git a/dom/tests/browser/browser_ConsoleStorageAPITests.js b/dom/tests/browser/browser_ConsoleStorageAPITests.js

>+var ConsoleObserver = {

>+  observe: function CO_observe(aSubject, aTopic, aData)

>+      dump("observe: " + aTopic + "\n\n\n");

remove this

>+        // close the window - the storage cache should now be empty
>+        gBrowser.removeTab(tab, {animate: false});
>+
>+        window.QueryInterface(Ci.nsIInterfaceRequestor)
>+          .getInterface(Ci.nsIDOMWindowUtils).garbageCollect();
>+        executeSoon(function (){

OK, so I looked into this further - this executeSoon *is* needed, but is potentially not even sufficient given nsGlobalWindow::TryClearWindowScope's behavior (potentially yielding multiple times before actually clearing the scope and firing the notification). I guess we can leave it as is for now and just keep an eye out for intermittent oranges, in which case we'll have to revisit.

r=me with those addressed (!)
Comment 80 David Dahl :ddahl 2011-06-15 14:53:25 PDT
Created attachment 539664 [details] [diff] [review]
v 0.14 Comments addressed
Comment 81 David Dahl :ddahl 2011-06-15 14:55:48 PDT
(In reply to comment #79)
> >+  /**
> >+   * Get a storage array by ID
> >+   *
> >+   * @param string aId [optional]
> >+   * @returns array
> >+   *          (with optional ID, an object containing all events without an ID)
> 
> I don't understand this parenthetical.
> 
It does not apply any longer, I removed it.

> >+  recordEvent: function CS_recordEvent(aWindowID, aEvent)
> 
> >+    if (gPrivBrowsing && gPrivBrowsing.privateBrowsingEnabled) {
> >+      return;
> >+    }
> 
> I think we'll probably want to revisit this - we should be able to store
> stuff in memory while in private browsing mode, we just need to clear it
> when we transition out. Let's do that in a followup.

I will file a followup bug

Thank you gavin!
Comment 82 Mihai Sucan [:msucan] 2011-07-06 12:59:44 PDT
Created attachment 544317 [details] [diff] [review]
v 0.15 fix for iframes

Updated the patch to fix issues with iframes. This is a minor change actually. We recordEvent() with the window.top inner ID instead of the window inner ID.

The window.console API messages need to be recorded into the window.top inner ID "bucket" so the Web Console code from bug 609890 doesn't have to call getEvents() for each iframe window inner ID. We can just call getEvents() with the window.top inner ID.

Please let me know if this is fine. Thank you!
Comment 83 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-06 13:04:16 PDT
Comment on attachment 544317 [details] [diff] [review]
v 0.15 fix for iframes

I don't think this consolidation should happen in the core code. I can see why it's annoying for bug 609890's use case to have things stored for each individual window separately, but I think that's an edge case that we shouldn't complicate the basic ConsoleStorage/ConsoleAPI code for.

For bug 609890, we should either just loop through iframes and getEvents() for all of them, or just ignore subframes and only retrieve cached events for the top level window.
Comment 84 Mihai Sucan [:msucan] 2011-07-06 13:12:28 PDT
(In reply to comment #83)
> Comment on attachment 544317 [details] [diff] [review] [review]
> v 0.15 fix for iframes
> 
> I don't think this consolidation should happen in the core code. I can see
> why it's annoying for bug 609890's use case to have things stored for each
> individual window separately, but I think that's an edge case that we
> shouldn't complicate the basic ConsoleStorage/ConsoleAPI code for.
> 
> For bug 609890, we should either just loop through iframes and getEvents()
> for all of them, or just ignore subframes and only retrieve cached events
> for the top level window.

I would say it's an unneeded complication to work around this issue in bug 609890 when we can do it here.

For example, we'll have problems with sorting the messages if we do getEvents() for each iframe, and so on. I believe that doing getEvents() for each iframe is like opening a can of worms.

Hence if you do not want it here I am just going to show the cached events from the top level window - in bug 609890. Is this fine? Shall I revert the change here?

Thanks for your quick review! :)
Comment 85 Mihai Sucan [:msucan] 2011-07-06 13:28:39 PDT
Tested in Chrome's Web Inspector: they do cache and display errors and console API messages coming from iframes.

In complex web apps I believe we'll want this to work. It's not just a corner case.
Comment 86 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-08 09:13:51 PDT
(In reply to comment #84)
> For example, we'll have problems with sorting the messages if we do
> getEvents() for each iframe, and so on. I believe that doing getEvents() for
> each iframe is like opening a can of worms.

You could add a timestamp to the message object and use that to sort them. Granted that makes the process of displaying cached messages more processing-intensive, but I don't think that is necessarily a deal breaker for that approach.

The problem with storing windows using the topInnerID as the key is that evicting the cache becomes more complicated - the inner-window-destroyed code in that patch is broken because it only attempts to remove the storage for an inner window at the top level, when that inner window's events may actually be stored on under its parent window's ID (in which case you'd need to iterate over events from that topInnerID and only remove those with the given innerID). Doing that work (get topmost inner ID, iterate over cache) every time any inner window is destroyed doesn't sound great.
Comment 87 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-08 09:18:21 PDT
We have somewhat conflicting goals:
- the retrieval task (on Web Console open) wants an ordered list of messages by topInnerID
- the cache eviction task (on inner window destruction) wants an easy way to clear out all messages for a given innerID

Of those two tasks, the latter should occur much more frequently in common practice (most people won't ever open the Web Console), so I think it makes sense to try to optimize for that case, even at the expense of the former.
Comment 88 Mihai Sucan [:msucan] 2011-07-08 09:32:14 PDT
(In reply to comment #87)
> We have somewhat conflicting goals:
> - the retrieval task (on Web Console open) wants an ordered list of messages
> by topInnerID
> - the cache eviction task (on inner window destruction) wants an easy way to
> clear out all messages for a given innerID
> 
> Of those two tasks, the latter should occur much more frequently in common
> practice (most people won't ever open the Web Console), so I think it makes
> sense to try to optimize for that case, even at the expense of the former.

Good points. Thanks for your explanation!
Comment 89 Mihai Sucan [:msucan] 2011-07-08 10:32:11 PDT
Created attachment 544849 [details] [diff] [review]
v 0.14b revert

Reverted the use of window.top inner ID to record Console API changes.

Not asking for review again since there's only a change that adds the jsdoc comment to the notifyObservers() method.

Thank you for your review Gavin!
Comment 90 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-08 11:45:39 PDT
It would be good to get someone like Boris or Sicking to SR this, I think, given that it's being added to dom/ (where I'm not a peer), and because it has the potential to introduce a perf/memory penalty for core code.
Comment 91 Mihai Sucan [:msucan] 2011-07-10 13:08:29 PDT
Comment on attachment 544849 [details] [diff] [review]
v 0.14b revert

Asking for superreview as suggested in comment 90.
Comment 92 Boris Zbarsky [:bz] (TPAC) 2011-07-11 07:04:02 PDT
If you want me to sr this, please write a decent description of exactly what this thing is doing _somewhere_.  Code comments ideal, but checkin comment is an OK fallback if needed.  In any case, the checkin comment needs to be a lot more useful than what's there now.
Comment 93 Mihai Sucan [:msucan] 2011-07-11 13:22:51 PDT
Created attachment 545243 [details] [diff] [review]
v 0.15 improved comments

Boris: I have updated the bug title and the commit message to better reflect what the patch does.

I have also added a comment to the new ConsoleAPIStorage.jsm, and improved the comments in general, to be clearer. Copying the ConsoleAPIStorage description:

-----

The ConsoleAPIStorage is meant to cache window.console API calls for later
reuse by other components when needed. For example, the Web Console code can
display the cached messages when it opens for the active tab.

ConsoleAPI messages are stored as they come from the ConsoleAPI code, with
all their properties. They are kept around until the window object that
created the messages is destroyed. Messages are indexed by the inner window
ID.

Usage:
   Cu.import("resource://gre/modules/ConsoleAPIStorage.jsm");

   // Get the cached events array for the window you want (use the inner
   // window ID).
   let events = ConsoleAPIStorage.getEvents(innerWindowID);
   events.forEach(function(event) { ... });

   // Clear the events for the given inner window ID.
   ConsoleAPIStorage.clearEvents(innerWindowID);

-----

I hope the changes help. Please let me know if any changes are needed. Thank you!
Comment 94 Dave Camp (:dcamp) 2011-07-21 14:24:36 PDT
Boris, is that description enough to get started on an sr?
Comment 95 Boris Zbarsky [:bz] (TPAC) 2011-07-21 14:32:53 PDT
Yes, I've been working on it...
Comment 96 Boris Zbarsky [:bz] (TPAC) 2011-07-21 22:05:43 PDT
Comment on attachment 545243 [details] [diff] [review]
v 0.15 improved comments

>They are kept around until the window object that
>+ * created the messages is destroyed.

This should probably specify that the messages are kept until the associated inner window is destroyed.

sr=me with that nit.  Sorry for the lag here, and I really appreciate the comments!
Comment 97 Mihai Sucan [:msucan] 2011-07-23 07:28:05 PDT
Created attachment 547930 [details] [diff] [review]
v 0.16

Updated the comment as suggested.

Thank you for your sr+!
Comment 98 Mihai Sucan [:msucan] 2011-08-04 05:35:48 PDT
Created attachment 550660 [details] [diff] [review]
v 0.17 rebase

Rebased the patch on top of fxteam.
Comment 99 Mihai Sucan [:msucan] 2011-08-25 04:48:43 PDT
Created attachment 555700 [details] [diff] [review]
[checked-in] v 0.18 rebase

another rebase
Comment 100 Mihai Sucan [:msucan] 2011-08-26 03:56:37 PDT
Comment on attachment 555700 [details] [diff] [review]
[checked-in] v 0.18 rebase

Landed:

http://hg.mozilla.org/integration/fx-team/rev/bb48d11f9c08
Comment 101 Rob Campbell [:rc] (:robcee) 2011-08-29 12:35:36 PDT
Comment on attachment 555700 [details] [diff] [review]
[checked-in] v 0.18 rebase

http://hg.mozilla.org/mozilla-central/rev/bb48d11f9c08

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