1125205 - Display console API messages from shared or service workers to the web console

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

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

Bug 1058644 enabled console API calls from shared and service workers. As these messages come from workers, they don't have any window ID associated with them. Web developers however, would expect to see them in the web console, as that is where they see all messages from their app.

These worker messages come with the "SharedWorker" or "ServiceWorker" inner ID (like the "jsm" ID used by Console.jsm) and the filename of the worker as the outer ID. The idea is that the web console would show these messages iff the worker filename matches one of the scripts loaded in the page.

That would require the web console to have some code that asks the debugger for the list of scripts, or obtain it directly from the thread actor. I think the first option is preferable. The console currently directly adds the content window as a debuggee in order to perform its own limited actions that require the Debugger API. This could be another instance of that pattern. The only caveat is that dbg.findScripts() is often slow and perhaps we wouldn't be able to do it for each worker message that arrives. Maybe findSources() (still unimplemented) would help here, or maybe we need to add a new query term to findScripts() to filter out non-worker scripts.

Comment 1

[Andrea Marchesini [:baku]]

The worker messages has 'SharedWorker', 'ServiceWorker' or 'Worker' in the innerID.
It can happen that the type is 'Worker' when a SharedWorker or a ServiceWorker (or a ChromeWorker) create a sub-Worker.

Btw would be nice to rename innerID to something more descriptive.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: c.patch

It doesn't seem super-clean and I'm open to a better approach.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: c.patch

Comment 4

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

Comment on attachment 8596440 [details] [diff] [review]
c.patch

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

This looks good, but I have a few comments. Do you have a demo page that I could use to play with this? Nothing useful came up in my (very) brief search.

Using a new dropdown menu doesn't seem right. The checkboxes control the console logging API, which has its own Logging menu. You could just add the new options there, using a separator to distinguish them from the log level options above.

We also need a test for this, perhaps something like browser_webconsole_console_logging_api.js?

::: browser/app/profile/firefox.js
@@ +1562,5 @@
>  pref("devtools.browserconsole.filter.secerror", true);
>  pref("devtools.browserconsole.filter.secwarn", true);
> +pref("devtools.browserconsole.filter.serviceworkers", false);
> +pref("devtools.browserconsole.filter.sharedworkers", false);
> +pref("devtools.browserconsole.filter.windowlessworkers", false);

I think we want the worker logging enabled in the browser console by default, just like everything else. It's the place where people expect to see everything that went wrong with the browser.

::: browser/devtools/webconsole/webconsole.js
@@ +138,5 @@
>    count: SEVERITY_LOG
>  };
>  
> +// List of the ConsoleEvent.innerID for workers.
> +const WORKERTYPES_NAMES = [ 'SharedWorker', 'ServiceWorker', 'Worker' ];

This should be imported from utils.js.

@@ +141,5 @@
> +// List of the ConsoleEvent.innerID for workers.
> +const WORKERTYPES_NAMES = [ 'SharedWorker', 'ServiceWorker', 'Worker' ];
> +
> +// This array contains the prefKey for the workers and it must be in sync with
> +// the previous one.

How about we clarify that it's the order that is important, with perhaps something like "...and it must keep them in the same order as the previous one"?

::: browser/locales/en-US/chrome/browser/devtools/webConsole.dtd
@@ +91,5 @@
> +  -  should not be translated. -->
> +<!ENTITY btnConsoleServiceWorkers "Service Workers">
> +<!-- LOCALIZATION NODE (btnConsoleWindowlessWorkers) the term "Workers"
> +  -  should not be translated. -->
> +<!ENTITY btnConsoleWindowlessWorkers "Workers from addons or from JSM scripts">

This seems a bit wordy. How about "Add-on or Chrome Workers"?

::: toolkit/devtools/server/actors/webconsole.js
@@ +45,5 @@
>      enumerable: true
>    });
>  }
>  
> +const WORKERS_IDS = [ 'SharedWorker', 'ServiceWorker', 'Worker' ];

These should be referenced from utils.js, provided you export them and import them in the block right above.

