The Console API should be using a proxy instead of __noSuchMethod__.

__noSuchMethod__ isn't used anymore

Sonny: this bug is still valid. See dom/base/ConsoleAPI.js - __noSuchMethod__ is still used.

Still valid. Filter on TARDIS.

Rob, please redirect review to the appropriate person. This patch doesn't actually replace __noSuchMethod__ with a proxy, it simply removes it. I was trying to implement a proxy solution but then realized that you have to expose properties via __exposedProps__ and proxied properties will never get there. So I ran a test *without* my patch to confirm that undefined method return undefined and not an empty function: [16:06:11.043] console.log [16:06:11.045] function log() { [native code] } [16:06:21.910] console.nomethod [16:06:21.911] undefined Try push: https://tbpl.mozilla.org/?tree=Try&rev=cdb7fb351b97

Nevermind, there are more oranges. FML.

yeah, was pretty sure this wouldn't cut it with our dom tests. Cancelling review.

Yeah I should have cancelled myself. I talked to some people on Friday and learned that a) you can't simply return a proxy from there and b) console code is very obscure because of all security stuff so I need to talk to someone who knows it well.

An `invoke` trap is being added to the Proxy API which can replace `__noSuchMethod__` (bug 878605).

Sweet. Will revisit this bug once bug 878605 is landed. Thanks!

I've been discussing this with bholley a bit. Basically the issue here is we don't have a clean way to return a Proxy from ConsoleAPI.init because we don't have a way to create a Proxy in the given window's compartment when it's a content window. Essentially we need an icky hack like `Cu.createProxyIn` or some more fundamental things need to be created, like Xrays for JavaScript builtins (we can only currently make Xray wrappers for DOM things).

I took a stab at hacking around something that would work, and it almost does. For some reason, though, when you try to access a property in content console that doesn't exist, it throws with "proxy cannot report a new property on non-extensible object" even though the proxied object is extensible. When using console from chrome it works as desired, so I presume this is some awkwardness with COWs. With the [[Invoke]] trap implemented, this solution would work (since you're allowed to invoke a non-existent property). It could also drop the hacking around with __exposedProps__.

Few small fixes.

As mentioned on IRC, I don't think we should go with this approach if we want a short-term fix. The safest thing is some sort of Cu.createProxyIn.

FWIW, bug 914970 may in fact be more tractable than I'd originally thought.

Proxy.invoke was removed from the latest ES6 spec draft, so whatever solution is implemented here, it won't be able to rely on that.

This bug is actually blocked on the addition of something like `Cu.createProxyIn`, rather than invoke. Having the get trap simply always return a function is good enough here, since all it needs to be is a no-op function.

(In reply to Brandon Benvie [:benvie] from comment #16) > This bug is actually blocked on the addition of something like > `Cu.createProxyIn`, rather than invoke. Having the get trap simply always > return a function is good enough here, since all it needs to be is a no-op > function. This may become possible with bug 933681. If it does though, I'd request that you take care to make sure _not_ to use __exposedProps__, since that's deprecated and going away. This effectively means that your proxy handler can't be a chrome object - you'll need to make your handler a content object with content functions. Let me know if you're working on this and I'll point you to the right stuff.

What about if we use WebIDL? ConsoleAPI in workers will be based on a WebIDL interface (bug 620935). This is a WIP. It works but the trace is empty, but I haven't investigated why yet.

baku++

This is the porting of Console API in WebIDL. It works. The only problem I have is about __mozillaConsole__ that is not supported by WebIDL because of the '__' at the beginning of the method/property name. I guess we should use a different way to check if Console is WebIDL or something else. If this bug is the wrong one for this patch, I can file a new one.

A small issue with time|timeEnd fixed

Comment on attachment 8359113 [details] [diff] [review] Console API in WebIDL >+ __noSuchMethod__: function(method, args) { Why do you need the args? You dropped a bunch of function names; I'd like someone who actually owns this code to review that part, please. >+nsGlobalWindow::GetConsole(ErrorResult& aRv) This seems to be duplicating, but somewhat differently, dom::ConstructJSImplementation. I'd rather we just asked bindings to pass us a JSContext, then set up the GlobalObject correctly and then called ConstructJSImplementation here. >+ void ___noSuchMethod__(DOMString methodName, any... data); Why do you need the args? >+// ConsoleAPI Is there really no spec for this? If there is, spec link, please! >+++ b/js/src/vm/OldDebugAPI.cpp Why are the changes here needed? I believe these will change the behavior of this API in quite undesirable ways (e.g. exposing cross-origin stack information) as things stand. Bobby, is that correct? If the issue is that we need to make the getStackTrace() call in queueCall work, then we should fix that part... somehow. But to the extent that the result is exposed to the content script we need to be really careful with it. :(

I just updated half of the bz's comments.

> >+// ConsoleAPI > > Is there really no spec for this? If there is, spec link, please! I don't think we have any spec for this API. Mihai, is it correct?

Closest I think there is, is this: https://github.com/DeveloperToolsWG/console-object

Comment on attachment 8359667 [details] [diff] [review] capi.patch Review of attachment 8359667 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good. However, toolkit/ and browser/ web console tests fail. I also see failures in the dom/base ConsoleAPI tests. More comments below. Also, output for console.assert() is broken. Try console.assert(0, "foobar %s", document.title, window) with and without this patch. (I only reviewed ConsoleAPI.js) ::: dom/base/ConsoleAPI.js @@ +54,5 @@ > _windowDestroyed: false, > _timer: null, > > // nsIDOMGlobalPropertyInitializer > + init: function(aWindow) { Are these changes needed for this patch? They cause no harm, but they dont seem to be needed either. ::: dom/webidl/Window.webidl @@ +345,5 @@ > + > +// ConsoleAPI > +partial interface Window { > + [GetterThrows] > + readonly attribute Console console; window.console is not readonly, we explicitly allow it to be changed.

> window.console is not readonly, we explicitly allow it to be changed. Just mark it [Replaceable] and that should work.

By the way, I'm assuming that the worker console should forward to the real console even if the page has set window.console...

> window.console is not readonly, we explicitly allow it to be changed. does it mean I can replace the console with any kind of jsvalue? For instance: window.console = 42 ?

(In reply to Andrea Marchesini (:baku) from comment #29) > does it mean I can replace the console with any kind of jsvalue? > For instance: window.console = 42 ? Yes

(In reply to Andrea Marchesini (:baku) from comment #29) > > window.console is not readonly, we explicitly allow it to be changed. > > does it mean I can replace the console with any kind of jsvalue? > For instance: window.console = 42 ? Yes, as Till said.

The OldDebugAPI changes are ok, because now we pass in a principal, which is used in a per-frame subsumes check.

OK, but does console feed the resulting string to the web page in any way? Why did we not make this change in bug 924905?

(In reply to Boris Zbarsky [:bz] from comment #33) > OK, but does console feed the resulting string to the web page in any way? Hm. I guess the principal check is only useful if the code that's invoking it isn't running with the system principal (which the JS-implemented WebIDL would be). Maybe we should enter the compartment of the script doing the call in C++, and then it wouldn't matter if the value was fed to a web page? > Why did we not make this change in bug 924905? We want to move to a model where we rely on the JSContext check, rather than a saved-frame-chain check, but luke didn't want to bite all of that off in that bug.

> Maybe we should enter the compartment of the script doing the call in C++, and then it > wouldn't matter if the value was fed to a web page? Not sure I follow. We have chrome code grabbing the stack.. which compartment do you want to enter? I guess the chrome code could pass in an object whose compartment we should enter, and pass in the relevant Console object (or rather an Xray for it; we'd need to unwrap it)?

(In reply to Boris Zbarsky [:bz] from comment #35) > Not sure I follow. We have chrome code grabbing the stack.. which > compartment do you want to enter? > > I guess the chrome code could pass in an object whose compartment we should > enter, and pass in the relevant Console object (or rather an Xray for it; > we'd need to unwrap it)? This was what I had in mind. Whether we need to do that depends on whether these stack descriptions are at any risk of being passed to content, or whether they're piped straight to the console.

Looks like we notify observers with the stack... What those observers do, who knows. Could include random extensions who do whatever. :(

Hm. I guess this is always sort of an issue, since extension code can always just do Components.stack and pass it to content. Though it's not like the current restriction (per-JSContext and per-saved-frame-chain) is all that secure either... I honestly think we probably want to output as much stack information as we can to the console. System code shouldn't be passing information from console notifications to random content.

The DOM ConsoleAPI.js implementation never returns anything to content. We use the observer service to send the console api log events to allow our codebase to do whatever we wish it to. The only console-api-log-event listener is in the web console actor's implementation. [1] The web console does not feed any of those messages to web content either. Extensions, unfortunately, can do really bad things - they can send Components.stack to pages without even listening to the console API notifications. Should we worry here about what extensions can do to the ConsoleAPI? [1] b2g/android/metro might also have mock implementations that dump() those messages.

OK. Andrea, can you please spin off the debugapi change into a separate bug and land it a day or two before this one so we can get separate regression reporting on it? I'd rather we didn't conflate it with the changes here...

This patch fixes a couple of issues and removes the JS::DescribeStack changes (Bug 960108).

Comment on attachment 8360471 [details] [diff] [review] Console API in WebIDL >+#include "nsIDOMGlobalPropertyInitializer.h" Shouldn't need this. >+ GlobalObject global(aCx, GetGlobalJSObject()); Should probably throw if !GetGlobalJSObject() before doing this part. Also probably need to enter the compartment of GetGlobalJSObject() on aCx. Otherwise doing .console from an Xray would end up mixing compartments here, right? >+++ b/dom/tests/mochitest/general/test_interfaces.html Let's just make this interface object [ChromeOnly] for now, until there's an actual spec? That way your instanceof check will still work, but there won't be a window.Console in random web pages. r=me with those fixed.

By the way, IE11 already exposes window.Console in random web pages.

nsIDOMWindow.idl

Comment on attachment 8361166 [details] [diff] [review] Console API in WebIDL You still need to enter the compartment of globalObj. _Please_ don't forget to do that before checking in! r=me if you make sure to do that.

Implemented in bug 965860.

Jean-Yves, this is an implemention detail of the console object. There are observable differences, but they are very subtle (and the reason the change is done is that these differences won't be observed in practice) Are you sure it's dev-doc-needed?

Also, the new setup is still using __noSuchMethod__. So I'm not sure why we called this fixed instead of wontfix...

because silently this bug moved to "Convert Console API to WebIDL". We can closed it as wontfix.

David, in fact the dev-doc-needed here is mainly needed because I want to be sure to remove the list __noSuchMethod__ that I added the other week (WIP). So it is more of a "check that this is correctly documented when you are done with Console/WorkerConsole" :-)

Console is still using __noSuchMethod__, as I said.