Andrea Marchesini [:baku] Assignee
	Description • 10 years ago

The reason why I'm opening this bug is this: at the moment we have
. ConsoleAPI.js - that implements the Console API for content
. Console.jsm - that implements the Console API for JSM
. Worker Console.cpp - that implements the Console API for content in workers

and we are planning to port Console.jsm to worker as well.

If we rewrite ConsoleAPI in C++ (and just that, we can still have ConsoleAPIStorage in JS) we can implement it so that it works on workers in JSM and in workers+JSM. Then we can get rid of ConsoleAPI.js, Console.jsm and WorkerConsole.cpp.
This new C++ component will be built with WebIDL.

Note: we cannot have components implemented in JS in workers.

I have a patch that implements it. It's not finished but I am almost there to complete it.
But first I want to have feedbacks.

	Andrea Marchesini [:baku] Assignee
	Comment 1 • 10 years ago

This first patch converts ConsoleAPIStorage to a XPCOM Service.
This is needed for the Console.cpp, that cannot 'require' a jsm component.

	Andrea Marchesini [:baku] Assignee
	Comment 2 • 10 years ago

This second patch is the rewriting of the ConsoleAPI.js in C++.
I'm still waiting for the results of try. The next step will be to remove Console.cpp in workers and use this implementation instead.

	Mihai Sucan [:msucan]
	Comment 3 • 10 years ago

