1425965 - make ServiceWorkerManager::ReportToAllClients() use dom/console

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently ServiceWorkerManager::ReportToAllClients() does a bunch of same-process operations based on nsIDocument, nsIChannel, etc.  Instead we should either use console service or ClientHandle to perform the reporting.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Andrea, do you understand the difference between the console code here:

https://searchfox.org/mozilla-central/source/dom/console/Console.cpp

And the console service here:

https://searchfox.org/mozilla-central/source/xpcom/base/nsConsoleService.cpp

Does one call the other?

Comment 2

[Andrea Marchesini [:baku]]

> https://searchfox.org/mozilla-central/source/dom/console/Console.cpp

This is the Console API implementation. It is exposed everywhere (window, worker, and soon system - bug 1425463).

> And the console service here: 
> https://searchfox.org/mozilla-central/source/xpcom/base/nsConsoleService.cpp

This is something that should go away. At some point. It implements nsIConsoleService interface and allows the 'dispatching' of console messages.

> Does one call the other?

none. How it works is this:

Console API passes the message (as ConsoleEvent) to ConsoleAPIStorage and this storage is used by browser console and devtools web console to display the message.

For workers, the message is stored by the Console object directly and it's exposed to the Worker devtool actor if we are in a debugging session. I had a meeting with bgrins during the AllHands in order to use actors also for main-thread when devtools web console is opened. But this is inrelevant here.

Console APIStorage is also used by Console.jsm. But I'm planning to get rid of Console.jsm soon: bug 1425574 and bug 1425463.

Devtools actors take care of sending the ConsoleEvent to the parent process if needed.

nsConsoleService is a different story: nsIConsoleService can be used to store messages (nsIConsoleMessage). If there are listeners, these messages are sent to them. ContentChild has a listener that is used to send nsIConsoleMessage objects to the parent process.

Devtools also has a listener for nsIConsoleService and, because of this, it is able to show nsIConsoleMessages in the browser console panel.

I'm planning to get rid of nsIConsoleService in 2018. Soon, Console API will be exposed to JSM (bug 1425463). After that, there are not reasons to have nsIConsoleService anymore and we can relay on devtools actors everywhere.

If you want, I can work on this bug.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Thats ok, I don't mind working on this bug as I'm already into it.  I think my main question is:

If I want to log a message to the console for a target window or worker in a child process, what API do I call?

Currently the code calls the nsContentUtils report methods which use console service.  While I refactor this I can make them call the right Console API, but its not clear what I would call.

In general these reports will have a file name and maybe a line number, but no exception stack trace.  This is for reporting errors out of SWM like "returned a non-Response to FetchEvent.respondWith()", etc.

My plan is to have the SWM in the parent use ClientHandle to send the message to the ClientSource in the child.  The ClientSource needs to call *something* to report.  Any hints what I should call there?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Basically, what is the dom/console equivalent of calling this:

https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/dom/base/nsContentUtils.h#1080

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Alternatively, maybe I could just use dom Console API in the parent process with the same ID values `console.log()` uses from the service worker global.  It sets the string "ServiceWorker" and the scope string as the two identifiers.

Its just unclear to me if this is propagated to all child processes to check for matches or only to the current process.

Comment 6

[Andrea Marchesini [:baku]]

> If I want to log a message to the console for a target window or worker in a
> child process, what API do I call?

If you have a DedicatedWorker, just use nsContentUtils::ReportToConsoleByWindowID() passing the inner Window ID.
If your code runs in a ServiceWorker, there is not direct function to call. If you call nsContentUtils::ReportToConsoleByWindowID() or any other method, without passing a valid innerID, the message will appear only on the browser console, but not in the web console.

Unfortunately, dom/console is not nice to be used on C++ code yet.


> Currently the code calls the nsContentUtils report methods which use console
> service.  While I refactor this I can make them call the right Console API,
> but its not clear what I would call.

