Closed
Bug 1429174
Opened 6 years ago
Closed 6 years ago
provide a static method for reporting a console message related to a particular service worker scope
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: bkelly, Assigned: baku)
References
Details
Attachments
(2 files)
15.95 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
5.05 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
In bug 1425965 I am in the process of trying to fix ServiceWorkerManager::ReportToAllClients() to work with multiple content processes. Currently this code uses nsContentUtils::ReportToConsole() based on local window id values. There are a few ways to fix this, but ideally I'd like to make this code use the same path we use for console.log() from a service worker context. When a service worker script calls console.log() we end up placing the service worker's scope in the console message id and the string "ServiceWorker" in the innerID: https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/dom/console/Console.cpp#585 The web console code now checks a window.shouldReportForServiceWorkerScope() method which does this: https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/dom/base/nsGlobalWindowInner.cpp#2391 This matches the guts of ServiceWorkerManager::ReportToClients(). If we can make ReportToClients() call into this same system then we can avoid code duplication. It also makes SWM more portable since we want to move it to the parent process in the future. Ideally it would be nice to have a dom/console static method SWM could call. Something like: Console::ReportForServiceWorkerScope(const nsCString& aScope, const nsString& aMessage, const nsString& aFilename, const nsString& aLine, uint32_t aLineNumber, uint32_t aColumnNumber, uint32_t aFlags); Note, the SWM won't have a jscontext and the reported value won't have any live javascript objects. Its just a flat string, file name, line number, etc. Andrea, what do you think?
Flags: needinfo?(amarchesini)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 1•6 years ago
|
||
> Andrea, what do you think?
I agree with you. I take this bug.
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8941403 -
Flags: review?(bkelly)
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8941404 -
Flags: review?(bkelly)
Reporter | ||
Comment 4•6 years ago
|
||
Comment on attachment 8941403 [details] [diff] [review] part 1 - ConsoleUtils Review of attachment 8941403 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the quick turn around! i think the only thing missing is the ability to pass an arg indicating if its a log/warning/error. ::: dom/console/ConsoleCommon.h @@ +23,5 @@ > + } > + > + ~ClearException() > + { > + JS_ClearPendingException(mCx); I realize you are just moving existing code into a separate file here, but maybe in the future this class could be replaced with: auto cleanup = MakeScopeExit([&] { JS_ClearPendingException(aCx); }); ::: dom/console/ConsoleUtils.cpp @@ +86,5 @@ > + > + event.mInnerID.Construct(); > + event.mInnerID.Value().SetAsString() = NS_LITERAL_STRING("ServiceWorker"); > + > + event.mLevel = NS_LITERAL_STRING("log"); I think we may want the ability to actually pass a flag indicating if its log/warning/error. For example, we use nsIScriptError::errorFlag here: https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/dom/workers/ServiceWorkerEvents.cpp#950 It could be a dom/console enum instead of nsIScriptError, though. Sorry I forgot to mention that.
Attachment #8941403 -
Flags: review?(bkelly) → review+
Reporter | ||
Updated•6 years ago
|
Attachment #8941404 -
Flags: review?(bkelly) → review+
Reporter | ||
Comment 5•6 years ago
|
||
Comment on attachment 8941403 [details] [diff] [review] part 1 - ConsoleUtils Review of attachment 8941403 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/console/ConsoleUtils.h @@ +20,5 @@ > + NS_INLINE_DECL_REFCOUNTING(ConsoleUtils) > + > + // Main-thread only, reports a console message from a ServiceWorker. > + static void > + ReportForServiceWorkerScope(const nsAString& aScope, It would also be a minor convenience if the scope was an nsACString as that is what we use in SWM.
Reporter | ||
Comment 6•6 years ago
|
||
So I did some testing and it seems if this is called in a child process then only windows in the same process will see it. I thought it propagated to all child processes. I also tested current nightly, though, and it seems it has the same problem with nsIConsoleService. So I guess we can live with it until we move SWM to the parent process.
Reporter | ||
Comment 7•6 years ago
|
||
Andrea, am I understanding ConsoleUtils is supposed to work? Does it propagate across processes automatically?
Flags: needinfo?(amarchesini)
Reporter | ||
Comment 8•6 years ago
|
||
Or do I need to do process jumps myself? I guess this will be a problem if we move service worker threads into their own worker-specific process. Then no window will be in the same process. Any console.log() from the service worker thread will be silently ignored and won't show up anywhere.
Reporter | ||
Comment 9•6 years ago
|
||
I guess I could file a separate bug to propagate across process boundaries.
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #9) > I guess I could file a separate bug to propagate across process boundaries. It's not needed. The message is propagate across process boundaries by devtools.
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 11•6 years ago
|
||
The message is sent to the parent process and it's displayed by devtools. Why do you need to propagate the message to any other content processes? I think what you need is that the message is correctly displayed to the web console. Am I wrong?
Flags: needinfo?(bkelly)
Comment 12•6 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8c42fc215b1b Introducing ConsoleUtils for logging messages to console, r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/75a5873327a6 Introducing ConsoleUtils for logging messages to console - tests, r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/6e74b98b5354 Introducing ConsoleUtils for logging messages to console - message level, r=bkelly
Reporter | ||
Comment 13•6 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #11) > The message is sent to the parent process and it's displayed by devtools. > Why do you need to propagate the message to any other content processes? I > think what you need is that the message is correctly displayed to the web > console. Am I wrong? Right. It needs to show in the webconsole, but the way webconsole knows which window should see a service worker message is by calling nsPIDOMWindowInner::ShouldReportForServiceWorkerScope(): https://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindowInner.cpp#2379 It needs to be in the right child process to call that. I could try to keep some of the ServiceWorkerManager::ReportToAllClients() code to send the internal console messages to the right child processes, but that would not help with a SW script directly calling console.log(). It seems to me either devtools needs to broadcast SW console messages to all child processes to query all window toolboxes. What do you think?
Flags: needinfo?(bkelly) → needinfo?(amarchesini)
Comment 14•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8c42fc215b1b https://hg.mozilla.org/mozilla-central/rev/75a5873327a6 https://hg.mozilla.org/mozilla-central/rev/6e74b98b5354
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
Assignee | ||
Updated•5 years ago
|
Flags: needinfo?(amarchesini)
You need to log in
before you can comment on or make changes to this bug.
Description
•