Bug 568629 (lazy-console)

Small footprint, lazy console component

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
9 years ago
10 months ago

People

(Reporter: ddahl, Assigned: ddahl)

Tracking

Trunk
x86
All
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(1 attachment, 27 obsolete attachments)

13.67 KB, patch
ddahl
: feedback+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
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.
Severity: normal → blocker
Priority: -- → P1
(Assignee)

Comment 2

9 years ago
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.
(Assignee)

Updated

9 years ago
Assignee: ddahl → nobody
(Assignee)

Updated

9 years ago
Depends on: 578353
(Assignee)

Updated

9 years ago
blocking2.0: --- → ?
(Assignee)

Comment 3

9 years ago
(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?
(Assignee)

Comment 4

9 years ago
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:)
(Assignee)

Updated

9 years ago
Assignee: nobody → ddahl

Comment 5

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

Comment 6

9 years ago
(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.
(Assignee)

Comment 7

9 years ago
(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
blocking2.0: ? → betaN+
(Assignee)

Comment 8

9 years ago
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.
Attachment #461427 - Flags: feedback?(jst)
(Assignee)

Comment 9

9 years ago
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.
Attachment #461427 - Attachment is obsolete: true
Attachment #461427 - Flags: feedback?(jst)
(Assignee)

Comment 10

9 years ago
CCing JJB to get feedback about how Firebug might use this.
(Assignee)

Comment 11

9 years ago
Attachment #461598 - Attachment is obsolete: true

Comment 12

9 years ago
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.
(Assignee)

Comment 13

9 years ago
(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.
(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

9 years ago
(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

9 years ago
(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.
(Assignee)

Updated

9 years ago
Summary: Small footprint, efficient global console service → Small footprint, lazy console component
(Assignee)

Comment 17

9 years ago
(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.
(Assignee)

Comment 18

9 years ago
Saving work, have test suites going.
Attachment #462269 - Attachment is obsolete: true
(Assignee)

Comment 19

9 years ago
jst:

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

Updated

9 years ago
Whiteboard: [kd4b4]
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
(Assignee)

Comment 22

9 years ago
Behold! passing test suites but alas, the addition of Firefox-specific gBrowser calls. How can we get rid of those?
Attachment #462593 - Attachment is obsolete: true
Attachment #462969 - Flags: feedback?
(Assignee)

Updated

9 years ago
Attachment #462969 - Flags: feedback? → feedback?(rcampbell)
(Assignee)

Comment 23

9 years ago
(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;
+  },
(Assignee)

Comment 24

9 years ago
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.
Attachment #462969 - Attachment is obsolete: true
Attachment #463363 - Flags: feedback?
Attachment #462969 - Flags: feedback?(rcampbell)
(Assignee)

Updated

9 years ago
Attachment #463363 - Flags: feedback?
(Assignee)

Comment 25

9 years ago
(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.
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 :)
(Assignee)

Comment 27

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

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

Updated

9 years ago
Depends on: 585232
(Assignee)

Comment 28

9 years ago
(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
(Assignee)

Comment 29

9 years ago
This thing is leaking like a sieve, do you have any idea what it might be?
Attachment #463363 - Attachment is obsolete: true
Attachment #464245 - Flags: feedback?(sdwilsh)
(Assignee)

Comment 30

9 years ago
Leaky test or leaky service? what is going on here? Let me know if you don't have time to look at this.
Attachment #464245 - Attachment is obsolete: true
Attachment #464246 - Flags: feedback?(sdwilsh)
Attachment #464245 - Flags: feedback?(sdwilsh)
(Assignee)

Comment 31

9 years ago
(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
You also need to build in layout/build if you're changing DOM stuff.
(Assignee)

Comment 33

9 years ago
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
(Assignee)

Comment 34

9 years ago
Attachment #464246 - Attachment is obsolete: true
Attachment #464246 - Flags: feedback?(sdwilsh)
(Assignee)

Comment 35

9 years ago
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?
I *think* you want the value property to be the contract id string, not the class id as a string.

Comment 37

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

https://developer.mozilla.org/en/XPCOMUtils.jsm
(Assignee)

Comment 38

9 years ago
(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:
(Assignee)

Comment 39

9 years ago
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.
(Assignee)

Comment 40

9 years ago
Attachment #464971 - Attachment is obsolete: true
Attachment #465495 - Flags: feedback?(jst)
(Assignee)

Comment 41

9 years ago
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
(Assignee)

Comment 42

9 years ago
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}]
(Assignee)

Updated

9 years ago
Attachment #465495 - Attachment is patch: true

Updated

9 years ago
Whiteboard: [kd4b4] → [kd4b5]
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.
(Assignee)

Comment 44

9 years ago
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.
(Assignee)

Comment 45

9 years ago
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.
(Assignee)

Comment 46

9 years ago
<saving work to move to another machine>
(Assignee)

Updated

9 years ago
Blocks: 587734
(Assignee)

Comment 47

9 years ago
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.
Attachment #465495 - Attachment is obsolete: true
Attachment #465815 - Attachment is obsolete: true
Attachment #466510 - Flags: review?(dietrich)
Attachment #465495 - Flags: feedback?(jst)
(Assignee)

Updated

9 years ago
No longer depends on: 585232
(Assignee)

Updated

9 years ago
Attachment #463676 - Flags: review?(mrbkap)

Updated

9 years ago
Duplicate of this bug: 552143

Updated

9 years ago
Duplicate of this bug: 570543
This blocks a feature, so it's beta5 or bust!
blocking2.0: betaN+ → beta5+
(Assignee)

Updated

9 years ago
Depends on: 589329
(Assignee)

Comment 51

9 years ago
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 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.
Attachment #466510 - Flags: review?(dietrich) → review-
(Assignee)

Comment 53

9 years ago
(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.
Attachment #463676 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 54

9 years ago
Attachment #468533 - Flags: review?(dietrich)
(Assignee)

Updated

9 years ago
Attachment #466510 - Attachment is obsolete: true
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.
Attachment #468533 - Flags: review?(dietrich) → review-
(Assignee)

Comment 56

9 years ago
(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.
(Assignee)

Comment 57

9 years ago
Actually, I just changed it back to the original idl method
(Assignee)

Comment 58

9 years ago
Also filed bug 590257 on the mochitest leakage
Attachment #468533 - Attachment is obsolete: true
Attachment #468762 - Flags: review?(dietrich)
(Assignee)

Comment 59

9 years ago
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.
(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 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.
Attachment #468762 - Flags: review?(dietrich) → review-
(Assignee)

Comment 62

9 years ago
(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?
(Assignee)

Comment 63

9 years ago
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?
Attachment #468762 - Attachment is obsolete: true
Attachment #469734 - Flags: review?(dietrich)
(Assignee)

Comment 64

9 years ago
a few tweaks to the xpcshell test
Attachment #469734 - Attachment is obsolete: true
Attachment #469750 - Flags: review?(dietrich)
Attachment #469734 - Flags: review?(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.
Attachment #469750 - Flags: review?(dietrich) → review+

Comment 66

9 years ago
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.
blocking2.0: beta5+ → ?

Updated

9 years ago
Whiteboard: [kd4b5] → [kd4b6]
--> beta6+ as per comment 66
blocking2.0: ? → beta6+
(Assignee)

Comment 68

9 years ago
Fixed as per review comments
Attachment #470607 - Flags: review+
(Assignee)

Comment 69

9 years ago
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.
(Assignee)

Updated

9 years ago
Depends on: 580128

Comment 70

9 years ago
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal

Updated

9 years ago
Blocks: devtools4b7
(Assignee)

Comment 71

9 years ago
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.
(Assignee)

Comment 72

9 years ago
Posted file Test Asserting (obsolete) —
Not sure if this is trouble or not as the console is being called from chrome, and a real test would be from content.
(Assignee)

Updated

9 years ago
Whiteboard: [kd4b6] → [kd4b6] [ready for landing after a few tests] [eta 2010-09-22]
Kevin asserts that this can be post-feature-freeze. Moving to betaN+
blocking2.0: beta7+ → betaN+

Updated

9 years ago
Whiteboard: [kd4b6] [ready for landing after a few tests] [eta 2010-09-22] → [ready for landing after a few tests] [eta 2010-09-22]
(Assignee)

Comment 74

9 years ago
Wacku
Whiteboard: [ready for landing after a few tests] [eta 2010-09-22]
(Assignee)

Comment 75

9 years ago
(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.

Updated

9 years ago
Blocks: devtools4b8
No longer blocks: devtools4b7
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Attachment #469750 - Attachment is obsolete: true
(Assignee)

Comment 76

9 years ago
jst updated this patch for me on my machine after the Compartments changes landed
Attachment #463676 - Attachment is obsolete: true
(Assignee)

Comment 77

9 years ago
Rebased
Attachment #470607 - Attachment is obsolete: true
This replaces the previous "window.console Registration and Initialization (Updated)" patch, and should fix bug 604760. ddahl, can you test?
Attachment #482617 - Attachment is obsolete: true
(Assignee)

Comment 79

9 years ago
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
Attachment #483550 - Flags: review?(mrbkap)
Alias: lazy-console
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.
No longer depends on: 578353
Duplicate of this bug: 578353
(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 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?
Attachment #483550 - Flags: review?(mrbkap)
(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.
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.
Attachment #476027 - Attachment is obsolete: true
Attachment #483550 - Attachment is obsolete: true
Attachment #486143 - Attachment is obsolete: true
Attachment #489262 - Flags: review+
(Assignee)

Comment 86

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

looks good to me, will run tests momentarily
Attachment #489262 - Attachment is obsolete: true
Attachment #489326 - Flags: review+
Posted patch updated patch (obsolete) — Splinter Review
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?
Attachment #489326 - Attachment is obsolete: true
(Assignee)

Comment 89

9 years ago
(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.
(Assignee)

Comment 90

9 years ago
(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

9 years ago
(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.
(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

9 years ago
(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.
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?
(Assignee)

Comment 95

9 years ago
(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.
(Assignee)

Comment 96

9 years ago
(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.
(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.
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

9 years ago
(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.
(Assignee)

Comment 100

9 years ago
(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
(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

9 years ago
(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)
(Assignee)

Comment 103

9 years ago
(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.
(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)
Posted patch moved to dom/ (obsolete) — Splinter Review
I moved it to dom/base. Global property addition is still disabled for the moment.
Attachment #489452 - Attachment is obsolete: true
Attachment #489569 - Flags: superreview?(jst)
Posted patch moved to dom/, v2 (obsolete) — Splinter Review
with a test fix to make it work when re-enabled.
Attachment #489569 - Attachment is obsolete: true
Attachment #489569 - Flags: superreview?(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...
Attachment #489596 - Flags: superreview?(jst) → superreview+
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)
Attachment #489596 - Attachment is obsolete: true
Attachment #489648 - Flags: feedback?(ddahl)
Bug 610078 was fixed recently, which is probably what made this work.
(Assignee)

Comment 110

9 years ago
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
Attachment #489648 - Flags: feedback?(ddahl) → feedback-
(Assignee)

Comment 111

9 years ago
I have deleted my obj dir 2 times now and rebuilt. Something else is causing my browser to crash. sigh.
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).
Attachment #489648 - Flags: feedback- → feedback?(ddahl)
(Assignee)

Comment 113

9 years ago
Just filed another JS crasher while testing this, looks unrelated though: Bug 611401
(Assignee)

Comment 114

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

ok, this works.
Attachment #489648 - Flags: feedback?(ddahl) → feedback+
(Assignee)

Comment 115

9 years ago
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/
(Assignee)

Comment 116

9 years ago
(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.
Landed:
http://hg.mozilla.org/mozilla-central/rev/edbc905d2cc2
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 118

9 years ago
Gavin weren't you going to file a bug against SeaMonkey for the packaging changes?
Blocks: 612337, 612338
No longer depends on: 612337, 612338

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.