1247629 - The mdn command prevents the server from starting on devices as it imports a module that's only available on the client

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Patrick Brosset <:pbro>]

In bug 768469 a new gcli command was added: mdn
This command exists in devtools/shared along with other commands.
This directory ends up on devices, along with the rest of the debugger server code.

However, this command relies on a devtools module that only exists on the client:

See \devtools\shared\gcli\commands\mdn.js :
const {
  getCssDocs,
  PAGE_LINK_URL
} = require("devtools/client/shared/widgets/MdnDocsWidget");

So when the server starts, gcli loads all its commands to know what's available, and that causes:

    "Failed to load module devtools/shared/gcli/commands/mdn: Module `devtools/client/shared/widgets/MdnDocsWidget` is not found at resource://devtools/client/shared/widgets/MdnDocsWidget.js" system.js:132
    Failed to load module devtools/shared/gcli/commands/mdn: Module `devtools/client/shared/widgets/MdnDocsWidget` is not found at resource://devtools/client/shared/widgets/MdnDocsWidget.js <unknown>
    @resource://devtools/shared/gcli/commands/mdn.js:11:5
    exports.createSystem/system.addItemsByModule/</options.loader@resource://devtools/shared/gcli/source/lib/gcli/system.js:165:20
    exports.createSystem/loadModule@resource://devtools/shared/gcli/source/lib/gcli/system.js:112:30
    exports.createSystem/system.load/promises<@resource://devtools/shared/gcli/source/lib/gcli/system.js:210:16
    exports.createSystem/system.load@resource://devtools/shared/gcli/source/lib/gcli/system.js:208:22
    GcliActor<._getRequisition@resource://devtools/server/actors/gcli.js:253:32
    GcliActor<.specs<@resource://devtools/server/actors/gcli.js:96:12
    actorProto/</handler@resource://devtools/server/protocol.js:1013:19
    DSC_onPacket@resource://devtools/server/main.js:1643:15
    DebuggerTransport.prototype._onJSONObjectReady/<@resource://devtools/shared/transport/transport.js:479:9
    exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
    exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
     system.js:133
    @resource://devtools/shared/gcli/commands/mdn.js:11:5
    exports.createSystem/system.addItemsByModule/</options.loader@resource://devtools/shared/gcli/source/lib/gcli/system.js:165:20
    exports.createSystem/loadModule@resource://devtools/shared/gcli/source/lib/gcli/system.js:112:30
    exports.createSystem/system.load/promises<@resource://devtools/shared/gcli/source/lib/gcli/system.js:210:16
    exports.createSystem/system.load@resource://devtools/shared/gcli/source/lib/gcli/system.js:208:22
    GcliActor<._getRequisition@resource://devtools/server/actors/gcli.js:253:32
    GcliActor<.specs<@resource://devtools/server/actors/gcli.js:96:12
    actorProto/</handler@resource://devtools/server/protocol.js:1013:19
    DSC_onPacket@resource://devtools/server/main.js:1643:15
    DebuggerTransport.prototype._onJSONObjectReady/<@resource://devtools/shared/transport/transport.js:479:9
    exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
    exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14 

We should:
- either try/catch the import and fail gracefully when the module can't be imported
- or use a lazyImporter

Comment 1

[Patrick Brosset <:pbro>]

I have no bandwidth at the moment to fix this, but it should be a really simple fix to do.
We must do this in the FF47 time frame as the mdn command landed in 47, and we don't want this bug to reach dev edition.

Comment 2

[Julian Descottes [:jdescottes]]

I'll take it.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: bug1247629.v1.patch (display error message)

In this patch, if the client widget could not be loaded, the mdn css command will display an error message if executed. 

It's very unlikely any user will ever face this issue though, so not sure it's worth the extra code & localization effort.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: bug1247629.v2.patch (do nothing)

In this patch we simply bail out if the widget was not loaded. From a user's perspective, the command is erased and nothing happens.

Again, AFAIK this use case should not really happen for a user.

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8718599 [details] [diff] [review]
bug1247629.v1.patch (display error message)

pbro: Let me know which one you prefer to go with.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8718611 [details] [diff] [review]
bug1247629.v2.patch (do nothing)

Review of attachment 8718611 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely prefer this approach. No need for something beautiful here. This command won't ever be executed on a device. It is a runAt=client command which means that it can only run in the process where the developerToolbar is loaded. And this only happens on Firefox Desktop.

Fwiw, it would probably make more sense to move the command file into devtools/client/ so that it doesn't even get shipped to devices.
We do have some commands in devtools/client. But most of our "general" commands are inside devtools/shared/ so that's why this one ended up there.

so, anyway, as long as it doesn't cause problems when the server starts on a device being debugged remotely (through webIDE), then we're fine.

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8718611 [details] [diff] [review]
bug1247629.v2.patch (do nothing)

Review of attachment 8718611 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/shared/gcli/commands/mdn.js
@@ +9,5 @@
> +var MdnDocsWidget;
> +try {
> +  MdnDocsWidget = require("devtools/client/shared/widgets/MdnDocsWidget");
> +} catch (e) {
> +  // DevTools MdnDocsWidget only available in Firefox

s/Firefox/Firefox Desktop

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: bug1247629.v3.patch

Thanks for the feedback and review.

Carry over r+.

try looks good : https://treeherder.mozilla.org/#/jobs?repo=try&revision=f38c8f7f56a6&selectedJob=16642601

Comment 9

[Julian Descottes [:jdescottes]]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #7)
> Comment on attachment 8718611 [details] [diff] [review]
> bug1247629.v2.patch (do nothing)
> 
> Review of attachment 8718611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/shared/gcli/commands/mdn.js
> @@ +9,5 @@
> > +var MdnDocsWidget;
> > +try {
> > +  MdnDocsWidget = require("devtools/client/shared/widgets/MdnDocsWidget");
> > +} catch (e) {
> > +  // DevTools MdnDocsWidget only available in Firefox
> 
> s/Firefox/Firefox Desktop

I updated the comment, but I actually had a lot of trouble to reproduce the issue. I had no problem remote debugging a Mobile Firefox, no problem debugging an old Firefox (version 37, so before the MDNDocs widget got introduced). T
he only way I could reproduce the issue was when debugging Chrome, hence the comment.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/18528724ae1c

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/18528724ae1c