make ServiceWorkerManager::ReportToAllClients() use dom/console

RESOLVED FIXED in Firefox 59

Status

()

P2
normal
RESOLVED FIXED
11 months ago
9 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(7 attachments, 5 obsolete attachments)

17.61 KB, patch
asuth
: review+
Details | Diff | Splinter Review
7.30 KB, patch
asuth
: review+
Details | Diff | Splinter Review
7.71 KB, patch
baku
: review+
Details | Diff | Splinter Review
4.17 KB, patch
baku
: review+
Details | Diff | Splinter Review
3.66 KB, patch
asuth
: review+
Details | Diff | Splinter Review
1.48 KB, patch
asuth
: review+
Details | Diff | Splinter Review
7.01 KB, patch
asuth
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 months ago
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.
(Assignee)

Comment 1

11 months ago
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?
Flags: needinfo?(amarchesini)
> 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.
Flags: needinfo?(amarchesini)

Updated

11 months ago
Flags: needinfo?(bkelly)
(Assignee)

Comment 3

11 months ago
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?
Flags: needinfo?(bkelly) → needinfo?(amarchesini)
(Assignee)

Comment 5

11 months ago
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.
> 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.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 7

11 months ago
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.
(Assignee)

Comment 8

11 months ago
Created attachment 8938548 [details] [diff] [review]
P1 Add a ClientHandle::ReportToConsole() method. r=asuth
(Assignee)

Comment 9

11 months ago
Created attachment 8938549 [details] [diff] [review]
P2 Make ServiceWorkerManager::ReportToAllClients() use mControlledClients and ClientHandle::ReportToConsole(). r=asuth
(Assignee)

Updated

10 months ago
Depends on: 1429174
(Assignee)

Comment 10

10 months ago
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.
(Assignee)

Comment 11

10 months ago
Created attachment 8941580 [details] [diff] [review]
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.
Attachment #8938548 - Attachment is obsolete: true
Attachment #8938549 - Attachment is obsolete: true
(Assignee)

Comment 12

10 months ago
Created attachment 8941605 [details] [diff] [review]
P2 Add nsIConsoleReportCollector::FlushReportsToConsoleForServiceWorkerScope(). r=baku
(Assignee)

Comment 13

10 months ago
Created attachment 8941607 [details] [diff] [review]
P3 Make ServiceWorkerManager::FlushReportsToAllClients() use FlushReportsToConsoleForServiceWorkerScope(). r=asuth
(Assignee)

Comment 14

10 months ago
Created attachment 8941609 [details] [diff] [review]
P4 Remove mControlledDocument, mRegisteringDocuments, and mNavigationIntercepts from ServiceWorkerManager. r=asuth
(Assignee)

Comment 15

10 months ago
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.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 16

10 months ago
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.
See Also: → bug 1429805
Summary: make ServiceWorkerManager::ReportToAllClients() support cross-process clients → make ServiceWorkerManager::ReportToAllClients() use dom/console
(Assignee)

Comment 17

10 months ago
Created attachment 8941881 [details] [diff] [review]
P1 Make ServiceWorkerManager::ReportToAllClients use ConsoleUtils::ReportForServiceWorkerScope. r=asuth
Attachment #8941580 - Attachment is obsolete: true
(Assignee)

Comment 18

10 months ago
Created attachment 8941882 [details] [diff] [review]
P2 Add nsIConsoleReportCollector::FlushReportsToConsoleForServiceWorkerScope(). r=baku
Attachment #8941605 - Attachment is obsolete: true
(Assignee)

Comment 19

10 months ago
I think I see how to at least write the service worker test with dom/console stuff.
(Assignee)

Comment 20

10 months ago
Created attachment 8941931 [details] [diff] [review]
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.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 21

10 months ago
Created attachment 8941932 [details] [diff] [review]
P6 Change test_fetch_integrity.html not to expect exact window ID values in console messages. r=asuth
(Assignee)

Comment 23

10 months ago
Created attachment 8941975 [details] [diff] [review]
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
(Assignee)

Comment 24

10 months ago
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.
Attachment #8941881 - Flags: review?(bugmail)
(Assignee)

Comment 25

10 months ago
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.
Attachment #8941882 - Flags: review?(amarchesini)
(Assignee)

Comment 26

10 months ago
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.
Attachment #8941607 - Flags: review?(bugmail)
(Assignee)

Comment 27

10 months ago
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.
Attachment #8941607 - Flags: review?(bugmail)
(Assignee)

Comment 28

10 months ago
Created attachment 8941982 [details] [diff] [review]
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.
Attachment #8941607 - Attachment is obsolete: true
Attachment #8941982 - Flags: review?(bugmail)
(Assignee)

Updated

10 months ago
Attachment #8941982 - Attachment description: 3 Make ServiceWorkerManager::FlushReportsToAllClients() use FlushReportsToConsoleForServiceWorkerScope(). r=asuth → P3 Make ServiceWorkerManager::FlushReportsToAllClients() use FlushReportsToConsoleForServiceWorkerScope(). r=asuth
(Assignee)

Comment 29

10 months ago
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.
Attachment #8941609 - Flags: review?(bugmail)
(Assignee)

Comment 30

10 months ago
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.
Attachment #8941931 - Flags: review?(amarchesini)
(Assignee)

Comment 31

10 months ago
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.
Attachment #8941932 - Flags: review?(bugmail)
(Assignee)

Comment 32

10 months ago
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.
Attachment #8941975 - Flags: review?(bugmail)
Attachment #8941881 - Flags: review?(bugmail) → review+
Attachment #8941982 - Flags: review?(bugmail) → review+
Attachment #8941609 - Flags: review?(bugmail) → review+
Attachment #8941932 - Flags: review?(bugmail) → review+
Attachment #8941975 - Flags: review?(bugmail) → review+

Updated

10 months ago
Priority: -- → P2

Updated

10 months ago
Attachment #8941882 - Flags: review?(amarchesini) → review+

Updated

10 months ago
Attachment #8941931 - Flags: review?(amarchesini) → review+

Comment 33

10 months ago
Pushed by bkelly@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.