Currently, there is no good way to debug Web Workers. JSD2 will be the best solution here, but in the meantime being able to do logging from workers would be a useful step. Users can proxy logging from the workers to the main thread using postMessage, but it would be nice if the console object was simply available in the workers. jjb pointed me at bug 507783 which explored this a bit. I don't know if the code has changed since then to make this more practical, but adding this would be a huge help for people using workers.

I have a quick and dirty implementation of |console| for _Chrome_ workers that outputs to stdout. If there is any interest, I can clean it up.

Interest yes. Can it be made more generic to work with plain' ol' Web Workers?

In order to log any js values I have to stringify them before sending them to the main thread.

Comment on attachment 651721 [details] [diff] [review] Bug 620935 - Make console object available in Web Workers Review of attachment 651721 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/Makefile.in @@ +60,4 @@ > $(NULL) > > LOCAL_INCLUDES = \ > + -I$(topsrcdir)/js/xpconnect/src \ What do you need this for?

> > LOCAL_INCLUDES = \ > > + -I$(topsrcdir)/js/xpconnect/src \ > > What do you need this for? Nothing. removed.

1. Title changed 2. an include entry in the makefile removed.

Comment on attachment 652344 [details] [diff] [review] Bug 620935 - Make console object available in Web Workers Review of attachment 652344 [details] [diff] [review]: ----------------------------------------------------------------- Is there any way this can be switched to using WebIDL bindings?

Please write a document :) https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Adding_WebIDL_bindings_to_a_class

> Is there any way this can be switched to using WebIDL bindings? I don't know. The console has all properties + noSuchMethod. Probably this requires to many changes in the webidl bindings implementation.

I think you can postpone __noSuchMethod__ things to a follow-up bug. The WebIDL "spec" (not impl) cannot handle identifiers starting with underscores.

In bug 863814 I wanted to approach things by adding a Console API object implemented in JavaScript. I see here the patch does it in c++. I also wanted output to the web console (via notification observer, console-api-log-event topic), similar to how ConsoleAPI.js works. What is holding us from having this new object implemented in JS and also have it output to the web console? For output to the web console we need the web worker's owning window ID. For object inspection we can do something similar to postMessage() if that's acceptable.

The C++ class is just a wrapper. It sends any console.foo() to the main thread, and then from there it uses the consoleAPI.js. The reason why this patch is not landed yet, is because we want to use webIDL. Let's ask to bent what he prefer for this issue.

I don't really care if we do this in C++ or JS, but either way we'll need to use WebIDL.

How to implement __noSuchMethod__ using WebIDL? AFAIK it's impossible. Follow-up bug?

What is __noSuchMethod__ being used for in this case?