Oh. This is nice. Calling 'Log()' should be totally fine. Would be nice if we can add an extra param to know if we should use log, warn or error.

About the stack trace, there is nothing ready, but, it should be relatively easy to add a method to Console that takes a ConsoleStackEntry object as param: currently we convert nsIStackFrame to ConsoleStackEntry when Console.something() is executed in workers because nsIStackFrame is not thread-safe. We could introduce a method that receives ConsoleStackEntry as param.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

While I would like to use dom/console for this, I think trying to figure out how to use it without a JSContext might be too hard.  I'm going to add an IPC message to ClientHandle for now and we can replace it once dom/console better supports this use case.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a ClientHandle::ReportToConsole() method. r=asuth

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make ServiceWorkerManager::ReportToAllClients() use mControlledClients and ClientHandle::ReportToConsole(). r=asuth

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Rather than go down the road of remote console reporting through ClientHandle I've decided to ask Andrea for help using console service in bug 1429174.  That will end up with less code and complexity over all.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make ServiceWorkerManager::ReportToAllClients use ConsoleUtils::ReportForServiceWorkerScope. r=asuth

This is a WIP that uses the API being added in bug 1429174.  It has some problems currently in that it doesn't propagate across processes at all.  That is the same as current nsIConsoleService, though.  In the long run we need dom/console to do that properly.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add nsIConsoleReportCollector::FlushReportsToConsoleForServiceWorkerScope(). r=baku

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make ServiceWorkerManager::FlushReportsToAllClients() use FlushReportsToConsoleForServiceWorkerScope(). r=asuth

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Remove mControlledDocument, mRegisteringDocuments, and mNavigationIntercepts from ServiceWorkerManager. r=asuth

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Currently these changes make things like test_error_reporting fail because it expects the console messages to be produced by SimpleTest.monitorConsole().  For example, this test uses this helper code:

https://searchfox.org/mozilla-central/source/dom/workers/test/serviceworkers/error_reporting_helpers.js

Andrea, is there any way we could make dom/console stuff go through SimpleTest.monitorConsole()?  Or can you suggest an alternative testing approach.

Using SimpleTest.monitorConsole() would be ideal because I believe some reports are still coming through nsIConsoleService while others will start using dom/console with these patches.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

More accurately this bug is making ServiceWorkerManager use the dom/console mechanism instead of nsContentUtils::ReportToConsole() with window IDs.  Until bug 1429805 is fixed this won't actually make us fully support multi-process.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make ServiceWorkerManager::ReportToAllClients use ConsoleUtils::ReportForServiceWorkerScope. r=asuth

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add nsIConsoleReportCollector::FlushReportsToConsoleForServiceWorkerScope(). r=baku

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

I think I see how to at least write the service worker test with dom/console stuff.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make SimpleTest.monitorConsole() and SpecialPowers.registerConsoleListener() observe dom/console events. r=baku

I think I figured out how to make SimpleTest.monitorConsole() look for dom/console stuff.  Service worker tests are green with it, but lets see if other tests fail in try.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Change test_fetch_integrity.html not to expect exact window ID values in console messages. r=asuth

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56f247de7ae6607d1e8c272a552ac7bfd299e744

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Fix dom/push/test/test_error_reporting.html to expect new console reporting mechanism. r=asuth

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

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8941881 [details] [diff] [review]
P1 Make ServiceWorkerManager::ReportToAllClients use ConsoleUtils::ReportForServiceWorkerScope. r=asuth

Andrew, this replaces the guts of ServiceWorkerManager::ReportToAllClients() with the new ConsoleUtils method Andrea added.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8941882 [details] [diff] [review]
P2 Add nsIConsoleReportCollector::FlushReportsToConsoleForServiceWorkerScope(). r=baku

Andrea, I need to be able to flush a ConsoleReportCollector to a service worker scope.  This adds a new FlushReportsToConsoleForServiceWorkerScope() method.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8941607 [details] [diff] [review]
P3 Make ServiceWorkerManager::FlushReportsToAllClients() use FlushReportsToConsoleForServiceWorkerScope(). r=asuth

Andrew, this patch makes FlushReportsToAllClients() use the new ConsoleReportCollector::FlushReportsToConsoleForServiceWorkerScope() method I added in P2.  I'll do a follow-up patch in this bug to remove this method altogether.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8941607 [details] [diff] [review]
P3 Make ServiceWorkerManager::FlushReportsToAllClients() use FlushReportsToConsoleForServiceWorkerScope(). r=asuth

Actually, there is only one caller.  Lets just do it in this patch.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make ServiceWorkerManager::FlushReportsToAllClients() use FlushReportsToConsoleForServiceWorkerScope(). r=asuth

Ok.  This patch removes the ServiceWorkerManager::FlushReportsToAllClients() and just makes dom/fetch call the ConsoleReportCollector method directly.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8941609 [details] [diff] [review]
P4 Remove mControlledDocument, mRegisteringDocuments, and mNavigationIntercepts from ServiceWorkerManager. r=asuth

This patch removes a ton of now stale code from ServiceWorkerManager.  Most importantly it removes:

* mControlledDocuments
* mRegisteringDocuments
* mNavigationInterceptions

These are all child process things we need to divorce from SWM and the main goal of this bug.

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8941931 [details] [diff] [review]
P5 Make SimpleTest.monitorConsole() and SpecialPowers.registerConsoleListener() observe dom/console events. r=baku

Andrea, this patch make SpecialPowers.registerConsoleListener(), and by extension SimpleTest.monitorConsole, listen to dom/console events in addition to nsIConsoleService.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8941932 [details] [diff] [review]
P6 Change test_fetch_integrity.html not to expect exact window ID values in console messages. r=asuth

The new console reporting system from SWM now uses the dom/console approach where a "ServiceWorker" token and the scope string are placed in the window ID fields.  Currently test_fetch_integrity was trying to verify that the window ID matched the real window values.  That obviously won't work any more.

This patch just removes the window ID comparisons.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8941975 [details] [diff] [review]
P7 Fix dom/push/test/test_error_reporting.html to expect new console reporting mechanism. r=asuth

Similar to integrity test, the dom/push/test/test_error_reporting.html file is trying to compare window IDs.  I think this was one added to avoid looking at a stray message that might appear, so here I do compare against the expected window ID scope value.

The test was also check "isScriptError".  I made SpecialPowers mark dom/console messages "isConsoleEvent" instead.  I loosened the check here to accept that.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbc20dc83879
P1 Make ServiceWorkerManager::ReportToAllClients use ConsoleUtils::ReportForServiceWorkerScope. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/57b875846062
P2 Add nsIConsoleReportCollector::FlushReportsToConsoleForServiceWorkerScope(). r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/79280bcea445
P3 Make ServiceWorkerManager::FlushReportsToAllClients() use FlushReportsToConsoleForServiceWorkerScope(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/4027039552fe
P4 Remove mControlledDocument, mRegisteringDocuments, and mNavigationIntercepts from ServiceWorkerManager. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/241b9e419ca4
P5 Make SimpleTest.monitorConsole() and SpecialPowers.registerConsoleListener() observe dom/console events. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/9972008ac105
P6 Change test_fetch_integrity.html not to expect exact window ID values in console messages. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d16fe1a6931e
P7 Fix dom/push/test/test_error_reporting.html to expect new console reporting mechanism. r=asuth

Comment 34

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/bbc20dc83879
https://hg.mozilla.org/mozilla-central/rev/57b875846062
https://hg.mozilla.org/mozilla-central/rev/79280bcea445
https://hg.mozilla.org/mozilla-central/rev/4027039552fe
https://hg.mozilla.org/mozilla-central/rev/241b9e419ca4
https://hg.mozilla.org/mozilla-central/rev/9972008ac105
https://hg.mozilla.org/mozilla-central/rev/d16fe1a6931e