1058644 - Console API doesn't work in SharedWorkers or ServiceWorkers

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrea Marchesini [:baku]]

The reason why is because a SharedWorker doesn't have a window parent.
This has to be fixed somehow.

Comment 1

[Boris Zbarsky [:bzbarsky]]

How exactly?  Which window's console should the messages go to?

What does Chrome do here, if anything?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

This is going to make it difficult for people to debug ServiceWorkers.  It seems we need some kind of devtools support in order to have somewhere to send the console output.

Comment 3

[Andrea Marchesini [:baku]]

We have Console API for JSM and we don't have a window there as well.
What the console API does, is the dispatching of events to the consoleAPI-storage service.

Currently these events have a InnerID of the inner window and a ID for the other window.
For JSM the events have ID 'jsm' and innerID the filename that called the Console method.

If we do something similar, devtools can show logs coming from SharedWorkers/SharedWorkers.

Comment 4

[Eddy Bruel [:ejpbruel]]

(In reply to Andrea Marchesini (:baku) from comment #3)
> We have Console API for JSM and we don't have a window there as well.
> What the console API does, is the dispatching of events to the
> consoleAPI-storage service.
> 
> Currently these events have a InnerID of the inner window and a ID for the
> other window.
> For JSM the events have ID 'jsm' and innerID the filename that called the
> Console method.
> 
> If we do something similar, devtools can show logs coming from
> SharedWorkers/SharedWorkers.

Using the consoleAPI-storage service sounds like a good idea, but that is an XPCOM service, is it not? We won't be able to use that from a Shared/ServiceWorker. It sounds to me like this would have to be fixed on the platform level.

Comment 5

[Andrea Marchesini [:baku]]

> Using the consoleAPI-storage service sounds like a good idea, but that is an
> XPCOM service, is it not? We won't be able to use that from a
> Shared/ServiceWorker. It sounds to me like this would have to be fixed on
> the platform level.

Yes. that is the plan.
But I guess, devtools has to dispatch these particular events dispatched by the Console API when received by the consoleAPI-storage. Maybe not by default.

Comment 6

[Andrea Marchesini [:baku]]

on #devtools they told me to NI paul about this proposal:

Currently we have 2 implementations of the Console API. For JSM we have Console.jsm and for content a C++ code. Both implementations dispatch events to the consoleAPI-storage.
These events are similar and the main difference is about the InnerID and the ID attributes:

https://mxr.mozilla.org/mozilla-central/source/dom/webidl/Console.webidl#35

Console API sets the innerID and the outer ID with the values of the window. Console.jsm sets 'jsm' in the ID and the filename of the callee in the innerID.

The proposal for SharedWorker and ServiceWorker is this:
can we dispatch such events with ID: 'SharedWorker' and innerID: the origin of the worker?
Should it be enough for the devtools to display them in the devtools console?

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Yes, that would work. But note that console messages from Console.jsm are only displayed in the Browser Console, not the Web Console in the current tab. The reason is that since these messages cannot be created by content pages, only browser/add-on developers would be interested in them. Is this OK for Shared/Service Workers, too?

If we want to display these messages to every web console as well, a few more changes to the webconsole filtering code would be required.

Comment 8

[Andrea Marchesini [:baku]]

> pages, only browser/add-on developers would be interested in them. Is this
> OK for Shared/Service Workers, too?

I think we should dispatch these messages in the webconsole maybe only if the developer wants to see them. In the end they are coming from the web, and not from addons.

The reason why I think the user has to choose to see them is because they don't have a reference to a window so they can arrive from a SharedWorkers running on another tabs.

What do you think about this? Is it possible?
Ehsan, what do you think about this idea?

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

It's certainly doable. We could make the extra filtering conditional on a pref that we expose in the web console's 'Logging' button as an additional option, or in the options panel of the toolbox.

Comment 10

[Andrea Marchesini [:baku]]

msucan, one of the problem I have to implement that I proposed in comment 6 is the timer that Console object uses to dispatch events to the ConsoleAPI-storage. I know that that timer comes from bug 761257.

At the moment the console API is written in C++. Do you think we still need that timer from a devtools point of view? Can we get rid of it completely or... better: can we send events directly to the consoleAPI-storage when they are dispatched from ServiceWorkers/SharedWorkers?

Comment 11

[(no longer active)]

FWIW I discussed this with baku, and we agreed on using a JSM like hack such as setting the ID to "sharedworker" and the inner ID to the URL of the worker script file name or something like that.

Comment 12

[Andrea Marchesini [:baku]]

msucan, I forgot to NI for comment 10.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Mihai is unavailable currently, but I'd be happy to help in any way that I can.

It's not clear to me why the timer is a problem, could you elaborate? Batching the calls had a significant impact in making the console faster and we wouldn't want to give up on that lightheartedly. We are still trying more optimizations in bug 1045715.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: Console API patch

This patch does theses changes:

1. The previous Console API sends messages to consoleAPI-Storage using a timer. With this patch this operation is done immediately and the scheduling of the messages is done into consoleAPI-Storage.

The reason of this a) is because nsITimer in Workers is a pain and 2) in this way we have 1 single timer for each window having the same result with less resources.

2. console-api-log-event is emitted by consoleAPI-Storage.

3. When Console runs on a SharedWorker or a ServiceWorker the messages are sent using the type of the worker as ID and the filename of the callee as innerID. I wrote a comment about this in Console.cpp.

This patch is not enough to fix this issue. Devtools team has to do something with these console-events.

Comment 15

[Boris Zbarsky [:bzbarsky]]

It will almost certainly be mid-next-week before I get to this.  :(

Comment 16

[Andrea Marchesini [:baku]]

Attachment: patch

I just released I completely forgot to implement something for Profile/ProfileEnd methods. This patch tests that as well.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: sw.patch

a better mochitest.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: sw.patch

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8483459 [details] [diff] [review]
sw.patch

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

Creating a sandbox here feels *extremely* heavyweight.  Let's talk about this in person this week.

::: dom/base/Console.cpp
@@ +137,5 @@
>      : mMethodName(Console::MethodLog)
>      , mPrivate(false)
>      , mTimeStamp(JS_Now() / PR_USEC_PER_MSEC)
>      , mMonotonicTimer(0)
> +  { }

Why did you get rid of the MOZ_COUNT_CTOR/DTOR macros?

@@ +181,3 @@
>    JS::Heap<JSObject*> mGlobal;
>  
> +  // The concept of outerID and innerID is misleading because when a

Yes, this is why they need better names :P

@@ +300,5 @@
> +    nsPIDOMWindow* window = wp->GetWindow();
> +    if (!window) {
> +      // Maybe this is a xpcshell test. But in any way we cannot continue
> +      // without a window.
> +      return;

Shouldn't we just call RunWindowless here now?

::: dom/base/nsIConsoleAPIStorage.idl
@@ +28,5 @@
>     *        The ID of the inner window for which the event occurred or "jsm" for
>     *        messages logged from JavaScript modules..
> +   * @param string aOuterId
> +   *        This ID is used as 3rd parameters for the console-api-log-event
> +   *        notification.

This needs a better name.  aExtraData, perhaps?  Given that the other ID is for the inner window this name feels like it should be for the outer window.

@@ +36,5 @@
> +  void recordEvent(in DOMString aId, in DOMString aOuterId, in jsval aEvent);
> +
> +  /**
> +   * Similar to recordEvent() but these events will be collected
> +   * and dispatched with a timer in order to do not flood the devtools

to avoid flooding

@@ +44,5 @@
> +   *        The ID of the inner window for which the event occurred or "jsm" for
> +   *        messages logged from JavaScript modules..
> +   * @param string aOuterId
> +   *        This ID is used as 3rd parameters for the console-api-log-event
> +   *        notification.

Same here

Comment 20

[Andrea Marchesini [:baku]]

> Creating a sandbox here feels *extremely* heavyweight.  Let's talk about
> this in person this week.

Sure. I write a few comments about your comments.


> Why did you get rid of the MOZ_COUNT_CTOR/DTOR macros?

Because this object is not allocated any more.

> Yes, this is why they need better names :P

Correct but this is not related to this issue. We already have this problem for the Console in JSM.
I suggest to fix it in a follow up.

> @@ +300,5 @@
> > +    nsPIDOMWindow* window = wp->GetWindow();
> > +    if (!window) {
> > +      // Maybe this is a xpcshell test. But in any way we cannot continue
> > +      // without a window.
> > +      return;
> 
> Shouldn't we just call RunWindowless here now?

Well.. why? I think we should just ignore the request. It's a corner case we can ignore: We don't have webconsole in xpcshell.

Comment 21

[Andrea Marchesini [:baku]]

Attachment: sw.patch

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

Comment 22

[Andrea Marchesini [:baku]]

Comment on attachment 8485948 [details] [diff] [review]
sw.patch

Some issue appears on tbpl.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: sw.patch

Finally I had time to fix the failure in the devtool mochitests.
Probably this should be green: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f6862ed78a07

Comment 24

[Andrea Marchesini [:baku]]

Comment on attachment 8509435 [details] [diff] [review]
sw.patch

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

::: dom/base/Console.cpp
@@ +1205,5 @@
>  
> +  MOZ_ASSERT(aData->mIDType != ConsoleCallData::eUnknown);
> +  if (aData->mIDType == ConsoleCallData::eString) {
> +    outerID = aData->mOuterIDString;
> +    outerID = aData->mInnerIDString;

This is innerID, and that is the reason of the mochitest failure. The rest of the patch is still valid.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Note, the maple tbpl showed a rooting hazard after this was applied there:

  https://tbpl.mozilla.org/?tree=Maple&rev=080ab8736c31&jobname=linux64-br-haz_maple_dep

Comment 26

[Andrea Marchesini [:baku]]

Attachment: sw.patch

hazard rooting fixed

Comment 27

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=451668f5c866

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Note, I saw this on maple:

  Assertion failure: mWorkerPrivate->IsSharedWorker() || mWorkerPrivate->IsServiceWorker(), at /builds/slave/map-l64-d-00000000000000000000/build/src/dom/base/Console.cpp:437

At:

  https://tbpl.mozilla.org/php/getParsedLog.php?id=54071206&tree=Maple&full=1#error1

Do nested child workers have a null window?  I don't think the window gets propagated.  Maybe this assert is too restrictive.

Comment 30

[Andrea Marchesini [:baku]]

Thanks for pointing to this. Yes, you are right, I'll add a mochitest.

Comment 31

[Andrea Marchesini [:baku]]

Attachment: sw.patch

Comment 32

[Andrea Marchesini [:baku]]

Comment on attachment 8532861 [details] [diff] [review]
sw.patch

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

::: dom/base/Console.cpp
@@ +441,5 @@
> +
> +      nsString id;
> +      if (mWorkerPrivate->IsSharedWorker()) {
> +        id = NS_LITERAL_STRING("SharedWorker");
> +      } else if(mWorkerPrivate->IsServiceWorker()) {

if<space>( already fixed. I don't upload a new patch just for this.

Comment 33

[Andrea Marchesini [:baku]]

bug 1107699 is blocked by this bug. khuey can you give me an ETA for the review?

Comment 34

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8532861 [details] [diff] [review]
sw.patch

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

::: dom/base/Console.cpp
@@ +591,5 @@
>  NS_IMPL_CYCLE_COLLECTION_CLASS(Console)
>  
> +// We don't need to traverse/unlink mStorage and mSanbox because they are not
> +// CCed objects and they are only used on main-thread also when this Console
> +// object runs on workers.

on the main thread, even when this Console object is used on workers.

::: dom/base/ConsoleAPIStorage.js
@@ +10,5 @@
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> +// The console API events have to be schedulated when stored using

scheduled

@@ +141,5 @@
>      Services.obs.notifyObservers(aEvent, "console-storage-cache-event", aId);
>    },
>  
>    /**
> +   * Similar to recordEvent, but these events are schedulated and stored any

ibid

Comment 35

[Andrea Marchesini [:baku]]

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

If green this can land.

Comment 36

[Andrea Marchesini [:baku]]

Attachment: sw.patch

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d32672ecbff9

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d32672ecbff9
https://hg.mozilla.org/mozilla-central/rev/e3e24120107b

Comment 39

[aneesiqbalbhatti]

This has been fixed? Wohooo!
Any ideas which version will have it?
and which tab will have the logs when I console.log from a shared worker?

Comment 40

[Boris Zbarsky [:bzbarsky]]

> Any ideas which version will have it?

See the "Target Milestone" field of this bug.  That's the general way to tell, for a fixed bug, which version it's fixed in.

> and which tab will have the logs when I console.log from a shared worker?

I _think_, based on reading the patch, that the answer is "none".  They will be in the global browser console, not the per-tab web consoles.  But I could be wrong, of course.

In any case, this definitely needs to be documented in MDN...

Comment 41

[Andrea Marchesini [:baku]]

> I _think_, based on reading the patch, that the answer is "none".  They will
> be in the global browser console, not the per-tab web consoles.  But I could
> be wrong, of course.

You are right. The devtools team told me that they are going to implement some UI for this new kind of log messages. But this will be implemented in a separated bug.

Comment 42

[Chris Mills [:cmills]]

This bug, and the detail in comment 40, documented at:

https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope.console