777428 - debug webapps running in desktop webapp runtime

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P3)

Description

[Myk Melez [:myk] [@mykmelez]]

Now that the debugger is remotable, we should make it possible to debug webapps running in the desktop webapp runtime.

I'm not sure how much knowledge of the runtime the eventual owner of this bug will need to implement the functionality; here's a general overview:

The runtime, like Firefox, is a GRE/XUL application.  It gets built and packaged with Firefox on Windows/Mac/Linux as the webapprt-stub[.exe] executable in the Firefox installation directory and the webapprt/ subdirectory of the Firefox installation directory.

Firefox makes a copy of the stub for each webapp it installs, but all copies share the webapprt/ subdirectory, which contains the runtime-specific code (chrome/, components/, modules/, the application INI file, which is called webapprt.ini due to bug 747394, etc.).  The runtime also has access to GRE code, including all code in toolkit/ that is built/packaged with Firefox.

The runtime's XUL code is minimal and stored in the webapprt/content/ source directory.  Starting a webapp opens a single window with one <browser> element in which the webapp's primary launch URL is loaded.

Webapps may open multiple windows via the standard mechanisms (<a target="something">, window.openWindow), each of which contains one <browser> element.  The runtime does not support tabs, so there is never more than one <browser> element per window.

Comment 1

[Rob Campbell [:rc] (:robcee)]

filter on CHOCOLATE

Comment 2

[Marco Castelluccio [:marco]]

What should we do to enable the remote debugger?
Do the actors that are in http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/ depend on Firefox features? (like tabs?)

I've tried enabling the pref in the webapp runtime and adding some actors, the connection is opened but is eventually lost (and in the devtools window there isn't anything, no styles, no network requests, etc.).

I get this error in the debuggee process (the debugger server):
> Handler function DebuggerTransport instance's this.hooks.onPacket threw an exception: TypeError: instance is undefined
> Call stack:
> DSC_onPacket@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js:900
> @resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/transport.js:183
> @resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:61

And this in the debugger process (the debugger client):
> JavaScript strict warning: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js, line 501: reference to undefined property this._currentToolId
> WebConsolePanel open failed. timeout: Connection timeout. Check the Error Console on both ends for potential error messages. Reopen the Web Console to try again.

Comment 3

[Marco Castelluccio [:marco]]

I've figured out how this works and I've hacked something together. I'll post a patch soon.

Comment 4

[Marco Castelluccio [:marco]]

Attachment: enable_debugger_webapprt

Please only look at dbg-webapp-actors.js and RemoteDebugger.jsm, the other changes are temporary because we still need to figure out when and how we should actually enable the remote debugger.

Comment 5

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 783469 [details] [diff] [review]
enable_debugger_webapprt

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

this looks decent. It should get you started, though might need to add more actors as they become available.

::: webapprt/RemoteDebugger.jsm
@@ +18,5 @@
> +  // Ask for remote connections.
> +  DebuggerServer.init();
> +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/webbrowser.js");
> +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/script.js");
> +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/webconsole.js");

don't need to add this twice.

@@ +20,5 @@
> +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/webbrowser.js");
> +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/script.js");
> +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/webconsole.js");
> +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/gcli.js");
> +  if ("nsIProfiler" in Ci) {

when are you going to not have Ci.nsIProfiler?

::: webapprt/content/dbg-webapp-actors.js
@@ +116,5 @@
> +    return this.browser.ownerDocument.defaultView.document.title;
> +  },
> +  enumerable: true,
> +  configurable: false
> +});

will you be implementing navigation and reload handlers?

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Note that in the second part of bug 899006 I'd like to refactor addBrowserActors() in a way that initializers just need to call DebuggerServer.addToolkitActors() to take advantage of any new actors coming along. I see this is making more and more sense, this would save you the work of yet another location where you need to add new actors as they come along.

Also, after bug 880511 you should be setting DebuggerServer.chromeWindowType accordingly so that you have the window object pointing to the correct window.

Comment 7

[Marco Castelluccio [:marco]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #5)
> ::: webapprt/RemoteDebugger.jsm
> @@ +18,5 @@
> > +  // Ask for remote connections.
> > +  DebuggerServer.init();
> > +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/webbrowser.js");
> > +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/script.js");
> > +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/webconsole.js");
> 
> don't need to add this twice.

What?

> ::: webapprt/content/dbg-webapp-actors.js
> @@ +116,5 @@
> > +    return this.browser.ownerDocument.defaultView.document.title;
> > +  },
> > +  enumerable: true,
> > +  configurable: false
> > +});
> 
> will you be implementing navigation and reload handlers?

I don't think so, at least for now (in the webapp runtime we don't have navigation or reload buttons).

(In reply to Philipp Kewisch [:Fallen] from comment #6)
> Note that in the second part of bug 899006 I'd like to refactor
> addBrowserActors() in a way that initializers just need to call
> DebuggerServer.addToolkitActors() to take advantage of any new actors coming
> along. I see this is making more and more sense, this would save you the
> work of yet another location where you need to add new actors as they come
> along.

That'd be great.

I think initially we can handle the debug mode through a command line flag, and maybe in the debugger client we could show a list of the installed apps and let the user launch an app in debug mode (this would be work for another bug).

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Marco Castelluccio [:marco] from comment #7)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #5)
> > ::: webapprt/RemoteDebugger.jsm
> > @@ +18,5 @@
> > > +  // Ask for remote connections.
> > > +  DebuggerServer.init();
> > > +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/webbrowser.js");
> > > +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/script.js");
> > > +  DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/webconsole.js");
> > 
> > don't need to add this twice.
> 
> What?

You call addActors with the webconsole.js actors twice.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Oops, one is webbrowser, the other webconsole. Perhaps Rob misread just as I did.

Comment 10

[Rob Campbell [:rc] (:robcee)]

I did, I did. Sorry! Disregard that.

Comment 11

[Marco Castelluccio [:marco]]

Attachment: Patch

(it should apply cleanly on top of the patch in bug 774521)

Comment 12

[Rob Campbell [:rc] (:robcee)]

I won't have time to review this today. I should get to it on Monday. Sorry for the delay.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 789719 [details] [diff] [review]
Patch

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

I'll mostly defer to robcee on this review, as I'm unfamiliar with the remote debugger implementation; but the command-line handler and build script modifications look good.

Nevertheless, I haven't tested this yet.  Can you provide a version of the patch that applies without requiring another patch on an unrelated bug?  I know it's cumbersome to maintain multiple conflicting patches, and it's ok for one patch to require another when the bugs in question are interdependent.  But the patches for independent bugs shouldn't depend on each other.

Comment 14

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 789719 [details] [diff] [review]
Patch

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

Some issues below. Canceling review until the dependency Myk mentioned and the below issues are addressed.

I think I'd like Panos to weigh in on the final review. Please tag :past for review when updated.

Thanks!

::: webapprt/CommandLineHandler.js
@@ +21,5 @@
>                 createInstance(Ci.nsIWritablePropertyBag);
>      let inTestMode = this._handleTestMode(cmdLine, args);
>  
> +    let debugPort = this._handleDebugMode(cmdLine);
> +    if (debugPort != NaN) {

that's a strange test. This is true for all kinds of non-numerics that will get passed to RemoteDebugger.init().

E.g., "Eggs" != NaN.

I think you want debugPort && typeof debugPort == "number"

or even Number.isInteger(debugPort).

@@ +46,5 @@
>        startup(window);
>      }
>    },
>  
> +  _handleDebugMode: function(cmdLine) {

you should document this function properly with a javadoc comment.

::: webapprt/content/dbg-webapp-actors.js
@@ +59,5 @@
> +  // the actors. Thus, the sequence yielded is always a snapshot of the
> +  // actors that were live when we began the iteration.
> +
> +  // Iterate over all webapprt:webapp XUL windows.
> +  for (let win of allAppShellDOMWindows("webapprt:webapp")) {

not familiar with allAppShellDOMWindows. Presumably this is part of the webRT API and is a nsIWindowMediator underneath.

@@ +70,5 @@
> +    } else {
> +      actor = new WebappTabActor(this._connection, browser);
> +      this._actorByBrowser.set(browser, actor);
> +    }
> +    actor.selected = true;

actor.selected has to be set on each actor you find or create? This could be documented a little better.

@@ +78,5 @@
> +    throw Error("_actorByBrowser map contained actors for dead tabs");
> +  }
> +
> +  this._mustNotify = true;
> +  this._checkListening();

what are these for?

::: webapprt/prefs.js
@@ +54,5 @@
>  
>  pref("plugin.allowed_types", "application/x-shockwave-flash,application/futuresplash");
>  
> +pref("devtools.debugger.remote-enabled", true);
> +pref("devtools.debugger.force-local", true);

this will force all webrt apps to be remote-enabled. Are you sure you want to do that? y/N?

Might be best to figure out a way to set that on selected webapps rather than use this option.

Comment 15

