Closed Bug 1058644 Opened 10 years ago Closed 9 years ago

Console API doesn't work in SharedWorkers or ServiceWorkers

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: baku, Assigned: baku)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [dev-doc see comment 40])

Attachments

(1 file, 8 obsolete files)

The reason why is because a SharedWorker doesn't have a window parent.
This has to be fixed somehow.
How exactly?  Which window's console should the messages go to?

What does Chrome do here, if anything?
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.
Summary: Console API doesn't work in SharedWorkers → Console API doesn't work in SharedWorkers or ServiceWorkers
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.
(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.
> 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.
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?
Flags: needinfo?(past)
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.
Flags: needinfo?(past)
> 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?
Flags: needinfo?(past)
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(past)
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?
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.
Flags: needinfo?(ehsan)
msucan, I forgot to NI for comment 10.
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.
Attached patch Console API patch (obsolete) — Splinter Review
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.
Attachment #8481379 - Flags: review?(bzbarsky)
It will almost certainly be mid-next-week before I get to this.  :(
Attached patch patch (obsolete) — Splinter Review
I just released I completely forgot to implement something for Profile/ProfileEnd methods. This patch tests that as well.
Attachment #8481379 - Attachment is obsolete: true
Attachment #8481379 - Flags: review?(bzbarsky)
Attachment #8481511 - Flags: review?(bzbarsky)
Attached patch sw.patch (obsolete) — Splinter Review
a better mochitest.
Attachment #8481511 - Attachment is obsolete: true
Attachment #8481511 - Flags: review?(bzbarsky)
Attachment #8482963 - Flags: review?(bzbarsky)
Attached patch sw.patch (obsolete) — Splinter Review
Attachment #8482963 - Attachment is obsolete: true
Attachment #8482963 - Flags: review?(bzbarsky)
Attachment #8483459 - Flags: review?(bzbarsky)
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
Attachment #8483459 - Flags: review?(bzbarsky) → review-
> 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.
Attached patch sw.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=858c58f7f2db
Attachment #8483459 - Attachment is obsolete: true
Attachment #8485948 - Flags: review?(khuey)
Comment on attachment 8485948 [details] [diff] [review]
sw.patch

Some issue appears on tbpl.
Attachment #8485948 - Flags: review?(khuey)
Attached patch sw.patch (obsolete) — Splinter Review
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
Attachment #8485948 - Attachment is obsolete: true
Attachment #8509435 - Flags: review?(khuey)
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.
Blocks: 1096858
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
Attached patch sw.patch (obsolete) — Splinter Review
hazard rooting fixed
Attachment #8509435 - Attachment is obsolete: true
Attachment #8509435 - Flags: review?(khuey)
Attachment #8521453 - Flags: review?(khuey)
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.
Thanks for pointing to this. Yes, you are right, I'll add a mochitest.
Attached patch sw.patch (obsolete) — Splinter Review
Attachment #8521453 - Attachment is obsolete: true
Attachment #8521453 - Flags: review?(khuey)
Attachment #8532861 - Flags: review?(khuey)
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.
bug 1107699 is blocked by this bug. khuey can you give me an ETA for the review?
Flags: needinfo?(khuey)
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
Attachment #8532861 - Flags: review?(khuey) → review+
Flags: needinfo?(khuey)
Attached patch sw.patchSplinter Review
Attachment #8532861 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d32672ecbff9
Assignee: nobody → amarchesini
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
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?
> 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...
Whiteboard: [dev-doc see comment 40]
> 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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.