Last Comment Bug 758696 - Add a dialog to the debugger to deny or allow incoming server connections
: Add a dialog to the debugger to deny or allow incoming server connections
Status: VERIFIED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 15
Assigned To: Panos Astithas [:past]
:
: James Long (:jlongster)
Mentors:
: 764675 (view as bug list)
Depends on:
Blocks: minotaur 755385 755513
  Show dependency treegraph
 
Reported: 2012-05-25 10:55 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2012-06-14 10:22 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (8.83 KB, patch)
2012-05-28 09:20 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Marionette patch (1.19 KB, patch)
2012-05-28 09:24 PDT, Panos Astithas [:past]
jgriffin: review+
Details | Diff | Splinter Review
Fennec patch (3.65 KB, patch)
2012-05-28 09:26 PDT, Panos Astithas [:past]
mark.finkle: review+
Details | Diff | Splinter Review
B2G patch (1.98 KB, patch)
2012-05-28 09:30 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Fennec patch v2 (3.40 KB, patch)
2012-05-31 08:32 PDT, Panos Astithas [:past]
past: review+
Details | Diff | Splinter Review
B2G patch v2 (1017 bytes, patch)
2012-05-31 08:42 PDT, Panos Astithas [:past]
21: review+
Details | Diff | Splinter Review
Patch v2 (17.38 KB, patch)
2012-05-31 09:58 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v3 (17.39 KB, patch)
2012-05-31 23:00 PDT, Panos Astithas [:past]
rcampbell: review+
Details | Diff | Splinter Review
Fennec patch v3 (3.44 KB, patch)
2012-05-31 23:04 PDT, Panos Astithas [:past]
past: review+
Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2012-05-25 10:55:23 PDT
As a security measure, we need to allow the user of a browser with the devtools.debugger.remote-enabled to accept or reject an incoming remote connection.

A third option to disable the remote-enabled pref would be nice to have as well.
Comment 1 Jonathan Griffin (:jgriffin) 2012-05-25 11:03:17 PDT
Will this affect Marionette?  If so, we'd need some way for test automation to allow the connection automatically.
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-25 12:49:57 PDT
The summary mentions a dialog, which on mobile usually means nsIPromptService. This would work, but if we need something more custom, I'd suggest using a notification and then adding some code to Fennec to display a doorhanger.
Comment 3 Panos Astithas [:past] 2012-05-28 09:20:56 PDT
Created attachment 627730 [details] [diff] [review]
Patch v1

This appears to work in my tests so far with desktop Firefox and desktop B2G and has the extra button to disable remote debugging. I'm using nsIPromptService at mfinkle's suggestion to get something working for all products. We can later file a followup for making a prettier prompt for each product. The easier way to check out the dialog, is probably to select "Browser Debugger", since it also establishes a remote debugging connection automatically.
Comment 4 Panos Astithas [:past] 2012-05-28 09:24:01 PDT
Created attachment 627733 [details] [diff] [review]
Marionette patch

This will make Marionette work like nothing has changed.
Comment 5 Panos Astithas [:past] 2012-05-28 09:26:39 PDT
Created attachment 627734 [details] [diff] [review]
Fennec patch

This should presumably work for Fennec. I haven't actually tested it, but I'll try to get my first Fennec build tomorrow.
Comment 6 Panos Astithas [:past] 2012-05-28 09:30:09 PDT
Created attachment 627736 [details] [diff] [review]
B2G patch

This appears to work on desktop B2G, but I haven't tested it on my Nexus S, yet. I'm not requesting a formal review at this point, because I haven't figured out how to properly localize the messages. Vivien, any pointers?
Comment 7 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-05-28 09:32:32 PDT
(In reply to Panos Astithas [:past] from comment #6)
> Created attachment 627736 [details] [diff] [review]
> B2G patch
> 
> This appears to work on desktop B2G, but I haven't tested it on my Nexus S,
> yet. I'm not requesting a formal review at this point, because I haven't
> figured out how to properly localize the messages. Vivien, any pointers?

There is no XUL UI in B2G/Gaia so instead of using the internal prompter you want to exchange some message with the main application in order to retrieve a window. All the UI look and feel and l10n localization will be done in Gaia. Does that help?
Comment 8 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-05-28 09:33:34 PDT
(In reply to Vivien Nicolas (:vingtetun) from comment #7)
> (In reply to Panos Astithas [:past] from comment #6)
> > Created attachment 627736 [details] [diff] [review]
> > B2G patch
> > 
> > This appears to work on desktop B2G, but I haven't tested it on my Nexus S,
> > yet. I'm not requesting a formal review at this point, because I haven't
> > figured out how to properly localize the messages. Vivien, any pointers?
> 
> There is no XUL UI in B2G/Gaia so instead of using the internal prompter you
> want to exchange some message with the main application in order to retrieve
> a window. All the UI look and feel and l10n localization will be done in
> Gaia. Does that help?

And in order to exchange some informations between the chrome and the content you may want to use mozChromeEvent/mozContentEvent. This is a temporary 'API' until there is something more robust/well designed/etc...
Comment 9 Rob Campbell [:rc] (:robcee) 2012-05-29 06:16:19 PDT
Comment on attachment 627730 [details] [diff] [review]
Patch v1

Do we want to set any of these buttons to be the default other than the OK button? Are we happy with the OK button being default here considering some people are just going to to slap their enter key to dismiss a dialog.

   openListener: function DH_openListener(aPort, aLocalOnly) {
+    if (!canListen) {
+      return false;
+    }

not used to seeing variables without a this. in front of them. OK here.

What are you thinking about how to test this?
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-29 14:49:03 PDT
Comment on attachment 627734 [details] [diff] [review]
Fennec patch

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

>+  _allowConnection: function rd_allowConnection() {

>+    let result = prompt.confirmEx(null, title, msg, flags, null, null, btn,
>+                                  null, { value: false });

nit: We keep mobile a bit more relaxed for line breaks like this. feel free to keep it all on one line

>+    if (result == 0) {
>+      return true;
>+    }

nit: No {} needed for one-lines

>diff --git a/mobile/android/locales/en-US/chrome/browser.properties b/mobile/android/locales/en-US/chrome/browser.properties

>+# Debugger
>+# LOCALIZATION NOTE (remoteIncomingPromptTitle): The title displayed on the
>+# dialog that prompts the user to allow the incoming connection.
>+remoteIncomingPromptTitle=Incoming Connection
>+# LOCALIZATION NOTE (remoteIncomingPromptMessage): The message displayed on the
>+# dialog that prompts the user to allow the incoming connection.
>+remoteIncomingPromptMessage=An incoming request to permit remote debugging connection was detected. With a remote debugging connection, the remote client can take complete control over your browser! Allow connection?

UX might want to shorten this a bit since it will wrap quickly on mobile. Maybe:

An incoming request to permit remote debugging connection was detected. A remote client can take complete control over your browser! Allow connection?

>+# LOCALIZATION NOTE (remoteIncomingPromptDisable): The label displayed on the
>+# third button in the incoming connection dialog that lets the user disable the
>+# remote debugger server.
>+remoteIncomingPromptDisable=Disable

nit: LOCALIZATION NOTEs are useful for some strings, but I don't know if we need them for simple stuff like this. up to you.

r+, I think the modal-ness of the prompt on mobile is warranted.
Comment 11 Panos Astithas [:past] 2012-05-31 04:02:10 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #9)
> Comment on attachment 627730 [details] [diff] [review]
> Patch v1
> 
> Do we want to set any of these buttons to be the default other than the OK
> button? Are we happy with the OK button being default here considering some
> people are just going to to slap their enter key to dismiss a dialog.

I opted for less harassment, but certainly we need the experts' opinion on this. mgoodwin, curtisk, what do you think? Would 'Cancel' be a better default than 'OK'?
Comment 12 Panos Astithas [:past] 2012-05-31 08:32:02 PDT
Created attachment 628756 [details] [diff] [review]
Fennec patch v2

Addressed nits. I like the shortened message, I'll use it in desktop as well, thanks!
Comment 13 Panos Astithas [:past] 2012-05-31 08:42:20 PDT
Created attachment 628761 [details] [diff] [review]
B2G patch v2

Vivien, thanks for the detailed comments. I don't think I have the time to implement that right now, and since there is no imminent release for B2G that I know of, would it be OK with you to leave it as it is (i.e. accept all connections) for now, and file a followup to implement the prompt?
Comment 14 Panos Astithas [:past] 2012-05-31 09:58:55 PDT
Created attachment 628785 [details] [diff] [review]
Patch v2

(In reply to Rob Campbell [:rc] (:robcee) from comment #9)
> What are you thinking about how to test this?

I spent some time today trying to write a test and I don't think I can do it. I mean, since we don't provide a UI for enabling the server to listen for remote connections yet, all I can test is if the API works. That is, if the callback is called when a connection is made, which seems not very useful. I propose we write a decent test when we implement a UI that starts the debugger server without the debugger frontend, for the remote debugging scenario.
Comment 15 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-05-31 10:37:14 PDT
Comment on attachment 628761 [details] [diff] [review]
B2G patch v2

Sounds good. I will do a patch for the dialog myself if you don't have the time.
Comment 16 Tanvi Vyas [:tanvi] 2012-05-31 13:40:17 PDT
(In reply to Panos Astithas [:past] from comment #11)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #9)
> > Comment on attachment 627730 [details] [diff] [review]
> > Patch v1
> > 
> > Do we want to set any of these buttons to be the default other than the OK
> > button? Are we happy with the OK button being default here considering some
> > people are just going to to slap their enter key to dismiss a dialog.
> 
> I opted for less harassment, but certainly we need the experts' opinion on
> this. mgoodwin, curtisk, what do you think? Would 'Cancel' be a better
> default than 'OK'?

We talked about this in the Devtools meeting today.  The default should be "Cancel".  Users who want to enable it can hit okay.  This will prevent users who don't know what it is from just clicking through it.
Comment 17 Panos Astithas [:past] 2012-05-31 23:00:28 PDT
Created attachment 629064 [details] [diff] [review]
Patch v3

Now with 'Cancel' as the default.
Comment 18 Panos Astithas [:past] 2012-05-31 23:04:59 PDT
Created attachment 629069 [details] [diff] [review]
Fennec patch v3

Now with 'Cancel' as the default.
Comment 19 Panos Astithas [:past] 2012-05-31 23:08:20 PDT
(In reply to Vivien Nicolas (:vingtetun) from comment #15)
> Sounds good. I will do a patch for the dialog myself if you don't have the
> time.

Thanks, I will take you up on that!
Comment 20 Panos Astithas [:past] 2012-06-01 03:25:21 PDT
Try run:
https://tbpl.mozilla.org/?tree=Try&rev=3cd6acf8be12
Comment 21 Rob Campbell [:rc] (:robcee) 2012-06-01 07:43:49 PDT
Comment on attachment 629064 [details] [diff] [review]
Patch v3

yes. cancel should be default.

This looks great! Land'er.
Comment 24 Jonathan Griffin (:jgriffin) 2012-06-04 09:55:25 PDT
Somehow, this ended up breaking Marionette on B2G.  I'll investigate and file a separate bug.
Comment 25 Panos Astithas [:past] 2012-06-04 10:05:10 PDT
(In reply to Jonathan Griffin (:jgriffin) from comment #24)
> Somehow, this ended up breaking Marionette on B2G.  I'll investigate and
> file a separate bug.

Can you see if the patch in bug 761153 fixes it?
Comment 26 Panos Astithas [:past] 2012-06-14 00:02:50 PDT
*** Bug 764675 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.