Closed Bug 723391 Opened 12 years ago Closed 12 years ago

Add a simple remote JS console to b2g

Categories

(Firefox OS Graveyard :: General, defect)

Other
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
DeveloperPhone

People

(Reporter: kanru, Assigned: kanru)

References

Details

Attachments

(1 file, 2 obsolete files)

It would be handy to have a REPL for experimenting APIs interactively.
Did we have a bug on file for this?  I don't remember.

I remember both you guys had started hacking on something like this :).
(In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> Did we have a bug on file for this?  I don't remember.
> 
> I remember both you guys had started hacking on something like this :).

This one is a short-turn hack :)
Attachment #593727 - Flags: review?(jones.chris.g)
Mark Finkle has a couple of add-ons to add remote debugging to Fennec : look for firefly-probe and firefly-shell here:
http://people.mozilla.com/~mfinkle/fennec/addons/

More complex, but also uses a sandbox instead of just eval() which is a bit scary.
(In reply to Fabrice Desré [:fabrice] from comment #4)
> Mark Finkle has a couple of add-ons to add remote debugging to Fennec : look
> for firefly-probe and firefly-shell here:
> http://people.mozilla.com/~mfinkle/fennec/addons/
> 
> More complex, but also uses a sandbox instead of just eval() which is a bit
> scary.

Not that scary since it's meant to touch chrome code :)
Comment on attachment 593727 [details] [diff] [review]
Add a simple remote JS console to b2g

You guys should figure out how to combine efforts.  This patch looks mostly ok on first skim.
Attachment #593727 - Flags: review?(jones.chris.g) → review?(21)
Comment on attachment 593727 [details] [diff] [review]
Add a simple remote JS console to b2g

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

Neat. I commented on some Gecko coding style nits.

::: b2g/chrome/content/shell.js
@@ +347,5 @@
>      });
>    }
>  };
>  
> +(function Repl() {

Why the anonymous function? There's no harm in putting an object or function in the global scope.

@@ +360,5 @@
> +                  .createInstance(Ci.nsIScriptableInputStream);
> +      sin.init(input);
> +      try {
> +        let val = eval(sin.read(sin.available()));
> +        let ret = val ? val.toString()+'\n' : "undefined\n";

Coding style: spaces between all operators.

An easier way to convert `val` into a string would be to simply do `"" + val`.

@@ +364,5 @@
> +        let ret = val ? val.toString()+'\n' : "undefined\n";
> +        output.write(ret, ret.length);
> +      } catch (e) {
> +        output.write(e, e.length);
> +      }

This convolutes several possible exceptions. sin.available() can throw if the socket has been closed, same for sin.read(). It's doubtful if it still makes sense to write anything to output in that case.

@@ +366,5 @@
> +      } catch (e) {
> +        output.write(e, e.length);
> +      }
> +      output.write(prompt, prompt.length);
> +      let tm = Cc["@mozilla.org/thread-manager;1"].getService();

Please use Services.tm.

@@ +372,5 @@
> +    }
> +  }
> +  let listener = {
> +    onSocketAccepted: function(serverSocket, clientSocket) {
> +      dump("Accepted connection on "+clientSocket.host + '\n');

Coding style: spaces between all operators.

@@ +374,5 @@
> +  let listener = {
> +    onSocketAccepted: function(serverSocket, clientSocket) {
> +      dump("Accepted connection on "+clientSocket.host + '\n');
> +      let input = clientSocket.openInputStream(0, 0, 0)
> +                          .QueryInterface(Ci.nsIAsyncInputStream);

Please align the `.` operators.

@@ +377,5 @@
> +      let input = clientSocket.openInputStream(0, 0, 0)
> +                          .QueryInterface(Ci.nsIAsyncInputStream);
> +      output = clientSocket.openOutputStream(Ci.nsITransport.OPEN_BLOCKING, 0, 0);
> +      output.write(prompt, prompt.length);
> +      let tm = Cc["@mozilla.org/thread-manager;1"].getService();

Services.tm

@@ +382,5 @@
> +      input.asyncWait(reader, 0, 0, tm.mainThread);
> +    }
> +  }
> +  let serverPort = Services.prefs.getIntPref("b2g.remote-js.port");
> +  if (serverPort) {

This will pretty much always be `true`. Otherwise we won't have gotten here. `Services.prefs.getIntPref()` throws if the pref isn't present.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> Did we have a bug on file for this?  I don't remember.
> 
> I remember both you guys had started hacking on something like this :).

My plan for the console is to have a remote web console that have the same set of features than the current web console of Firefox.

I have start hacking on https://github.com/vingtetun/remote-web-console and at the end I was having something with a full support of of console.[log/warn/debug/stack/...] but I think it's not enough and my addon is just a hack on top of the HUDService.

But the good thing about this addon is the fact that I have been able to see what's missing in the platform for having a real remote web console and so... I'm pushing to see bug 673148 resolved. 

Once this bug is resolved it will be pretty simple to hack on top of it and to have a remote web console by using:
 - a frame script on the server side (the desktop firefox) to listen for the console commands and forward them via the network to a client

 - some code in shell.js that listen on the network and forward the received message to the content process.

(In reply to Fabrice Desré [:fabrice] from comment #4)
> Mark Finkle has a couple of add-ons to add remote debugging to Fennec : look
> for firefly-probe and firefly-shell here:
> http://people.mozilla.com/~mfinkle/fennec/addons/
> 
> More complex, but also uses a sandbox instead of just eval() which is a bit
> scary.

I have started to do that too - I have looked at the code too - but at the end I feel it was not enough and I would rather prefer sonmething that is shipped with the product.

Also Jetpack (and possibly PDF.js in the future) needs a web console that support <browser remote="true"> so if there is a remote web console in the mozilla codebase, Jetpack, B2G *and* Fennec will be a winner here! :)
(In reply to Kan-Ru Chen [:kanru] from comment #2)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> > Did we have a bug on file for this?  I don't remember.
> > 
> > I remember both you guys had started hacking on something like this :).
> 
> This one is a short-turn hack :)

I will try to update my extension code to have something with more features.
What's the plan for getting something landed?  Simple is fine if it doesn't lead in the wrong direction.

Vivien how do you feel about taking this patch for now?
(In reply to Philipp von Weitershausen [:philikon] from comment #7)
> Comment on attachment 593727 [details] [diff] [review]
> Add a simple remote JS console to b2g
> 
> Review of attachment 593727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Neat. I commented on some Gecko coding style nits.
> 
> ::: b2g/chrome/content/shell.js
> @@ +347,5 @@
> >      });
> >    }
> >  };
> >  
> > +(function Repl() {
> 
> Why the anonymous function? There's no harm in putting an object or function
> in the global scope.

No particular reason, just so that it will run once and only once in the beginning.
(In reply to Kan-Ru Chen [:kanru] from comment #11)
> > Why the anonymous function? There's no harm in putting an object or function
> > in the global scope.
> 
> No particular reason, just so that it will run once and only once in the
> beginning.

Sure, I understand that. I was just trying to point out that it's a pretty uncommon (and also unnecessary) patterns in Gecko code. ;)
Attachment #593727 - Attachment is obsolete: true
Attachment #593727 - Flags: review?(21)
(In reply to Kan-Ru Chen [:kanru] from comment #13)
> Created attachment 596618 [details] [diff] [review]
> Add a simple remote JS console to b2g. v2

The other solution I as hoping for takes more time than planned. I will r? this patch.
Comment on attachment 596618 [details] [diff] [review]
Add a simple remote JS console to b2g. v2

>--- a/b2g/chrome/content/shell.js
>+(function Repl() {
>+  if (!Services.prefs.getBoolPref("b2g.remote-js.enabled")) {
>+    return;
>+  }
>+  const prompt = "JS> ";
>+  let output;
>+  let reader = {
>+    onInputStreamReady : function(input) {

Can you add a name to your anonymous function, it's helpful when you are using a profiler/debugger. Instead of seeing 'anonymous' you have the name of the function instead.

>+      let sin = Cc['@mozilla.org/scriptableinputstream;1']
>+                  .createInstance(Ci.nsIScriptableInputStream);
>+      sin.init(input);
>+      try {
>+        let val = eval(sin.read(sin.available()));
>+        let ret = val ? val + '\n' : "undefined\n";

You code will return 'undefined' if val == 0 or if val == null

>+        output.write(ret, ret.length);
>+        // TODO: check if socket has been closed
>+      } catch (e) {
>+        if (e.result === Cr.NS_BASE_STREAM_CLOSED ||
>+            (typeof e === "object" && e.result === Cr.NS_BASE_STREAM_CLOSED)) {
>+          return;
>+        }
>+        let message = typeof e === "object" ? e.message + '\n' : e + '\n';
>+        output.write(message, message.length);
>+      }
>+      output.write(prompt, prompt.length);
>+      input.asyncWait(reader, 0, 0, Services.tm.mainThread);
>+    }
>+  }
>+  let listener = {
>+    onSocketAccepted: function(serverSocket, clientSocket) {

Add a name to your function.

>+      dump('Accepted connection on ' + clientSocket.host + '\n');
>+      let input = clientSocket.openInputStream(0, 0, 0)
>+                              .QueryInterface(Ci.nsIAsyncInputStream);
>+      output = clientSocket.openOutputStream(Ci.nsITransport.OPEN_BLOCKING, 0, 0);

Use the flag name in clientsocket,openInputStream for the first parameter (similar to what you have done in clientSocket.openOutputStream)

>+      output.write(prompt, prompt.length);
>+      input.asyncWait(reader, 0, 0, Services.tm.mainThread);
>+    }
>+  }
>+  let serverPort = Services.prefs.getIntPref("b2g.remote-js.port");
>+  let serverSocket = Cc['@mozilla.org/network/server-socket;1']
>+                       .createInstance(Ci.nsIServerSocket);
>+  serverSocket.init(serverPort, true, 5);

Just curious, if you allow connection only on the loopback interface, do you |adb shell| first?
Also is there any reason to use 5 for the backlog instead of -1?

>+  dump('Opened socket on ' + serverSocket.port + '\n');
>+  serverSocket.asyncListen(listener);
>+})();

Also use single quote everywhere instead of a mix of single/double quotes (to make gjslint happy)
(In reply to Vivien Nicolas (:vingtetun) from comment #15)
> >+  dump('Opened socket on ' + serverSocket.port + '\n');
> >+  serverSocket.asyncListen(listener);
> >+})();
> 
> Also use single quote everywhere instead of a mix of single/double quotes
> (to make gjslint happy)

Not in Gecko code, please. Gecko's coding style prefers double quotes over single quotes (https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Literals). (You're not going to have much luck linting Gecko's JS code anyway, I suspect. And the fact that gslint doesn't seem to be configurable in that respect and just for this tool gaia violates various Mozilla coding style rules makes me sad.)
(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> (In reply to Vivien Nicolas (:vingtetun) from comment #15)
> > >+  dump('Opened socket on ' + serverSocket.port + '\n');
> > >+  serverSocket.asyncListen(listener);
> > >+})();
> > 
> > Also use single quote everywhere instead of a mix of single/double quotes
> > (to make gjslint happy)
> 
> Not in Gecko code, please. Gecko's coding style prefers double quotes over
> single quotes
> (https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Literals).
> (You're not going to have much luck linting Gecko's JS code anyway, I
> suspect. And the fact that gslint doesn't seem to be configurable in that
> respect and just for this tool gaia violates various Mozilla coding style
> rules makes me sad.)

The code has first been checked-in with single quotes. To stay consistent let's use those for now.
If that's a problem and you want to do some bikeshedding about the coding guidelines, please open an other bug.
(In reply to Vivien Nicolas (:vingtetun) from comment #15)
> >+      let sin = Cc['@mozilla.org/scriptableinputstream;1']
> >+                  .createInstance(Ci.nsIScriptableInputStream);
> >+      sin.init(input);
> >+      try {
> >+        let val = eval(sin.read(sin.available()));
> >+        let ret = val ? val + '\n' : "undefined\n";
> 
> You code will return 'undefined' if val == 0 or if val == null

Doh, I have fixed this in another copy of this patch. I must forgot to sync it back.

> >+      output.write(prompt, prompt.length);
> >+      input.asyncWait(reader, 0, 0, Services.tm.mainThread);
> >+    }
> >+  }
> >+  let serverPort = Services.prefs.getIntPref("b2g.remote-js.port");
> >+  let serverSocket = Cc['@mozilla.org/network/server-socket;1']
> >+                       .createInstance(Ci.nsIServerSocket);
> >+  serverSocket.init(serverPort, true, 5);
> 
> Just curious, if you allow connection only on the loopback interface, do you
> |adb shell| first?

I |adb forward| first, then I connect to it locally.

> Also is there any reason to use 5 for the backlog instead of -1?

Hum.. I can't remember the reason.. I think I should use -1 here.

> >+  dump('Opened socket on ' + serverSocket.port + '\n');
> >+  serverSocket.asyncListen(listener);
> >+})();
> 
> Also use single quote everywhere instead of a mix of single/double quotes
> (to make gjslint happy)

Since the rest of the file uses single quote, I will match the style here.
Attachment #596618 - Attachment is obsolete: true
Attachment #596910 - Flags: review?(21)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fd3817a300ef
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: