568629 - (lazy-console) Small footprint, lazy console component

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[David Dahl :ddahl]

We still need to be able to show console messages that emanated from a tab's context before the HeadsUpDisplay lazy getter is invoked. One way or another we need something that sits and listens for errors, messages and events.

the scope of this service should be very small and it should truncate it's data often. Perhaps we only keep message data for 3 or 5 minutes?

When a HeadsUpdisplay is invoked, this service will be queried for messages that occurred before the console was created.

Comment 1

[David Dahl :ddahl]

I think we need to think about how this effects the existing error console. perhaps this will replace it? Also under this new toolset we need to figure out the best way to expose chrome/xpconnect errors to a global console.

Comment 2

[David Dahl :ddahl]

Talked with jst about this bug, seems like this should be a fast, always on C++ XPCOM service that any console (e.g: firebug, ours) could observe to get js exceptions, css errors and the chrome or contentWindow ID where this error occurred.

Comment 3

[David Dahl :ddahl]

(In reply to comment #2)
> Talked with jst about this bug, seems like this should be a fast, always on C++
> XPCOM service that any console (e.g: firebug, ours) could observe to get js
> exceptions, css errors and the chrome or contentWindow ID where this error
> occurred

After speaking with jst yesterday, it seems a better solution is to write this as a JS component - not in C++ - so that we can lazily add window.console to any window as requested.

Once a user starts the Web Console front end, the front end "Web Console" should observe the already-in-place console API, logging the console.log/info/warn/error messages. 

This does change our existing code a bit as the console that is attached to each window will come from an external source, inside the DOM code.

jst also told me we could/should land this code in dom/ somewhere, which will be good for getting platform review and additional help we may need. 

We will also need to move our storage module into dom with all of the refactoring we need to do: truncation, etc.

btw, jst: what path inside dom should we put this code?

Comment 4

[David Dahl :ddahl]

Also: since it seems that we are calling the devtools console front end "Web Console", we should figure out a name for the console being created in this bug so we can have conversations about them without misunderstanding each other:)

Comment 5

[Julian Viereck]

There are a lot more functions on the console object then just log/info/warn/error. How does these get added on the page?

Comment 6

[David Dahl :ddahl]

