Closed Bug 1356229 Opened 7 years ago Closed 7 years ago

Remove marionette.forcelocal pref

Categories

(Remote Protocol :: Marionette, enhancement)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ato, Assigned: ato)

Details

Attachments

(2 files)

Bug 1356229 - Remove marionette.forcelocal preference;
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1356229 - Prevent non-loopback connections in Marionette;
59 bytes, text/x-review-board-request
whimboo
: review+
Details 
As there is no current use case for marionette.forcelocal (it was used for B2G), we will remove it to reduce the potential attack surface for Marionette.
Group: mozilla-employee-confidential
Assignee: nobody → ato
Status: NEW → ASSIGNED
Group: mozilla-employee-confidential
Comment on attachment 8857925 [details]
Bug 1356229 - Remove marionette.forcelocal preference;

https://reviewboard.mozilla.org/r/129962/#review133772

::: commit-message-819a6:1
(Diff revision 1)
> +Bug 1356229 - Remove marionette.forcelocal pref; r?whimboo

nit: Please use `preference`, and replace the ';' with a dot.

::: testing/marionette/server.js:277
(Diff revision 1)
>     *     Listen only to connections from loopback if true (default).
>     *     When false, accept all connections.
>     */
>    constructor (port, forceLocal = true) {
>      this.port = port;
>      this.forceLocal = forceLocal;

We should also remove this property, or is there a reason why you left this behind?
Attachment #8857925 - Flags: review?(hskupin) → review-
Comment on attachment 8857925 [details]
Bug 1356229 - Remove marionette.forcelocal preference;

https://reviewboard.mozilla.org/r/129962/#review133772

> nit: Please use `preference`, and replace the ';' with a dot.

Updated to s/pref/preference/, but there isn’t any consensus about what character to separate the meta information in the commit message with.

The norm in all other source code revision repositories I know of is to append meta information at the bottom of the message such as in https://github.com/git/git/commit/882add136fa8319832ef373b8797ef58edb80efc, but Mozilla has adopted their own style which isn’t documented anywhere last I looked.  In any case, the use of a semicolon seems common enough amongst a number of Gecko developers (data point: https://hg.mozilla.org/mozilla-central/log?rev=%3B).

> We should also remove this property, or is there a reason why you left this behind?

Well I left it behind because I didn’t see the need to remove it.  I can see, admittedly a somewhat limited, use case to be able to start the server programmatically without the force-loopback-device restriction.  Since it doesn’t do any harm to keep this in, I left it in, but I can be persuaded that we shouldn’t leave in code that isn’t being used.
Comment on attachment 8857925 [details]
Bug 1356229 - Remove marionette.forcelocal preference;

https://reviewboard.mozilla.org/r/129962/#review133772

> Well I left it behind because I didn’t see the need to remove it.  I can see, admittedly a somewhat limited, use case to be able to start the server programmatically without the force-loopback-device restriction.  Since it doesn’t do any harm to keep this in, I left it in, but I can be persuaded that we shouldn’t leave in code that isn’t being used.

Yeah, dead code is always bad. We have source control to bring it back whenever there would be a need for that. But given by our meeting last week no-one of use has seen a reason why remote connections should be allowed. So based on that when should we ever have the need for that, or even allow it.
Comment on attachment 8857925 [details]
Bug 1356229 - Remove marionette.forcelocal preference;

https://reviewboard.mozilla.org/r/129962/#review134226
Attachment #8857925 - Flags: review?(hskupin)
Comment on attachment 8857925 [details]
Bug 1356229 - Remove marionette.forcelocal preference;

https://reviewboard.mozilla.org/r/129962/#review133772

> Yeah, dead code is always bad. We have source control to bring it back whenever there would be a need for that. But given by our meeting last week no-one of use has seen a reason why remote connections should be allowed. So based on that when should we ever have the need for that, or even allow it.

Removed it in the next commit: https://reviewboard.mozilla.org/r/131590/diff/#index_header
Comment on attachment 8859593 [details]
Bug 1356229 - Prevent non-loopback connections in Marionette;

https://reviewboard.mozilla.org/r/131590/#review134942

::: testing/marionette/server.js:27
(Diff revision 1)
>  // Bug 1083711: Load transport.js as an SDK module instead of subscript
>  loader.loadSubScript("resource://devtools/shared/transport/transport.js");
>  
>  const logger = Log.repository.getLogger("Marionette");
>  
> +const {KeepWhenOffline, LoopbackOnly} = Ci.nsIServerSocket;

Personally I prefer to keep the reference to an external class/instance to make it easier to search for specific usages of variables. Also with this change it looks like that both variables are owned and managed by server.js.

Also no other JS code in the tree (as far as I can see) uses those shortcuts.

I would prefer if you could leave it as it was, but it's not a blocker for landing.

::: testing/marionette/server.js:323
(Diff revision 1)
>          }
>        }
>      }
>  
> -    let flags = Ci.nsIServerSocket.KeepWhenOffline;
> -    if (this.forceLocal) {
> +    const flags = KeepWhenOffline | LoopbackOnly;
> +    const backlog = 1;

Please add a comment that a value of `1` means that the queue is limited to a single item.
Attachment #8859593 - Flags: review?(hskupin) → review+
Comment on attachment 8857925 [details]
Bug 1356229 - Remove marionette.forcelocal preference;

https://reviewboard.mozilla.org/r/129962/#review134944

Interesting that the link to Treeherder in mozreview tells about 41 tests which failed, but when I click the link I only see 3 failing tests. I assume that might have been an old reference?

::: testing/marionette/prefs/marionette.js
(Diff revision 3)
>  
>  // Port to start Marionette server on.
>  pref("marionette.port", 2828);
>  
> -// Forces client connections to come from a loopback device.
> -pref("marionette.forcelocal", true);

Please remember the backward compatibility. What happens to our update tests when this pref does not exist?

As long as it doesn't break tests, I'm ok with it. But I would still lean more towards to mark it as deprecated similar to `marionette.enabled`.
Attachment #8857925 - Flags: review?(hskupin) → review+
Comment on attachment 8857925 [details]
Bug 1356229 - Remove marionette.forcelocal preference;

https://reviewboard.mozilla.org/r/129962/#review134944

That overview has been broken for quite some time.  I’ve complained to mdoglio about it, but he’s since left the project.  From what he has told me, the bridge between Treeherder and mozreview is… interesting.

> Please remember the backward compatibility. What happens to our update tests when this pref does not exist?
> 
> As long as it doesn't break tests, I'm ok with it. But I would still lean more towards to mark it as deprecated similar to `marionette.enabled`.

Preferences that don’t exist are silently ignored.  I also haven’t found any evidence of internal consumers of the marionette.forcelocal preference.
Comment on attachment 8859593 [details]
Bug 1356229 - Prevent non-loopback connections in Marionette;

https://reviewboard.mozilla.org/r/131590/#review134942

> Personally I prefer to keep the reference to an external class/instance to make it easier to search for specific usages of variables. Also with this change it looks like that both variables are owned and managed by server.js.
> 
> Also no other JS code in the tree (as far as I can see) uses those shortcuts.
> 
> I would prefer if you could leave it as it was, but it's not a blocker for landing.

There are plentiful examples in mozilla-central of shortening code lines by assigning constants to local variables.  In fact, this is done constantly with Constants and modules imported with Components.utils.import.

> Please add a comment that a value of `1` means that the queue is limited to a single item.

I can’t tell if you’re serious or not about the need to add a comment to explain that "backlog = 1" means the backlog is one?

I removed the magic number from the argument list and assigned it to a variable explicitly so that it would carry more meaning.
Comment on attachment 8857925 [details]
Bug 1356229 - Remove marionette.forcelocal preference;

https://reviewboard.mozilla.org/r/129962/#review134944

> Preferences that don’t exist are silently ignored.  I also haven’t found any evidence of internal consumers of the marionette.forcelocal preference.

But it means we would initialize it with false? And as such we do not force local connections, but allow connections from everywhere.
Comment on attachment 8857925 [details]
Bug 1356229 - Remove marionette.forcelocal preference;

https://reviewboard.mozilla.org/r/129962/#review134944

> But it means we would initialize it with false? And as such we do not force local connections, but allow connections from everywhere.

The update tests will interact with certain Firefox versions that for some time into the future will have a marionette.forcelocal option.  Starting with the Nightly produced after this patch is integrated, there will no longer be such an option.  marionette.forcelocal is not in use, and the tests don’t rely on this functionality.  They will continue to work when run against Firefox versions with this patch.

I don’t understand what you mean by whether we will initialise this to false?  After this patch we will not initialise anything related to marionette.forcelocal.  If the pref is written to the profile, it will be ignored because Marionette isn’t looking for it.  I can’t see how it will open us up to allow connections from everywhere by _removing the pref_.
Comment on attachment 8857925 [details]
Bug 1356229 - Remove marionette.forcelocal preference;

https://reviewboard.mozilla.org/r/129962/#review134944

> The update tests will interact with certain Firefox versions that for some time into the future will have a marionette.forcelocal option.  Starting with the Nightly produced after this patch is integrated, there will no longer be such an option.  marionette.forcelocal is not in use, and the tests don’t rely on this functionality.  They will continue to work when run against Firefox versions with this patch.
> 
> I don’t understand what you mean by whether we will initialise this to false?  After this patch we will not initialise anything related to marionette.forcelocal.  If the pref is written to the profile, it will be ignored because Marionette isn’t looking for it.  I can’t see how it will open us up to allow connections from everywhere by _removing the pref_.

I was under the impression that we also set this preference in geckoinstance.py. But checking all the older branches up to esr52 now showed me that this is not done. So it means you are right, and we can safely land this patch.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d201ceaff82
Remove marionette.forcelocal preference; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/0625a195fcf5
Prevent non-loopback connections in Marionette; r=whimboo
https://hg.mozilla.org/mozilla-central/rev/6d201ceaff82
https://hg.mozilla.org/mozilla-central/rev/0625a195fcf5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: