Enable script debugger in Fennec

RESOLVED FIXED in Firefox 14

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: lucasr, Unassigned)

Tracking

({dev-doc-needed})

Trunk
Firefox 14
All
Android
dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
Created attachment 610112 [details] [diff] [review]
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.
Attachment #610112 - Flags: review?(dcamp)
(Reporter)

Comment 2

5 years ago
Created attachment 610113 [details] [diff] [review]
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.
Attachment #610113 - Flags: review?(mark.finkle)

Comment 3

5 years ago
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?
Attachment #610112 - Flags: review?(past)
Attachment #610112 - Flags: review?(dcamp)
Attachment #610112 - Flags: feedback+

Comment 4

5 years ago
(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.
General question: Why does dbg-fennec-actors.js live in toolkit? Shouldn't it live in /mobile/android somewhere?

Comment 6

5 years ago
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 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
Attachment #610112 - Flags: feedback+
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
Attachment #610113 - Flags: review?(mark.finkle) → review-
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
Attachment #610112 - Flags: review?(past) → review+
(Reporter)

Comment 10

5 years ago
Created attachment 611819 [details] [diff] [review]
Add Fennec's remote debugger actors
Attachment #610112 - Attachment is obsolete: true
Attachment #611819 - Flags: review?(past)
Attachment #611819 - Flags: feedback?(mark.finkle)
(Reporter)

Comment 11

5 years ago
Created attachment 611820 [details] [diff] [review]
Add a Debugger listener to Fennec
Attachment #611820 - Flags: review?(mark.finkle)
Attachment #611820 - Flags: feedback?(past)
(Reporter)

Comment 12

5 years ago
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) :-)
(Reporter)

Updated

5 years ago
Attachment #610113 - Attachment is obsolete: true
Attachment #611819 - Flags: feedback?(mark.finkle) → feedback+
Attachment #611820 - Flags: review?(mark.finkle) → review+
Attachment #611820 - Flags: feedback?(past) → feedback+
Attachment #611819 - Flags: review?(past) → review+
(Reporter)

Comment 13

5 years ago
Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/357ebe57a7ee
http://hg.mozilla.org/integration/mozilla-inbound/rev/9ea318f07099
https://hg.mozilla.org/mozilla-central/rev/357ebe57a7ee
https://hg.mozilla.org/mozilla-central/rev/9ea318f07099
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14

Updated

5 years ago
Keywords: dev-doc-needed
Duplicate of this bug: 518262
You need to log in before you can comment on or make changes to this bug.