(In reply to Boris Zbarsky (:bz) from comment #17) > What is __noSuchMethod__ being used for in this case? The idea is that you can do: Console.foobar(); and it doesn't throw. If we want to get rid of it, Console can be ported to WebIDL.. and here a question: do we want to port Console object to WebIDL in main thread too? Currently it's implemented here: https://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPI.js https://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPI.manifest

Note that there are quite a few console methods from Firebug's laundry list (which is the de-facto standard) that we don't implement: https://getfirebug.com/wiki/index.php/Console_API I don't think they are in widespread use, but there is a chance of breaking web sites if we don't implement __noSuchMethod__.

If there is no way to get __noSuchMethod__ that should not be too much of a problem - I would not make it a requirement for workers. Take into consideration that web workers have no console object at this point, so if we add a console object that implements a well defined set of methods that should already be more than satisfactory. The set of methods I would like to see implemented is the same as those in the DOM ConsoleAPI.js. However, do note that for the DOM |window.console| object we still need something like __noSuchMethod__ as pointed out by Panos. (In reply to Andrea Marchesini (:baku) from comment #18) > (In reply to Boris Zbarsky (:bz) from comment #17) > > What is __noSuchMethod__ being used for in this case? > > The idea is that you can do: Console.foobar(); and it doesn't throw. > If we want to get rid of it, Console can be ported to WebIDL.. and here a > question: do we want to port Console object to WebIDL in main thread too? > Currently it's implemented here: > > https://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPI.js > https://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPI.manifest Do we need to port the DOM window.console object to WebIDL? Are there any benefits? For example, can we reuse the code for the web worker |console| object and the DOM |window.console| object?

> The idea is that you can do: Console.foobar(); and it doesn't throw. Ah. Well, does putting: void __noSuchMethod__(); in the IDL (and adjusting the IDL parser to allow it), work?

(In reply to Panos Astithas [:past] from comment #19) > I don't think they are in widespread use, but there is a chance of breaking > web sites if we don't implement __noSuchMethod__. These site (if at all present) will break on Chrome and IE10 because both browsers throw with |console.foobar()|. We should add explicit dummy methods for possible names in the real world rather than catching all blindly.

(In reply to Boris Zbarsky (:bz) from comment #21) > > The idea is that you can do: Console.foobar(); and it doesn't throw. > > Ah. > > Well, does putting: > > void __noSuchMethod__(); > > in the IDL (and adjusting the IDL parser to allow it), work? IIRC WebIDL spec disallows defining an identifier starting with underscores. It should be realized by somthing like "serializers" (if we modify the spec).

> IIRC WebIDL spec disallows defining an identifier starting with underscores. Yes, yes. Hence "adjusting the IDL parser to allow it". But yes, we could come up with custom syntax for it if we really need to.

(In reply to Boris Zbarsky (:bz) from comment #24) > > IIRC WebIDL spec disallows defining an identifier starting with underscores. > > Yes, yes. Hence "adjusting the IDL parser to allow it". I'm not a fan of adding a dependency on the Mozilla-specific magic keyword. > But yes, we could come up with custom syntax for it if we really need to. Hence the question: do we really want to modify the spec due to a fear of breaking a fictional site? The site (again, if at all present) should be evangelized IMO.

The goal behind noSuchMethod support isn't to support people calling made-up methods, it's to support differences in what methods exist on .console in different browsers/configurations. When we were implementing .console, there were sites that assumed that the existence of .console implied that it was the Firebug console object (which had methods we didn't support), see bug 614350. That was a long time ago, though. We now support more methods, and other browsers now also support .console by default, so the need for this is probably worth revisiting. I agree that adding specific stub methods as needed is a cleaner solution anyhow.

(In reply to Andrea Marchesini (:baku) from comment #18) > (In reply to Boris Zbarsky (:bz) from comment #17) > > What is __noSuchMethod__ being used for in this case? > > The idea is that you can do: Console.foobar(); and it doesn't throw. I don't know the WebIDL bits for that, but you can make console a proxy for that purpose: console = new Proxy(actualConsoleObject, { get: function(target, name){ if(name in target){ return target[name]; } else{ return function(){ // noop target.warn('The console object does not support the '+name+' method') } } } }) Proxies have been introduced to provide a clean alternative to __noSuchMethod__

(In reply to David Bruant from comment #27) > (In reply to Andrea Marchesini (:baku) from comment #18) > > (In reply to Boris Zbarsky (:bz) from comment #17) > > > What is __noSuchMethod__ being used for in this case? > > > > The idea is that you can do: Console.foobar(); and it doesn't throw. > I don't know the WebIDL bits for that, but you can make console a proxy for > that purpose: > > console = new Proxy(actualConsoleObject, { > get: function(target, name){ > if(name in target){ > return target[name]; > } > else{ > return function(){ // noop > target.warn('The console object does not support the '+name+' > method') > } > } > } > }) > > Proxies have been introduced to provide a clean alternative to > __noSuchMethod__ Yes, we've had a long-standing bug on file to implement the console api using a proxy. see bug 629607. I think that'd be the cleanest solution and allow us to get rid of the __noSuchMethod__ magic without having to keep implementing stubs whenever another browser adds a new method. FWIW, I think the practice of looking for the existence of a console object to determine if Firebug is installed has been mostly eradicated. I hope.

If the console object will be implemented using Proxy, WebIDL is not usable anyway (unless new custom syntax is added). (In reply to Rob Campbell [:rc] (:robcee) from comment #28) > I think that'd be the cleanest solution and allow us to get rid of the > __noSuchMethod__ magic without having to keep implementing stubs whenever > another browser adds a new method. We should standardize the console object rather than allowing browsers to fun with sticking random methods on the console object forever. Feature detection is the established convention for all other DOM objects. Why only the console object is the exception? Why not use __noSuchMethods__ (or Proxy or whatever your favorite catch-all trick) everywhere to wipe out annoying TypeErrors?

I definitely agree that the console object needs to be standardized just like any other part of the platform. The noSuchMethod hack should eventually go away. That said, we shouldn't block worker support on that. I.e. I think standardizing console is a different bug than this one. I'd really like to see console working in workers with the same behavior as we have on the main thread. noSuchMethod crap and all. But if we really want to use WebIDL (which I know we do), then skipping the noSuchMethod stuff in workers is probably ok. However there's definitely a risk that it'll cause breakage. Ben: It's ultimately your call.

It looks like there is movement in bug 629607 (and its dependencies). Once that is fixed we can have identical behavior on main thread and in workers, right? If that can happen in the short term then I think that is the best way forward. In parallel we should definitely push to standardize the console object. Eventually the proxy stuff should go away.

That's still not moving console to use WebIDL. So I don't think it's helping with the solution that people have been asking for. As far as I can tell at least.

I really don't care!

Ok, I propose that we move forward with the current patch. Console doesn't look like it's going to move to WebIDL anytime soon so we can't block on that. So unless someone has proposals for how to improve the current patch which works on existing architecture and gives the existing behavior, I think we should move forward with what we have.

+1

Andrea: So does that mean that the current patch is good as-is? Or are there still comments that needs to be fixed before it's ready for review?

The patch must be updated. But it's good enough for a review. I can upload a new version of it this week.

baku: having console.log available from workers would be truly wonderful. Any chance you might be able to upload that new version and request review sooner rather than later?

I think Baku is off on vacation and not back until next year :(

(In reply to Dan Mosedale (:dmose) from comment #39) > baku: having console.log available from workers would be truly wonderful. > Any chance you might be able to upload that new version and request review > sooner rather than later? I don't think I'm able to work on this before January. I'll try to rebase my patch and see how bad/good it still works.

This patch is almost ready. It still needs a couple of dependences to be fixed in order to work properly. What it's not implemented (and I don't think it's going to be soon) is the __NoSuchMethod__. This is not supported by webidl and it ca be a follow up if needed.

Andrea: thanks for your work on this patch. We have more methods now, in ConsoleAPI.js. We have exception(), which is generic, like log()/error()/debug()/etc. We also added assert(). Can we have these added as well in this patch? The profiler also supports console.profile/profileEnd() - I assume this would need a follow-up bug report.

(In reply to Mihai Sucan [:msucan] from comment #43) > Andrea: thanks for your work on this patch. We have more methods now, in > ConsoleAPI.js. We have exception(), which is generic, like > log()/error()/debug()/etc. We also added assert(). Can we have these added > as well in this patch? The profiler also supports > console.profile/profileEnd() - I assume this would need a follow-up bug > report. Sure! Currently we have: void log(any... data); void info(any... data); void warn(any... data); void error(any... data); void debug(any... data); void trace(); void dir(any data); void group(); void groupCollapsed(); void groupEnd(); void time(optional DOMString time); void timeEnd(optional DOMString time); The best approach is to use the same webidl file for ConsoleAPI.js and this code. Once this patch is working, reviewed and landed we can maybe update ConsoleAPI.js and rewrite it using webidl. How important is __noSuchMethod__ for ConsoleAPI ? Can we remove it?

(In reply to Andrea Marchesini (:baku) from comment #44) > (In reply to Mihai Sucan [:msucan] from comment #43) > > Andrea: thanks for your work on this patch. We have more methods now, in > > ConsoleAPI.js. We have exception(), which is generic, like > > log()/error()/debug()/etc. We also added assert(). Can we have these added > > as well in this patch? The profiler also supports > > console.profile/profileEnd() - I assume this would need a follow-up bug > > report. > > Sure! Currently we have: > [...] Great! Thanks! > The best approach is to use the same webidl file for ConsoleAPI.js and this > code. Once this patch is working, reviewed and landed we can maybe update > ConsoleAPI.js and rewrite it using webidl. > > How important is __noSuchMethod__ for ConsoleAPI ? Can we remove it? I do not know how important __noSuchMethod__ is. We implement the majority of console methods, but we could still see pages breaking. We would need data about web apps and their use of console APIs, or we can just 'bite the bullet' and remove __noSuchMethod__.

(In reply to Mihai Sucan [:msucan] from comment #45) > (In reply to Andrea Marchesini (:baku) from comment #44) > > The best approach is to use the same webidl file for ConsoleAPI.js and this > > code. Once this patch is working, reviewed and landed we can maybe update > > ConsoleAPI.js and rewrite it using webidl. > > > > How important is __noSuchMethod__ for ConsoleAPI ? Can we remove it? > > I do not know how important __noSuchMethod__ is. We implement the majority > of console methods, but we could still see pages breaking. We would need > data about web apps and their use of console APIs, or we can just 'bite the > bullet' and remove __noSuchMethod__. Since console wasn't available in workers until recently (even in Chrome), it's unlikely the console object was used at all in worker. I wouldn't worry about __noSuchMethod__ and wait until someone comes across the problem (which might just never happen). Note bug 629607 where it's suggested to replace __noSuchMethod__ with a proxy for console (main thread I guess?). You may want to do that directly if you're re-implementing a console object.

Another important problem is with 'exception'. This is a reserved word for webIDL so we cannot have a method with such name. I guess this is a problem too if we migrate ConsoleAPI to webIDL on main-thread.

(In reply to Andrea Marchesini (:baku) from comment #47) > Another important problem is with 'exception'. This is a reserved word for > webIDL so we cannot have a method with such name. I guess this is a problem > too if we migrate ConsoleAPI to webIDL on main-thread. _exception

IMHO, it is important to implement the noSuchMethod behavior, but it's ok if it is a follow up. Other devtools may soon be implementing this, and it would be a shame if we went backwards: https://github.com/DeveloperToolsWG/console-object#ideas

We could just whitelist the __noSuchMethod__ name in the WebIDL parser so you can use it...

This enables __noSuchMethod__ but maybe it's a bit dirty :)

Comment on attachment 8355274 [details] [diff] [review] noSuchMethod.patch >+ if name[:2] == "__" and name != "__content" and name != "__noSuchMethod__" and not allowDoubleUnderscore: This should check for "___noSuchMethod__" (three leading underscores), imo. >- if name[0] == '_' and not allowDoubleUnderscore: >+ if name[0] == '_' and name != "__noSuchMethod__" and not allowDoubleUnderscore: And then this change is not needed. r=me with that.

msucan can you check if the behaviour implemented by this patch is what you expect? It's hard to test the ConsoleAPI with mochitests so I don't know if the output I see in the right one. Thanks.

Andrea: this seems to be working nicely. Thank you! However, I do get a segfault if I try: onmessage = function(ev) { console.log(ev); } in a worker. This crasher could be quite common. To write tests please look into dom/base/ConsoleAPI.js. You should be able to add a new test script which loads a page with a worker that has console API calls. You might also add tests in {browser|toolkit}/devtools/webconsole/test.

Comment on attachment 8356677 [details] [diff] [review] patch 2 - console API for workers Handing off to ehsan, but please feel free to re-r? me if anything is confusing!

Comment on attachment 8356677 [details] [diff] [review] patch 2 - console API for workers Review of attachment 8356677 [details] [diff] [review]: ----------------------------------------------------------------- I hope this will be carefully reviewed by someone who knows JSAPI. ::: dom/workers/Console.cpp @@ +23,5 @@ > + > +BEGIN_WORKERS_NAMESPACE > + > +static JSObject* > +ConsoleStructuredCloneCallbacksRead(JSContext* aCx, Add docs @@ +38,5 @@ > + return nullptr; > + } > + > + JS::Rooted<JS::Value> value(aCx, > + STRING_TO_JSVAL(JS_NewUCStringCopyZ(aCx, Null check, and use JS::StringValue. Actually, there should be functions in dom/bindings/ somewhere that do this better @@ +42,5 @@ > + STRING_TO_JSVAL(JS_NewUCStringCopyZ(aCx, > + strings->ElementAt(aData).get()))); > + > + JS::Rooted<JSObject*> obj(aCx); > + if (!JS_ValueToObject(aCx, value, &obj)) { Sure this is the right behaviour? @@ +50,5 @@ > + return obj; > +} > + > +static bool > +ConsoleStructuredCloneCallbacksWrite(JSContext* aCx, Add docs @@ +55,5 @@ > + JSStructuredCloneWriter* aWriter, > + JS::Handle<JSObject*> aObj, > + void* aClosure) > +{ > + JS::Rooted<JS::Value> value(aCx, OBJECT_TO_JSVAL(aObj)); JS::ObjectOrNullValue() (similar throughout) @@ +56,5 @@ > + JS::Handle<JSObject*> aObj, > + void* aClosure) > +{ > + JS::Rooted<JS::Value> value(aCx, OBJECT_TO_JSVAL(aObj)); > + JS::Rooted<JSString*> jsString(aCx, JS::ToString(aCx, value)); null check @@ +65,5 @@ > + } > + > + nsTArray<nsString>* strings = static_cast<nsTArray<nsString>*>(aClosure); > + > + JS_WriteUint32Pair(aWriter, CONSOLE_TAG, strings->Length()); Failure check @@ +72,5 @@ > + return true; > +} > + > +static void > +ConsoleStructuredCloneCallbacksError(JSContext* aCx, uint32_t /* aErrorId */) Consistency please, both are unused @@ +74,5 @@ > + > +static void > +ConsoleStructuredCloneCallbacksError(JSContext* aCx, uint32_t /* aErrorId */) > +{ > + MOZ_ASSERT("This cannot happen!"); First argument to MOZ_ASSERT is a boolean @@ +87,5 @@ > +class ConsoleStackData > +{ > +public: > + ConsoleStackData() > + : mLineNumber(0) Two more spaces @@ +101,5 @@ > +public: > + ConsoleRunnable(WorkerPrivate* aWorkerPrivate, > + JSContext* aCx, > + const char* aMethod, > + JS::HandleValue aArguments, JS::Handle<JS::Value> @@ +105,5 @@ > + JS::HandleValue aArguments, > + const nsTArray<ConsoleStackData>& aStackData) > + : mWorkerPrivate(aWorkerPrivate) > + , mMethod(aMethod) > + , mStackData(aStackData) Two more spaces @@ +148,5 @@ > + return NS_OK; > + } > + > + void > + RunConsole() What happens with exceptions thrown below? @@ +151,5 @@ > + void > + RunConsole() > + { > + // recursive worker private > + WorkerPrivate *wp = mWorkerPrivate; * to the left @@ +186,5 @@ > + JSAutoCompartment ac(cx, consoleObj); > + > + // 3 args for the queueCall. > + JS::Rooted<JS::Value> methodValue(cx, > + STRING_TO_JSVAL(JS_NewStringCopyZ(cx, mMethod))); As before @@ +190,5 @@ > + STRING_TO_JSVAL(JS_NewStringCopyZ(cx, mMethod))); > + > + JS::Rooted<JS::Value> argumentsValue(cx); > + if (!mArguments.read(cx, &argumentsValue, &gConsoleCallbacks, > + &mStrings)) { Fits on one line @@ +197,5 @@ > + > + JS::Rooted<JS::Value> stackValue(cx); > + { > + JS::Rooted<JSObject*> stackObj(cx, > + JS_NewArrayObject(cx, mStackData.Length(), nullptr)); null @@ +200,5 @@ > + JS::Rooted<JSObject*> stackObj(cx, > + JS_NewArrayObject(cx, mStackData.Length(), nullptr)); > + for (uint32_t i = 0; i < mStackData.Length(); ++i) { > + JS::Rooted<JSObject*> obj(cx, > + JS_NewObject(cx, nullptr, nullptr, nullptr)); null @@ +203,5 @@ > + JS::Rooted<JSObject*> obj(cx, > + JS_NewObject(cx, nullptr, nullptr, nullptr)); > + > + JS::Rooted<JS::Value> filenameValue(cx, > + STRING_TO_JSVAL(JS_NewUCStringCopyZ(cx, null @@ +211,5 @@ > + return; > + } > + > + JS::Rooted<JS::Value> lineNumberValue(cx, > + INT_TO_JSVAL(mStackData[i].mLineNumber)); JS::Int32Value() @@ +218,5 @@ > + return; > + } > + > + JS::Rooted<JS::Value> functionValue(cx, > + STRING_TO_JSVAL(JS_NewUCStringCopyZ(cx, null @@ +241,5 @@ > + > + stackValue = OBJECT_TO_JSVAL(stackObj); > + } > + > + JS::Value argv[] = { methodValue, argumentsValue, stackValue }; Rooting hazard (read: sg-crit) @@ +281,5 @@ > + const AutoSequence<JS::Value>& aData, > + uint32_t aStackLevel) > +{ > + JS::Rooted<JSObject*> arguments(aCx, > + JS_NewArrayObject(aCx, aData.Length(), nullptr)); null @@ +304,5 @@ > + > + runnable->Dispatch(aCx); > +} > + > +#define METHOD( name, jsName ) \ Extraneous spaces @@ +352,5 @@ > + nsString string; > + string.Assign(aTimer.Value()); > + > + JS::Rooted<JS::Value> value(aCx, > + STRING_TO_JSVAL(JS_NewUCStringCopyZ(aCx, string.get()))); null @@ +369,5 @@ > + nsString string; > + string.Assign(aTimer.Value()); > + > + JS::Rooted<JS::Value> value(aCx, > + STRING_TO_JSVAL(JS_NewUCStringCopyZ(aCx, string.get()))); null @@ +383,5 @@ > +void > +WorkerConsole::Assert(JSContext* aCx, JS::Handle<JS::Value> aCondition, > + const AutoSequence<JS::Value>& aData) > +{ > + if (!JS::ToBoolean(aCondition)) { Why not make it a boolean argument? ::: dom/workers/Console.h @@ +3,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef mozilla_dom_workers_console_h__ > +#define mozilla_dom_workers_console_h__ No trailing underscores @@ +32,5 @@ > + > + virtual JSObject* > + WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; > + > + nsISupports* GetParentObject() const { { on next line ::: dom/workers/WorkerScope.cpp @@ +77,5 @@ > { > MOZ_CRASH("We should never get here!"); > } > > +already_AddRefed<WorkerConsole> Why a_A? ::: dom/workers/test/mochitest.ini @@ +122,5 @@ > skip-if = (os == "win") || (os == "mac") > [test_url_exceptions.html] > [test_urlSearchParams.html] > [test_jsversion.html] > +[test_console.html] Looks like test_jsversion.html is in the wrong place too, but please put it in its alphabetical place

Comment on attachment 8356677 [details] [diff] [review] patch 2 - console API for workers Review of attachment 8356677 [details] [diff] [review]: ----------------------------------------------------------------- r- on top of the comments from Ms2ger. Note that I did my best to do a good job of reviewing the jsapi calls, but I think you want somebody to look over those usages in the final version of the patch. ::: dom/webidl/WorkerConsole.webidl @@ +20,5 @@ > + void timeEnd(optional DOMString time); > + void profile(any... data); > + void profileEnd(any... data); > + void assert(any condition, any... data); > + void ___noSuchMethod__(DOMString methodName, any... data); Why do you need the args here? ::: dom/workers/Console.cpp @@ +38,5 @@ > + return nullptr; > + } > + > + JS::Rooted<JS::Value> value(aCx, > + STRING_TO_JSVAL(JS_NewUCStringCopyZ(aCx, If I'm reading the code right, JS_NewUCStringCopyZ can never return null while the JSRuntime object is alive (but I could be wrong.) That being said, please add a new overload of ByteStringToJsval (and NonVoidByteStringToJsval) to BindingUtils.h, make them accept a nsAString, and use them here. @@ +42,5 @@ > + STRING_TO_JSVAL(JS_NewUCStringCopyZ(aCx, > + strings->ElementAt(aData).get()))); > + > + JS::Rooted<JSObject*> obj(aCx); > + if (!JS_ValueToObject(aCx, value, &obj)) { I'm not sure either. Looking at NS_DOMReadStructuredClone, it tries to at least report an XPConnect error. But I also don't understand the purpose of this function as a whole. For example, you're never even accessing the structured clone reader... @@ +74,5 @@ > + > +static void > +ConsoleStructuredCloneCallbacksError(JSContext* aCx, uint32_t /* aErrorId */) > +{ > + MOZ_ASSERT("This cannot happen!"); What guarantees this? Please document that! @@ +109,5 @@ > + , mStackData(aStackData) > + { > + mWorkerPrivate->AssertIsOnWorkerThread(); > + > + mArguments.write(aCx, aArguments, &gConsoleCallbacks, &mStrings); Shouldn't you handle the failure of this method somehow? @@ +122,5 @@ > + > + mSyncLoopTarget = syncLoop.EventTarget(); > + > + if (NS_FAILED(NS_DispatchToMainThread(this, NS_DISPATCH_NORMAL))) { > + JS_ReportError(aCx, "Failed to dispatch to main thread!"); Please use a more descriptive error message, at least mention the worker console API and the method name here! @@ +291,5 @@ > + } > + JS::Rooted<JS::Value> argumentsValue(aCx, OBJECT_TO_JSVAL(arguments)); > + > + nsTArray<ConsoleStackData> stackData; > + if (!GetStackTrace(aCx, aStackLevel, stackData)) { I think it's better to do this first, to not bother with creating the arguments array if this fails. @@ +310,5 @@ > + WorkerConsole::name(JSContext* aCx, \ > + const AutoSequence<JS::Value>& aData) \ > + { \ > + Method(aCx, jsName, aData, 1); \ > + } Nit: please #undef METHOD when you're done with it. @@ +383,5 @@ > +void > +WorkerConsole::Assert(JSContext* aCx, JS::Handle<JS::Value> aCondition, > + const AutoSequence<JS::Value>& aData) > +{ > + if (!JS::ToBoolean(aCondition)) { Yeah, I think that would be better. @@ +397,5 @@ > +} > + > +bool > +WorkerConsole::GetStackTrace(JSContext* aCx, uint32_t aMaxDepth, > + nsTArray<ConsoleStackData>& aStackData) This function is almost an exact copy of JSStackFrame::CreateStack, except for the format it returns its data in. Could you please refactor the common logic in a BindingUtils method? @@ +423,5 @@ > + > + // FunctionName: > + JS::Rooted<JSFunction*> fun(aCx, desc->frames[i].fun); > + if (fun) { > + JS::Rooted<JSString*> funid(aCx, JS_GetFunctionId(fun)); You want JS_GetFunctionDisplayId to get good names for anonymous functions. @@ +437,5 @@ > + > + aStackData.AppendElement(data); > + } > + > + JS::FreeStackDescription(aCx, desc); Please create a RAII class which does this, otherwise it will be skipped in the early return case above. @@ +439,5 @@ > + } > + > + JS::FreeStackDescription(aCx, desc); > + > + // Let's return ONLY if the stack is not empty: Nit: s/return/return true/ ::: dom/workers/Console.h @@ +3,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef mozilla_dom_workers_console_h__ > +#define mozilla_dom_workers_console_h__ Also, technically, this should be mozilla_dom_workers_Console_h. @@ +27,5 @@ > + NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(WorkerConsole) > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(WorkerConsole) > + > + static already_AddRefed<WorkerConsole> > + Create(); Nit: please put the return type on the same line as the function name in all declarations. (This is the style for function definitions.) And yes, the worker code violates our style. :-) ::: dom/workers/test/console_worker.js @@ +9,5 @@ > + > + postMessage({event: 'trace without function', status: true, last : false}); > + > + for (var prop in console) { > + dump(prop + " => " + console[prop] + "\n"); Why do you need these dump calls? ::: dom/workers/test/test_console.html @@ +21,5 @@ > +<script class="testbody" language="javascript"> > + var worker = new Worker("console_worker.js"); > + > + worker.onmessage = function(event) { > + is(event.target, worker); Nit: please pass a description argument to is.

> @@ +42,5 @@ > > + STRING_TO_JSVAL(JS_NewUCStringCopyZ(aCx, > > + strings->ElementAt(aData).get()))); > > + > > + JS::Rooted<JSObject*> obj(aCx); > > + if (!JS_ValueToObject(aCx, value, &obj)) { > > I'm not sure either. Looking at NS_DOMReadStructuredClone, it tries to at > least report an XPConnect error. > > But I also don't understand the purpose of this function as a whole. For > example, you're never even accessing the structured clone reader... I don't need to use it. This method is called with a tag and data. Data is the ID of the string in the array of 'written' strings. > > +static void > > +ConsoleStructuredCloneCallbacksError(JSContext* aCx, uint32_t /* aErrorId */) > > +{ > > + MOZ_ASSERT("This cannot happen!"); Now we are introducing a couple of reasons why this could happen (failure in write()).

This is the interdiff.

> Nit: please put the return type on the same line as the function name in all > declarations. (This is the style for function definitions.) > > And yes, the worker code violates our style. :-) The rest of the worker code is using the style of this patch. Why do you prefer to change my patch is in this way?

(In reply to Andrea Marchesini (:baku) from comment #64) > > Nit: please put the return type on the same line as the function name in all > > declarations. (This is the style for function definitions.) > > > > And yes, the worker code violates our style. :-) > > The rest of the worker code is using the style of this patch. > Why do you prefer to change my patch is in this way? Mostly because of the recent coding style conversations on dev-platform. But we can also fix the style of the entire worker code whole-sale at some point, so I wouldn't block this patch only because of the style.

Comment on attachment 8357765 [details] [diff] [review] patch 2 - console API for workers Review of attachment 8357765 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Please ask somebody familiar with the jsapi to go through the usages here before landing. (Also, thanks a lot for the interdiff, it really helped me review this faster!) ::: dom/workers/Console.cpp @@ +122,5 @@ > + > +class ConsoleRunnable MOZ_FINAL : public nsRunnable > +{ > +public: > + ConsoleRunnable(WorkerPrivate* aWorkerPrivate) Nit: please make the constructor explicit. @@ +214,5 @@ > + > + JSAutoCompartment ac(cx, consoleObj); > + > + // 3 args for the queueCall. > + JS::Rooted<JSString*> str(cx, JS_NewStringCopyZ(cx, mMethod)); Can't you use ByteStringToJsval here? @@ +298,5 @@ > + > + nsTArray<nsString> mStrings; > + > + nsString mFilename; > + nsString mFunctionName; I think these two members are unused. Please remove them.

bz, this patch has been reviewed by ehsan and (partially?) by ms2ger. What we need is a review about the JS API usage. can you take a look? As usual, any additional comment is more than welcome.

Comment on attachment 8358124 [details] [diff] [review] patch 2 - console API for workers Why are we adding ByteStringToJsval bits here? ByteString is a narrow string; if you have an nsString it's not a ByteString. What was wrong with xpc::StringToJsval? >+++ b/dom/bindings/Exceptions.cpp The code you're changing here was pretty heavily rewritten in bug 932837. You should probably merge to those changes and then ask for another quick review of this part... >+++ b/dom/workers/Console.cpp >+class ConsoleRunnable MOZ_FINAL : public nsRunnable >+ Dispatch(JSContext* aCx, If aStackData were not const, you could mStackData.SwapElements(aStackData) instead of copying it all, right? Probably better to do that. I wish we had a way to assert that aMethod is a static string. :( >+ RunConsole() >+ // recursive worker private Maybe "Walk up to our containing page"? >+ nsCOMPtr<nsISupports> cInstance = >+ do_CreateInstance("@mozilla.org/console-api;1"); We really create a new instance of this JS component for every console operation in a worker? :( Since each instance registers as an inner-window-destroyed observer (albeit a weak one) this will leak one word of memory per console API call until the next inner window destruction, not to mention whatever other performance issues it has. I guess we don't want to get .console from the window because the page might have munged it? If console were on WebIDL on the main thread we could probably just call into the C++ parts of it, but as things are that's a bit of a pain. I'm probably OK with this landing as-is for now so we can work on shaking out other issues, but please make sure that a followup is filed on this and fixed before we ship this in a release! >+ nsCString method; >+ method.Assign(mMethod); Why not nsDependentCString method(mMethod)? >+ if (!ByteStringToJsval(cx, method, &methodValue)) { You need to clear the pending exception on cx here or something, no? >+ if (!mArguments.read(cx, &argumentsValue, &gConsoleCallbacks, &mStrings)) Likewise. Maybe you should just do this in the caller? So have RunConsole return the JSContext* and have the caller clear any still-pending exceptions? >+ JS_NewArrayObject(cx, mStackData.Length(), nullptr)); I wish you could just use our sequence codegen here... ah, well. >+ JS_NewObject(cx, nullptr, nullptr, nullptr)); But this part you _can_ do with dictionary codegen, and should. That is, define a WebIDL dictionary with the members you want, set it up, then convert it to a JSObject. That'll avoid various pitfalls like using JS_SetProperty instead of JS_DefineProperty and whatnot. I didn't review this part very carefully, since I think it should all go away. >+ if (!JS_SetElement(cx, stackObj, i, &value)) { Needs to be JS_DefineElement. >+ stackValue = OBJECT_TO_JSVAL(stackObj); stackValue.setObject(*stackObj); >+ JS::Value argv[] = { methodValue, argumentsValue, stackValue }; Like ms2ger said, this is a rooting hazard of the "exploitable security bug" kind. You want to use JS::AutoValueVector here. >+ JS_CallFunctionName(cx, consoleObj, "queueCall", 3, argv, ret.address()); Again, this can throw. But since you plan to clear any exceptions on cx, that's ok, I guess... >+ WorkerPrivate* mWorkerPrivate; I assume something makes sure it stays alive? >+WorkerConsole::Method(JSContext* aCx, const char* aMethodName, >+ const AutoSequence<JS::Value>& aData, AutoSequence is an implementation detail. Use Sequence, please. >+ if (NS_FAILED(stack->GetFilename(&string))) { Please file a followup to fix the nsIStackFrame API to use sane strings? >+ data.mFilename.Assign(string); >+ nsMemory::Free(string); data.mFilename.Adopt(string); perhaps? Same for mFunctionName. >+ stackData.AppendElement(data); That copies all the strings an extra time, no? How about doing, at the top of this block: ConsoleStackData& data = *stackData.AppendElement(); and then not needing the extra data copies? >+ stack = caller; stack.swap(caller) is probably a tiny bit faster, though on top of all the work going on here it might not matter... >+ JS_NewArrayObject(aCx, aData.Length(), nullptr)); Why not: JS_NewArrayObject(aCx, aData.Length(), aData.Data()) and skip the manual value-setting loop? If you do keep the loop for some reason, it should be using JS_DefineElement. >+ JS::Rooted<JS::Value> argumentsValue(aCx, OBJECT_TO_JSVAL(arguments)); JS::Rooted<JS::Value> argumentsValue(aCx, JS::ObjectValue(*arguments)); >+ WorkerConsole::name(JSContext* aCx, \ >+ const AutoSequence<JS::Value>& aData) \ Again, Sequence. And everywhere below here. >+WorkerConsole::Time(JSContext* aCx, const Optional<nsAString>& aTimer) So we take the incoming JS string, convert to an XPCOM string, then convert to a JS string, which we serialize via structured clone (which will stringify things if it doesn't know what they are anyway) and send to main thread where we rematerialize a JS string on which the console API code then calls toString().... How about we just take "any" here? If we really think we should be calling toString eagerly instead of just structured cloning the object over, doing JS::ToString and storing the result in the Value we stick in "data" will be less work than what happens now, and no less safe. >+WorkerConsole::TimeEnd(JSContext* aCx, const Optional<nsAString>& aTimer) Similar here. r=me with the above fixed, but I'd like to see the updated patch, and if possible an interdiff...

> We really create a new instance of this JS component for every console > operation in a worker? :( I proposed a patch for ConsoleAPI in WebIDL. Maybe once this patch lands, I can ask you to review that one too :) > Why not: > > JS_NewArrayObject(aCx, aData.Length(), aData.Data()) Data() doesn't exist. Right? > and skip the manual value-setting loop? If you do keep the loop for some > reason, it should be using JS_DefineElement. I don't know why (yet), but if I use JS_DefineElement the output is an empty array (or empty arguments).

> Data() doesn't exist. Right? Er, I meant aData.Elements(). > I don't know why (yet), but if I use JS_DefineElement the output is an empty array (or > empty arguments). Did you forget JSPROP_ENUMERATE?

Comment on attachment 8358348 [details] [diff] [review] console.patch As discussed on IRC, just kill off ConsoleStringToJsval; it's not needed. Please add a comment in Exceptions.h explaining what the aMaxDepth parameter means and how it behaves. > Dispatch(JSContext* aCx, >+ JS_ClearPendingException(aCx); Hrm. I guess we do need this, but it means that console API calls will just silently fail if the structured clone writing fails. Is that the behavior we want? An alternative would be to propagate this exception out to the caller, since at this point we are in fact being called from the worker JS... Followup bug OK for changes to this, I guess. You can give your WorkerConsoleStack members default values in WebIDL if you want to skip the annoying Construct() calls. >+ stack.mFilename.Value() = >+ NS_ConvertUTF8toUTF16(mStackData[i].mFilename); CopyUTF8toUTF16(mStackData[i].mFilename, stack.mFilename.Value()); Or stack.mFilename for the second arg if you do the default values thing. Similar for the function name. >+ if (!JS_DefineElement(cx, stackObj, i, value, nullptr, nullptr, 0)) { JSPROP_ENUMERATE. > stackValue = OBJECT_TO_JSVAL(stackObj); See previous review comments. >+ if (!argv.resize(3)) >+ return; Curlies? >+ JS::Rooted<JS::Value> argumentsValue(aCx, OBJECT_TO_JSVAL(arguments)); See previous review comments. I'd like someone familiar with the console API to check over the new behavior of time() and timeEnd(). r=me with that and the bits in comment 72.

Comment on attachment 8358347 [details] [diff] [review] interdiff Thank you for posting this!

Just another check. msucan, can you give a feedback about timer()?

interdiff

Comment on attachment 8358497 [details] [diff] [review] patch 2 - console API for workers >+// aMaxDepth can be used to define how depth the stack trace has to be. If the >+// value is -1 the stack trace is fully generated. How about: // aMaxDepth can be used to define a maximal depth for the stack trace. If the // value is -1, a default maximal depth will be selected. >+ stackValue = JS::ObjectOrNullValue(stackObj); It can't be null. JS::ObjectValue(*stackObj). >+ JS::Rooted<JS::Value> argumentsValue(aCx, JS::ObjectOrNullValue(arguments)); Similar here. r=me with that fixed.

Comment on attachment 8358530 [details] [diff] [review] patch 2 - console API for workers Review of attachment 8358530 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Andrea Marchesini (:baku) from comment #75) > Created attachment 8358497 [details] [diff] [review] > patch 2 - console API for workers > > Just another check. msucan, can you give a feedback about timer()? What changed wrt. time()/timeEnd() since I checked the patch? I played with console.time/timeEnd() in workers and they dont seem to work properly. When I call time() I get the correct message in the web console, but timeEnd() never shows. I enabled debugger protocol logging and I see timeEnd() comes without timing information (which causes the console to discard the message). bz mentioned you create a new instance of the console object every time a console method is accessed. Timer information gets lost - this would explain why timeEnd() doesnt work. Can you cache/reuse the same console instance per worker?

Problem fixed. Interdiff in 2 secs.

Still need to address comment 77. I'll try to sort through the ConsoleProxy bits on Monday, but do they not need to be CCed? Why not?

I don't think it has to be CCed because ConsoleProxy is created, used and destroyed only from the main-thread. I used the same pattern in URL in workers.

The key part is "destroyed". Is that destruction explicit? I guess it's done by the TeardownRunnable, and WorkerConsole can't be owned by anything reachable from ConsoleProxy?

Comment on attachment 8358782 [details] [diff] [review] interdiff ConsoleProxy::SetWrappedJS is unused. Please remove it. GetWrappedJS should be doing AssertIsOnMainThread(), right? What makes sure that ConsoleRunnable::RunConsole() can't run after ~WorkerConsole of its mConsole? I don't see anything doing that.... Should it be keeping mConsole alive somehow? What about its mWorkerPrivate? r=me with that addressed

> What makes sure that ConsoleRunnable::RunConsole() can't run after > ~WorkerConsole of its mConsole? I don't see anything doing that.... Should > it be keeping mConsole alive somehow? What about its mWorkerPrivate? ConsoleRunnable runs synchronously. https://tbpl.mozilla.org/?tree=Try&rev=1463d62033ed

ConsoleRunnable runs synchronously is just part of the answer. I talked with bent, and he confirmed that: Workers can't go away while they have an active sync loop even if the window is closed, so the worker is fine. Workers hold on to their windows until they shut down.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3da2b2a2899b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fbc3bb6c002f

Backed out for debug B2G mochitest-13 crashes. https://hg.mozilla.org/integration/mozilla-inbound/rev/36744b165ec6 https://tbpl.mozilla.org/php/getParsedLog.php?id=33453941&tree=Mozilla-Inbound

The reason why it was orange was bug 931887. Now this is landed we I can push these patches again. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/235d4c57787f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/691a1d12372a

(In reply to Andrea Marchesini (:baku) from comment #90) > The reason why it was orange was bug 931887. Now this is landed we I can > push these patches again. > > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/235d4c57787f > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/691a1d12372a sorry, had to back this out again for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=33519356&tree=Mozilla-Inbound

Just to have a backlog of this, msvc2013 doesn't compile with this patch, it complains about assert(), likely conflict with its keyword

https://tbpl.mozilla.org/?tree=Try&rev=109f83b89f1b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/faca83ad9b80 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/99c61f40109b

https://hg.mozilla.org/mozilla-central/rev/faca83ad9b80 https://hg.mozilla.org/mozilla-central/rev/99c61f40109b

relnote request. This feature is nice to be shared.

According to my tests, this doesn't work in Chrome Workers: bug 966486.

Guys, now that it's fixed, can somebody tell me if console is also available in Shared Web Workers?

Not yet. Or more precisely the API is there, but doesn't actually do anything useful. See bug 1058644.