This is interesting work Andrea. Really good to see progress on unification of the console API implementations. I would have prefered that the unification happens in JS land, without a C++ rewrite (I was under the impression it's possible to do it in JS land as well). For devtools we use a lot of JS and having things in JS kept the codebase easier to approach for new people, and it's also easier/quicker to prototype new features.

Going to review the patches. Thank you!

	Andrea Marchesini [:baku] Assignee
	Comment 4 • 10 years ago

I agree with you. But unfortunately we cannot have JS components in workers.
You will find that the names of the methods match the old JS implementation.
Soon you we will receive other 2 patches:

1. Console API in workers
2. Console API in jsm

	Mihai Sucan [:msucan]
	Comment 5 • 10 years ago

Comment on attachment 8369990 [details] [diff] [review]
patch 1 - ConsoleAPIStorage as a XPCOM Service

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

Patch looks fine.

Build error:

mozpack.errors.ErrorMessage: Symlink target path does not exist: /home/mihai/src/mozilla/fx-team/dom/base/nsIConsoleAPIStorage.idl

Given this error, I cannot test the patches. r+ with the build failure fixed.

::: dom/base/ConsoleAPI.js
@@ +257,5 @@
>        this._timerInitialized = false;
>        this._timer.cancel();
>  
>        if (this._windowDestroyed) {
> +        let ConsoleAPIStorage = Cc["@mozilla.org/consoleAPI-storage;1"]

Why not put this at the top of the file?

@@ +353,5 @@
>    notifyObservers: function CA_notifyObservers(aLevel, aConsoleEvent)
>    {
>      aConsoleEvent.wrappedJSObject = aConsoleEvent;
> +    let ConsoleAPIStorage = Cc["@mozilla.org/consoleAPI-storage;1"]
> +                              .getService(Ci.nsIConsoleAPIStorage);

Same as above.

	Rob Campbell [:rc] (:robcee)
	Comment 6 • 10 years ago

can we do this in Rust instead? ;)

	Rob Campbell [:rc] (:robcee)
	Comment 7 • 10 years ago

Andrea, a more serious question: Why are we doing this? Who is the "we" that you mention in comment 0?

This will make it harder to implement new consoleapi functions in the future so I don't want to take this without a very clear benefit.

	Andrea Marchesini [:baku] Assignee
	Comment 8 • 10 years ago

new patch with the missing file.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 9 • 10 years ago

> This will make it harder to implement new consoleapi functions in the future
> so I don't want to take this without a very clear benefit.

Is that true even when said API needs to work in Workers as well? And Workers-from-jsm? The fact that the current setup is heading towards 4 different implementations of the same API does seem to complicate any modifications?

I haven't looked at the relevant code, so I'm actually asking.

	Mihai Sucan [:msucan]
	Comment 10 • 10 years ago

(In reply to Andrea Marchesini (:baku) from comment #4)
> I agree with you. But unfortunately we cannot have JS components in workers.

There should be a way to add a JS object to worker globals. I am not a Gecko dev, unfortunately, and I cannot help with that.

Sorry, but it's surprising we are now moving the console API to C++ when DOM has webidl, methods move from C++ to JS implementations. We see this even in core JS objects, like Date which is implemented in JS.

(personally, I do not mind if we go further with the move to C++ but it seems to me it's not clearly needed)

	Mihai Sucan [:msucan]
	Comment 11 • 10 years ago

(In reply to Jonas Sicking (:sicking) from comment #9)
> > This will make it harder to implement new consoleapi functions in the future
> > so I don't want to take this without a very clear benefit.
> 
> Is that true even when said API needs to work in Workers as well? And
> Workers-from-jsm? The fact that the current setup is heading towards 4
> different implementations of the same API does seem to complicate any
> modifications?

The current state owes it to the evolution of different code paths. Unification was and is possible for Console.jsm and ConsoleAPI.js.

I'm glad to see this stuff unified, my only concern was about the choice of C++.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 10 years ago

> There should be a way to add a JS object to worker globals.

There is, but there is no way to make it do anything privileged without going into C++ first.

So if you want an API exposed in workers that needs to be able to do anything a worker couldn't do directly, it needs a C++ implementation.  Changing that would take a fair amount of work and complicate workers considerably.

> like Date which is implemented in JS.

Er... Date is very much implemented in C++ except for a few methods that are self-hosted by doing some syntactic sugar that a web page could do itself and then calling a C++ implementation for the actual work.

> This will make it harder to implement new consoleapi functions in the future

In today's world, you need to add such a new function to the WebIDL, add it to the worker API in C++, add it to ConsoleAPI.js, add it to Console.jsm, and hope that we don't have a console in chrome workers yet, because that would need to be a separate implementation from all of the above.

The proposal here, if done right, may actually make it _easier_ to implement new consoleapi functions...

	Bobby Holley (:bholley)
	Comment 13 • 10 years ago

Baku asked me to explain why we can't implement APIs in JS on workers:

1 - The JS-Implemented WebIDL machinery handles registration with JS-implemented XPCOM components, which go through XPConnect and cannot be used off-main-thread.

2 - Securely implementing a privileged API that interacts with content requires XrayWrappers, which we don't have on workers.

3 - In fact, workers don't even support multiple globals in a given runtime, and adding such support is likely to be a big PITA.

4 - C++ API implementations can share bytecode across arbitrary threads, whereas JS API implementations need to clone to bytecode to each JSRuntime (and therefore each thread). This kills us memory-wise on b2g.

There was a 5th reason that I can't remember off-hand. But this should be more than enough.

Also, inspecting the execution stack in C++ is a lot simpler, because the VM is still executing the content script (rather than in the JS-implemented WebIDL case, wherein we've already thrown up a bunch of barriers to create a new execution context for chrome).

	Mihai Sucan [:msucan]
	Comment 14 • 10 years ago

Thank you bholley for your explanation. This is making things clearer. Given the situation, a C++ implementation makes sense. It wasn't clear that adding a JS-based console API to workers would be a hard task.

	Mihai Sucan [:msucan]
	Comment 15 • 10 years ago

Comment on attachment 8369993 [details] [diff] [review]
patch 2 - Console API in C++

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

(Thanks for the updated patch for part1. That works)

This c++ code goes beyond my Gecko experience. Please also ask someone like bz for review.

console.dir() seems to be broken. console.dir(window) doesn't show anything.

Web console tests from toolkit and browser fail.

	Andrea Marchesini [:baku] Assignee
	Comment 17 • 10 years ago

bz, can you review this patch?
What I'm not 100% sure is:

1. nsTArray<JS::Heap<JS::Value>> in CallData + trace macros
2. Drop/Hold JS Objects
3. Compartments

... in the end any JS stuff :)

	Andrea Marchesini [:baku] Assignee
	Comment 18 • 10 years ago

Comment on attachment 8370642 [details] [diff] [review]
patch 2 - Console API in C++

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

::: dom/bindings/Bindings.conf
@@ +1499,5 @@
>  'Window': {
>      'nativeType': 'nsGlobalWindow',
>      'hasXPConnectImpls': True,
>      'register': False,
> +    'implicitJSContext': [ 'setInterval', 'setTimeout', 'console' ],

I can remove 'console' from here. I don't need jscontext.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 10 years ago

Comment on attachment 8370642 [details] [diff] [review]
patch 2 - Console API in C++

I'm not done yet, but here's what I have so far:

>+++ b/addon-sdk/source/lib/sdk/console/plain-text.js
>+++ b/addon-sdk/source/lib/sdk/content/sandbox.js
>+++ b/addon-sdk/source/lib/sdk/deprecated/traits-worker.js

I'd like someone familiar with the .js files here to review these bits.

>+++ b/dom/base/Console.cpp
>+#include "Console.h"

mozilla/dom/Console.h, right?

>+class ConsoleCallData
>+  ConsoleCallData(Console::MethodName aName, const nsAString& aString,

I think you'll want a no-arg constructor and some setters.  More below.

>+      JS::Heap<JS::Value> arg(aArguments[i]);

Why is this temporary needed?

>+  bool mPrivate;

It might be good to reorder this struct to place this next to the mMethodName enum.  Will pack better.

>+EnterCompartment(Maybe<JSAutoCompartment>& aAc, JSContext* aCx,

This seems like a hack.  I'd rather we just always preserve our wrapper and then enter the compartment of the wrapper, since that should be the right compartment for us to work in.  Note in particular that this function does nothing useful when called on a string value, which is what you're typically calling it on....

>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Console)

You need to unlink whatever your trace method traces too, no?

>+Console::Console(nsPIDOMWindow* aWindow)
>+    nsRefPtr<nsPIDOMWindow> outerWindow = mWindow->GetOuterWindow();

Why do you need a strong ref here?

>+#define METHOD(name, method, string)                                  \

You don't need the "method" argument here.  You can just replace it with Method##name in the macro exapnsion, I believe.

And you may not need NS_LITERAL_STRING() around the third argument in the callers; that could go in the macro too.

>+    Method(aCx, method, string, aData, 1);                            \

Could also just examine the passed-in "method" in Method to decide the stack depth... Either way is fine, I guess; just won't need all the ", 1" sprinkled about.

>+Console::Trace(JSContext* aCx)
>+  Sequence<JS::Value> data;

Please make that const, to make it clear that we won't put anything in it, since it's not rooted.

>+Console::Time(JSContext* aCx, const Optional<JS::Handle<JS::Value>>& aTime)
>+  Sequence<JS::Value> data;

This, on the other hand, needs to be rooted, since we might put stuff in it.  SequenceRooter<JS::Value> rooter(aCx, &data) should do the trick.

>+Console::TimeEnd(JSContext* aCx, const Optional<JS::Handle<JS::Value>>& aTime)
>+  Sequence<JS::Value> data;

And here.

>+Console::ProfileMethod(JSContext* aCx, const nsAString& aAction,
>+  ConsoleProfileEvent event;

Why is ConsoleProfileEvent.arguments of type "any"?  Wouldn't "sequence<any>" save you the trouble of doing your own array conversion and the like?  Just make sure to use a RootedDictionary here!  I'm skipping reviewing this bit since it should go away.

>+  if (!event.ToObject(aCx, JS::NullPtr(), &eventValue)) {

Then you need to throw on the ErrorResult; otherwise the JS engine will get all confused.  Need to mark this the profile/profileEnd methods as [Throws].

>+  MOZ_ASSERT(eventValue.isObject());
>+  JS::Rooted<JSObject*> eventObj(aCx, &eventValue.toObject());

For what it's worth, toObject() will assert isObject(), so no need for you to do it.

>+  if (!JS_SetProperty(aCx, eventObj, "wrappedJSObject", eventValue)) {

JS_DefineProperty.

>+    return;

Again, need to throw.

>+  if (NS_FAILED(xpc->WrapJS(aCx, eventObj, iid, getter_AddRefs(wrapper)))) {

Throw?

>+Console::Method(JSContext* aCx, MethodName aMethodName,
>+  ConsoleCallData callData(aMethodName, aMethodString, aData);

No, this is bad.  This is not rooted, you put JS values in here, but then you do stuff that can GC (e.g. the CreateStack() call).

What you probably want to do instead is something like this:

  ConsoleCallData& callData = *mQueuedCalls.AppendElement();

and then your trace hook will keep it alive.  Assuming you've called HoldJSObjects(), that is, but I think you should just do that in your constructor.  Check with smaug or mccr8 on that part, and maybe ask them to write some documentation for HoldJSObjects/DropJSObject?

And then on early returns you'll need to remove the last element from mQueuedCalls.  Might want a RAII helper for that.

>+    nsCOMPtr<nsIWebNavigation> navigator = do_GetInterface(mWindow);

Maybe "webNav"?  This isn't a Navigator object....

>+    MOZ_ASSERT(navigator);

Actually, it could come out null, if someone does console API calls on a window that had its docshell torn down.  The current console API looks like it might throw in this case; I would not be opposed to you ding the same.

>+  nsCOMPtr<nsIStackFrame> stack = CreateStack(aCx, aMaxDepth);
>+  if (!stack) {
>+    return;

Should probably throw here.

>+  while (stack) {

You know stack is non-null, so could make this a do { ... } while (stack);

>+    ConsoleStackData& data = *callData.mStack.AppendElement();

In particular, .mStack is never empty.  We'll want that below.

>+  // Monotonic timer for 'time' and 'timeEnd'
>+  if ((aMethodName == MethodTime || aMethodName == MethodTimeEnd) && mWindow) {
>+    nsRefPtr<nsDocument> doc = static_cast<nsDocument*>(mWindow->GetDoc());

I'd rather you just did mWindow->GetPerformance() (throwing if it throws)and then did Now() on that.  I guess with a null-check. You may need to static_cast the window to nsGlobalWindow*, but I like that more than adding a maybe-incompatible implementation of performance.now() here.

>+Console::Notify(nsITimer *timer)
>+    mQueuedCalls.RemoveElementAt(0);

This seems like it will keep memmoving the list.  How about we go while i < MESSAGES_IN_INTERVAL && i < mQueuedCalls.Length() and then RemoveElementsAt(0, i) in one fell swoop?

>+    mozilla::DropJSObjects(this);

I _think_ we won't want this.

>+Console::ProcessCallData(const ConsoleCallData& aData)
>+  if (!aData.mStack.IsEmpty()) {

How could it end up being empty?  See comments above.

If I'm right that it can't, we can just do:

  ConsoleCallData& frame = aData.mStack[0];

Which would be good, because otherwise we need to somehow root "frame".

>+  ConsoleEvent event;

  AutoSafeJSContext cx;
  RootedDictionary<ConsoleEvent> event(cx);

otherwise you have unrooted values floating around and the ggc tests will turn colors.

>+    event.mInnerID.Value().SetAsString() = NS_ConvertUTF8toUTF16(frame.mFilename);

It's worth documenting why this is a useful thing to do.  Especially because event.mFilename will already contain this string.....


>+  switch (aData.mMethodName) {
>+    case MethodGroup:
>+    case MethodGroupCollapsed:
>+    case MethodGroupEnd:
>+      ComposeGroupName(aData.mArguments, event.mGroupName);

Don't we need to set event.mArguments = ArgumentsToValue(aData.mArguments) too?  The old code seems to, in this case...

>+    case MethodTime:
>+    case MethodTimeEnd:

And here.

>+  if (!event.ToObject(cx, JS::NullPtr(), &eventValue)) {

Then need to report or clear the exception, right?

>+  MOZ_ASSERT(eventValue.isObject());

Again, no need.

>+  if (!JS_SetProperty(cx, eventObj, "wrappedJSObject", eventValue)) {

JS_DefineProperty.  And again, failure needs to be handled.

>+  nsCOMPtr<nsIConsoleAPIStorage> service =
>+    do_GetService("@mozilla.org/consoleAPI-storage;1");

It's a bit annoying to do this for every message.  Could we cache this somewhere (e.g. nsContentUtils)?

>+  nsString innerID;

nsAutoString, maybe?

>+  JS::Rooted<JS::Value> value(cx, JS::ObjectValue(*eventObj));

You aleady have eventValue, no?

>+  if (NS_FAILED(service->RecordEvent(innerID, value))) {

Might be nice to have someone familiar with console stuf review the interactions with this service.

>+    nsString outerID;

nsAutoString.

>+Console::StackDataToValue(const nsTArray<ConsoleStackData>& aStack)
>+  JS::Rooted<JSObject*> stackObj(cx,
>+    JS_NewArrayObject(cx, aStack.Length(), nullptr));
>+  if(!stackObj) {

Clear pending exception?

>+    if (!stack.ToObject(cx, JS::NullPtr(), &value)) {

And here?

>+    if (!JS_DefineElement(cx, stackObj, i, value, nullptr, nullptr, 0)) {

And here.  Also, last arg should be JSPROP_ENUMERATE.

>+  JS::Rooted<JS::Value> stacktrace(cx, JS::ObjectValue(*stackObj));
>+  return stacktrace;

Just |return JS::ObjectValue(*stackObj)|.

As usual, interdiff would be nice, but you may want to wait until I finish the review.

Note to self: Stopped right at the start of Console::ProcessArguments.

	Andrea Marchesini [:baku] Assignee
	Comment 21 • 10 years ago

This third patch uses the Console object in worker.

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 10 years ago

Comment on attachment 8373650 [details] [diff] [review]
patch 2 - Console API in C++

Missed old review comments:

1)  Why is ConsoleProfileEvent.arguments of type "any"?  Wouldn't "sequence<any>" save you the trouble of doing your own array conversion and the like?

2) "It's worth documenting why this is a useful thing to do.  Especially because event.mFilename will already contain this string....."

New comments:

Need to clearly document the difference between ProcessArguments and ArgumentsToValue.  

Need to clearly document what ComposeGroupName is trying to do.

>+Console::ProcessCallData(const ConsoleCallData& aData)
>+      event.mArguments.Value() = ProcessArguments(cx, aData.mGlobal,

This does slow array copies.  Wouldn't it be better to pass event.mArguments.Value() as an argument to ProcessArguments?

>+    case MethodTrace:

Why do you need this?  The default: case is hitting this anyway...

>+      event.mArguments.Value() = ArgumentsToValue(cx, aData.mGlobal,

Similar here.

>+Console::StackDataToValue(const nsTArray<ConsoleStackData>& aStack)

Why is ConsoleEvent.stackTrace "any" instead of sequence<ConsoleStack> (which should perhaps be renamed to ConsoleStackEntry)?  Would be better to do the latter and not need the manual JSAPI bits here.  I've skipped reviewing the JSAPI stuff, since it should go away (though I did review it last time, and it looks like those comments didn't get applied...).  Again pass the array to write to into this function as an argument.

>+Sequence<JS::Value>
>+Console::ProcessArguments(JSContext* aCx, JSObject* aGlobal,

This should have a Sequence<JS::Value>& outparam.

Something in this method needs to clear exceptions as needed.

>+          JS::Rooted<JSString*> str(aCx, JS_NewUCStringCopyZ(aCx,
>+                                                             output.get()));

At the very least, please use JS_NewUCStringCopyN here.

>+          JS::Rooted<JS::Value> v(aCx, JS::StringValue(str));

Why do you need this temporary in the %o case?

>+          JS::Rooted<JSString*> jsString(aCx, JS::ToString(aCx, value));

This is in the %s case.  ToString() can return null.  Please make sure to handle that correctly, since the page can control whether it does that!

>+          double d = v.ToDouble(&rv);

So we convert to string, then we convert from there to double, then we AppendPrintf?

Why not start with JS::ToNumber(aCx, value) instead, like we do JS::ToInt32 in the int case?

>+    JS::Rooted<JSString*> str(aCx, JS_NewUCStringCopyZ(aCx, output.get()));

Again, CopyN.

>+    JS::Rooted<JS::Value> v(aCx, JS::StringValue(str));

No need for this temporary either.

>+Console::ComposeGroupName(JSObject* aGlobal,

Might as well pass a JSContext* here, since the caller has one?

>+    JS::Rooted<JSString*> jsString(cx, JS::ToString(cx, value));

Again, page can make this return null.

You need to clear pending exceptions in this method or something.

>+Console::StartTimer(JSObject* aGlobal, const JS::Value& aName,

Again, need to clear exceptions?

>+  JS::Rooted<JSObject*> obj(cx, JS_NewObject(cx, nullptr, JS::NullPtr(),
>+                            JS::NullPtr()));

Weird indentation here.

>+    if (!JS_DefineProperty(cx, obj, "error", prop, nullptr, nullptr,
>+        JSPROP_ENUMERATE)) {

And here.

>+    timerValue = JS::ObjectValue(*obj);
>+    return timerValue;

You don't need that temporary write to timerValue here.

>+  if (!JS_DefineProperty(cx, obj, "name", name, nullptr, nullptr,
>+      JSPROP_ENUMERATE)) {

Fix indent.  Also, I'd really prefer if we just used a webidl dictionary here too....

>+  JS::Rooted<JSString*> jsString(cx, JS::ToString(cx, name));

Can return null.

>+  JS::Rooted<JS::Value> started(cx, DOUBLE_TO_JSVAL(aTimestamp));

JS::NumberValue().

>+  if (!JS_DefineProperty(cx, obj, "started", started, nullptr, nullptr,
>+      JSPROP_ENUMERATE)) {

Another weird indent.

>+  timerValue = JS::ObjectValue(*obj);

Again, no need for timerValue here.

>+Console::StopTimer(JSObject* aGlobal, const JS::Value& aName,
>+  JS::Rooted<JSString*> jsString(cx, JS::ToString(cx, name));

Can return null.

>+  JS::Rooted<JSObject*> obj(cx, JS_NewObject(cx, nullptr, JS::NullPtr(),
>+                            JS::NullPtr()));

Weird indent.  Again, dictionary would be better.

>+  if (!JS_DefineProperty(cx, obj, "name", name, nullptr, nullptr,
>+      JSPROP_ENUMERATE)) {

Weird indent.

>+  if (!JS_DefineProperty(cx, obj, "duration", duration, nullptr, nullptr,
>+      JSPROP_ENUMERATE)) {

Weird indent.

>+  JS::Rooted<JS::Value> timerValue(cx, JS::ObjectValue(*obj));

Don't need this temporary.

>+Console::ArgumentsToValue(JSContext* aCx, JSObject* aGlobal,

You don't need aGlobal here; it's not really used.  And if you pass in the Sequence<Value>&, you don't need aCx either.

General comment: the consoleAPI.js code listened for inner-window-destroyed.  Why does this new code not need to?

r=me with all the above fixed, but again I'd like to see an interdiff, and please do make sure you fix them all....

	Andrea Marchesini [:baku] Assignee
	Comment 24 • 10 years ago

> >+    JS::Rooted<JSString*> jsString(cx, JS::ToString(cx, value));
> 
> Again, page can make this return null.
> 
> You need to clear pending exceptions in this method or something.

ClearException is called in ProcessCallData. It should cover any exception of the other methods.

> General comment: the consoleAPI.js code listened for inner-window-destroyed.
> Why does this new code not need to?

Because for what I have seen, the Console lives in the inner Window so it's destroyed with it.
The inner-window-destroyed event was useful to clear the timerRegistry. This step is not needed if we have a new console for each inner-window. Correct?

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 10 years ago

> ClearException is called in ProcessCallData.

I see.  You should probably document that all these methods expect to be called with a ClearException on the stack.

also, given this, the "Throw(cx, NS_ERROR_FAILURE);" bit in ProcessCallData is not needed.

> This step is not needed if we have a new console for each inner-window. Correct?

Ah, I see.  Yes, that makes sense.

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 10 years ago

Comment on attachment 8374723 [details] [diff] [review]
interdiff patch 2

>+++ b/dom/base/Console.cpp
> Console::ProfileMethod(JSContext* aCx, const nsAString& aAction,
>+  JS::Rooted<JSObject*> global(aCx, JS::CurrentGlobalOrNull(aCx));

Why do you need this?

>+  Sequence<JS::Value> sequence;

Just do:

  event.mArguments.Construct();
  Sequence<JS::Value>& sequence = event.mArguments.Value();

and then you don't need the extra rooter or the array copy at the end.

>+  JSAutoCompartment ac(aCx, global);

aCx is already in the compartment of global, so this line does absolutely nothing and we can remove it.

>@@ -380,42 +368,47 @@ Console::Method(JSContext* aCx, MethodNa
>+    data.mFilename = NS_ConvertUTF8toUTF16(string);

  CopyUTF8toUTF16(string, data.mFilename);

will save a temporary.

>+    data.mFunctionName = NS_ConvertUTF8toUTF16(string);

Same.

> Console::ProcessCallData(const ConsoleCallData& aData)

I think it would be a good idea to enter the compartment of aData.mGlobal at the top of this function and then you don't have to worry about it in the callees and can remove the JSAutoCompartments in StartTimer/StopTimer and ProcessArguments.  As things stand, you're not in the right compartment in ComposeGroupName...

>+    // If we are in a JSM, the window doesn't exist.

That doesn't explain why we're using frame.mFilename as the mInnerID.  That's what needs documenting.  In particular, won't callers expect an integer?  Why is a string the right thing here?

>+      ArgumentsToValue(aData.mArguments, event.mArguments.Value());

Looking at this more closely, maybe it should be called ArgumentsToValueList?  It's a bit silly that we have to do this copy at all, but I just don't see a better option.  :(

>+    event.mStacktrace.Value() = aData.mStack;

Could this use SwapElements?  Is there a reason aData here needs to be const?  That is, are we ok just stealing this array from aData instead of copying it?

>+Console::StartTimer(JSContext* aCx, JSObject* aGlobal, const JS::Value& aName,
>+    if (!error.ToObject(aCx, JS::NullPtr(), &value)) {

If aGlobal were a handle here, you could just pass it as the second argument here and elsewhere.  It _may_ be worth doing that (and using fromMarkedLocation or a Rooted somewhere up the stack).

On the other hand, this is safe enough as long as the dictionary only contains strings and numbers...

r=me

	Andrea Marchesini [:baku] Assignee
	Comment 29 • 10 years ago

> >+    // If we are in a JSM, the window doesn't exist.

The real reason why we use 'jsm' as string and the file name is unknown to me.
That is taken from Console.jsm. So probably I guess some component is expecting that behaviour.

I would ask this to a devtool developer. Who actually will review this code as well. msucan?

	Andrea Marchesini [:baku] Assignee
	Comment 31 • 10 years ago

msucan, this patch is almost fully reviewed by bz. I'm wondering if you want to review it from a devtools point of view. Thanks!

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 10 years ago

> That is taken from Console.jsm. 

Aha.  That was the missing piece!  Please comment.  ;)

	Boris Zbarsky [:bzbarsky]
	Comment 33 • 10 years ago

Comment on attachment 8375481 [details] [diff] [review]
patch 2 - interdiff

r=me

	Andrea Marchesini [:baku] Assignee
	Comment 34 • 10 years ago

This patch works... but I'm not 100% I want to use this approach.

	Mihai Sucan [:msucan]
	Comment 35 • 10 years ago

(In reply to Andrea Marchesini (:baku) from comment #29)
> > >+    // If we are in a JSM, the window doesn't exist.
> 
> The real reason why we use 'jsm' as string and the file name is unknown to
> me.
> That is taken from Console.jsm. So probably I guess some component is
> expecting that behaviour.
> 
> I would ask this to a devtool developer. Who actually will review this code
> as well. msucan?

We use 'jsm' to not collide with any window ID. We can use any string, really, but I picked one, 'jsm', such that it's easy to clear all messages from jsm-land.

	Andrea Marchesini [:baku] Assignee
	Comment 36 • 10 years ago

> We use 'jsm' to not collide with any window ID. We can use any string,
> really, but I picked one, 'jsm', such that it's easy to clear all messages
> from jsm-land.

Ok. Any why 'innerID' is the filename?

	Mihai Sucan [:msucan]
	Comment 37 • 10 years ago

Comment on attachment 8375483 [details] [diff] [review]
patch 2 - Console API in C++

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

This is working better. Thank you for keeping me in the loop.

- console.trace() shows a frame with function name unknown (anonymous) and source unknown as well. This is always the oldest frame. Why is it included? Tested with http://mihaisucan.github.io/mozilla-work/test.html

- web console tests still break.

I read the Console.cpp file and from my understanding of the code, it seems fine. Once you have fixes for the console tests, please ping me again so I can test more. Thanks!

I tried to test the workers patch part 3 as well, but it needs a rebase - patch failed to apply.

(I cannot r+ the patch due to the test failures)

::: dom/base/Console.cpp
@@ +487,5 @@
> +    case MethodException:
> +    case MethodDebug:
> +    case MethodAssert:
> +      event.mArguments.Construct();
> +      ProcessArguments(cx, aData.mArguments, event.mArguments.Value());

What happens when the arguments refer to a dead object? See bug 931304 and the comments.

	Mihai Sucan [:msucan]
	Comment 38 • 10 years ago

(In reply to Andrea Marchesini (:baku) from comment #36)
> > We use 'jsm' to not collide with any window ID. We can use any string,
> > really, but I picked one, 'jsm', such that it's easy to clear all messages
> > from jsm-land.
> 
> Ok. Any why 'innerID' is the filename?

In the future we want to allow the user to filter messages by addon, by jsm, etc - in the browser console. Afaik we have a bug about filtering messages by addons.

	Andrea Marchesini [:baku] Assignee
	Comment 39 • 10 years ago

> - web console tests still break.

It turned out that is not this second patch to break them. But the first one.
The nsIConsoleStorage interface is not available somehow.

	Andrea Marchesini [:baku] Assignee
	Comment 40 • 10 years ago

This version fixes the issue about the anonymous entry in the console.trace() output.

	Boris Zbarsky [:bzbarsky]
	Comment 41 • 10 years ago

With that change, are we no longer guaranteed that callData.mStack is nonempty?  If so, we need to fix ProcessCallData to handle the case when the stack is empty.  Can we create a testcase testing that situation?  Perhaps something like:

  window.setTimeout(console.log.bind(console), 0, "xyz");

or:

  window.addEventListener("fake", console.log.bind(console, "xyz"));
  window.dispatchEvent(new Event("fake"));

or something?

	Boris Zbarsky [:bzbarsky]
	Comment 43 • 10 years ago

well, the real question is whether we should send an event at all if we have no stack, because it seems like we'd be sending a pretty useless event.  Check with Mihai?

And still need that testcase.

	Mihai Sucan [:msucan]
	Comment 45 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #43)
> well, the real question is whether we should send an event at all if we have
> no stack, because it seems like we'd be sending a pretty useless event. 
> Check with Mihai?

A console.log() call that has an unknown call location from a content script is still useful for a webdev. I would say we should send an event even in such cases.

	Andrea Marchesini [:baku] Assignee
	Comment 46 • 10 years ago

> And still need that testcase.

The test case is in the patch. is it not what you meant?

	Boris Zbarsky [:bzbarsky]
	Comment 47 • 10 years ago

> I would say we should send an event even in such cases.

OK.  ConsoleStackEntry looks safe enough to copy now, so I guess this is ok.

> The test case is in the patch.

So it is.  I just can't read.  OK, so part 2 is good to go.  ;)

	Andrea Marchesini [:baku] Assignee
	Comment 48 • 10 years ago

This is a new method implemented in bug 922208

	Boris Zbarsky [:bzbarsky]
	Comment 51 • 10 years ago

Comment on attachment 8376543 [details] [diff] [review]
patch 1 - ConsoleAPIStorage as a XPCOM Service

Kyle offered to take over finishingup this review, to unblock this.

	Mihai Sucan [:msucan]
	Comment 52 • 10 years ago

Comment on attachment 8377720 [details] [diff] [review]
patch 2.1 - Console.count()

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

Thanks for the update.

I can see one problem with the patch, see below. Also note that console tests still fail with these patches applied.

::: dom/base/Console.cpp
@@ +841,5 @@
> +                         const JS::Value& aLabel)
> +{
> +  JS::Rooted<JS::Value> labelValue(aCx, aLabel);
> +  JS::Rooted<JSString*> jsString(aCx, JS::ToString(aCx, labelValue));
> +  if (!jsString) {

This implementation to require a label to be given to count() calls. The label is not required. The webconsole test for count() fails because of this.

	Andrea Marchesini [:baku] Assignee
	Comment 53 • 10 years ago

Rebased

	Andrea Marchesini [:baku] Assignee
	Comment 54 • 10 years ago

This patch is needed for %1.2f and so. They are not "fake" as in ConsoleAPI.js but the value is used for real. Covered by a couple of tests.

	Andrea Marchesini [:baku] Assignee
	Comment 55 • 10 years ago

This patch is needed to cancel the timer when the window goes away.

	Andrea Marchesini [:baku] Assignee
	Comment 56 • 10 years ago

This should be compatible with the previous implementation.

	Andrea Marchesini [:baku] Assignee
	Comment 57 • 10 years ago

It should be green on try a part jetpack. But I really need to talk with somebody from the jetpack team to see what that test does and why it fails.

	Mihai Sucan [:msucan]
	Comment 59 • 10 years ago

Comment on attachment 8379806 [details] [diff] [review]
patch 2.1 - Format String

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

Thanks for implementing this. Patch seems to be fine.

::: dom/tests/browser/test-console-api.html
@@ +50,5 @@
>          console.groupEnd("b", "group");
>        }
>  
>        function nativeCallback() {
> +        new Promise(function(resolve, reject) { resolve(42); }).then(console.log.bind(console));

The following code:

foo = console.log;
foo('test');

used to work. With these patches it doesnt. Should we require bind()ing functions to the console object? I would prefer the convenience of using the methods directly, without being required to bind them.

	Boris Zbarsky [:bzbarsky]
	Comment 60 • 10 years ago

> Should we require bind()ing functions to the console object? 

Chrome does, and that's how every single other Web API works (for non-static functions).  So I don't think it's unreasonable to make this one work like other APIs, and since other UAs require that behavior already I doubt there is a web compat issue.

	Mihai Sucan [:msucan]
	Comment 61 • 10 years ago

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #60)
> > Should we require bind()ing functions to the console object? 
> 
> Chrome does, and that's how every single other Web API works (for non-static
> functions).  So I don't think it's unreasonable to make this one work like
> other APIs, and since other UAs require that behavior already I doubt there
> is a web compat issue.

Agreed. I only asked because of bug 896535 where it seemed that some devs would expect that the console methods can be used without binding.

	Mihai Sucan [:msucan]
	Comment 62 • 10 years ago

Comment on attachment 8379809 [details] [diff] [review]
patch 2.3 - Console.count()

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

Thanks for the updated patch. Looks good, almost all tests pass here, except the PI checks from in dom/tests/browser/browser_ConsoleAPITests.js. (tested with all patches applied, except part 4 which needs rebasing)

	Gabor Krizsanits (INACTIVE)
	Comment 63 • 10 years ago

So the AddonSDK for some tests overwrites the console object, so it can intercept the calls and store the logs from a certain addon for testing. Does a content window has a console property? Is it visible through xray? I'm not very familiar with the patches, but here is an easy way to test this: if you create a sandbox with a content window in it's proto chain (sandboxProto), try to overwrite its console and see what happens. I think that prior to these patches that added a property to the sandbox, post patch it find the console of the content window, and puts an expando on the xray instead. Given the nature of xray expandos in sandboxes, that won't be detectable from inside the sandbox, hence will have no effect on console calls from the sandbox.

If my theory is correct the proposed fix is to overwrite the console from inside the sandbox (in the addon SDK).

	Tomislav Jovanovic :zombie
	Comment 64 • 10 years ago

Gabor, your theory is (partly?) correct (i wish i have read your comment before investigating this).

that LoaderWithHookedConsole was a red herring. this is not a problem just with jetpack tests, a custom console is used for every ContentWorker that the SDK creates [1], because content scripts attached to a public web page should not log to the same console as the page itself (reasons).

setting `contentWindow.console` from the outside throws "setting a property that has only a getter". 

but the SDK already sets the console from inside the sandbox [1] (created with that same window in sandboxPrototype [2]), which doesn't throw, but also has no effect (huh?!).

any idea for the next step to investigate?

[1] https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/content/content-worker.js#L103
[2] https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/content/sandbox.js#L126

	Boris Zbarsky [:bzbarsky]
	Comment 65 • 10 years ago

Sounds like you're doing the set over an Xray, and since Window is not on WebIDL bindings yet not getting the replaceable behavior?

The simple solution is to make the xpidl property writable and manually have the setter redefine the prop...  Of course doing that with an Xray is a _really_ bad thing to do, in general, since it affects what content sees.

	Boris Zbarsky [:bzbarsky]
	Comment 66 • 10 years ago

Though actually, if we redefine on the thisobj of the operation (which is the xray), it wouldn't be too bad.  Would need to wrap the window into the caller compartment, etc.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 67 • 10 years ago

Isn't the use-case here addons that *want* to override the console API that is exposed to webpages? Unclear why they want to do that though. If they want to listen to data that is logged they can use nsIConsoleService for that I thought? And if they want to extend the API that's there they can do that without replacing the whole console object.

	Gabor Krizsanits (INACTIVE)
	Comment 68 • 10 years ago

(In reply to Tomislav Jovanovic [:zombie] from comment #64)
> but the SDK already sets the console from inside the sandbox [1] (created
> with that same window in sandboxPrototype [2]), which doesn't throw, but
> also has no effect (huh?!).

It should have effect from inside the same sandbox... problem is we have two sandboxes I think, one for
the extension APIs and one for the content-script. Can it be that we overwrite it in one of them and call console.log from the other one? This is weird.

> [1]
> https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/content/content-
> worker.js#L103
> [2]
> https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/content/sandbox.
> js#L126

This code tells me very little, too much abstraction layer for me to follow what is exactly happening, a minimal example would be nice, in scratch-pad without the SDK complexity... I don't think I will have time to look into this this week...

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #65)
> The simple solution is to make the xpidl property writable and manually have
> the setter redefine the prop...  Of course doing that with an Xray is a
> _really_ bad thing to do, in general, since it affects what content sees.

Actually we don't want to overwrite it on the window itself, we just want to shadow it for the sandboxes who has the window in their proto chain. So Console.log will have different result called from the addon than from the site itself.

(In reply to Jonas Sicking (:sicking) from comment #67)
> Isn't the use-case here addons that *want* to override the console API that
> is exposed to webpages?

No, this is only for testing. We just want to see what did a particular addon logged during the test via Consol. If there is another simple way to do that we should just migrate to that.

	Boris Zbarsky [:bzbarsky]
	Comment 69 • 10 years ago

Gabor, just making the xpidl property writable and manually doing the replaceable bits on the Xray should do what you want.

	Boris Zbarsky [:bzbarsky]
	Comment 71 • 10 years ago

Comment on attachment 8382311 [details] [diff] [review]
patch 2.01 - IDL replacable

>+  JS::Rooted<JSObject*> consoleObj(aCx, console->WrapObject(aCx, global));

No, please don't do that; that method is very much not idempotent.  Just crib what the webidl code does in the generated get_console instead.

>+  AutoJSContext cx;

Why?  You got passed aCx.  And that's even in the right compartment, which I wouldn't guarantee for "cx" here.

Apart from that the setter looks fine.

	Bobby Holley (:bholley)
	Comment 72 • 10 years ago

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #71)
> >+  AutoJSContext cx;
> 
> Why?  You got passed aCx.  And that's even in the right compartment, which I
> wouldn't guarantee for "cx" here.

Any time you're passed a cx, AutoJSContext should give you the very same cx. So it's unnecessary, but it's not incorrect. I know you've historically been suspicious of the JSContext stack, but we rely on it everywhere at this point.

If I hooked things up so that assertSameCompartment also asserted that the given cx was stack-top, would that allay your fears?

Also, baku - you can nix the JSAutoRequest.

	Boris Zbarsky [:bzbarsky]
	Comment 73 • 10 years ago

> would that allay your fears?

Somewhat, yes.

	Boris Zbarsky [:bzbarsky]
	Comment 75 • 10 years ago

Comment on attachment 8382423 [details] [diff] [review]
patch 2.01 - IDL replacable

r=me

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 76 • 10 years ago

Comment on attachment 8379806 [details] [diff] [review]
patch 2.1 - Format String

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

::: dom/base/Console.cpp
@@ +618,5 @@
>        ++start;
>        continue;
>      }
>  
> +    nsString tmp;

nsAutoString

@@ +628,5 @@
> +    // Let's parse %<number>.<number> for %d and %f
> +    if (*start >= '0' && *start <= '9') {
> +      integer = 0;
> +
> +      while (*start >= '0' && *start <= '9') {

might as well make this a do ... while since you've already tested the condition

@@ +651,5 @@
> +      while (*start >= '0' && *start <= '9') {
> +        mantissa = mantissa * 10 + *start - '0';
> +        tmp.Append(*start);
> +        ++start;
> +      }

here too, you already know the answer to the condition on the first iteration.

::: dom/base/Console.h
@@ +137,5 @@
>    ProcessArguments(JSContext* aCx, const nsTArray<JS::Heap<JS::Value>>& aData,
>                     Sequence<JS::Value>& aSequence);
>  
> +  void
> +  MakeFormatString(nsCString& aFormat, int32_t aInterer, int32_t aMantissa,

aInteger

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 77 • 10 years ago

Comment on attachment 8379807 [details] [diff] [review]
patch 2.2 - inner-window-destroyed observer

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

::: dom/base/Console.h
@@ +24,5 @@
>  class ConsoleCallData;
>  
>  class Console MOZ_FINAL : public nsITimerCallback
>                          , public nsWrapperCache
> +                        , public nsIObserver

please put the interfaces above nsWrapperCache.

::: dom/base/nsGlobalWindow.cpp
@@ +13367,5 @@
>  NS_IMETHODIMP
>  nsGlobalWindow::GetConsole(nsISupports** aConsole)
>  {
>    ErrorResult rv;
> +  nsCOMPtr<nsIObserver> console = GetConsole(rv);

nsIObserver is not the canonical nsISupports of the console.  Please return the canonical nsISupports.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 78 • 10 years ago

Comment on attachment 8379809 [details] [diff] [review]
patch 2.3 - Console.count()

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

::: dom/base/Console.cpp
@@ +881,5 @@
>  }
>  
> +JS::Value
> +Console::IncreaseCounter(JSContext* aCx, const ConsoleStackEntry& aFrame,
> +                          const nsTArray<JS::Heap<JS::Value>>& aArguments)

Just keep using Sequence<JS::Value>?

@@ +905,5 @@
> +    key.Append(NS_LITERAL_STRING(":"));
> +    key.AppendInt(aFrame.mLineNumber);
> +  }
> +
> +  uint32_t count;

this is hyper nit-picky, but I would init this to zero

@@ +906,5 @@
> +    key.AppendInt(aFrame.mLineNumber);
> +  }
> +
> +  uint32_t count;
> +  if (!mCounterRegistry.Get(key, &count)) {

Is supporting empty keys intentional?

@@ +921,5 @@
> +
> +    count = 1;
> +  } else {
> +    ++count;
> +  }

and then increment unconditionally.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 79 • 10 years ago

Comment on attachment 8379811 [details] [diff] [review]
patch 3 - Console API in workers

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

::: dom/base/Console.cpp
@@ +75,5 @@
> +
> +  nsTArray<nsString>* strings = static_cast<nsTArray<nsString>*>(aClosure);
> +  if (strings->Length() <= aData) {
> +    return nullptr;
> +  }

Can't you assert this?

@@ +306,5 @@
> +      wp = wp->GetParent();
> +    }
> +
> +    AutoPushJSContext cx(wp->ParentJSContext());
> +    JSAutoRequest ar(cx);

AutoPushJSContext should enter the request for you.

@@ +415,5 @@
> +      wp = wp->GetParent();
> +    }
> +
> +    AutoPushJSContext cx(wp->ParentJSContext());
> +    JSAutoRequest ar(cx);

same here

::: dom/bindings/Codegen.py
@@ +11766,5 @@
>          curr = CGNamespace.build(['mozilla', 'dom'], unions)
>  
>          curr = CGWrapper(curr, post='\n')
>  
> +        headers.update(["nsDebug.h", "mozilla/dom/UnionTypes.h"])

Why are you doing this?

::: dom/workers/WorkerScope.h
@@ +60,5 @@
>      return nsRefPtr<WorkerGlobalScope>(this).forget();
>    }
>  
> +  Console*
> +  GetConsole();

return an already_AddRefed.

	Tomislav Jovanovic :zombie
	Comment 80 • 10 years ago

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #68)
> It should have effect from inside the same sandbox... problem is we have two
> sandboxes I think, one for the extension APIs and one for the content-script. 
> Can it be that we overwrite it in one of them and call console.log from the 
> other one? This is weird.
yes, you are right about two sandboxes (i forgot to mention, it was late), but that wasn't the issue here. i tested right before and after the line setting the new console, but it simply didn't stick, and didn't throw either!

> This code tells me very little, too much abstraction layer for me to follow
> what is exactly happening, a minimal example would be nice, in scratch-pad
> without the SDK complexity... 
if this doesn't get resolved otherwise, of if you still wish to investigate later, i'll provide the minimal STR.

> No, this is only for testing. We just want to see what did a particular
> addon logged during the test via Consol. If there is another simple way to
> do that we should just migrate to that.
again, this isn't only for testing. all content scripts have their console overwritten to be different from the window they attach to, which wasn't working, and tests just detected that.


btw, if i understand these new patches, the console will simply be a writable property? that makes sense, after all, even the public web pages are allowed to override their own console, but we shouldn't from chrome?

	Andrea Marchesini [:baku] Assignee
	Comment 81 • 10 years ago

> >    ErrorResult rv;
> > +  nsCOMPtr<nsIObserver> console = GetConsole(rv);
> 
> nsIObserver is not the canonical nsISupports of the console.  Please return
> the canonical nsISupports.

The reason why I did this is that nsISupports in an ambiguous class (nsIObserver and nsITimerCallback).
Any other idea?

	Andrea Marchesini [:baku] Assignee
	Comment 82 • 10 years ago

> > +        headers.update(["nsDebug.h", "mozilla/dom/UnionTypes.h"])
> 
> Why are you doing this?

Because XPCWrapper.h is not exposed. And exposing it causes a lot of include issues. And then, it's not needed.

https://tbpl.mozilla.org/?tree=Try&rev=e7f1882bc89b

Green on try.

	Andrea Marchesini [:baku] Assignee
	Comment 88 • 10 years ago

do we want to wait for JSM part too or do we want to land Console for main-thread and workers?

	Mihai Sucan [:msucan]
	Comment 89 • 10 years ago

(In reply to Andrea Marchesini (:baku) from comment #88)
> do we want to wait for JSM part too or do we want to land Console for
> main-thread and workers?

As far as I know, we do not need to wait for the JSM part. You can land the patches that are ready.

	Andrea Marchesini [:baku] Assignee
	Comment 90 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ed97de4f1d28
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3f528e61aacf
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/428fa1a58cf3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/39be88a5ce66
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/200b26e5e847
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4d27a920dbf9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ec85e0c7c060

First 7 patches in m-i.

	Carsten Book [:Tomcat]
	Comment 91 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/ed97de4f1d28
https://hg.mozilla.org/mozilla-central/rev/3f528e61aacf
https://hg.mozilla.org/mozilla-central/rev/428fa1a58cf3
https://hg.mozilla.org/mozilla-central/rev/39be88a5ce66
https://hg.mozilla.org/mozilla-central/rev/200b26e5e847
https://hg.mozilla.org/mozilla-central/rev/4d27a920dbf9
https://hg.mozilla.org/mozilla-central/rev/ec85e0c7c060

	Florian Quèze [:florian]
	Comment 92 • 10 years ago

Fixed comm-central bustage: https://hg.mozilla.org/comm-central/rev/a8c1018ac16e

	[github robot]
	Comment 93 • 10 years ago

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/55c026f611008d704b695e38e0891da5620c18c4
Bug 965860 - patch 2 - ConsoleAPI written in C++, r=bz

	Bobby Holley (:bholley)
	Comment 95 • 10 years ago

So, for bug 960108 to be safe, we need to kill both ConsoleAPI.js and Console.jsm. IIUC, Console.jsm is part 8, which hasn't landed yet. But shouldn't ConsoleAPI.js have been removed already?

	Andrea Marchesini [:baku] Assignee
	Comment 96 • 10 years ago

(In reply to Bobby Holley (:bholley) from comment #95)
> So, for bug 960108 to be safe, we need to kill both ConsoleAPI.js and
> Console.jsm. IIUC, Console.jsm is part 8, which hasn't landed yet. But
> shouldn't ConsoleAPI.js have been removed already?

Correct. The first 7 patches are landed. ConsoleAPI.js is gone.
Console.jsm is removed by patch 8 but is not landed yet. For this set of patches bug 960108 is not needed.

	Bobby Holley (:bholley)
	Comment 97 • 10 years ago

(In reply to Andrea Marchesini (:baku) from comment #96)
> (In reply to Bobby Holley (:bholley) from comment #95)
> Correct. The first 7 patches are landed. ConsoleAPI.js is gone.

So it is. Looks like my tree was just out of date.

> Console.jsm is removed by patch 8 but is not landed yet. For this set of
> patches bug 960108 is not needed.

Yes, but I'm more concerned about the converse - i.e. security vulnerabilities related from the stuff described in bug 960108 comment 10. Though I guess Console.jsm is only used by chrome, right? So maybe it's fine.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 98 • 10 years ago

Comment on attachment 8384670 [details] [diff] [review]
patch 8 - Console API for JSM

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

r- for the leak

::: dom/base/Console.cpp
@@ +140,5 @@
> +
> +  ConsoleProxy(Console* aConsole)
> +    : mConsole(aConsole)
> +  {
> +    AssertIsOnMainThread();

You will have already died trying to refcount on the wrong thread here.

@@ +145,5 @@
> +  }
> +
> +  ~ConsoleProxy()
> +  {
> +     MOZ_ASSERT(!mConsole);

nit: indentation

@@ +622,5 @@
>    if (NS_IsMainThread()) {
>      nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
>      if (obs) {
>        obs->AddObserver(this, "inner-window-destroyed", false);
> +      obs->AddObserver(this, "xpcom-shutdown", false);

Where is this reference from the observer service released?

	Boris Zbarsky [:bzbarsky]
	Comment 100 • 10 years ago

Could we please resolve this with the right target milestone so it's clear when window.console got webidlified and move further work to a new bug?

	Colby Russell :crussell (no longer Mozilla-ing)
	Comment 101 • 10 years ago

Before undertaking all this, did anyone float the idea with the JS team of just outright implementing the console object in js/?  I see jwalden is CCed, but no comment.

	Jeff Walden [:Waldo]
	Comment 103 • 10 years ago

Self-hosting is still a work in progress.  It's a purely engine-internal thing right now, with self-hosted code necessarily built directly into the engine, in a non-configurable way.  And when self-hosted code wants to do something trickier, better done in C++, it happens through an "intrinsic" C++ function defined inside the engine.  This isn't all that extensible.

Additionally, correctly self-hosting is *hard*.  At least, not if you want it to be unobservable that you're self-hosting.  Any value that comes from the user, you can't trust -- no doing .call(...) on functions, no doing .prop unless you really truly meant to do a [[Get]] invoking arbitrary user code, no indexing on arrays unless you somehow are guaranteed the index will be present, no creating new arrays and initializing them using [] accesses, and more.  Self-hosted code is deceptively simple-looking: it usually does what you expect, but it will do much you don't expect if you're not really careful what you do.  Even JS engine hackers don't really understand the set of things that are safe, yet.  See bug 987243, bug 988416, bug 822080, bug 834989, and likely others I've forgotten, or that we haven't found yet.  (v8 has similar issues -- a recent exploit against them worked because their typed-array self-hosted code was using .byteLength internally, rather than a trusted intrinsic operation whose functionality couldn't be overridden.)

At some point we probably will be able to expose self-hosting as a service (tm).  And maybe there are things we can do to make writing self-hosted code safer.  But we're not there on either front yet.  Which is why I'm fairly sure I deliberately didn't mention self-hosting as a possibility here.

	Emanuel Hoogeveen [:ehoogeveen]
	Comment 104 • 10 years ago

Those problems sound very similar to the problems that Xrays solve for chrome objects that have to interact with things exposed to content (I hope I'm putting that right). Would bug 914970 (and specifically bug 972987) add machinery to help make self-hosted code less of a footgun? If not, has something similar been considered?

	Bobby Holley (:bholley)
	Comment 105 • 10 years ago

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #104)
> Those problems sound very similar to the problems that Xrays solve for
> chrome objects that have to interact with things exposed to content (I hope
> I'm putting that right).

Yes, I agree. I asked Jeff about this when I discovered the self-hosting setup. He assured me that self-hosting code was being written by SpiderMonks who knew what they were doing. But bug 987243, bug 988416, bug 822080, and bug 834989 suggest that they mess up too on a not-too-infrequent basis.

The main question that would probably arise is performance - in particular, whether the interposition of a proxy (the security wrapper, which especially in the Xray case isn't exactly fast) would kill the intended jit performance benefits of self-hosting.

Of course, this is all moot until we get more ES Xrays up and running.

> (and specifically bug 972987)

That bug is about resolving self-hosted method implementations when interacting with objects over Xrays, which is slightly different.

	Emanuel Hoogeveen [:ehoogeveen]
	Comment 106 • 10 years ago

(In reply to Bobby Holley (:bholley) from comment #105)
> The main question that would probably arise is performance - in particular,
> whether the interposition of a proxy (the security wrapper, which especially
> in the Xray case isn't exactly fast) would kill the intended jit performance
> benefits of self-hosting.

As a layman I would actually expect that since Xrays always point to the original object (untouched by content), they might actually be easier to bake into the JITed code than the general case (which can be invalidated in more ways). But I'm sure that's a gross oversimplification ;)

> > (and specifically bug 972987)
> 
> That bug is about resolving self-hosted method implementations when
> interacting with objects over Xrays, which is slightly different.

Ah, right. Thanks for the response!

	Bobby Holley (:bholley)
	Comment 107 • 10 years ago

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #106)
> (In reply to Bobby Holley (:bholley) from comment #105)
> > The main question that would probably arise is performance - in particular,
> > whether the interposition of a proxy (the security wrapper, which especially
> > in the Xray case isn't exactly fast) would kill the intended jit performance
> > benefits of self-hosting.
> 
> As a layman I would actually expect that since Xrays always point to the
> original object (untouched by content), they might actually be easier to
> bake into the JITed code than the general case (which can be invalidated in
> more ways). But I'm sure that's a gross oversimplification ;)

Well, that would be assuming that we implemented a JIT code implementation of Xrays, which is not a very appealing task. In general, Xrays focus more on correctness and security than speed, and operations on them are several orders of magnitude slower than operations on a non-cross-compartment wrapper, even in the interpreter.

	Jeff Walden [:Waldo]
	Comment 108 • 10 years ago

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #104)
> Those problems sound very similar to the problems that Xrays solve for
> chrome objects that have to interact with things exposed to content (I hope
> I'm putting that right). Would bug 914970 (and specifically bug 972987) add
> machinery to help make self-hosted code less of a footgun? If not, has
> something similar been considered?

Speed considerations of Xrays aside (I would note that the alternatives that avoid those bugs are themselves *faster* as well), I personally consider Xrays a workaround for a design flaw in how extensions work.  Letting extensions directly touch untrusted content, and expect to have a coherent (but fake) view of the world, is a mistake.  The means for performing those operations should look visibly different, so that it's clear a trust boundary is being crossed.  (Which is what the various self-hosted intrinsics and such do.)  And the "naive" ways to perform those operations really just shouldn't work at all.  IMO.

Of course, maybe at this point chrome directly touching content is a mistake we have to live with, until we can get people to use message passing and content scripts and similar sorts of things.  But I really don't want to repeat the mistake elsewhere (for self-hosting embedder-provided code), and require yet further fragile work on that sort of thing.

	Jean-Yves Perrier [:teoli]
	Comment 109 • 10 years ago

Doc updated:
WorkerConsole has been removed. It points to Console now, that have the info about being usable on dedicated workers.
https://developer.mozilla.org/en-US/docs/Web/API/Console
and
https://developer.mozilla.org/en-US/Firefox/Releases/30

	Firefox Bug Husbandry Bot
	Comment 110 • 6 years ago

Removing leave-open keyword from resolved bugs, per :sylvestre.