[Marco Castelluccio [:marco]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #14)
> ::: webapprt/CommandLineHandler.js
> @@ +21,5 @@
> >                 createInstance(Ci.nsIWritablePropertyBag);
> >      let inTestMode = this._handleTestMode(cmdLine, args);
> >  
> > +    let debugPort = this._handleDebugMode(cmdLine);
> > +    if (debugPort != NaN) {
> 
> that's a strange test. This is true for all kinds of non-numerics that will
> get passed to RemoteDebugger.init().

_handleDebugMode will return an integer or NaN.

> ::: webapprt/content/dbg-webapp-actors.js
> @@ +59,5 @@
> > +  // the actors. Thus, the sequence yielded is always a snapshot of the
> > +  // actors that were live when we began the iteration.
> > +
> > +  // Iterate over all webapprt:webapp XUL windows.
> > +  for (let win of allAppShellDOMWindows("webapprt:webapp")) {
> 
> not familiar with allAppShellDOMWindows. Presumably this is part of the
> webRT API and is a nsIWindowMediator underneath.

It's in the webbrowser actor and yes, it just iterates over windows using a nsIWindowMediator:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#17

> @@ +78,5 @@
> > +    throw Error("_actorByBrowser map contained actors for dead tabs");
> > +  }
> > +
> > +  this._mustNotify = true;
> > +  this._checkListening();
> 
> what are these for?

These should be needed to add listeners to know when windows are opened/closed. I think I should also override the BrowserTabList._checkListening, BrowserTabList.onOpenWindow and BrowserTabList.onClosedWindow functions by the way.

> ::: webapprt/prefs.js
> @@ +54,5 @@
> >  
> >  pref("plugin.allowed_types", "application/x-shockwave-flash,application/futuresplash");
> >  
> > +pref("devtools.debugger.remote-enabled", true);
> > +pref("devtools.debugger.force-local", true);
> 
> this will force all webrt apps to be remote-enabled. Are you sure you want
> to do that? y/N?
> 
> Might be best to figure out a way to set that on selected webapps rather
> than use this option.

Maybe we can set the pref only if we're in debug mode? Does modifying the pref require a restart?

Comment 16

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 796812 [details] [diff] [review]
Patch v2

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

This patch breaks test_add_actors.js, you need to fix that. Try running 'mach xpcshell-test toolkit/devtools' to make sure you catch everything.

I am not familiar with the webapprt/ policies for tests, but I think it would be safe to assume that without some basic test coverage we will not be able to catch any related breakage from the devtools side, unless you regularly test things manually from your side. Some very basic testing would suffice I think, like connecting to the server and making sure you get back the expected set of actors.

::: toolkit/devtools/server/actors/webbrowser.js
@@ +182,5 @@
>  
>    /* True if we're testing, and should throw if consistency checks fail. */
>    this._testing = false;
> +
> +  this._windowType = aWindowType ? aWindowType : "navigator:browser";

Why don't you just reuse DebuggerServer.chromeWindowType here instead of modifying the signature of BrowserTabList?

::: webapprt/CommandLineHandler.js
@@ +71,5 @@
> +        return port;
> +      }
> +    }
> +
> +    return 6000;

Can't you return the value of devtools.debugger.remote-port here for consistency?

::: webapprt/RemoteDebugger.jsm
@@ +24,5 @@
> +      DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/webconsole.js");
> +      DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/gcli.js");
> +      DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/profiler.js");
> +      DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/styleeditor.js");
> +      DebuggerServer.addActors("chrome://webapprt/content/dbg-webapp-actors.js");

Is the reason you don't simply call DebuggerServer.addBrowserActors here the fact that it sets its own chromeWindowType? If that is the case, I think it would be better to make it a parameter of that method, instead of forcing you (and other embeddings) to copy everything here. See for example how some late additions to the actor list are not present in your patch.

Note also that registering the ChromeDebuggerActor is essential if you want to provide the ability to debug chrome code in the app runtime.

Comment 18

[Marco Castelluccio [:marco]]

(In reply to Panos Astithas [:past] from comment #17)
> Is the reason you don't simply call DebuggerServer.addBrowserActors here the
> fact that it sets its own chromeWindowType? If that is the case, I think it
> would be better to make it a parameter of that method, instead of forcing
> you (and other embeddings) to copy everything here. See for example how some
> late additions to the actor list are not present in your patch.

The problem was that addBrowserActors loads also the webapps actor. But I guess we should remove it from addBrowserActors as it shouldn't be used in Firefox.

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Marco Castelluccio [:marco] from comment #18)
> The problem was that addBrowserActors loads also the webapps actor. But I
> guess we should remove it from addBrowserActors as it shouldn't be used in
> Firefox.

Alex, Marco tells me that installing apps on desktop isn't working. Was it supposed to? Should we file another bug for that?

Comment 20

[Alexandre Poirot [:ochameau]]

(In reply to Panos Astithas [:past] from comment #19)
> (In reply to Marco Castelluccio [:marco] from comment #18)
> > The problem was that addBrowserActors loads also the webapps actor. But I
> > guess we should remove it from addBrowserActors as it shouldn't be used in
> > Firefox.
> 
> Alex, Marco tells me that installing apps on desktop isn't working. Was it
> supposed to? Should we file another bug for that?

It isn't explicitely meant to fail on desktop. Actually I want this actor to work on all platform.
So far, we all focused on b2g, but when moving it from b2g/ to toolkit/devtools/ I verified that all features work on desktop, b2g and fennec and it was working!
The install method was really weird to use as you have to copy the file to /tmp folder, and also, it wasn't really playing nice with webappsrt, when connecting to firefox desktop, it was installing the app to firefox itself, not the webappsrt. I had to set the allAppsLaunchable flag it order to have stuff working.

Otherwise, this patch hack the tabList in order to expose apps as tabs.
It won't work with the upcoming app manager. It will also force to go through the connect screen, that is still preffed off (and way less nice looking than the app manager ;))

I'd suggest to tweak the existing webapps actor in order to make it work with webappsrt.
It should be about tweaking _appFrames function:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#456
And also watchApps/unwatchApps:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#575
Then, we should figure out why getAppActor doesn't work with non-oop frames. That's something I want to look at ASAP (bug 912213).

Comment 21

[Alexandre Poirot [:ochameau]]

Here is how looks like the app manager:
http://people.mozilla.com/~prouget/appmanager.webm

Most of the backend code is already landed, and most of the UI code is going to land this week.

Comment 22

[Marco Castelluccio [:marco]]

(In reply to Alexandre Poirot (:ochameau) from comment #20)
> It isn't explicitely meant to fail on desktop. Actually I want this actor to
> work on all platform.
> So far, we all focused on b2g, but when moving it from b2g/ to
> toolkit/devtools/ I verified that all features work on desktop, b2g and
> fennec and it was working!
> The install method was really weird to use as you have to copy the file to
> /tmp folder, and also, it wasn't really playing nice with webappsrt, when
> connecting to firefox desktop, it was installing the app to firefox itself,
> not the webappsrt. I had to set the allAppsLaunchable flag it order to have
> stuff working.

What was working? Could you install an application and then run it as a separate OS application (in its own process, with its own window, completely separated from Firefox)?
The WebappsInstaller code is not executed at all, so that shouldn't be possible. And indeed you had to set allAppsLaunchable to true because otherwise the isLaunchable function would've returned false (because the app isn't installed!).

> Otherwise, this patch hack the tabList in order to expose apps as tabs.
> It won't work with the upcoming app manager. It will also force to go
> through the connect screen, that is still preffed off (and way less nice
> looking than the app manager ;))

Why it won't work? The webapp runtime is like a remote browser with a single tab :D

Comment 23

[Alexandre Poirot [:ochameau]]

(In reply to Marco Castelluccio [:marco] from comment #22)
> What was working? Could you install an application and then run it as a
> separate OS application (in its own process, with its own window, completely
> separated from Firefox)?
> The WebappsInstaller code is not executed at all, so that shouldn't be
> possible. And indeed you had to set allAppsLaunchable to true because
> otherwise the isLaunchable function would've returned false (because the app
> isn't installed!).

TBH, I don't remember exactly what and how much it was working. Consider the webapps actor to be working on b2g and should also be working on other platform with some additional work.
I'm fine with disabling it on browser until it gets proper integration.

> 
> > Otherwise, this patch hack the tabList in order to expose apps as tabs.
> > It won't work with the upcoming app manager. It will also force to go
> > through the connect screen, that is still preffed off (and way less nice
> > looking than the app manager ;))
> 
> Why it won't work? The webapp runtime is like a remote browser with a single
> tab :D

Sure, it is going to "work". But you won't benefit from the app manager features.
Apps come with various new specifics compared to regular content: permissions, zip package, a manifest, ...

Comment 24

[Marco Castelluccio [:marco]]

(In reply to Panos Astithas [:past] from comment #17)
> I am not familiar with the webapprt/ policies for tests, but I think it
> would be safe to assume that without some basic test coverage we will not be
> able to catch any related breakage from the devtools side, unless you
> regularly test things manually from your side. Some very basic testing would
> suffice I think, like connecting to the server and making sure you get back
> the expected set of actors.

Writing these tests is harder than I thought :(

Comment 25

[Marco Castelluccio [:marco]]

Attachment: Patch v3

Comment 26

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 801696 [details] [diff] [review]
Patch v3

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

This looks great!

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Once again, a friendly reminder that you commit message should be saying what the patch is actually doing, not just restating the bug summary.

https://hg.mozilla.org/integration/fx-team/rev/8a0b5eb332ff

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a0b5eb332ff

Comment 29

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

In this blog comment[1], Myk describes how to make use of this.  Adding dev-doc-needed, so we can expand our Remote Debugging docs[2] to cover this type of debugging too.

[1]: https://hacks.mozilla.org/2014/03/better-integration-for-open-web-apps-on-android/comment-page-1/#comment-2159140
[2]: https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging