1259603 - Use ConsoleEvents for cached messages and observing messages when webconsole actor is running in a worker

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Brian Grinstead [:bgrins]]

After Bugs 1249709 and 1246091 we should be able to access ConsoleEvents from the webconsole actor running in the worker.  This means we should be able to fetch console API calls that happened inside the worker, like when debugging a Service Worker in a separate toolbox

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: console-worker-message-WIP.patch

really rough work in progress but it gets messages to show up on the frontend!

There's a test in place (test_console_worker.html) but it isn't yet passing due to some mysterious issue where console logs aren't showing up in the mochitest once the thread is attached

Todo:
* Move non-worker listener bits from utils.js into a new file so we can load the shared utils when in a worker instead of needing to copy/paste as this patch does
* Figure out correct way to clean up the handler, right now I'm just doing `setConsoleEventHandler(() => {})` because setConsoleEventHandler(null) throws an error.
* Figure out what's going on with the test

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> Todo:
> * Figure out correct way to clean up the handler, right now I'm just doing
> `setConsoleEventHandler(() => {})` because setConsoleEventHandler(null)
> throws an error.

Andrea, is there a "correct" way to remove the listener to the console event handler within a worker?  Or, is setting it to a no-op good enough?  (see worker-utils.js line 91 in attachment 8735035 [details] [diff] [review])

Comment 3

[Andrea Marchesini [:baku]]

I filed bug 1260414 for this.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: console-worker-message.patch

Rebased

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: Bug 1259603 - Use ConsoleEvents for cached messages and observing messages when webconsole actor is running;

Review commit: https://reviewboard.mozilla.org/r/92396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92396/

Comment 6

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8809914 [details]
Bug 1259603 - Use ConsoleEvents for cached messages and observing messages when webconsole actor is running;

https://reviewboard.mozilla.org/r/92396/#review93432

::: devtools/server/actors/utils/webconsole-worker-utils.js:22
(Diff revision 1)
> -exports.ConsoleAPIListener = function () {};
> +// XXX: Share utils in a single file between worker and non and have custom
> +// files for listeners
> +exports.Utils = {
> +  L10n: function () {},
> +  cloneObject: function(object, recursive, filter) {
> +    if (typeof object != "object") {

Don't we need a null check here?

::: devtools/server/actors/utils/webconsole-worker-utils.js:55
(Diff revision 1)
> +    return CONSOLE_WORKER_IDS[CONSOLE_WORKER_IDS.indexOf(id)] || null;
> +  },
> + };
> +
> +// XXX: Not implemented yet for workers, see Bug 1280736
> +function ConsoleServiceListener(window, listener) {

Then we should probably only define it if isWorker is false?

::: devtools/server/actors/webconsole.js
(Diff revision 1)
>     * @return object
>     *         The response object which holds the startedListeners array.
>     */
>    onStartListeners: function WCA_onStartListeners(aRequest)
>    {
> -    // XXXworkers: Not handling the Console API yet for workers (Bug 1209353).

I don't think Workers support the PageError message type, but I don't see that being disabled here.

::: devtools/shared/webconsole/client.js:609
(Diff revision 1)
>      this.clearNetworkRequests();
>      this._networkRequests = null;
>    },
>  
>    clearNetworkRequests: function () {
> +    if (this._networkRequests) {

Not sure what this line is trying to accomplish. Did we have a race condition here?

Comment 7

[Eddy Bruel [:ejpbruel]]

I did a preliminary review, but apparently this is still a work in progress? I'm also not quite sure what this patch is trying to accomplish, so I could use some background here.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Eddy Bruel [:ejpbruel] from comment #7)
> I did a preliminary review, but apparently this is still a work in progress?
> I'm also not quite sure what this patch is trying to accomplish, so I could
> use some background here.

This is implementing a console API observer for console messages logged from workers.  Right now we don't actually print any console messages in a worker popup toolbox (like one opened from about:debugging).  So this fixes that by getting an implementation in place for that listener - using the new setConsoleEventHandler function rather than a Services observer.  Note that webconsole-worker-utils.js is only loaded in a worker context (from inside the webconsole actor).

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8809914 [details]
Bug 1259603 - Use ConsoleEvents for cached messages and observing messages when webconsole actor is running;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92396/diff/1-2/

Comment 10

[Brian Grinstead [:bgrins]]

I've pushed what I hope is a more straightforward implementation that's ready for review.  Here's the summary:

It used to be that the webconsole actor would conditionally load a number of modules out of webconsole-utils or webconsole-worker-utils depending on if it is a worker or not.  This meant that webconsole-worker-utils had to stub out a lot of unnecessary exports, so what I've done now is:
1) taken the listener specific bits out of webconsole-utils (and named that webconsole-listeners.js)
2) renamed webconsole-worker-utils to webconsole-worker-listeners.
3) this leaves webconsole-utils really as utils that can be loaded in both worker and non worker environments.
4) also renamed an export in webconsole-utils from Utils to WebConsoleUtils since that's how it was used everywhere anyway and it made the import in the webconsole actor easier.

(In reply to Eddy Bruel [:ejpbruel] from comment #6)
> ::: devtools/server/actors/utils/webconsole-worker-utils.js:22
> (Diff revision 1)
> > -exports.ConsoleAPIListener = function () {};
> > +// XXX: Share utils in a single file between worker and non and have custom
> > +// files for listeners
> > +exports.Utils = {
> > +  L10n: function () {},
> > +  cloneObject: function(object, recursive, filter) {
> > +    if (typeof object != "object") {
> 
> Don't we need a null check here?

Removed this (duplicated) code after the reorganization described above.

> ::: devtools/server/actors/utils/webconsole-worker-utils.js:55
> (Diff revision 1)
> > +    return CONSOLE_WORKER_IDS[CONSOLE_WORKER_IDS.indexOf(id)] || null;
> > +  },
> > + };
> > +
> > +// XXX: Not implemented yet for workers, see Bug 1280736
> > +function ConsoleServiceListener(window, listener) {
> 
> Then we should probably only define it if isWorker is false?

I've removed that code altogether after the reorganization described above.

> ::: devtools/server/actors/webconsole.js
> (Diff revision 1)
> >     * @return object
> >     *         The response object which holds the startedListeners array.
> >     */
> >    onStartListeners: function WCA_onStartListeners(aRequest)
> >    {
> > -    // XXXworkers: Not handling the Console API yet for workers (Bug 1209353).
> 
> I don't think Workers support the PageError message type, but I don't see
> that being disabled here.

Any unsupported worker listeners (right now, anything *except* for console api) should be handled by the various `if (isWorker) { break; }` below.

> ::: devtools/shared/webconsole/client.js:609
> (Diff revision 1)
> >      this.clearNetworkRequests();
> >      this._networkRequests = null;
> >    },
> >  
> >    clearNetworkRequests: function () {
> > +    if (this._networkRequests) {
> 
> Not sure what this line is trying to accomplish. Did we have a race
> condition here?

I saw this error at some point during development: 'TypeError: this._networkRequests is null' but I haven't been able to reproduce, so I've removed that code.  We can debug/fix that in another bug if it pops up again

Comment 11

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8809914 [details]
Bug 1259603 - Use ConsoleEvents for cached messages and observing messages when webconsole actor is running;

https://reviewboard.mozilla.org/r/92396/#review96510

Can't really find anything wrong with this patch. I just have a few minor comments.

::: devtools/server/actors/webconsole.js:29
(Diff revision 2)
>  loader.lazyRequireGetter(this, "JSPropertyProvider", "devtools/shared/webconsole/js-property-provider", true);
>  loader.lazyRequireGetter(this, "Parser", "resource://devtools/shared/Parser.jsm", true);
>  loader.lazyRequireGetter(this, "NetUtil", "resource://gre/modules/NetUtil.jsm", true);
> +loader.lazyRequireGetter(this, "addWebConsoleCommands", "devtools/server/actors/utils/webconsole-utils", true);
> +loader.lazyRequireGetter(this, "CONSOLE_WORKER_IDS", "devtools/server/actors/utils/webconsole-utils", true);
> +loader.lazyRequireGetter(this, "WebConsoleUtils", "devtools/server/actors/utils/webconsole-utils", true);

Is there ever a case where we load webconsole.js, but then don't subsequently need WebConsoleUtils? In other words, why is it useful to use a lazy require here?

::: devtools/server/actors/webconsole.js:319
(Diff revision 2)
>    },
>  
>    hasNativeConsoleAPI: function WCA_hasNativeConsoleAPI(aWindow) {
> +    if (isWorker) {
> +      // Can't use XPCNativeWrapper as a way to check for console API in workers
> +      return true;

However, it is always safe to return true here, because... ?

::: devtools/server/actors/webconsole.js:577
(Diff revision 2)
>  
>      while (aRequest.listeners.length > 0) {
>        let listener = aRequest.listeners.shift();
>        switch (listener) {
>          case "PageError":
> +          // Workers don't support this message type

I might be better to say 'not yet'. As it stands, this suggests that its not possible to do this in workers at all. That's not really the case though, we just haven't implemented the parts we need for it yet.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Eddy Bruel [:ejpbruel] from comment #11)
> Comment on attachment 8809914 [details]
> Bug 1259603 - Use ConsoleEvents for cached messages and observing messages
> when webconsole actor is running;
> 
> https://reviewboard.mozilla.org/r/92396/#review96510
> 
> Can't really find anything wrong with this patch. I just have a few minor
> comments.
> 
> ::: devtools/server/actors/webconsole.js:29
> (Diff revision 2)
> >  loader.lazyRequireGetter(this, "JSPropertyProvider", "devtools/shared/webconsole/js-property-provider", true);
> >  loader.lazyRequireGetter(this, "Parser", "resource://devtools/shared/Parser.jsm", true);
> >  loader.lazyRequireGetter(this, "NetUtil", "resource://gre/modules/NetUtil.jsm", true);
> > +loader.lazyRequireGetter(this, "addWebConsoleCommands", "devtools/server/actors/utils/webconsole-utils", true);
> > +loader.lazyRequireGetter(this, "CONSOLE_WORKER_IDS", "devtools/server/actors/utils/webconsole-utils", true);
> > +loader.lazyRequireGetter(this, "WebConsoleUtils", "devtools/server/actors/utils/webconsole-utils", true);
> 
> Is there ever a case where we load webconsole.js, but then don't
> subsequently need WebConsoleUtils? In other words, why is it useful to use a
> lazy require here?

Just added this to follow the pattern of most of the rest of the imports in this file.  It's not used immediately - basically until prepareConsoleMessageForRemote is called (i.e. a message was received).  We should revisit all of the requires here (both lazy/eager) and make sure each one is appropriate in another bug.

> ::: devtools/server/actors/webconsole.js:319
> (Diff revision 2)
> >    },
> >  
> >    hasNativeConsoleAPI: function WCA_hasNativeConsoleAPI(aWindow) {
> > +    if (isWorker) {
> > +      // Can't use XPCNativeWrapper as a way to check for console API in workers
> > +      return true;
> 
> However, it is always safe to return true here, because... ?

The failure case here is that we don't show a warning about the console API being replaced when it was.  So not ideal, but not breaking anything.  If you have an idea how to determine if the console object in a worker has been replaced by the user I could update that.

> ::: devtools/server/actors/webconsole.js:577
> (Diff revision 2)
> >  
> >      while (aRequest.listeners.length > 0) {
> >        let listener = aRequest.listeners.shift();
> >        switch (listener) {
> >          case "PageError":
> > +          // Workers don't support this message type
> 
> I might be better to say 'not yet'. As it stands, this suggests that its not
> possible to do this in workers at all. That's not really the case though, we
> just haven't implemented the parts we need for it yet.

Done

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8809914 [details]
Bug 1259603 - Use ConsoleEvents for cached messages and observing messages when webconsole actor is running;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92396/diff/2-3/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c94c769aaef
Use ConsoleEvents for cached messages and observing messages when webconsole actor is running;r=ejpbruel

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6c94c769aaef