Closed Bug 1416105 Opened 2 years ago Closed 2 years ago

cannot open browser console in nightly (58) from web-ext run

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox57 wontfix, firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: dietrich, Assigned: rpl)

References

Details

Attachments

(1 file)

58.0a1 (2017-11-09) (64-bit) Mac OS X

STR:

1. > web-ext run -f /Applications/FirefoxNightly.app/Contents/MacOS/firefox
2. Firefox release opens
3. Open browser console

Expected: browser console opens, from menu or keyboard shortcut

Actual: nothing happens. cannot open browser console at all.

Using --bc works when invoking web-ext though.
Guessing this is more a developer tools issue. Are there any logs or information giving an idea what the problem might be?
Component: WebExtensions: Developer Tools → Developer Tools
Product: Toolkit → Firefox
Nothing in the console.

Only happens when Firefox is opened via web-ext, and not passing --bc option.
Andy, who's the right person to ask about web-ext?

The browser console is crucial to extension dev. We probably don't want extension devs to just have a broken experience here.:
Flags: needinfo?(amckay)
I'm going to take a look at this, in the meantime I'm re-assigning the needinfo.
Flags: needinfo?(amckay) → needinfo?(lgreco)
I took a brief look at this issue, the starting point to investigate the issue is the
following error logged when we click the "Browser Console" menu item on a 
Firefox instance started from "web-ext run":

  JavaScript error: resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js, line 1230: 
  TypeError: this.createRootActor is not a function

Follows an analysis related to the reasons behind the above exception:

- In devtools-startup.js, when we handle the --start-debugger-server CLI option
  (https://searchfox.org/mozilla-central/rev/02bc7e2b1bcbab2d7b604ca713b42bcefd47ad2d/devtools/shim/devtools-startup.js#693,720-722,724-725)
  we create a separate and isolated module loader, invisible to the debugger
  so that we can use it to the debug the DebuggerServer instance loaded in the
  regular devtools module loader (e.g. when the user interact with the 
  developer tools for the first time), and inside this invisible module loader
  a DebuggerServer instance is created and fully initialized.

- In devtools-startup.js, when we handle the --jsconsole CLI options
  (which is what web-ext passes to Firefox on the command line when the
  --bc CLI option is specified), we toggle the browser console immediately
  (which will basically create and fully initialize another DebuggerServer instance
  in the regular devtools module loader).
  https://searchfox.org/mozilla-central/rev/02bc7e2b1bcbab2d7b604ca713b42bcefd47ad2d/devtools/shim/devtools-startup.js#594,597-600

When web-ext starts Firefox, it always passes the --start-debugger-server CLI option,
then it immediatelly connects to the DebuggerServer running in the invisible module loader 
and uses it to list the installed extensions, installs new extensions temporarily, reloads the
extensions etc.

When web-ext interacts with the webextension actors, the "devtools/server/child.js" module
is going to be loaded and it initializes the DebuggerServer retrieved from the 
regular devtools module loader if it is not yet inizialized, but for a webextension that 
is not running in the child extension process (which is currently enabled by default on windows, 
but it is still disabled by default on linux, osx and android) this module is going to run in the main
process instead of an isolated child extension process.

And so, if the user has never interacted with a developer tools until then (e.g. basically 
if we have not passed the --jsconsole CLI options to the Firefox instance) and the webextensions 
are running in the main process, the "devtools/server/child.js" module is loaded in the main
process, the DebuggerServer instance retrieved from the regular devtools module loader is still
not initialized and "devtools/server/child.js" is going to initialize it without also calling 
`DebuggerServer.addBrowserActors()` on it, unfortunately for a DebuggerServer instance related
to the main process this leaves the instance only partially initialized (because
DebuggerServer.initialized will be true, but the DebuggerServer.createRootActor property
has not been assigned and the browser actors not registered to the instance, as expected for a
DebuggerServer instance related to the main process).

This scenario never happens when we connect to a webextension actor from the "about:debugging" page,
because the about:debugging page initializes the DebuggerServer instance retrieved from
the regular module loader in the main process before the "devtools/server/child.js"
has been loaded.
Flags: needinfo?(lgreco)
Attachment #8927667 - Flags: review?(jdescottes)
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Attachment #8927667 - Flags: review?(jdescottes) → review?(poirot.alex)
I would prefer if Alex reviewed this one. I think it's pretty urgent and I would need too much time to make sure the approach is correct here. Sorry!
Comment on attachment 8927667 [details]
Bug 1416105 - devtools/server/child.js should call DebuggerServer.addBrowserActorsadd when initializes a DebuggerServer running in the main process.

https://reviewboard.mozilla.org/r/198944/#review204070

::: devtools/server/child.js:29
(Diff revision 1)
> +      if (!DebuggerServer.isInChildProcess) {
> +        // If we are initializing the DebuggerServer running in the main process,
> +        // we also have to be sure that we add the standard browser actors
> +        // (See Bug 1416105 for rationale).
> +        DebuggerServer.addBrowserActors();
> +      }

I'm not sure this is the best place to put that.
To me, the original issue is in HUDService. This is what is broken actually.

So, instead I would make HUDService.toggleBrowserConsole:connect
force calling addBrowserActors. You can as it shouldn't throw if the browser actors are already registered.
  if (!DebuggerServer.initialized) {
    DebuggerServer.init();
  }
  DebuggerServer.addBrowserActors();

Note that this is what we do for the most important codepath, the regular website toolbox:
https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#405-415
(in addition, we use a newer, simplified and slightly more powerful API: registerActors)
You can also switch to it with:
  DebuggerServer.registerActors({ browser: true });
Attachment #8927667 - Flags: review?(poirot.alex)
I opened bug 1416711 to followup on this as it clearly highlights flaws in many code that do like HUDService.
Attachment #8927667 - Flags: review?(jdescottes)
Comment on attachment 8927667 [details]
Bug 1416105 - devtools/server/child.js should call DebuggerServer.addBrowserActorsadd when initializes a DebuggerServer running in the main process.

https://reviewboard.mozilla.org/r/198944/#review204070

> I'm not sure this is the best place to put that.
> To me, the original issue is in HUDService. This is what is broken actually.
> 
> So, instead I would make HUDService.toggleBrowserConsole:connect
> force calling addBrowserActors. You can as it shouldn't throw if the browser actors are already registered.
>   if (!DebuggerServer.initialized) {
>     DebuggerServer.init();
>   }
>   DebuggerServer.addBrowserActors();
> 
> Note that this is what we do for the most important codepath, the regular website toolbox:
> https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#405-415
> (in addition, we use a newer, simplified and slightly more powerful API: registerActors)
> You can also switch to it with:
>   DebuggerServer.registerActors({ browser: true });

Thanks Alex, I've updated the patch based on your suggestions (the fix is not applied to the hudservice and it uses the new API method, DebuggerServer.registerActors, to ensure that the browser actors have been added to the DebuggerServer instance).
Comment on attachment 8927667 [details]
Bug 1416105 - devtools/server/child.js should call DebuggerServer.addBrowserActorsadd when initializes a DebuggerServer running in the main process.

https://reviewboard.mozilla.org/r/198944/#review204120

::: devtools/client/webconsole/hudservice.js:197
(Diff revision 2)
> -        DebuggerServer.addBrowserActors();
>        }
> +
> +      // Ensure that the browser actors have been registered on the DebuggerServer
> +      // (See Bug 1416105 for rationale).
> +      DebuggerServer.registerActors({ browser: true });

for consistency we might want to specify all options here? 

{ root: false, browser: true, tab: false }
Comment on attachment 8927667 [details]
Bug 1416105 - devtools/server/child.js should call DebuggerServer.addBrowserActorsadd when initializes a DebuggerServer running in the main process.

forwarding r?
Attachment #8927667 - Flags: review?(poirot.alex)
Comment on attachment 8927667 [details]
Bug 1416105 - devtools/server/child.js should call DebuggerServer.addBrowserActorsadd when initializes a DebuggerServer running in the main process.

https://reviewboard.mozilla.org/r/198944/#review204120

> for consistency we might want to specify all options here? 
> 
> { root: false, browser: true, tab: false }

Including all the other options sounds good to me, but I see that the default value for `root` (and `tab`) is `true` and so I'm wondering if it would be better to also keep their value to `true` in the explicit options, as `DebuggerServer.registerActors({ root: true, browser: true, tab: true });`
(In reply to Luca Greco [:rpl] from comment #14)
> Comment on attachment 8927667 [details]
> Bug 1416105 - devtools/server/child.js should call
> DebuggerServer.addBrowserActorsadd when initializes a DebuggerServer running
> in the main process.
> 
> https://reviewboard.mozilla.org/r/198944/#review204120
> 
> > for consistency we might want to specify all options here? 
> > 
> > { root: false, browser: true, tab: false }
> 
> Including all the other options sounds good to me, but I see that the
> default value for `root` (and `tab`) is `true` and so I'm wondering if it
> would be better to also keep their value to `true` in the explicit options,
> as `DebuggerServer.registerActors({ root: true, browser: true, tab: true });`

That's interesting question.
addBrowserActors should be equivalent to that (all fields to true).
But I think we can be more fine grained with registerActors with:
  DebuggerServer.registerActors({ root: true, browser: false, tab: true });
We need the root actor to be able to query the console actors, which is a tab actor.
But we don't need to browser actors like prefs, addons and others.
(In reply to Alexandre Poirot [:ochameau] from comment #16)
> (In reply to Luca Greco [:rpl] from comment #14)
> That's interesting question.
> addBrowserActors should be equivalent to that (all fields to true).
> But I think we can be more fine grained with registerActors with:
>   DebuggerServer.registerActors({ root: true, browser: false, tab: true });
> We need the root actor to be able to query the console actors, which is a
> tab actor.
> But we don't need to browser actors like prefs, addons and others.

Great, it sounds reasonable to me, I've updated the patch accordingly 
(and also the inline comment to better describe the reasons for the options passed to registerActors).
Comment on attachment 8927667 [details]
Bug 1416105 - devtools/server/child.js should call DebuggerServer.addBrowserActorsadd when initializes a DebuggerServer running in the main process.

https://reviewboard.mozilla.org/r/198944/#review204830

Thanks for fixing that Luca!

::: commit-message-57eb0:29
(Diff revision 4)
> +on the contrary when the "devtools/server/child.js" module initializes
> +the DebuggerServer module for the first time, it doesn't add the browser actors
> +to it and so the DebuggerServer instance is going initialized but it will be
> +missing the createRootActor attribute (which is expected if the DebuggerServer
> +instance is initialized in a child process, but it is unexpected when the DebuggerServer
> +instance is the one that serves the main process).

developer tools s/has/have/ been already
Attachment #8927667 - Flags: review?(poirot.alex) → review+
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/bb29d72aa071
devtools/server/child.js should call DebuggerServer.addBrowserActorsadd when initializes a DebuggerServer running in the main process. r=ochameau
https://hg.mozilla.org/mozilla-central/rev/bb29d72aa071
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Yay, thanks everyone for getting this fixed and landed!

Just tested, and confirmed the fix worked in latest nightly build.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.