739966 - Enable script debugger in Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Add Fennec's remote debugger actors

This is pretty much a port of dbg-browser-actors.js for Fennec. One known issue: sometimes navigating to a new page leads to script debugger getting detached forever (I have to reconnect the script debugger to get it attached properly again). Still investigating.

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Add a Debugger listener to Fennec

This is the Fennec-side patch. It adds a couple of prefs to enable/disable the remote debugger listener and to define the listened port. I haven't added any Android UI for enabling the debugger yet as this is probably not a feature we want to expose at this point. You can go to about:config and toggle remote-debugger.enabled for now.

Comment 3

[Dave Camp (:dcamp)]

Comment on attachment 610112 [details] [diff] [review]
Add Fennec's remote debugger actors

This looks fine to me, although I bet we could do something to reduce the duplication.  I'd be OK doing that as a followup, but I'll leave the final r+ decision to panos.

Where do you load the fennec actors?  Is that a separate patch?

Comment 4

[Dave Camp (:dcamp)]

(In reply to Dave Camp (:dcamp) from comment #3)
> Where do you load the fennec actors?  Is that a separate patch?

Obviously I didn't look closely enough.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

General question: Why does dbg-fennec-actors.js live in toolkit? Shouldn't it live in /mobile/android somewhere?

Comment 6

[Dave Camp (:dcamp)]

The browser-specific stuff is in toolkit mostly because while we were writing the server I was more concerned with someone being able to put the puzzle pieces together by looking in one place than a strict separation of files.  Putting this file in /mobile/android would be fine too.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 610112 [details] [diff] [review]
Add Fennec's remote debugger actors

Just a drive by

>diff --git a/toolkit/devtools/debugger/server/dbg-fennec-actors.js b/toolkit/devtools/debugger/server/dbg-fennec-actors.js

I'd think we'd want these actors to be in the application folder in the long run. Adding one for Firefox desktop, Firefox Mobile, Thunderbird, etc in toolkit doesn't seem like the best organization.

Also, let's drop the "fennec" part of the name. We stick to "mobile" or "browser" (yes, just like desktop since we are a browser too). Moving to mobile/android/chrome/content/dbg-browser-actors.js would save us a naming conflict too.

>+/* ***** BEGIN LICENSE BLOCK *****
>+ * ***** END LICENSE BLOCK ***** */

I think we are using the new shorter license style now

>+  disconnect: function BRA_disconnect() {

>+    // We may have registered event listeners on browser windows to
>+    // watch for tab closes, remove those.
>+    let e = windowMediator.getEnumerator("navigator:browser");
>+    let win = e.getNext();

Just an FYI, we might need a real loop here someday, when webapps live in their own windows. This is fine for now. You could even use:
  let win = windowMediator.getMostRecentWindow(""navigator:browser");

>+  onTabClosed: function BRA_onTabClosed(aEvent) {
>+    this.exitTabActor(aEvent.target.linkedBrowser);

linkedBrowser -> browser


>+  onWindowCreated: function BRA_onWindowCreated(evt) {
>+    if (evt.target === this.browser.contentDocument) {

Is this.browser sure to be not null here?

>+  exitTabActor: function BRA_exitTabActor(aWindow) {
>+    this.browser.removeEventListener("DOMWindowCreated", this._onWindowCreated, true);

same

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 610113 [details] [diff] [review]
Add a Debugger listener to Fennec

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+Cu.import("resource://gre/modules/devtools/dbg-server.jsm");
> 
> XPCOMUtils.defineLazyGetter(this, "PluralForm", function() {
>   Cu.import("resource://gre/modules/PluralForm.jsm");
>   return PluralForm;
> });

Let's lazy import dbg-server.jsm just like we do PluralForm.jsm so every user doesn't pay for the load. Looks like you want to trigger it on "DebuggerServer"

>+var RemoteDebugger = {
>+  init: function rd_init() {
>+    Services.prefs.addObserver("", this, false);

Let's limit to just the remote-debugger prefs:

       Services.prefs.addObserver("remote-debugger.", this, false);

>+  uninit: function rd_uninit() {
>+    this._stop();

Let's remove the pref observer:

       Services.prefs.removeObserver("remote-debugger.", this);

>\ No newline at end of file

Add one

r- looks good, but I want to see a new patch

Comment 9

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

Comment on attachment 610112 [details] [diff] [review]
Add Fennec's remote debugger actors

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

Let me preface this with my agreement to moving the actors to app-specific directories. I'll do the same for B2G actors. I also agree with Dave's comment about code duplication, but I had suggested to Lucas to move on with this, until I can get a better picture of how best to address this, after having an initial Fennec and B2G actor implementation. I should file this followup.

(In reply to Mark Finkle (:mfinkle) from comment #7)
> >+  onWindowCreated: function BRA_onWindowCreated(evt) {
> >+    if (evt.target === this.browser.contentDocument) {
> 
> Is this.browser sure to be not null here?

Yes, because it gets set in _preInitTabActor, before the onWindowCreated handler is registered.

> >+  exitTabActor: function BRA_exitTabActor(aWindow) {
> >+    this.browser.removeEventListener("DOMWindowCreated", this._onWindowCreated, true);
> 
> same

This, too, is never reached without a browser stored.

(In reply to Lucas Rocha (:lucasr) from comment #1)
> This is pretty much a port of dbg-browser-actors.js for Fennec. One known
> issue: sometimes navigating to a new page leads to script debugger getting
> detached forever (I have to reconnect the script debugger to get it attached
> properly again). Still investigating.

Is this the issue we saw where old actors are returned in listTabs? I think it would be best to fix this before this patch lands, but I'm not feeling too strong about it, if you prefer to work on it in a followup.

::: toolkit/devtools/debugger/server/dbg-fennec-actors.js
@@ +77,5 @@
> +   */
> +  sayHello: function BRA_sayHello() {
> +    // Create the tab actor for the selected tab right away so that it gets a
> +    // chance to listen to onNewScript notifications.
> +    this._preInitTabActor();

This was a workaround that will no longer be necessary when bug 723563 lands (it landed and was backed out due to oranges). The exact changes to revert can be obtained by an interdiff in two obsolete patches in bug 728244, WIP and WIP 2. If you prefer to leave it as it is for now, I'll make sure to clean it up when I revisit the B2G actors.

@@ +361,5 @@
> +    this._contextPool = new ActorPool(this.conn);
> +    this.conn.addActorPool(this._contextPool);
> +
> +    this.threadActor = new ThreadActor(this);
> +    this.threadActor.addDebuggee(this.browser.contentWindow.wrappedJSObject);

I just landed a fix for debugging content in iframes as well. You'll need to add this hunk to get it:

https://hg.mozilla.org/integration/fx-team/diff/5759f9c0d1eb/toolkit/devtools/debugger/server/dbg-browser-actors.js

Comment 10

[Lucas Rocha (:lucasr)]

Attachment: Add Fennec's remote debugger actors

Comment 11

[Lucas Rocha (:lucasr)]

Attachment: Add a Debugger listener to Fennec

Comment 12

[Lucas Rocha (:lucasr)]

Updated patches with suggested changes and fixed the issue with old actors are returned in listTabs. Everything seems to be working fine now.

Note to self: don't try to test the debugger on chrome pages (at least not until bug 740803 is fixed) :-)

Comment 13

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/357ebe57a7ee
http://hg.mozilla.org/integration/mozilla-inbound/rev/9ea318f07099

Comment 14

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/357ebe57a7ee
https://hg.mozilla.org/mozilla-central/rev/9ea318f07099