Closed Bug 629607 Opened 11 years ago Closed 8 years ago

Convert the Console API to use a proxy instead of the deprecated __noSuchMethod__

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: pcwalton, Assigned: baku)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [console-2])

Attachments

(3 files, 6 obsolete files)

The Console API should be using a proxy instead of __noSuchMethod__.
Whiteboard: [console-2]
__noSuchMethod__ isn't used anymore
Status: NEW → RESOLVED
Closed: 10 years ago
Component: Developer Tools → Developer Tools: Console
QA Contact: developer.tools → developer.tools.console
Resolution: --- → INVALID
Sonny: this bug is still valid. See dom/base/ConsoleAPI.js - __noSuchMethod__ is still used.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Still valid.

Filter on TARDIS.
Priority: -- → P3
Assignee: nobody → anton
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
Attachment #690616 - Flags: review?(rcampbell)
Nevermind, there are more oranges. FML.
yeah, was pretty sure this wouldn't cut it with our dom tests. Cancelling review.
Attachment #690616 - Flags: review?(rcampbell)
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!
Depends on: 878605
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).
Flags: needinfo?(bobbyholley+bmo)
Blocks: 620935
Attached patch bug-629607.patch (obsolete) — Splinter Review
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__.
Attached patch bug-629607.patchSplinter Review
Few small fixes.
Attachment #802714 - Attachment is obsolete: true
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.
Flags: needinfo?(bobbyholley+bmo)
Depends on: XrayToJS
FWIW, bug 914970 may in fact be more tractable than I'd originally thought.
Assignee: anton → nobody
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.
Attached patch WebIDL - WIP (obsolete) — Splinter Review
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++
Attached patch Console API in WebIDL (obsolete) — Splinter Review
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.
Attachment #8355281 - Attachment is obsolete: true
Attachment #8359105 - Flags: review?(bzbarsky)
Attached patch Console API in WebIDL (obsolete) — Splinter Review
A small issue with time|timeEnd fixed
Attachment #8359105 - Attachment is obsolete: true
Attachment #8359105 - Flags: review?(bzbarsky)
Attachment #8359113 - Flags: review?(bzbarsky)
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.  :(
Attachment #8359113 - Flags: review?(bzbarsky) → review-
Flags: needinfo?(bobbyholley+bmo)
Attached patch capi.patch (obsolete) — Splinter Review
I just updated half of the bz's comments.
Attachment #8359113 - Attachment is obsolete: true
Attachment #8359667 - Flags: review?(mihai.sucan)
> >+// 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?
Assignee: nobody → amarchesini
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.
Attachment #8359667 - Flags: review?(mihai.sucan)
> 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.
Flags: needinfo?(bobbyholley+bmo)
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...
Flags: needinfo?(amarchesini)
Attached patch Console API in WebIDL (obsolete) — Splinter Review
This patch fixes a couple of issues and removes the JS::DescribeStack changes (Bug 960108).
Attachment #8359667 - Attachment is obsolete: true
Attachment #8360471 - Flags: review?(bzbarsky)
Flags: needinfo?(amarchesini)
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.
Attachment #8360471 - Flags: review?(bzbarsky) → review+
By the way, IE11 already exposes window.Console in random web pages.
nsIDOMWindow.idl
Attachment #8360471 - Attachment is obsolete: true
Attachment #8361166 - Flags: review?(bzbarsky)
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.
Attachment #8361166 - Flags: review?(bzbarsky) → review+
Blocks: 963041
Implemented in bug 965860.
Status: REOPENED → RESOLVED
Closed: 10 years ago8 years ago
Resolution: --- → FIXED
Depends on: 965860
Target Milestone: --- → Firefox 30
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?
Flags: needinfo?(jypenator)
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" :-)
Flags: needinfo?(jypenator)
Console is still using __noSuchMethod__, as I said.
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.