(In reply to comment #5)
> There are a lot more functions on the console object then just
> log/info/warn/error. How does these get added on the page?

Which functions? can you list them here?

The current work is only implementing log/info/warn/error as a starting point. The FireBUg API is here:

http://getfirebug.com/wiki/index.php/Console_API

On this first iteration we were not interested in implementing the entire API, but I am pretty sure we plan on it.

Comment 7

[David Dahl :ddahl]

(In reply to comment #3)

> After speaking with jst yesterday, it seems a better solution is to write this
> as a JS component - not in C++ - so that we can lazily add window.console to
> any window as requested.

I should clarify that the reason to do this in JS now is because of the 'unlimited args' issue brought up in https://bugzilla.mozilla.org/show_bug.cgi?id=578353#c7

Comment 8

[David Dahl :ddahl]

Attachment: v 0.0 ConsoleApi xpcom component

Just getting started, this patch is in toolkit, but should not be, I have not even created any Makefiles. A little feedback would be nice.

Comment 9

[David Dahl :ddahl]

Attachment: v 0.1 ConsoleApi xpcom component, ConsoleStorage xpcom service

Saving my work, will ask for feedback after I have a little more fleshed out in the storage area. I am attempting to write an even lighter-weight storage service for this globally-always-on service. The existing storage module in the WebConsole is the model, but is a "catch all". I want this functionality to be very narrowly defined. perhaps we even destroy the storage backend for the browser when the WebConsole is opened, and it takes over the storage duty.

Comment 10

[David Dahl :ddahl]

CCing JJB to get feedback about how Firebug might use this.

Comment 11

[David Dahl :ddahl]

Attachment: v 0.1.0 WIP console and storage components

Comment 12

[John J. Barton]

Sorry, I don't understand what this is about. Are you proposing to make window.console a built-in DOM object? These are messages from the web page to the dev tools.  In that case "global" here refers to "a global object in the Javascript Window", not  a "Global service" in the XPCOM sense right? 

I think you don't need all of the apparatus of comment 11 for this purpose. The idl doesn't help because your just calling from JS to JS. FF4 makes attaching objects simple (so I hear), so the real question is how we share the resource. The observer service as you use could be ok, but like
  log: function CA_log()
  {
    this.notifyObservers(win, "console-log", arguments);
  }
So the subject is the originating window, topic is 'console-log', and everything passed to the function is passed to the observers.  Then to support the Firebug console API, you just list the methods and you're done. You don't need to spec the parameters, we can do that separately.

Since FF4 supports an event on window creation, I don't think you need the queue in this service. If clients of this service want to buffer events and not show them, they can do that (we do this for nsIConsoleService now). That makes this bug much easier and way less likely to break.  Of course I suppose 15 clients all buffering is less efficient than one service buffer, but most likely the number here is one or two.

And if I'm completely off base, sorry.

Comment 13

[David Dahl :ddahl]

(In reply to comment #12)
> Sorry, I don't understand what this is about. Are you proposing to make
> window.console a built-in DOM object? 
Yes! a lazy one for now, which is attached to the window on first call.

> These are messages from the web page to
> the dev tools.  In that case "global" here refers to "a global object in the
> Javascript Window", not  a "Global service" in the XPCOM sense right? 
> 
Correct. However, my first impulse for the storage of logging data was to create a service, maybe that is overkill.

> I think you don't need all of the apparatus of comment 11 for this purpose. 
That is good news then. I want this to be as little code as possible.

> The
> idl doesn't help because your just calling from JS to JS. FF4 makes attaching
> objects simple (so I hear), 
correct, but not yet - according to jst I still need an IDL - for now, to get my tests setup and running. I assume it will all go away eventually. In fact, all of this code should be moving over into mozilla/dom/

> so the real question is how we share the resource.
> The observer service as you use could be ok, but like
>   log: function CA_log()
>   {
>     this.notifyObservers(win, "console-log", arguments);
>   }
> So the subject is the originating window, topic is 'console-log', and
> everything passed to the function is passed to the observers.  Then to support
> the Firebug console API, you just list the methods and you're done. You don't
> need to spec the parameters, we can do that separately.
Ok, cool.

> 
> Since FF4 supports an event on window creation, I don't think you need the
> queue in this service. If clients of this service want to buffer events and not
> show them, they can do that (we do this for nsIConsoleService now). 
I did not know that, do you have some example code you can point me to?

> That makes
> this bug much easier and way less likely to break.  Of course I suppose 15
> clients all buffering is less efficient than one service buffer, but most
> likely the number here is one or two.
> 
agreed.

> And if I'm completely off base, sorry.
not at all, thanks!

We need to make this as easy and useful for all consumers.

Comment 14

[Johnny Stenback (:jst)]

(In reply to comment #13)
> (In reply to comment #12)
> > Sorry, I don't understand what this is about. Are you proposing to make
> > window.console a built-in DOM object? 
> Yes! a lazy one for now, which is attached to the window on first call.

I think "lazy" is the key here. We don't want to eagerly have to create the console object, or have it do anything for each and every window that's ever created. The vast majority of them will never need a console object.

> > The
> > idl doesn't help because your just calling from JS to JS. FF4 makes attaching
> > objects simple (so I hear), 
> correct, but not yet - according to jst I still need an IDL - for now, to get
> my tests setup and running. I assume it will all go away eventually. In fact,
> all of this code should be moving over into mozilla/dom/

The system that I'm prototyping will need 'a' IDL file for initializing the lazily created console object (so that the console can be told what its window is), but we won't need IDL for the console object itself any more.

[...]
> > Since FF4 supports an event on window creation, I don't think you need the
> > queue in this service. If clients of this service want to buffer events and not
> > show them, they can do that (we do this for nsIConsoleService now). 
> I did not know that, do you have some example code you can point me to?

It's true that we support such an event, but if we go this route we'll end up doing some amount of work for each and every window we create (each page navigation etc), which I think we should avoid if possible.

Comment 15

[John J. Barton]

(In reply to comment #13)
> > queue in this service. If clients of this service want to buffer events and not
> > show them, they can do that (we do this for nsIConsoleService now). 
> I did not know that, do you have some example code you can point me to?

The delayed logging is in our module that sorts error messages from nsIConsoleService into the appropriate window:
http://code.google.com/p/fbug/source/browse/branches/firebug1.6/content/firebug/errors.js#263

Comment 16

[John J. Barton]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Sorry, I don't understand what this is about. Are you proposing to make
> > > window.console a built-in DOM object? 
> > Yes! a lazy one for now, which is attached to the window on first call.
> 
> I think "lazy" is the key here.

There are two ways to be lazy here: 1) don't add console API unless the web page calls window.console functions and 2) don't add console API unless the web dev is watching the page. For best performance we want to be really lazy, but we need to also ensure that the web page cannot detect that it is being watched. That is, we don't want tests on |window.console| to be used to prevent users from accessing the content of their browser. 

Firebug uses "first JS use" for #1 (not perfect) but we only inject a getter for window.console, which then inits the rest of the in-page code. For #2 we have web-dev opt-in per site, with the ability to disable only the console for a page to solve the test-for-console problem.

Seems like you can do something similar by injecting a getter only when you have observers registered and only register the built-in logger when it is open. I don't know how you can do it only on pages with JS, we use jsd for that.

Comment 17

[David Dahl :ddahl]

(In reply to comment #15)
> The delayed logging is in our module that sorts error messages from
> nsIConsoleService into the appropriate window:
> http://code.google.com/p/fbug/source/browse/branches/firebug1.6/content/firebug/errors.js#263

Oh, ok, I was looking at that trying to figure out if the functionality is baked in to gecko somewhere or was FB specific.

Comment 18

[David Dahl :ddahl]

Attachment: v 0.1.1 ConsoleApi xpcom component, ConsoleStorage xpcom service

Saving work, have test suites going.

Comment 19

[David Dahl :ddahl]

jst:

What is the DOM bug that this depends on? Also, should this code go in mozilla/dom/? if so, where?

Comment 20

[Rob Campbell [:rc] (:robcee)]

to follow up from our conversation, it should be easily possible for consumers of this service to add new features to the console just by adding properties to it. This'll be important for downstream consumers like Firebug who may want a richer API than we're implementing initially.

initial functions should include (but not limited to!)

console.log
console.warn
console.error
console.info

Comment 21

[Rob Campbell [:rc] (:robcee)]

and for a comprehensive list, see:

http://getfirebug.com/wiki/index.php/Console_API

and,

http://developer.apple.com/safari/library/documentation/AppleApplications/Conceptual/Safari_Developer_Guide/DebuggingYourWebsite/DebuggingYourWebsite.html#//apple_ref/doc/uid/TP40007874-CH8-SW20

Comment 22

[David Dahl :ddahl]

Attachment: v 0.1.2 ConsoleApi xpcom component, ConsoleStorage xpcom service

Behold! passing test suites but alas, the addition of Firefox-specific gBrowser calls. How can we get rid of those?

Comment 23

[David Dahl :ddahl]

(In reply to comment #22) 
> Behold! passing test suites but alas, the addition of Firefox-specific gBrowser
> calls. How can we get rid of those?

like this:

 /**
+   * Retrieve the unique ID of a window object.
+   *
+   * @param nsIDOMWindow aWindow
+   * @returns integer ID
+   */
+  getWindowID: function IUI_getWindowID(aWindow)
+  {
+    if (!aWindow) {
+      return null;
+    }
+
+    let util = {};
+
+    try {
+      util = aWindow.QueryInterface(Ci.nsIInterfaceRequestor).
+        getInterface(Ci.nsIDOMWindowUtils);
+    } catch (ex) { }
+
+    return util.currentInnerWindowID;
+  },

Comment 24

[David Dahl :ddahl]

Attachment: v 0.1.2-1 ConsoleApi xpcom component, ConsoleStorage xpcom service

The ConsoelStorageService tests fail BAD right now. Saving work.

###!!! ASSERTION: trying to convert a jsval to const jsval & without allocator : this would leak: 'useAllocator', file /home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/xpconnect/src/xpcconvert.cpp, line 628
.L625 (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/xpconnect/src/xpcconvert.cpp:628)
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1784)
nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/xpconnect/src/xpcwrappedjs.cpp:571)
PrepareAndDispatch (/home/ddahl/code/moz/mozilla-central/mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.cpp:95)
NS_InvokeByIndex_P (/home/ddahl/code/moz/mozilla-central/mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_gcc_x86_unix.cpp:69)
CallMethodHelper::Invoke() (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/xpconnect/src/xpcwrappednative.cpp:3073)
CallMethodHelper::Call() (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/xpconnect/src/xpcwrappednative.cpp:2340)
XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/xpconnect/src/xpcwrappednative.cpp:2304)
XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, jsval_layout*, jsval_layout*) (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1738)
js::callJSNative(JSContext*, int (*)(JSContext*, JSObject*, unsigned int, js::Value*, js::Value*), JSObject*, unsigned int, js::Value*, js::Value*) (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/jscntxtinlines.h:355)
bool js::InvokeCommon<int (*)(JSContext*, JSObject*, unsigned int, js::Value*, js::Value*)>(JSContext*, JSFunction*, JSScript*, int (*)(JSContext*, JSObject*, unsigned int, js::Value*, js::Value*), js::InvokeArgsGuard const&, unsigned int) (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/jsinterp.cpp:577)
js::Invoke(JSContext*, js::InvokeArgsGuard const&, unsigned int) (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/jsinterp.cpp:714)
.L3888 (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/jsinterp.cpp:4728)
bool js::InvokeCommon<int (*)(JSContext*, JSObject*, unsigned int, js::Value*, js::Value*)>(JSContext*, JSFunction*, JSScript*, int (*)(JSContext*, JSObject*, unsigned int, js::Value*, js::Value*), js::InvokeArgsGuard const&, unsigned int) (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/jsinterp.cpp:588)
js::Invoke(JSContext*, js::InvokeArgsGuard const&, unsigned int) (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/jsinterp.cpp:714)
js::InvokeFriendAPI(JSContext*, js::InvokeArgsGuard const&, unsigned int) (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/jsinterp.cpp:738)
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1692)
nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (/home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/xpconnect/src/xpcwrappedjs.cpp:571)
PrepareAndDispatch (/home/ddahl/code/moz/mozilla-central/mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.cpp:95)
nsThread::ProcessNextEvent(int, int*) (/home/ddahl/code/moz/mozilla-central/mozilla-central/xpcom/threads/nsThread.cpp:547)
NS_ProcessNextEvent_P(nsIThread*, int) (/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-debug/xpcom/build/nsThreadUtils.cpp:250)
mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/ddahl/code/moz/mozilla-central/mozilla-central/ipc/glue/MessagePump.cpp:118)
MessageLoop::RunInternal() (/home/ddahl/code/moz/mozilla-central/mozilla-central/ipc/chromium/src/base/message_loop.cc:220)
MessageLoop::RunHandler() (/home/ddahl/code/moz/mozilla-central/mozilla-central/ipc/chromium/src/base/message_loop.cc:203)
MessageLoop::Run() (/home/ddahl/code/moz/mozilla-central/mozilla-central/ipc/chromium/src/base/message_loop.cc:176)
nsBaseAppShell::Run() (/home/ddahl/code/moz/mozilla-central/mozilla-central/widget/src/xpwidgets/nsBaseAppShell.cpp:181)
nsAppStartup::Run() (/home/ddahl/code/moz/mozilla-central/mozilla-central/toolkit/components/startup/src/nsAppStartup.cpp:191)
XRE_main (/home/ddahl/code/moz/mozilla-central/mozilla-central/toolkit/xre/nsAppRunner.cpp:3673)
main (/home/ddahl/code/moz/mozilla-central/mozilla-central/browser/app/nsBrowserApp.cpp:158)
__libc_start_main+0x000000E6  (/lib/tls/i686/cmov/libc.so.6)
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_ConsoleStorageServiceTests.js | Exited with code 11 during test run
INFO | automation.py | Application ran for: 0:00:16.306405
INFO | automation.py | Reading PID log: /tmp/tmpxAtU_Opidlog
PROCESS-CRASH | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_ConsoleStorageServiceTests.js | application crashed (minidump found)
Neither MINIDUMP_STACKWALK nor MINIDUMP_STACKWALK_CGI is set, can't process dump.

Comment 25

[David Dahl :ddahl]

(In reply to comment #24)
> Created attachment 463363 [details] [diff] [review]
> v 0.1.2-1 ConsoleApi xpcom component, ConsoleStorage xpcom service
> 
> The ConsoelStorageService tests fail BAD right now. Saving work.
> 
> ###!!! ASSERTION: trying to convert a jsval to const jsval & without allocator
> : this would leak: 'useAllocator', file
> /home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/xpconnect/src/xpcconvert.cpp,
> line 628
> .L625

Looks like i hit some detritus from a broken tm-merge.

Comment 26

[Johnny Stenback (:jst)]

Attachment: WIP for being able to register and initialize a JS component as window.console.

David, with this patch you should be able to register a JS component with the entry name "console" in the category "JavaScript-global-property", with the value being the class id of your JS component and then your class should be initialized when JS touches window.console. When that happens, this code will try to QI your object to nsIDOMGlobalPropertyInitializer and call the init() method in it, passing in the window. If the init() method returns an object, that object will be used, or if it returns a primitive value, we'll wrap the instance of the component itself and expose that.

This is totally untested, let me know how it goes :)

Comment 27

[David Dahl :ddahl]

Hmm the massive xpconnect erorr is still with me: http://pastebin.mozilla.org/763234

Not good. anyone know the bug this may be from?

Comment 28

[David Dahl :ddahl]

(In reply to comment #27)
> Hmm the massive xpconnect erorr is still with me:
> http://pastebin.mozilla.org/763234
> 
> Not good. anyone know the bug this may be from?

Filed bug 585232

Comment 29

[David Dahl :ddahl]

Attachment: v 0.1.3 ConsoleApi xpcom component, ConsoleStorage xpcom service

This thing is leaking like a sieve, do you have any idea what it might be?

Comment 30

[David Dahl :ddahl]

Attachment: v 0.1.3 ConsoleApi xpcom component, ConsoleStorage xpcom service

Leaky test or leaky service? what is going on here? Let me know if you don't have time to look at this.

Comment 31

[David Dahl :ddahl]

(In reply to comment #26)
> Created attachment 463676 [details] [diff] [review]
> WIP for being able to register and initialize a JS component as window.console.
> 

> This is totally untested, let me know how it goes :)

I wonder if I did not build correctly:

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_ConsoleApiTests.js | we have a console attached
JavaScript strict warning: chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_ConsoleApiTests.js, line 71: reference to undefined property win.wrappedJSObject.console
JavaScript error: chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_ConsoleApiTests.js, line 71: win.wrappedJSObject.console is undefined 

etc...

I built like this:

make -C dom/base && make -C dom/base && make -C toolkit/components/console && make -C toolkit/library

Comment 32

[Johnny Stenback (:jst)]

You also need to build in layout/build if you're changing DOM stuff.

Comment 33

[David Dahl :ddahl]

I just did a build like:

make -C dom && make -C layout/build && make -C toolkit/components/console && make -C toolkit/library 


and the tests fail:

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_ConsoleApiTests.js | we have a console attached
JavaScript strict warning: chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_ConsoleApiTests.js, line 71: reference to undefined property win.wrappedJSObject.console

I will attach my latest patch, which added the registration stuff

Comment 34

[David Dahl :ddahl]

Attachment: v 0.1.4 ConsoleApi xpcom component, ConsoleStorage xpcom service

Comment 35

[David Dahl :ddahl]

ok, maybe it is my component properties:

ConsoleApi.prototype = {

  classDescription: "Console API XPCOM Component",

  classID: Components.ID("{b49c18f8-3379-4fc0-8c90-d7772c1a9ff3}"),

  contractID: "@mozilla.org/console-api;1",

  QueryInterface: XPCOMUtils.generateQI([Ci.nsIConsoleApi]),

  category: "JavaScript-global-property",

  entry: "console",

  value: "{b49c18f8-3379-4fc0-8c90-d7772c1a9ff3}",

is that correct?

Comment 36

[Johnny Stenback (:jst)]

I *think* you want the value property to be the contract id string, not the class id as a string.

Comment 37

[John J. Barton]

Maybe this helps:
 _xpcom_categories: [ {category: STARTUP_TOPIC, entry: CLASS_NAME, value: CONTRACT_ID, service: true} ],

https://developer.mozilla.org/en/XPCOMUtils.jsm

Comment 38

[David Dahl :ddahl]

(In reply to comment #36)
> I *think* you want the value property to be the contract id string, not the
> class id as a string.

(In reply to comment #37)
> Maybe this helps:
>  _xpcom_categories: [ {category: STARTUP_TOPIC, entry: CLASS_NAME, value:
> CONTRACT_ID, service: true} ],
> 
> https://developer.mozilla.org/en/XPCOMUtils.jsm

here is the latest code bits:

ConsoleApi.prototype = {

  classDescription: "Console API XPCOM Component",

  classID: Components.ID("{b49c18f8-3379-4fc0-8c90-d7772c1a9ff3}"),

  contractID: "@mozilla.org/console-api;1",

  QueryInterface: XPCOMUtils.generateQI([Ci.nsIConsoleApi]),

  _xpcom_categories: [ 
                       { category: "JavaScript-global-property", 
                         entry: "console", 
                         value: "@mozilla.org/console-api;1", 
                         service: true,
                       } 
                     ],

...

I have tried both of these tips, but still do not have this working. NOt much to go on here:

pldhash: for the table at address 07013830, the given entrySize of 52 probably favors chaining over double hashing.
WARNING: Could not fetch previous flags, URI will be treated like referrer: 'NS_FAILED(rv)', file c:/moz/mozilla-central/obj-dbg/docshell/ba
se/../../../mozilla-central/docshell/base/nsDocShell.cpp, line 10193
++DOMWINDOW == 14 (06F073E8) [serial = 14] [outer = 06A2A580]
i am dumping thisJavaScript strict warning: chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_Co
nsoleApiTests.js, line 70: reference to undefined property win.wrappedJSObject.console
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_ConsoleApiTests.js | we
 have a console attached
JavaScript strict warning: chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_ConsoleApiTests.js,
 line 71: reference to undefined property win.wrappedJSObject.console
JavaScript error: chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_ConsoleApiTests.js, line 71:

Comment 39

[David Dahl :ddahl]

So I have gotten most of this to work, however, the window weak reference and window.ID that I tried to store in the console component are undefined. 

The weird thing is that I try to log messages in the init method to no avail. very strange. I can log messages in the constructor, and all of the consoleApi is working: log(), info(), warn(), error()

very strange indeed. I will attach the latest patch.

Comment 40

[David Dahl :ddahl]

Attachment: v 0.1.5 ConsoleApi xpcom comp. & storage svc

Comment 41

[David Dahl :ddahl]

Another weird thing is that I get this warning, but the immediate test passes:

JavaScript strict warning: chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_Co
nsoleApiTests.js, line 59: reference to undefined property win.wrappedJSObject.console
TEST-PASS | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_ConsoleApiTests.js | we have a con
sole attached

Comment 42

[David Dahl :ddahl]

I tried to log this.window (the getter that gets the window from the weakref) inside the console.log method and i get a failure like:

JavaScript error: , line 0: uncaught exception: [Exception... "'[JavaScript Error: "this._window is null" {file: "file:///c:/moz/mozilla-cen
tral/obj-dbg/dist/bin/components/ConsoleApi.js" line: 140}]' when calling method: [nsIConsoleApi::log]"  nsresult: "0x80570021 (NS_ERROR_XPC
_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
TEST-INFO | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_ConsoleApiTests.js | Console messa
ge: [JavaScript Warning: "reference to undefined property win.wrappedJSObject.console" {file: "chrome://mochikit/content/browser/toolkit/com
ponents/console/hudservice/tests/browser/browser_ConsoleApiTests.js" line: 59}]

Comment 43

[Mihai Sucan [:msucan]]

I looked through the JS code of the ConsoleApi and ConsoleStorage. I have some feedback:

- In ConsoleApi.notifyObserver the consoleEvent id property should perhaps be 
renamed to windowID, because it's not a unique ID of the event, but it's a 
unique ID of the window from where the console message originates.

- Perhaps it would be good to put a unique ID for every consoleEvent message 
object?

- In ConsoleStorageService.recordEvent you add a timestamp to the consoleEvent 
when you store it. Why not put a timestamp in the first place? (in ConsoleApi)

I think that console messages should be timestamped.

- In ConsoleStorageService.recordEvent you use new Number(Date.now()). Why? I 
would suggest Date.now() directly - the number constructor seems redundant.

- In ConsoleStorageService.truncateBackend you use array.shift(). This has the 
potential of becoming very slow with big arrays. One approach that really 
improves performance is to used delayed shift: you splice the first 100 messages 
when there are more 100 messages over the limit, and so on. In a web application 
with lots of logging this would help with performance.

That's all for now. It's a really good idea to have the Console object API implemented separately from the WebConsole.

Comment 44

[David Dahl :ddahl]

the code is in a state of extreme debugging right now. the new Number, etc was for tracking down leaks and stuff. I'll be cleaning it up today.

Comment 45

[David Dahl :ddahl]

I removed my init method in the ConsoleApi.idl. Everything still works the same. I also commented out the entire init method in the component, and everything still works the same. It appears that my init function is never called.

Comment 46

[David Dahl :ddahl]

Attachment: v 0.1.6 ConsoleApi xpcom comp. & storage svc

<saving work to move to another machine>

Comment 47

[David Dahl :ddahl]

Attachment: v 0.1.7 ConsoleApi xpcom component, ConsoleStorage xpcom service

requires patch 463676: "WIP for being able to register and initialize a JS component as window.console"

Weird thing is that the consoleStorge tests leak, but the consoleApi tests do not, which heavily consume the consoleStorage service.

This code may end up living in dom/ but jst said I should get a toolkit person to review.

Comment 50

[Benjamin Smedberg]

This blocks a feature, so it's beta5 or bust!

Comment 51

[David Dahl :ddahl]

a note on the leaks:

<dietrich> ddahl: so what's the story with the leak? which test leaks, storage?
<ddahl> the storage tests leak
<ddahl> if you start commenting out api calls, its the recordEvent call that leaks
<ddahl> i should update the bug
<ddahl> however, that method is called 200+ times in the consoleapi tests and no leaks!

Comment 52

[Dietrich Ayala (:dietrich)]

Comment on attachment 466510 [details] [diff] [review]
v 0.1.7 ConsoleApi xpcom component, ConsoleStorage xpcom service

>+
>+#include "nsISupports.idl"
>+#include "nsIDOMWindow.idl"
>+#include "nsIVariant.idl"
>+
>+[scriptable, uuid(b49c18f8-3379-4fc0-8c90-d7772c1a9ff3)]
>+interface nsIConsoleApi : nsISupports
>+{


put a summary comment above the interface definition, explaining what it's for, who it's used by, and how.

>diff --git a/toolkit/components/console/hudservice/ConsoleApi.js b/toolkit/components/console/hudservice/ConsoleApi.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/console/hudservice/ConsoleApi.js
>@@ -0,0 +1,241 @@
>+
>+let log = function _log(msg) {
>+  dump("*** ConsoleApi Log: " + msg + "\n");
>+};

remove
 
>+
>+let Cu = Components.utils;
>+let Ci = Components.interfaces;
>+let Cc = Components.classes;
>+
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource://gre/modules/Services.jsm");
>+
>+XPCOMUtils.defineLazyServiceGetter(this, "consoleStorage",
>+                                   "@mozilla.org/console-storage-service;1",
>+                                   "nsIConsoleStorageService");
>+
>+function ConsoleApi()

nit: s/Api/API/ here and everywhere

>+{
>+
>+}
>+
>+ConsoleApi.prototype = {
>+
>+  classDescription: "Console API XPCOM Component",

XPCOM Component is not necessary. and it's obviously an API.

maybe something like "Logging interface for Web content"?

>+  /**
>+   * nsIClassInfo property that specifies the programming language
>+   * this object is implemented in
>+   */

nit: period at the end of the comment, here and through the patch.

>+  /**
>+   * A unique id that probably ties back to a DOM node's id.
>+   * In Firefox, this id is the tab's (notificationBox) id
>+   */
>+  _id: null,
>+
>+  /**
>+   * Retrieve the unique ID of a window object.
>+   *
>+   * @returns integer ID
>+   */

s/ID/id/ here and elsewhere. "id" is a truncation, not an acronym. and if you like ID better, that's fine, but it should be consistent throughout.

>+  getWindowId: function CA_getWindowId(aOuterWindow)
>+  {
>+    // aOuterWindow is an XPCWrappedNative
>+    let util = {};
>+    try {
>+  util = aOuterWindow.QueryInterface(Ci.nsIInterfaceRequestor).
>+         getInterface(Ci.nsIDOMWindowUtils);
>+      return util.outerWindowID;
>+    }
>+    catch (ex) {
>+      Cu.reportError(ex);
>+      return null;
>+    }
>+  },

nit: fix indent

>+  /**
>+   * The window this console compoennt is attached to

typo

>+  /**
>+   * console.log API method
>+   *
>+   * @param JSObject aMessage
>+   *        Multiple arguments are supported via js 'argumments'

typo, here and in the other apis.

>+  notifyObservers: function CA_notifyObservers(aLevel, aMessage, aArguments)
>+  {
>+    var self = this;
>+
>+    let consoleEvent = {
>+      level: aLevel,
>+      message: aMessage,
>+      arguments: aArguments,
>+      id: self._id,
>+    };
>+
>+    consoleStorage.recordEvent(this._id, consoleEvent);
>+
>+    consoleEvent.wrappedJSObject = consoleEvent;
>+
>+    Services.obs.notifyObservers(consoleEvent,
>+                                 "console-api-log-event", self._id);
>+  }

1. use let or var consistently.

2. just do let id = this.id, since that's the only property you're using self for.

>+interface nsIConsoleStorageService : nsISupports
>+{
>+  /**
>+   * Create a named storage backend
>+   * @param string aId
>+   * @returns JS array
>+   */
>+  nsIVariant createStorageBackend(in AString aId);
>+  
>+  /**
>+   * remove a named storageBackend
>+   * @param nsIVariant aEvent
>+   * @returns void

s/event/id/

>+   */
>+  void removeStorageBackend(in AString aId);
>+
>+  /**
>+   * record an event to a named storageBackend
>+   * @param nsIVariant aEvent
>+   * @returns void
>+   */
>+  void recordEvent(in AString aId, in nsIVariant aEvent);

document aId

>+  /**
>+   * get a named storage backend
>+   * @param string aId
>+   * @returns nsIVariant JS array
>+   */
>+  nsIVariant getStorageBackend(in AString aId);  
>+
>+  /**
>+   * get the global storage backend
>+   * @param string aId
>+   * @returns nsIVariant JS array
>+   */
>+  nsIVariant getGlobalStorageBackend();  

are these references that can be used to modify the contents of the backend? or static copies? should make it clear in the docs.

>+  
>+};  

remove whitespace throughout


>+function ConsoleStorageService()
>+{
>+  this.storageBackends[GLOBAL_STORAGE_BACKEND_ID] = [];

you don't protect this in createStorageBackend, can be overwritten/reset.

>+ConsoleStorageService.prototype = {
>+  classDescription: "Console Storage XPCOM Service",

XPCOM is unnecessary here.

>+  /**
>+   * create a new array to store console events.
>+   *
>+   * @param string aId
>+   *        an ID used to segregate storage backend arrays by browser tab, or
>+   *        any other purpose
>+   * @returns array
>+   */
>+  createStorageBackend: function CS_createStorageBackend(aId)
>+  {
>+    this.storageBackends[aId] = [];
>+    return this.storageBackends[aId];
>+  },

what if one by that name already exists?

>+  removeStorageBackend: function CS_removeStorageBackend(aId)
>+  {
>+    if (aId == GLOBAL_STORAGE_BACKEND_ID) {
>+      return;
>+    }
>+    else {
>+      delete this.storageBackends[aId];
>+    }
>+  },

check that it exists before delete?

>+  truncateBackend: function CS_truncateBackend(aId)
>+  {
>+    var maxLength;
>+    var backend = this.storageBackends[aId];
>+    var len = backend.length;
>+
>+    if (aId == GLOBAL_STORAGE_BACKEND_ID) {
>+      maxLength = GLOBAL_STORAGE_BACKEND_MAX_EVENTS;
>+    }
>+    else {
>+      maxLength = STORAGE_BACKEND_MAX_EVENTS;
>+    }
>+    if (len > maxLength) {
>+      backend.shift();
>+    }
>+  },

per mihai's comment, use splice() to knock it down to the max length.

close, but r- for the above fixes. also, i'm not hot on the use of the term "backend" in the storage service. maybe instead of "storage backend", could do "console storage", which is more descriptive. eg: createConsoleStorage(id), removeConsoleStorage, etc.

Comment 53

[David Dahl :ddahl]

(In reply to comment #52)

> >+  /**
> >+   * get a named storage backend
> >+   * @param string aId
> >+   * @returns nsIVariant JS array
> >+   */
> >+  nsIVariant getStorageBackend(in AString aId);  
> >+
> >+  /**
> >+   * get the global storage backend
> >+   * @param string aId
> >+   * @returns nsIVariant JS array
> >+   */
> >+  nsIVariant getGlobalStorageBackend();  
> 
> are these references that can be used to modify the contents of the backend? or
> static copies? should make it clear in the docs.

I am thinking they should be static copies, what is the fastest way to make them so?  


> >+  truncateBackend: function CS_truncateBackend(aId)
> >+  {
> >+    var maxLength;
> >+    var backend = this.storageBackends[aId];
> >+    var len = backend.length;
> >+
> >+    if (aId == GLOBAL_STORAGE_BACKEND_ID) {
> >+      maxLength = GLOBAL_STORAGE_BACKEND_MAX_EVENTS;
> >+    }
> >+    else {
> >+      maxLength = STORAGE_BACKEND_MAX_EVENTS;
> >+    }
> >+    if (len > maxLength) {
> >+      backend.shift();
> >+    }
> >+  },
> 
> per mihai's comment, use splice() to knock it down to the max length.
> 
Is the issue that splice is faster than shift? 

> close, but r- for the above fixes. also, i'm not hot on the use of the term
> "backend" in the storage service. maybe instead of "storage backend", could do
> "console storage", which is more descriptive. eg: createConsoleStorage(id),
> removeConsoleStorage, etc.

ok, will change that naming.

thank you.

Comment 54

[David Dahl :ddahl]

Attachment: v 0.1.8 ConsoleApi xpcom component, ConsoleStorage xpcom service

Comment 55

[Dietrich Ayala (:dietrich)]

Comment on attachment 468533 [details] [diff] [review]
v 0.1.8 ConsoleApi xpcom component, ConsoleStorage xpcom service

>+/**
>+ * nsIConsoleAPI lazily provides a console property on any contentWindow.
>+ * console.info, .log, .warn and .error are provided
>+ */

nit: the console property on a window is an implementation example. this is just the abstract interface for a multi-level logging facility. maybe just generalize it to reflect that?

>+[scriptable, uuid(b49c18f8-3379-4fc0-8c90-d7772c1a9ff3)]
>+interface nsIConsoleAPI : nsISupports
>+{
>+  void log(in nsIVariant aMessage);
>+
>+  void info(in nsIVariant aMessage);
>+
>+  void warn(in nsIVariant aMessage);
>+
>+  void error(in nsIVariant aMessage);
>+};  

nit: remove extra whitespace

>+function ConsoleAPI()
>+{
>+
>+}
>+
>+ConsoleAPI.prototype = {
>+
>+  classDescription: "Logging interface for Web content",

I was wrong. This is the implementation, so s/interface/API/.

>+  /**
>+   * A weakReference to the window this console is attached to
>+   */
>+  _window: null,
>+
>+  /**
>+   * A unique id that probably ties back to a DOM node's id.
>+   * In Firefox, this id is the tab's (notificationBox) id
>+   */
>+  ID: null,

MEGA-NIT: Per the last review, make your comments sentences, or not. I don't care which, but please pick one and stick with it! Throughout the patch you've got:

Starts like a sentence, but no period to end it
ends like a sentence, but doesn't start like one.
definitely not a sentence, and that's fine as long they're all that way

>+  getWindowId: function CA_getWindowId(aOuterWindow)
>+  {
>+    // aOuterWindow is an XPCWrappedNative
>+    let util = {};
>+    try {
>+      util = aOuterWindow.QueryInterface(Ci.nsIInterfaceRequestor).
>+	         getInterface(Ci.nsIDOMWindowUtils);
>+      return util.outerWindowID;
>+    }
>+    catch (ex) {
>+      Cu.reportError(ex);
>+      return null;
>+    }
>+  },

util isn't used outside of the block, so that whole line could go away, and declare util when you do the QI in the block.

>+  init: function CA_init(aOuterWindow)
>+  {
>+    var self = this;
>+    this._window = Cu.getWeakReference(aOuterWindow);
>+    this.ID = this.getWindowId(aOuterWindow);
>+    this.window.addEventListener("close", self.destroy, false);
>+  },

hm, not sure self is necessary here. test and see?

>+
>+  destroy: function CA_destroy()
>+  {
>+    consoleStorage = null;
>+  },

hmmm, either make consoleStorage a lazy getter off consoleAPI so you can null out this.consoleStorage, or rename it gConsoleStorage.

>+  notifyObservers: function CA_notifyObservers(aLevel, aMessage, aArguments)
>+  {
>+    let self = this;
>+
>+    let consoleEvent = {
>+      level: aLevel,
>+      message: aMessage,
>+      arguments: aArguments,
>+      id: self.ID,
>+    };
>+
>+    consoleStorage.recordEvent(this.ID, consoleEvent);
>+
>+    consoleEvent.wrappedJSObject = consoleEvent;
>+
>+    Services.obs.notifyObservers(consoleEvent,
>+                                 "console-api-log-event", self.ID);
>+  }
>+};

s/self/ID/ per previous review.

>+function ConsoleStorageService()
>+{
>+  this.consoleStorage[GLOBAL_STORAGE_ID] = [];

hm, this line isn't necessary, since you'll create the storage on the fly, right?

>+  /**
>+   * gets a storage array by ID.
>+   *
>+   * @param string aId
>+   * @returns array
>+   */
>+  getConsoleStorage: function CS_getConsoleStorage(aId)
>+  {
>+    return this.consoleStorage[aId];
>+  },

should throw if the id is bad

>+  createConsoleStorage: function CS_createConsoleStorage(aId)
>+  {
>+    if (this.consoleStorage[aId]) {
>+      return this.consoleStorage[aId];
>+    }
>+    this.consoleStorage[aId] = [];
>+    return this.consoleStorage[aId];
>+  },

could save a line of code by creating it if not exists, and only returning once.

>+
>+  /**
>+   * Removes a console storage array by ID.
>+   *
>+   * @param string aId
>+   * @param boolean aRemoveAll
>+   * @returns void
>+   */
>+  removeConsoleStorage: function CS_removeConsoleStorage(aId, aRemoveAll)
>+  {
>+    if (aId && aRemoveAll) {
>+      for (let aStorage in this.consoleStorage) {
>+        delete this.consoleStorage[aStorage];
>+      }
>+    }
>+    else {
>+      if (this.consoleStorage[aId]) {
>+        delete this.consoleStorage[aId];
>+      }
>+    }
>+  },

aRemoveAll needs to be documented. Wait, this doesn't actually match up with the IDL. And why is aId a factor at all in the first 'if' clause?

r- for this last bit. Everything else looks ok, outside of the cleanup comments above.

Comment 56

[David Dahl :ddahl]

(In reply to comment #55)

> >+function ConsoleStorageService()
> >+{
> >+  this.consoleStorage[GLOBAL_STORAGE_ID] = [];
> 
> hm, this line isn't necessary, since you'll create the storage on the fly,
> right?
it is, since we store things in the global array with a different internal api, and we potentially need this to be initialized from the get go.


> >+
> >+  /**
> >+   * Removes a console storage array by ID.
> >+   *
> >+   * @param string aId
> >+   * @param boolean aRemoveAll
> >+   * @returns void
> >+   */
> >+  removeConsoleStorage: function CS_removeConsoleStorage(aId, aRemoveAll)
> >+  {
> >+    if (aId && aRemoveAll) {
> >+      for (let aStorage in this.consoleStorage) {
> >+        delete this.consoleStorage[aStorage];
> >+      }
> >+    }
> >+    else {
> >+      if (this.consoleStorage[aId]) {
> >+        delete this.consoleStorage[aId];
> >+      }
> >+    }
> >+  },
> 
> aRemoveAll needs to be documented. Wait, this doesn't actually match up with
> the IDL. And why is aId a factor at all in the first 'if' clause?
> 
My bad, I was doing some things to figure out the leaks. I need to create 2 methods to make things clear.

Comment 57

[David Dahl :ddahl]

Actually, I just changed it back to the original idl method

Comment 58

[David Dahl :ddahl]

Attachment: v 0.1.9 ConsoleApi xpcom component, ConsoleStorage xpcom service

Also filed bug 590257 on the mochitest leakage

Comment 59

[David Dahl :ddahl]

As i try to integrate this new lazy console into the WebConsole code bug 587734, I am getting some wrapper errors:

pldhash: for the table at address 0xac0ca5d8, the given entrySize of 52 probably favors chaining over double hashing.
*** HUDService: >>>>>>>>>>>>>>>>>>>> CAO INIT
*** HUDService: WI: We have a browser window!
*** HUDService: hudId: hud_panel1282690069739
*** HUDService: can activate context!
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /home/ddahl/code/moz/mozilla-central/mozilla-central/dom/base/nsDOMClassInfo.cpp, line 7892
JavaScript error: , line 0: uncaught exception: [Exception... "Component is not available"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource://gre/modules/HUDService.jsm :: JSPropertyProvider :: line 2668"  data: no]
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /home/ddahl/code/moz/mozilla-central/mozilla-central/dom/base/nsDOMClassInfo.cpp, line 7892
JavaScript error: , line 0: uncaught exception: [Exception... "Component is not available"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource://gre/modules/HUDService.jsm :: JSPropertyProvider :: line 2668"  data: no]
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /home/ddahl/code/moz/mozilla-central/mozilla-central/dom/base/nsDOMClassInfo.cpp, line 7892
JavaScript error: , line 0: uncaught exception: [Exception... "Component is not available"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource://gre/modules/HUDService.jsm :: JSPropertyProvider :: line 2668"  data: no]
###!!! ASSERTION: bad hint in chrome code: 'hint != XOW && hint != SOW && hint != COW', file /home/ddahl/code/moz/mozilla-central/mozilla-central/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 1055


I call XPCNativeWrapper.unwrap(aWindow.console) before set it as a property on the WebConsole front end object (UI). I get that error when ever I try to call console.log() from the console's front end input node.

The console is still xpconnect wrapped as well.

Comment 60

[Dietrich Ayala (:dietrich)]

(In reply to comment #58)
> Created attachment 468762 [details] [diff] [review]
> v 0.1.9 ConsoleApi xpcom component, ConsoleStorage xpcom service
> 
> Also filed bug 590257 on the mochitest leakage

Please close that bug, and fix the leak here. It's very unlikely that it's due to a separate issue. Keep reducing and isolating the code as necessary until it's found.

Comment 61

[Dietrich Ayala (:dietrich)]

Comment on attachment 468762 [details] [diff] [review]
v 0.1.9 ConsoleApi xpcom component, ConsoleStorage xpcom service


>+  removeConsoleStorage: function CS_removeConsoleStorage(aId)
>+  {
>+    if (aId) {
>+      if (this.consoleStorage[aId]) {
>+        delete this.consoleStorage[aId];
>+      }
>+    }
>+  },

don't need to test for presence of aId since xpcom/xpconnect/xpidl will throw if the call doesn't match the idl definition for this function.

however, you should throw if aId is not a storage id.

the rest of the code looks good, thanks for making the changes. however, going to keep this r- until we figure out the leak.

Comment 62

[David Dahl :ddahl]

(In reply to comment #60)
> (In reply to comment #58)
> > Created attachment 468762 [details] [diff] [review] [details]
> > v 0.1.9 ConsoleApi xpcom component, ConsoleStorage xpcom service
> > 
> > Also filed bug 590257 on the mochitest leakage
> 
> Please close that bug, and fix the leak here. It's very unlikely that it's due
> to a separate issue. Keep reducing and isolating the code as necessary until
> it's found.

As the test code now stands there are no leaks. The ConsoleAPI tests do test the service as well - exactly how we use it, a couple a hundred times. The storage service tests are testing that we can store data in the service, just not the exact object.

If it is not the test itself that has the leak problem, why is it that the leak dissipates with an integer passed to it, and disappears with a string? Somehow, an object keeps a reference to the current window. 

In a way the test is not all that realistic in the first place as the storage service is normally used inside the scope of another xpcom component. 

Maybe I should move that test to xpcshell and see what happens?

Comment 63

[David Dahl :ddahl]

Attachment: v 0.1.10 ConsoleApi xpcom component, ConsoleStorage xpcom service

Removed the browser test for the StorageService, add an XPCShell test. It runs so fast and says it passes? can that be true? 

Is this why sdwilsh refuses to work on "front end code"? speed?

Comment 64

[David Dahl :ddahl]

Attachment: v 0.1.11 ConsoleApi xpcom component, ConsoleStorage xpcom service

a few tweaks to the xpcshell test

Comment 65

[Dietrich Ayala (:dietrich)]

Comment on attachment 469750 [details] [diff] [review]
v 0.1.11 ConsoleApi xpcom component, ConsoleStorage xpcom service

you need to throw in removeConsoleStorage if aId doesn't exist. there's no valid reason for that happening, ever. that comment was in the last review, and wasn't fixed.

r=me with that fixed.

Comment 66

[Kevin Dangoor]

Though this bug is an important improvement, the console functions without it and there are no strings. This should not block beta 5 (but likely betaN+).

I bring this up because this *could* land in beta 5 if it weren't for bug 587734 which might not be ready in time and we really need to land both at the same time.

Comment 67

[Mike Beltzner [:beltzner, not reading bugmail]]

--> beta6+ as per comment 66

Comment 68

[David Dahl :ddahl]

Attachment: v 0.1.12 ConsoleApi xpcom component, ConsoleStorage xpcom service

Fixed as per review comments

Comment 69

[David Dahl :ddahl]

After working on the integration patch in bug 587734 and speaking with jst and mrbkap, it turns out that we need some tweaks to the platform bits so that we can return a COW JS object that can take advantage of __exposedProps__ and do away with the IDL altogether. jst can create a new patch after mrbkap lands bug 580128. In the meantime I have hacked up the IDL so that it has an additional 20 args (lame, I know) - which is only a stopgap measure.

Comment 70

[Kevin Dangoor]

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment 71

[David Dahl :ddahl]

Just wanted to note that this is only blocking b6/b7 because we would like it to bake. The integration patch is coming along well in bug 587734, which this depends on (for the console to take advantage) so it should be able to land soon.

Comment 72

[David Dahl :ddahl]

Attachment: Test Asserting

Not sure if this is trouble or not as the console is being called from chrome, and a real test would be from content.

Comment 73

[Johnathan Nightingale [:johnath]]

Kevin asserts that this can be post-feature-freeze. Moving to betaN+

Comment 74

[David Dahl :ddahl]

Wacku

Comment 75

[David Dahl :ddahl]

(In reply to comment #74)
> Wacku

saved a typo, whoops.

Just some feedback for jst:

Wacky thing happened as I am trying to get my unbitrotted patch working properly. For the fun of it i typed console.init(window) into the console and it crashed HARD. no assertion or message or prompt to break into the code. Since you can see all of the component properties - at this point - you can easily crash Fx. This is probably not a big deal as we are hoping to land this with "__exposedProps__" and proper wrapping of the console.

Comment 76

[David Dahl :ddahl]

Attachment: window.console Registration and Initialization (Updated)

jst updated this patch for me on my machine after the Compartments changes landed

Comment 77

[David Dahl :ddahl]

Attachment: v 0.1.13 ConsoleApi xpcom component, ConsoleStorage xpcom service

Rebased

Comment 78

[Johnny Stenback (:jst)]

Attachment: Updated window.console registration patch.

This replaces the previous "window.console Registration and Initialization (Updated)" patch, and should fix bug 604760. ddahl, can you test?

Comment 79

[David Dahl :ddahl]

Comment on attachment 483550 [details] [diff] [review]
v 0.1.13 ConsoleApi xpcom component, ConsoleStorage xpcom service

I am hoping we can get rid of the IDL for the ConsoleAPI component on this patch now that compartments have landed.

Our biggest issue is that we cannot have arbitrary numbers of args passed to the API

Comment 80

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 483550 [details] [diff] [review]
v 0.1.13 ConsoleApi xpcom component, ConsoleStorage xpcom service

>diff --git a/toolkit/components/console/hudservice/ConsoleAPI.js b/toolkit/components/console/hudservice/ConsoleAPI.js

>+  getWindowId: function CA_getWindowId(aOuterWindow)

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

>+function getWindowId(aWindow)
>+function getWindowByWindowId(aId)

See comments relevant to these in bug 587734 comment 77.

Comment 82

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #78)
> Created attachment 486143 [details] [diff] [review]
> Updated window.console registration patch.

Filed bug 610714 for this patch.

I think the console storage part of this patch is best deferred to another bug. I think this is ready to land with that change and the changes to this component in the patch for bug 587734 rolled in.

Comment 83

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 483550 [details] [diff] [review]
v 0.1.13 ConsoleApi xpcom component, ConsoleStorage xpcom service

A couple of other things I noticed:

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

> MODULE = hudservice

You should probably change this to "console" before we actually make use of it (that requires updating the package-manifest.in XPT addition as well). 

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

>+function createManyLogEntries()
>+{
>+  for (let i = 0; i < 55; i++) {
>+    win.wrappedJSObject.console.log("My message number", i);
>+  }
>+  ok(consoleStorage.getConsoleStorage(windowId).length < 201,

Shouldn't you call log more than 55 times if the goal is to test that no more than 200 entries are kept?

Comment 84

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #83)
> Shouldn't you call log more than 55 times if the goal is to test that no more
> than 200 entries are kept?

Though I guess this is a consoleStorage test, and we're removing that from this patch, so no need to worry about it now.

Comment 85

[:Gavin Sharp [email: [e-mail]]]

Attachment: ConsoleAPI component, comments addressed

This is the previous patch, with comments in this bug addressed and the changes from bug 587734 rolled in. I think this is ready to land, though my build with it is still running and I haven't been able to run tests yet.

Comment 86

[David Dahl :ddahl]

Comment on attachment 489262 [details] [diff] [review]
ConsoleAPI component, comments addressed

looks good to me, will run tests momentarily

Comment 87

[:Gavin Sharp [email: [e-mail]]]

Attachment: ConsoleAPI component, typo fix

Comment 88

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

I realized later that this can't be enabled without bug 587734, because otherwise it will interfere with existing console tests (because it overrides window.console methods). So this patch disables the global property for the moment. It doesn't interfere with any existing tests, and passes its own test (also disabled while the functionality is).

Some other thoughts that crossed my mind:
- this should probably live in the core, rather than in a toolkit directory that only Firefox builds.
- this is likely to have much more web compatibility impact than the current setup (which only affects people who open the console, and only on the window/tab that they open it on). We'll need to at the very least keep an eye on that. I hadn't really considered that aspect before; it makes me a bit nervous to be doing this at a late stage in the release cycle. 
- it seems like we should also be making an effort to standardize this API, like we do with any other web-exposed functionality. Have we made any efforts on that front?

Comment 89

[David Dahl :ddahl]

(In reply to comment #88)
> Created attachment 489452 [details] [diff] [review]
> updated patch
> 
> I realized later that this can't be enabled without bug 587734, because
> otherwise it will interfere with existing console tests (because it overrides
> window.console methods). So this patch disables the global property for the
> moment. It doesn't interfere with any existing tests, and passes its own test
> (also disabled while the functionality is).

I was confused why you wanted to split up the patch on 587734, it felt like a waste of time. It does not matter - as all of these patches depend on each other. The patches as I have been working on them work (barring the platform issue you found yesterday) - with all tests passing on my local build and on tinderbox.

> 
> Some other thoughts that crossed my mind:
> - this should probably live in the core, rather than in a toolkit directory
> that only Firefox builds.

jst said we could move the console component code into dom/ - but I think we kept it in toolkit for now to keep it all together. Wasn't sure if it mattered.

> - this is likely to have much more web compatibility impact than the current
> setup (which only affects people who open the console, and only on the
> window/tab that they open it on).

That was the whole point of all of this, to have an always-on console object. In fact, this code will give you a console on any chrome window as well. (no tested)

> We'll need to at the very least keep an eye
> on that. I hadn't really considered that aspect before; it makes me a bit
> nervous to be doing this at a late stage in the release cycle. 
> - it seems like we should also be making an effort to standardize this API,
> like we do with any other web-exposed functionality. Have we made any efforts
> on that front?

Firebug has basically done that already (de facto). We are following their lead here, as did webkit.
No one has tried to officially standardize things.

Comment 90

[David Dahl :ddahl]

(In reply to comment #88)

> - this is likely to have much more web compatibility impact than the current
> setup (which only affects people who open the console, and only on the
> window/tab that they open it on). We'll need to at the very least keep an eye
> on that. I hadn't really considered that aspect before; it makes me a bit
> nervous to be doing this at a late stage in the release cycle. 

Also, many people think the console is incomplete without being able to cache messages before they are displayed in the console. Without the lazy console and new ConsoleStorageService, this is not possible. The followups to this will also unhook the observers from the HUDService constructor so we can observe errors, cache them, correlate them with a window ID and display them when a web console is opened. This is a longstanding critique of Firebug that we are trying to make work.

Comment 91

[John J. Barton]

(In reply to comment #90)
>...This is a longstanding critique of Firebug that we are
> trying to make work.

I've never seen any such critique. Quite the opposite, users want "off" to mean "don't do anything that will have any impact on the browser".  I suppose this critique is really a misunderstanding.

Comment 92

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #91)
> (In reply to comment #90)
> >...This is a longstanding critique of Firebug that we are
> > trying to make work.
> 
> I've never seen any such critique. Quite the opposite, users want "off" to mean
> "don't do anything that will have any impact on the browser".  I suppose this
> critique is really a misunderstanding.

I think so. I believe David's referring to the confusion of some users around Firebug's activation model or maybe around the inability to properly associate all messages with a given site/tab.

In any case, having cached messages we can associate with particular consoles will be a big win if we can get it.

Comment 93

[John J. Barton]

(In reply to comment #92)
> I think so. I believe David's referring to the confusion of some users around
> Firebug's activation model or maybe around the inability to properly associate
> all messages with a given site/tab.
> 
> In any case, having cached messages we can associate with particular consoles
> will be a big win if we can get it.

But my point is that the whole reason we have the idiotic activation model is specifically to insure that development-only information does not impact performance. Caching information in case developers need it impacts performance when they don't.

Comment 94

[Nochum Sossonko [:Natch]]

If a web author decides to implement their own "console" object or to overwrite the functions, developers will not be able to see any errors/warnings/logs. Is that correct?

Comment 95

[David Dahl :ddahl]

(In reply to comment #94)
> If a web author decides to implement their own "console" object or to overwrite
> the functions, developers will not be able to see any errors/warnings/logs. Is
> that correct?

They will not see the messages in the web console, no.

Comment 96

[David Dahl :ddahl]

(In reply to comment #95)

> They will not see the messages in the web console, no.

However - we do warn the user that the web console api has been replaced.

Comment 97

[Nochum Sossonko [:Natch]]

(In reply to comment #96)
> (In reply to comment #95)
> 
> > They will not see the messages in the web console, no.
> 
> However - we do warn the user that the web console api has been replaced.

That just explains to the user why he can't see errors/messages, doesn't give him the option of getting them though. I've commented in bug 602006 about this.

Comment 98

[Rob Campbell [:rc] (:robcee)]

I think most sites declare a console object if one isn't found. Or do so to prevent logging on sites that they'd rather not expose debugging information to users on.

We don't want to get into an arms race here. We're providing something web developers can use in a consistent way with their chosen debugging tools or browsers. If developers do something that breaks this functionality, it's up to them.

as for your concern about performance impact, John. We're measuring the impact along the way with try builds. If there's a hit, we'll have to evaluate if it's something we can live with at landing time.

Have we seen any regressions in performance with these patches in place, David?

Comment 99

[John J. Barton]

(In reply to comment #98)
...
> as for your concern about performance impact, John. We're measuring the impact
> along the way with try builds. 

But the real world case is error/warning messages generated by long running AJAX applications.  Buffering errors doesn't scale.

Comment 100

[David Dahl :ddahl]

(In reply to comment #93)
> But my point is that the whole reason we have the idiotic activation model is
> specifically to insure that development-only information does not impact
> performance. Caching information in case developers need it impacts performance
> when they don't.

Which is why i filed bug 611032

Comment 101

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #89)
> I was confused why you wanted to split up the patch on 587734, it felt like a
> waste of time. It does not matter - as all of these patches depend on each
> other. The patches as I have been working on them work (barring the platform
> issue you found yesterday) - with all tests passing on my local build and on
> tinderbox.

Keeping the separate pieces separate has value, even if the end goal is to get everything landed. This work impacts more than just Firefox, for example, while the followup to hook up the HUD service doesn't. Splitting work into independent pieces helps review (both now and in the future when people try to figure out what we changed).

> jst said we could move the console component code into dom/ - but I think we
> kept it in toolkit for now to keep it all together. Wasn't sure if it mattered.

We should do that, I think. It's cheap to move.

> Firebug has basically done that already (de facto). We are following their lead
> here, as did webkit.

Does WebKit's API match ours exactly? Is it always-on the same way this one is, regardless of whether the user activates the console?

Comment 102

[Kevin Dangoor]

(In reply to comment #101)
> Does WebKit's API match ours exactly? Is it always-on the same way this one is,
> regardless of whether the user activates the console?

WebKit and Firebug both implement some additional methods on the console object (not always the same additional methods), but the console.log/error/warn api is consistent among all of these.

Chrome, at least, hangs onto log output even when the console is not visible. (Note that Chrome's log output is not as useful as the Web Console's which includes network traffic, imho)

Comment 103

[David Dahl :ddahl]

(In reply to comment #101)
> the followup to hook up the HUD service doesn't. Splitting work into
> independent pieces helps review (both now and in the future when people try to
> figure out what we changed).

I understand the rationale and agree with it...

> 
> > jst said we could move the console component code into dom/ - but I think we
> > kept it in toolkit for now to keep it all together. Wasn't sure if it mattered.
> 
> We should do that, I think. It's cheap to move.
ok, as a followup bug? 

> 
> Does WebKit's API match ours exactly? Is it always-on the same way this one is,
> regardless of whether the user activates the console?
Yes and yes. This was the impetus for this bug.

Comment 104

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #103)
> (In reply to comment #101)

> > We should do that, I think. It's cheap to move.
> ok, as a followup bug? 

I think we should do it as part of this. Might as well land in its final destination.

(sorry)

Comment 105

[:Gavin Sharp [email: [e-mail]]]

Attachment: moved to dom/

I moved it to dom/base. Global property addition is still disabled for the moment.

Comment 106

[:Gavin Sharp [email: [e-mail]]]

Attachment: moved to dom/, v2

with a test fix to make it work when re-enabled.

Comment 107

[Johnny Stenback (:jst)]

Comment on attachment 489596 [details] [diff] [review]
moved to dom/, v2

sr=jst, but this "API" will change, the long term plan is not to ship IDL for the console API is it? Once 610714 lands we can remove the IDL, if our plan is to do that for 4, maybe we should land that first and never expose this IDL to the world at all? I'm fine either way, just suggesting we might be able to lower the amount of API changes if we swap the order of things a bit here...

Comment 108

[:Gavin Sharp [email: [e-mail]]]

Attachment: moved to dom/, v3 (simplified)

Indeed, it seems like this works without the interface. This simplifies the component considerably. I made some improvements to the test to cover different scenarios, it still passes (you'll need to undo the disabling in ConsoleAPI.manifest and browser_ConsoleAPITests.js to run it)

Comment 109

[:Gavin Sharp [email: [e-mail]]]

Bug 610078 was fixed recently, which is probably what made this work.

Comment 110

[David Dahl :ddahl]

Comment on attachment 489648 [details] [diff] [review]
moved to dom/, v3 (simplified)

This just crashes firefox:

(gdb) bt
#0  0xb7835416 in __kernel_vsyscall ()
#1  0xb44031a6 in nanosleep () at ../sysdeps/unix/syscall-template.S:82
#2  0xb4402fa0 in __sleep (seconds=0) at ../sysdeps/unix/sysv/linux/sleep.c:138
#3  0xb534f199 in ah_crap_handler (signum=6) at /home/ddahl/code/moz/mozilla-central/mozilla/toolkit/xre/nsSigHandlers.cpp:132
#4  <signal handler called>
#5  0xb7835416 in __kernel_vsyscall ()
#6  0xb7813990 in raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42
#7  0xb6d2c6ad in JS_Assert (s=0xb759a268 "thread->data.nativeStackBase == GetNativeStackBase()", file=0xb7599f60 "/home/ddahl/code/moz/mozilla-central/mozilla/js/src/jscntxt.cpp", ln=645) at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsutil.cpp:83
#8  0xb6c0e4ef in js_CurrentThread (rt=0xb21af000) at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jscntxt.cpp:645
#9  0xb6c0e510 in js_InitContextThread (cx=0xb3b5a240) at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jscntxt.cpp:653
#10 0xb6c0ea40 in js_NewContext (rt=0xb21af000, stackChunkSize=8192) at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jscntxt.cpp:790
#11 0xb6bd37e1 in JS_NewContext (rt=0xb21af000, stackChunkSize=8192) at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsapi.cpp:970
#12 0xb5c5b88e in nsJSContext::nsJSContext (this=0xaf767680, aRuntime=0xb21af000) at /home/ddahl/code/moz/mozilla-central/mozilla/dom/base/nsJSEnvironment.cpp:1303
#13 0xb5c63126 in nsJSRuntime::CreateContext (this=0xb02b4510, aContext=0xbf841050) at /home/ddahl/code/moz/mozilla-central/mozilla/dom/base/nsJSEnvironment.cpp:3897
#14 0xb5c421c4 in nsXULPDGlobalObject::EnsureScriptEnvironment (this=0xaf7b17c0, lang_id=2) at /home/ddahl/code/moz/mozilla-central/mozilla/content/xul/document/src/nsXULPrototypeDocument.cpp:725
#15 0xb5c424ef in nsXULPDGlobalObject::GetScriptContext (this=0xaf7b17c0, lang_id=2) at /home/ddahl/code/moz/mozilla-central/mozilla/content/xul/document/src/nsXULPrototypeDocument.cpp:761
#16 0xb5f3cb5f in nsXULPrototypeScript::Compile (this=0xaed5ac40, aText=0xaed75008, aTextLength=9426, aURI=0xaf761f00, aLineNo=1, aDocument=0xb02ef400, aGlobalOwner=0xaf7705e0) at /home/ddahl/code/moz/mozilla-central/mozilla/content/xul/content/src/nsXULElement.cpp:3186
#17 0xb5c30d34 in nsXULDocument::OnStreamComplete (this=0xb02ef400, aLoader=0xaed5d130, context=0x0, aStatus=0, stringLen=9426, 
    string=0xb21b1000 "/* -*- Mode: C; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-\n *\n * ***** BEGIN LICENSE BLOCK *****\n * Version: MPL 1.1/GPL 2.0/LGPL 2.1\n *\n * The contents of this file are subject to the"...)
    at /home/ddahl/code/moz/mozilla-central/mozilla/content/xul/document/src/nsXULDocument.cpp:3507
#18 0xb53cca94 in nsStreamLoader::OnStopRequest (this=0xaed5d130, request=0xaf7328a0, ctxt=0x0, aStatus=0) at /home/ddahl/code/moz/mozilla-central/mozilla/netwerk/base/src/nsStreamLoader.cpp:125
#19 0xb537f48f in nsBaseChannel::OnStopRequest (this=0xaf732870, request=0xaf7b1940, ctxt=0x0, status=0) at /home/ddahl/code/moz/mozilla-central/mozilla/netwerk/base/src/nsBaseChannel.cpp:727
#20 0xb5394437 in nsInputStreamPump::OnStateStop (this=0xaf7b1940) at /home/ddahl/code/moz/mozilla-central/mozilla/netwerk/base/src/nsInputStreamPump.cpp:578
#21 0xb5393cd5 in nsInputStreamPump::OnInputStreamReady (this=0xaf7b1940, stream=0xb21cbf2c) at /home/ddahl/code/moz/mozilla-central/mozilla/netwerk/base/src/nsInputStreamPump.cpp:403
#22 0xb69bfe64 in nsInputStreamReadyEvent::Run (this=0xaf7956c0) at /home/ddahl/code/moz/mozilla-central/mozilla/xpcom/io/nsStreamUtils.cpp:112
#23 0xb69ead78 in nsThread::ProcessNextEvent (this=0xb3bde1f0, mayWait=1, result=0xbf84146c) at /home/ddahl/code/moz/mozilla-central/mozilla/xpcom/threads/nsThread.cpp:609
#24 0xb6975f95 in NS_ProcessNextEvent_P (thread=0xb3bde1f0, mayWait=1) at nsThreadUtils.cpp:250
#25 0xb639a514 in nsXULWindow::ShowModal (this=0xb0248660) at /home/ddahl/code/moz/mozilla-central/mozilla/xpfe/appshell/src/nsXULWindow.cpp:419
#26 0xb6395e6f in nsContentTreeOwner::ShowAsModal (this=0xb028b340) at /home/ddahl/code/moz/mozilla-central/mozilla/xpfe/appshell/src/nsContentTreeOwner.cpp:551
#27 0xb634b9c6 in nsWindowWatcher::OpenWindowJSInternal (this=0xb02a4d00, aParent=0x0, aUrl=0xb70ad440 "chrome://mozapps/content/profile/profileSelection.xul", aName=0xb70ac59b "_blank", aFeatures=0xb70ac578 "centerscreen,chrome,modal,titlebar", aDialog=1, 
    argv=0xb02acb80, aCalledFromJS=0, _retval=0xbf841a28) at /home/ddahl/code/moz/mozilla-central/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:1010
#28 0xb63490c1 in nsWindowWatcher::OpenWindow (this=0xb02a4d00, aParent=0x0, aUrl=0xb70ad440 "chrome://mozapps/content/profile/profileSelection.xul", aName=0xb70ac59b "_blank", aFeatures=0xb70ac578 "centerscreen,chrome,modal,titlebar", aArguments=0xb0257940, 
    _retval=0xbf841a28) at /home/ddahl/code/moz/mozilla-central/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:423
#29 0xb533b112 in ShowProfileManager (aProfileSvc=0xb3b97340, aNative=0xb3b96150) at /home/ddahl/code/moz/mozilla-central/mozilla/toolkit/xre/nsAppRunner.cpp:1992
#30 0xb533c62e in SelectProfile (aResult=0xbf841fdc, aNative=0xb3b96150, aStartOffline=0xbf841fd8, aProfileName=0xbf842114) at /home/ddahl/code/moz/mozilla-central/mozilla/toolkit/xre/nsAppRunner.cpp:2231
#31 0xb533fe6f in XRE_main (argc=3, argv=0xbf842314, aAppData=0xb3b0e380) at /home/ddahl/code/moz/mozilla-central/mozilla/toolkit/xre/nsAppRunner.cpp:3316
#32 0x08049817 in main (argc=3, argv=0xbf842314) at /home/ddahl/code/moz/mozilla-central/mozilla/browser/app/nsBrowserApp.cpp:158
(gdb) call DumpJSStack()
zsh: segmentation fault  sudo gdb ./dist/bin/firefox-bin 4876

Comment 111

[David Dahl :ddahl]

I have deleted my obj dir 2 times now and rebuilt. Something else is causing my browser to crash. sigh.

Comment 112

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 489648 [details] [diff] [review]
moved to dom/, v3 (simplified)

I pushed this to try and it seems to be fine: http://tbpl.mozilla.org/?tree=MozillaTry&rev=ec53ff5ee6a4 , though that's with the functionality/test disabled.

Looks like your crash is bug 608526 (I see you commented there).

Comment 113

[David Dahl :ddahl]

Just filed another JS crasher while testing this, looks unrelated though: Bug 611401

Comment 114

[David Dahl :ddahl]

Comment on attachment 489648 [details] [diff] [review]
moved to dom/, v3 (simplified)

ok, this works.

Comment 115

[David Dahl :ddahl]

hmmm, getting some test failures now:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_593003_iframe_wrong_hud.js | Test
 timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_595934_message_categories.js | Te
st timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_595934_message_categories.js | te

this is after I disable the manifest again. and build like so on windows:

make -C dom && make -C layout/build && make -C toolkit/ && make -C toolkit/library/

Comment 116

[David Dahl :ddahl]

(In reply to comment #115)
> hmmm, getting some test failures now:
> 
> TEST-UNEXPECTED-FAIL |

Never trust an old obj_dir's crufty tests. I thought running make -C toolkit/components/console/hudservice would overwrite all of the tests?

All tests pass. Can we land these two patches then? There are now further issues with the patch in bug 587734 - one of which is how we determine that the console object on the window is ours, not a 3rd party console. I was doing a console instanceof Ci.nsIConsoleAPI, but that will not work anymore with no IDL. ideas? This issue might have to be cleared up in this patch.

Comment 117

[:Gavin Sharp [email: [e-mail]]]

Landed:
http://hg.mozilla.org/mozilla-central/rev/edbc905d2cc2

Comment 118

[Philip Chee]

Gavin weren't you going to file a bug against SeaMonkey for the packaging changes?