@@ +1435,5 @@
>    {
>      let result = WebConsoleUtils.cloneObject(aMessage);
> +
> +    result.workerType = WORKERS_IDS.indexOf(result.innerID) == -1
> +                          ? 'none' : result.innerID;

In general the Remote Debugging Protocol strives to be easy to inspect by humans, so we tend to use self-explanatory strings instead of number IDs where possible. Do you think that you could just use the workerID name here and then look it up in the client?

::: toolkit/devtools/webconsole/utils.js
@@ +1422,5 @@
>  
>    /**
> +   * List of the worker IDs for the ConsoleEvent
> +   */
> +  workersIDs: [ 'SharedWorker', 'ServiceWorker', 'Worker' ],

Nit: I think workerIDs is better.

@@ +1499,5 @@
>        messages = messages.filter((m) => m.consoleID == this.consoleID);
>      }
>  
> +    messages = messages.sort(function(a, b) {
> +      return a.timeStamp - b.timeStamp;

I assume we need to sort the messages because we are multiplexing events from different threads? It would be good to add a comment in any case.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: c.patch

Comment 6

[Andrea Marchesini [:baku]]

> > +    result.workerType = WORKERS_IDS.indexOf(result.innerID) == -1
> > +                          ? 'none' : result.innerID;
> 
> In general the Remote Debugging Protocol strives to be easy to inspect by
> humans, so we tend to use self-explanatory strings instead of number IDs
> where possible. Do you think that you could just use the workerID name here
> and then look it up in the client?

Oh yes, it is. We use the IDs of workers but they are strings.

> > +    messages = messages.sort(function(a, b) {
> > +      return a.timeStamp - b.timeStamp;
> 
> I assume we need to sort the messages because we are multiplexing events
> from different threads? It would be good to add a comment in any case.

No, the reason why I'm sorting them is that we get, first all the events for the window, then the events for the SharedWorkers, ServiceWorkers and ChromeWorkers. But they are not sorted by timeStamp. They are just concatenated. Better to put them in the correct order before showing them.

Comment 7

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

Comment on attachment 8596612 [details] [diff] [review]
c.patch

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

Minor nits only, this looks good, thanks!

::: browser/devtools/webconsole/test/browser_webconsole_console_logging_workers_api.js
@@ +1,4 @@
> +/* vim:set ts=2 sw=2 sts=2 et: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * 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/. */

Nit: we generally use the Public Domain (CC0) license for test files.

::: browser/devtools/webconsole/test/test-console-workers.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html dir="ltr" xml:lang="en-US" lang="en-US"><head>
> +    <meta charset="utf-8">
> +    <title>Console test</title>
> +  </head>

Nit: forgot to include the CC0 license comment in this file.

::: browser/devtools/webconsole/webconsole.js
@@ +138,5 @@
>    count: SEVERITY_LOG
>  };
>  
> +// List of the ConsoleEvent.innerID for workers.
> +const WORKERTYPES_NAMES = require("devtools/toolkit/webconsole/utils").CONSOLE_WORKER_IDS;

There is another require(".../utils") in the beginning of the file and you could combine them using destructuring assignment like this:

const { WebConsoleUtils, CONSOLE_WORKER_IDS } = require(".../utils");

::: toolkit/devtools/webconsole/utils.js
@@ +1495,5 @@
>      if (this.consoleID) {
>        messages = messages.filter((m) => m.consoleID == this.consoleID);
>      }
>  
> +    messages = messages.sort(function(a, b) {

You didn't add a brief comment about the reason for sorting.

Comment 8

[Pulsebot]

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

Comment 9

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71881daba146

Comment 10

[Andrea Marchesini [:baku]]

Talking with :past, he suggested to mark this bug for relnote, but maybe this is part of a bigger picture about ServiceWorkers. Ehsan?

Comment 11

[(no longer active)]

(In reply to Andrea Marchesini (:baku) from comment #10)
> Talking with :past, he suggested to mark this bug for relnote, but maybe
> this is part of a bigger picture about ServiceWorkers. Ehsan?

We'll most likely look at shipping service workers in 41, and also I think this would be a nice thing to relnote on its own since it affects also shared workers.

Comment 12

[Andrea Marchesini [:baku]]

Release Note Request (optional, but appreciated)
[Why is this notable]: With this patch we display any use of the console API from ServiceWorkers, SharedWorkers and Add-on/JSM Workers.

Comment 13

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8e7257aeee45 for devtools bustage (that was present in that try run):

https://treeherder.mozilla.org/logviewer.html#?job_id=9280824&repo=mozilla-inbound

Comment 14

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=296725738b06

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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c8f9adecb1bc

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Release Note Request (optional, but appreciated)
[Why is this notable]:  See comment 12
[Suggested wording]: Console API messages from SharedWorker and ServiceWorker are now displayed in web console
[Links (documentation, blog post, etc)]: