Closed Bug 962607 Opened 6 years ago Closed 6 years ago

automatically enable remote debugger when native app built in debug mode

Categories

(Firefox for Android :: Web Apps (PWAs), defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: myk, Assigned: myk)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch work in progress (obsolete) — Splinter Review
In order to enable webapps to be debugged using the remote debugger, we need a way to configure a webapp APK to enable the remote debugger.  Webapp APK builds automatically set a "debuggable" flag during the Android build process if the APK is being built in "debug" mode, so it seems sensible to build on this existing native functionality.

Here's a work in progress that enables the remote debugger if that flag is set.

WesJ: I know you're getting rid of synchronous return values from handleGeckoMessage calls, but since you haven't gotten rid of them yet, I'm still using them in this patch.  I also considered using JNI, but it wasn't clear how to make the getIsDebuggable method static.
Attachment #8363687 - Flags: feedback?(wjohnston)
Comment on attachment 8363687 [details] [diff] [review]
work in progress

A few thoughts:
* _nativeAppIsDebuggable will return | true | for local Fennec builds too, which is fine. But maybe we make the property and messages more general and drop the "native app" context?
* Using JNI seems doable and would be cleaner, IMO. Wes can show you how.
* We might need to use a different debugger port number for webapps, since Firefox could respond to the remote debugger request too. Having a different port for each webapp would be ideal (I think) but also hard to manage.
* A different approach to skipping the debugger prompt is to set "devtools.debugger.prompt-connection" = false, in case that is useable somehow.
Comment on attachment 8363687 [details] [diff] [review]
work in progress

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

