Closed Bug 851695 Opened 9 years ago Closed 9 years ago

Add a |console| object to every JSM global

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX
Firefox 23

People

(Reporter: msucan, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

We should add support for better message logging within jsm's, by default - better than dump().

We have a |console| implementation in toolkit/devtools/Console.jsm. We should expose that, without requiring an explicit Cu.import().
Attached patch patch that causes a crash (obsolete) — Splinter Review
This is the work in progress patch I have. Even if I removed the lazy getter, I still have the crash.

Any thoughts?

Here is my full patch queue:

https://github.com/mihaisucan/mozilla-patch-queue/tree/master/patches-console-api-global

Thank you!
Attachment #725724 - Flags: feedback?(gkrizsanits)
So the problem was in my patch that I quickly cooked-up for you. Forgot to addref in BackstagePass::GetHelperForLanguage. So the method should look something like:

NS_IMETHODIMP
BackstagePass::GetHelperForLanguage(uint32_t language,
                                    nsISupports **retval)
{
    *retval = static_cast<nsIGlobalObject*>(this);
    NS_ADDREF(this);
    return NS_OK;
}

Although probably I want to get away from the old style NS_ADDREF... anyway, if you want to play with it please adjust my patch accordingly and your patch should work. I will get back to this next week when I work again.
Comment on attachment 725724 [details] [diff] [review]
patch that causes a crash

Review of attachment 725724 [details] [diff] [review]:
-----------------------------------------------------------------

Mihai, did you have time to test if it works now with the mentioned fix?
Attachment #725724 - Flags: feedback?(gkrizsanits) → feedback+
I talked about this with Mihai today and this bug might or might not be needed... But since the last time when I tried to help him out with a patch we agreed that we need this part anyway, here is a patch for it.
Attachment #734663 - Flags: review?(bobbyholley+bmo)
Comment on attachment 734663 [details] [diff] [review]
PreCreate for BackstagePass

Review of attachment 734663 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for following up on this!

r=bholley with comments addressed

::: js/xpconnect/src/XPCRuntimeService.cpp
@@ +109,5 @@
>  BackstagePass::GetHelperForLanguage(uint32_t language,
>                                      nsISupports **retval)
>  {
> +    *retval = static_cast<nsIGlobalObject*>(this);
> +    NS_ADDREF(this);

Is the point of the static_cast to get us to the canonical nsISupports? I really don't want to depend on class inheritance ordering. In this case it actually doesn't matter at all (because the only caller of GetHelperForLanguage immediately QIs the result to nsIXPCScriptable), but I'd still like to do the safe thing here. So do:

nsCOMPtr<nsISupports> supports = do_QueryInterface(this);
supports.forget(*retval);
return NS_OK;

@@ +175,5 @@
> +BackstagePass::PreCreate(nsISupports *nativeObj, JSContext *cx,
> +                         JSObject *globalObj, JSObject **parentObj)
> +{
> +    // We do the same trick here as for WindowSH. Return the js global
> +    // as parent, so XPConenct can find the right scope and the wrapper

Make a note here that the BSP may or may not have been wrapped already, and this does the right thing in either case.

@@ +178,5 @@
> +    // We do the same trick here as for WindowSH. Return the js global
> +    // as parent, so XPConenct can find the right scope and the wrapper
> +    // that already exists.
> +    nsCOMPtr<nsIGlobalObject> global(do_QueryInterface(nativeObj));
> +    NS_ASSERTION(global, "nativeObj not a global object!");

MOZ_ASSERT
Attachment #734663 - Flags: review?(bobbyholley+bmo) → review+
(In reply to Bobby Holley (:bholley) from comment #5)
> Is the point of the static_cast to get us to the canonical nsISupports? I
> really don't want to depend on class inheritance ordering. In this case it
> actually doesn't matter at all (because the only caller of
> GetHelperForLanguage immediately QIs the result to nsIXPCScriptable), but
> I'd still like to do the safe thing here. So do:
> 
> nsCOMPtr<nsISupports> supports = do_QueryInterface(this);
> supports.forget(*retval);
> return NS_OK;

It does not matter really which nsISupport we return here since it has to be QI-d anyway by the consumer of this function I guess, but I didn't like this part either, I usually prefer QI over static cast. But since in nsXPCComponents::GetHelperForLanguage we does this I was wondering if this is in some hot call path where we care about the extra QI for some reason. Do you mind if I change it there too to this QI version in this patch? Just for consistency...
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6)
> It does not matter really which nsISupport we return here since it has to be
> QI-d anyway by the consumer of this function I guess, but I didn't like this
> part either, I usually prefer QI over static cast. But since in
> nsXPCComponents::GetHelperForLanguage we does this I was wondering if this
> is in some hot call path where we care about the extra QI for some reason.
> Do you mind if I change it there too to this QI version in this patch? Just
> for consistency...

Yeah, go for it. We only do this when creating XPCWNs, which is not exactly fast.
Actually this is the closes match that the compiler r+'s as well:

+    nsCOMPtr<nsISupports> supports;
+    QueryInterface(NS_GET_IID(nsISupports), getter_AddRefs(supports));
+    supports.forget(retval);

Because do_QueryInterface does not know which nsISupport to pick from the 'this' we have here. Alternately I can do a static cast and do the QI after that like:
nsCOMPtr<nsISupports> supports = do_QueryInterface(static_cast<nsIGlobalObject*>(this)); but that seems somewhat uglier to me.
do_QueryInterface is always preferred. The static_cast is a necessary and understood evil; you could use NS_ISUPPORTS_CAST(nsIGlobalObject*, this) if you really preferred.
(In reply to Josh Matthews [:jdm] from comment #9)
> do_QueryInterface is always preferred. The static_cast is a necessary and
> understood evil; you could use NS_ISUPPORTS_CAST(nsIGlobalObject*, this) if
> you really preferred.

I do what is the more consistent with the rest of the code. They are pretty equivalent to me. if the static_cast version is the preferred way then fine by me, thanks for the info :)
https://hg.mozilla.org/mozilla-central/rev/1381f98f22f5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Reopening: only Gabor's patch landed which is the PreCreate for BackstagePass thing that is required for adding an observer notification whenever a new JSM global is created. I did not finish the second patch which actually adds the notification and the |console| object to every JSM global.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch wip2Splinter Review
This patch works. It adds the |console| object as implemented by Console.jsm, to almost all jsm's that load after Services.jsm. Any console API call has output to the terminal and to the browser console. This is also remotable - you'll be able to call console.log() from any jsm and have the output show in the remote console.

I am going to push this patch to the try servers tomorrow once servers work again (IT problems for now).

Before I do further fixes for bustage, can you please check the patch and see if this is something we can land? Is the Services.jsm change acceptable? Should we put that observer in some other place?

Thanks!
Attachment #725724 - Attachment is obsolete: true
Attachment #747105 - Flags: feedback?(dtownsend+bugmail)
Putting this in Services.jsm seems quite hacky.

I guess I'm a bit skeptical that having this behavior for JSMs by default is a good idea. Having JSMs that want to use console.* import() the relevant module doesn't seem overly burdensome, and trying to put it in all JSMs by default has the potential to conflict with existing JSM variables.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #16)
> I guess I'm a bit skeptical that having this behavior for JSMs by default is
> a good idea. Having JSMs that want to use console.* import() the relevant
> module doesn't seem overly burdensome, and trying to put it in all JSMs by
> default has the potential to conflict with existing JSM variables.

I think its about expectations for new developers. console is an expectation these days because it's everywhere else.

We can always reject things because they might break something, and we have tests for this.
Comment on attachment 747105 [details] [diff] [review]
wip2

Review of attachment 747105 [details] [diff] [review]:
-----------------------------------------------------------------

I think adding console everywhere is the right thing to do. But we need something better than assuming Services.jsm is the first console to do it. This may mean we end up having to use C++, which is sort of icky but probably the only safe way.
Attachment #747105 - Flags: feedback?(dtownsend+bugmail) → feedback+
(In reply to Joe Walker [:jwalker] from comment #17)
> I think its about expectations for new developers. console is an expectation
> these days because it's everywhere else.

In chrome code?

Doesn't this implementation of console output to a different location than the one you'd get in a chrome window?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19)
> (In reply to Joe Walker [:jwalker] from comment #17)
> > I think its about expectations for new developers. console is an expectation
> > these days because it's everywhere else.
> 
> In chrome code?

dump() certainly disappointed me when I started on mozilla code, and I looked for console.log etc at the time. I'm trying to think of another JS engine that doesn't have a console object - maybe Rhino doesn't, but I don't think there are many.

> Doesn't this implementation of console output to a different location than
> the one you'd get in a chrome window?

(Assuming you mean content window here?) console.log from a content window goes to the webconsole in the developer tools. console.log from chrome code goes to dump() and to the browser console.
No, I was comparing this implementation (used for JSMs) vs. the one used for chrome windows (maybe they're the same?). Can you point me to the code that distinguishes chrome vs. content windows?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #16)
> Putting this in Services.jsm seems quite hacky.
> 
> I guess I'm a bit skeptical that having this behavior for JSMs by default is
> a good idea. Having JSMs that want to use console.* import() the relevant
> module doesn't seem overly burdensome, and trying to put it in all JSMs by
> default has the potential to conflict with existing JSM variables.

I thought of putting this in XPCOMUtils.jsm. This approach seems hacky, indeed - this is what we came up with at the meetup week in Sunnyvale (me and Mossop).

Dave's suggestion to do it from C++ makes most sense, however for me to implement that I need some guidance. I would like the actual console object implementation to continue to live in JavaScript - so maybe load a JS in C++ and take the exported |console| object from the file and add it to every new JSM global.

I think we can agree that the default JSM globals provide poor logging mechanisms - mainly dump(), then we have Cu.reportError(), and Services.console.logStringMessage(). The console API implemented in Console.jsm closely resembles that of the window.console API - objects you give to the log methods are inspectable in the Browser Console, you can get stacktraces, timers, and so on. We also try to provide nice formatted output to the terminal (admittedly this can be better).

The burden of having to Cu.import() Console.jsm manually should not be underestimated. Many don't know it exists, and trying to keep a patch in the queue, separately, that adds the Cu.import() to the files you work with is cumbersome.

As for conflicts: we have tests and this on its own should not be a reason to avoid improvements to the way we can debug code during development. This is not only an improvement for Mozilla, it's also for extension devs.


(In reply to Dave Townsend (:Mossop) from comment #18)
> Comment on attachment 747105 [details] [diff] [review]
> wip2
> 
> Review of attachment 747105 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think adding console everywhere is the right thing to do. But we need
> something better than assuming Services.jsm is the first console to do it.
> This may mean we end up having to use C++, which is sort of icky but
> probably the only safe way.

We could try XPCOMUtils.jsm. One thought here: if we can get the |console| object in 99% of jsm's that might be sufficient for practical purposes - it's rare we need it in very low-level jsm's. Still, a C++ way of loading the Console.jsm file and adding its |console| object to other jsm would be better. Can you please provide me with pointers to how I can do that?


(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19)
> (In reply to Joe Walker [:jwalker] from comment #17)
> > I think its about expectations for new developers. console is an expectation
> > these days because it's everywhere else.
> 
> In chrome code?

Why not? We get console.log() in nodejs.


> Doesn't this implementation of console output to a different location than
> the one you'd get in a chrome window?

Content window.console outputs to the web console. Chrome window.console outputs to the browser console - share the DOM implementation (dom/base/ConsoleAPI.js). The Console.jsm module is only for JSMs, and it outputs to the terminal and to the Browser Console - allowing object inspection and remoting (dump() doesn't give us this).

I hope that at this point we do not disagree on the goal of improving the development experience for Firefox developers and extension devs. The technical solution proposed in this patch is indeed something that can be improved upon. Suggestions are very much welcome.

Thank you!
(In reply to Mihai Sucan [:msucan] from comment #22)
> The burden of having to Cu.import() Console.jsm manually should not be
> underestimated. Many don't know it exists, and trying to keep a patch in the
> queue, separately, that adds the Cu.import() to the files you work with is
> cumbersome.

(Having it be defined magically won't really help with letting people know it exists.)

Let me be clear that I don't have any objection to fixing this bug, assuming the fix is relatively simple/clean, and doesn't cause compatibility issues.

I had some concerns:
a) the mechanism by which we were fixing it (observer in Services.jsm) was hacky
b) there are some inconsistencies between console.log in our various contexts (chrome window, content window, JSMs after this patch) that are confusing and make it not useful for actual chrome developers (e.g. me), which is the use case we're trying to address here

a) we can address with a cleaner solution.

b) isn't strictly related to this bug, so we don't necessarily need to dive into it here. But it's tied to the utility of the feature (and therefore the hackiness that I'd be willing to accept to have it), which is why I brought it up. I would appreciate an answer to comment 21 so that I can sort out how this stuff works.

(I have an obvious bias: I don't use console.log and don't see much value in it. Particularly if its output goes somewhere other than stdout and/or its behavior differs from the console.log in chrome windows. But I can appreciate that my perspective might not be as representative as I think it is, and I don't want my comments to be perceived as stop-energy.)
(In reply to Mihai Sucan [:msucan] from comment #22)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #16)
> (In reply to Dave Townsend (:Mossop) from comment #18)
> > Comment on attachment 747105 [details] [diff] [review]
> > wip2
> > 
> > Review of attachment 747105 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think adding console everywhere is the right thing to do. But we need
> > something better than assuming Services.jsm is the first console to do it.
> > This may mean we end up having to use C++, which is sort of icky but
> > probably the only safe way.
> 
> We could try XPCOMUtils.jsm. One thought here: if we can get the |console|
> object in 99% of jsm's that might be sufficient for practical purposes -
> it's rare we need it in very low-level jsm's. Still, a C++ way of loading
> the Console.jsm file and adding its |console| object to other jsm would be
> better. Can you please provide me with pointers to how I can do that?

I think if we're trying to make console.log standard then we should actually do so and not surprise people when certain jsms don't have it. I think the only way to do that safely is to do it explicitly from the c++, perhaps by adding an implicit call to the Cu.import code for every module to load in a "globals.jsm" or importing a JS component instance into the scope. Eddy can probably steer you better here than I.

> > Doesn't this implementation of console output to a different location than
> > the one you'd get in a chrome window?
> 
> Content window.console outputs to the web console. Chrome window.console
> outputs to the browser console - share the DOM implementation
> (dom/base/ConsoleAPI.js). The Console.jsm module is only for JSMs, and it
> outputs to the terminal and to the Browser Console - allowing object
> inspection and remoting (dump() doesn't give us this).
> 
> I hope that at this point we do not disagree on the goal of improving the
> development experience for Firefox developers and extension devs. The
> technical solution proposed in this patch is indeed something that can be
> improved upon. Suggestions are very much welcome.

We should endeavour to have the jsm console.* and the chrome window console.* do the same things (ideally by the same implementation but not necessarily). so that chrome developers only have to look in one place. Looks like the browser console is currently that place so that probably works out. Either way getting something landed and then iterating is best here I think.
(In reply to Mihai Sucan [:msucan] from comment #22)
> Dave's suggestion to do it from C++ makes most sense, however for me to
> implement that I need some guidance. I would like the actual console object
> implementation to continue to live in JavaScript - so maybe load a JS in C++
> and take the exported |console| object from the file and add it to every new
> JSM global.

I think what you probably want to do here, is as simple as calling mozJSComponentLoader::Import from mozJSComponentLoader::PrepareObjectForLocation after the point where we define all the utility functions on the global (JS_DefineProfilingFunctions), with.

You want to pass in the global as the target object, and check ofc beforehand, that the jsm we are loading and attaching the console to, is not the console.jsm itself.

This call will be just like if you'd called import from js.
Thank you Gabor. I will look into doing that.
By the way, if you want me to do it just let me know, just the last time we talked you were looking for some platform side hacking so I thought this might be a good one :)
Blocks: 877389
Unassigning myself: this bug is not high-priority for devtools. We mostly switched to the devtools loader which adds the console object for us.
Assignee: mihai.sucan → nobody
Can we just close it then? Since I think it was only important for devtools... I close it for now and if someone disagrees can reopen it.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → WONTFIX
Attachment #734663 - Flags: checkin+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.