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)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bkelly, Assigned: baku)

References

Details

Attachments

(2 files)

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: nobody → amarchesini
Flags: needinfo?(amarchesini)
> Andrea, what do you think?

I agree with you. I take this bug.
Attachment #8941403 - Flags: review?(bkelly)
Attached patch part 2 - testsSplinter Review
Attachment #8941404 - Flags: review?(bkelly)
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+
Attachment #8941404 - Flags: review?(bkelly) → review+
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.
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.
Andrea, am I understanding ConsoleUtils is supposed to work?  Does it propagate across processes automatically?
Flags: needinfo?(amarchesini)
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.
I guess I could file a separate bug to propagate across process boundaries.
(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)
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)
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
(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)
Depends on: 1432391
Component: DOM → DOM: Core & HTML
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.