::: mobile/android/chrome/content/browser.js
@@ +7383,5 @@
>    _isEnabled: function rd_isEnabled() {
>      return Services.prefs.getBoolPref("devtools.debugger.remote-enabled");
>    },
>  
> +  get _nativeAppIsDebuggable() {

I'd be fine removing all the 'nativeApp' references in here and just saying "isDebuggable" or even 'shouldPrompt'.

There's a quick example of JNI here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2803 i.e. something like:

let jni = new JNI();
let cls = jni.findClass("org/mozilla/gecko/GeckoAppShell");
let method = jni.getStaticMethodID(cls, "isDebuggable", "()I");
this._nativeAppIsDebuggable = jni.callStaticIntMethod(cls, method) > 0;
jni.close();
return this._nativeAppIsDebuggable;

Then you'd need to add a static method to GeckoAppShell (or some class) that queries for whether we're in a native app somehow. I used an Int return because we don't have a bool one set up in http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/JNI.jsm#132 and because of the way its written, its non-trivial to add another one. i.e. You'd have to add code here:

http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJNIWrapper.h#28

or we could finish up bug 918309. This rabbit hole will go deep if you chase it :)
Attachment #8363687 - Flags: feedback?(wjohnston) → feedback+
Attached patch patch v1: implements feature (obsolete) — Splinter Review
(In reply to Mark Finkle (:mfinkle) from comment #1)

> A few thoughts:
> * _nativeAppIsDebuggable will return | true | for local Fennec builds too,
> which is fine. But maybe we make the property and messages more general and
> drop the "native app" context?

Good idea!


> * Using JNI seems doable and would be cleaner, IMO. Wes can show you how.

Wes showed me how, but I still don't see how.  See below for the details.


> * We might need to use a different debugger port number for webapps, since
> Firefox could respond to the remote debugger request too. Having a different
> port for each webapp would be ideal (I think) but also hard to manage.

I implemented this via some code that finds a free port and then tells the user debugging is enabled via a notification that includes the port number.


> * A different approach to skipping the debugger prompt is to set
> "devtools.debugger.prompt-connection" = false, in case that is useable
> somehow.

It's usable, although setting it persists across sessions.  That's probably ok, since devtools.debugger.remote-enabled also persists across sessions.  But ideally neither of them would, such that a developer could install a debuggable version of an app, debug it, and then install a non-debuggable version of the app that doesn't continue to start the server automatically because of the leftover pref.

Thus I've left this as-is, to avoid making the problem worse, and I'll follow up with the DevTools folks about enabling the server within a session without persisting remote-enabled.


(In reply to Wesley Johnston (:wesj) from comment #2)
> > +  get _nativeAppIsDebuggable() {
> 
> I'd be fine removing all the 'nativeApp' references in here and just saying
> "isDebuggable" or even 'shouldPrompt'.

I changed it to _isDebuggable, since it now does more than suppress the prompt: it also finds a free port and notifies the user about it.


> There's a quick example of JNI here:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#2803 i.e. something like:
> 
> let jni = new JNI();
> let cls = jni.findClass("org/mozilla/gecko/GeckoAppShell");
> let method = jni.getStaticMethodID(cls, "isDebuggable", "()I");
> this._nativeAppIsDebuggable = jni.callStaticIntMethod(cls, method) > 0;
> jni.close();
> return this._nativeAppIsDebuggable;
> 
> Then you'd need to add a static method to GeckoAppShell (or some class) that
> queries for whether we're in a native app somehow.

I understand these basics, but what I don't understand is how to write a static method that can figure out if an app's android:debuggable flag is set.

As far as I can tell, GeckoApp.getIsDebuggable needs FragmentActivity.getPackageManager and FragmentActivity.getPackageName to look up the state of that flag, neither of which are static methods.  And WebAppImpl needs to override GeckoApp.getIsDebuggable with a version that depends on the intent passed into the WebAppImpl constructor, since the intent is what provides the name of the webapp package.


> This rabbit hole will go deep if you chase it :)

I'm a huge fan of falling down rabbit holes like these!  And would happily do so if it seemed like there'd be a magical kingdom at the bottom.


In this version, I also added a browser.debuggable.enabled pref so Fennec developers who dislike this behavior (f.e. because they don't want Fennec to pick a new free port on every restart of their local build) can disable it.  Wordsmithing welcome!  My thinking was simply: "browser"-specific (so not a more general "devtools.debugger" pref); related to the "debuggable" flag; determines whether or not the behavior indicated by that flag is "enabled".

mfinkle: I'm requesting review from you because wesj already has a humongous patch from me in his review queue.  And because I know you like wordsmithing!  But feel free to reassign as appropriate.
Attachment #8363687 - Attachment is obsolete: true
Attachment #8367176 - Flags: review?(mark.finkle)
Blocks: 888391
Comment on attachment 8367176 [details] [diff] [review]
patch v1: implements feature

>diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js

>+// Whether or not to automatically enable the remote debugger server and notify
>+// the user of the port it's using if the android:debuggable manifest flag
>+// is enabled.  True by default, so webapp developers can debug their apps
>+// without having to set hidden prefs, although note that Fennec also observes
>+// this preference.
>+pref("browser.debuggable.enabled", true);

I am trying to think of a way to not need this. Setting it to true seems wrong for Fennec.

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+    protected boolean getIsDebuggable() {
>+        int flags = 0;
>+        try {
>+            flags = getPackageManager().getPackageInfo(getPackageName(), 0).applicationInfo.flags;
>+        } catch (NameNotFoundException e) {
>+            Log.wtf(LOGTAG, getPackageName() + " not found", e);
>+        }
>+        return (flags & ApplicationInfo.FLAG_DEBUGGABLE) != 0;
>+    }

Can you return false here. Then override in WebAppImpl? Or maybe WebApp. That would mean that Fennec would not be affected at all and only webapps would be returning the state.

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

> var RemoteDebugger = {
>   init: function rd_init() {

>+    // If the app is in debug mode, then make sure the server is enabled.
>+    // This ensures the server will start (either because of the _start call
>+    // above or because the pref change makes the pref observer start it).
>+    // TODO: enable the server without persisting the pref across sessions.
>+    if (this._isDebuggable)
>+      Services.prefs.setBoolPref("devtools.debugger.remote-enabled", true);

I wonder if we could move this to WebAppRT.js? RemoteDebugger will watch the "devtools.debugger." prefs and enable the debugger.

>   _getPort: function _rd_getPort() {
>-    return Services.prefs.getIntPref("devtools.debugger.remote-port");
>+    let port;
>+
>+    if (this._isDebuggable) {
>+      // When the app is debuggable, and we automatically enable the server,
>+      // automatically choose its port instead of consulting the hidden pref.
>+      let serv = Cc['@mozilla.org/network/server-socket;1'].createInstance(Ci.nsIServerSocket);
>+      serv.init(-1, true, -1);
>+      port = serv.port;
>+      serv.close();
>+    } else {
>+      port = Services.prefs.getIntPref("devtools.debugger.remote-port");
>+    }
>+
>+    return port;

I think you could move this to WebAppRT.js too and set the pref itself. The code in RemoteDebugger will catch it and adjust.

>+  get _isDebuggable() {
>+    return Services.prefs.getBoolPref("browser.debuggable.enabled") &&
>+           sendMessageToJava({ type: "NativeApp:IsDebuggable" });
>+  },

And yes, we'd move this to WebAppRT.js too.

>   _start: function rd_start() {
>     try {
>       if (!DebuggerServer.initialized) {
>-        DebuggerServer.init(this._showConnectionPrompt.bind(this));
>+        // If the app is debuggable, skip the connection prompt in favor of
>+        // notifying the user below that we automatically enabled the server.
>+        DebuggerServer.init(this._isDebuggable ? () => true : this._showConnectionPrompt.bind(this));

Moving to WebAppRT means we'd just set the pref here too. The pref to not use a prompt.

>+      if (this._isDebuggable) {
>+        // When the app is debuggable, and we automatically enable the server,
>+        // we also automatically choose its port instead of consulting
>+        // the hidden pref; so we need to tell the user which port we picked.
>+        let brandShortName = Strings.brand.GetStringFromName("brandShortName");
>+        Notifications.create({
>+          title: Strings.browser.formatStringFromName("remoteNotificationTitle", [brandShortName], 1),
>+          message: Strings.browser.formatStringFromName("remoteNotificationMessage", [port], 1),
>+          icon: "drawable://warning_doorhanger",
>+        });

This notification could also be moved to WebAppRT and fired after setting the prefs.

My main concern is keeping the code more decoupled. If we want to move the code to WebAPpRT in a followup, I could live with it. But I do want to keep things more separated.

Let me know your intention
(In reply to Mark Finkle (:mfinkle) from comment #4)
> My main concern is keeping the code more decoupled. If we want to move the
> code to WebAPpRT in a followup, I could live with it. But I do want to keep
> things more separated.

I like that idea a lot, especially since it avoids changing behavior to which Fennec hackers are accustomed!  I'll do as you say: make isDebuggable always false on Fennec proper, set it as appropriate in the WebAppImpl subclass, and then make WebAppRT query it and set prefs accordingly.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #5)
> I like that idea a lot, especially since it avoids changing behavior to
> which Fennec hackers are accustomed!  I'll do as you say: make isDebuggable
> always false on Fennec proper, set it as appropriate in the WebAppImpl
> subclass, and then make WebAppRT query it and set prefs accordingly.

Here's a patch that does that.


https://tbpl.mozilla.org/?tree=Try&rev=a2c31b95c8ee
Attachment #8367176 - Attachment is obsolete: true
Attachment #8367176 - Flags: review?(mark.finkle)
Attachment #8368462 - Flags: review?(mark.finkle)
Comment on attachment 8368462 [details] [diff] [review]
patch v2: automatically enable debugging only for webapps

Very nice
Attachment #8368462 - Flags: review?(mark.finkle) → review+
Per bug 934760, comment 24, this version of the patch removes the period from the notification message "Listening on port ###".  This is the version I'll land.
Attachment #8368462 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/3d4a162d116c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Depends on: 967218
You need to log in before you can comment on or make changes to this bug.