Closed Bug 712643 Opened 8 years ago Closed 8 years ago

land Marionette on m-c

Categories

(Testing :: Marionette, defect)

x86
All
defect
Not set

Tracking

(firefox27 fixed, firefox28 fixed, firefox29 fixed, firefox30 fixed, firefox-esr24 fixed, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.4 fixed)

RESOLVED FIXED
mozilla14
Tracking Status
firefox27 --- fixed
firefox28 --- fixed
firefox29 --- fixed
firefox30 --- fixed
firefox-esr24 --- fixed
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: mdas, Assigned: mdas)

References

Details

(Keywords: addon-compat, Whiteboard: [secr:gkw])

Attachments

(1 file, 8 obsolete files)

103.37 KB, patch
mdas
: review+
rcampbell
: review+
Details | Diff | Splinter Review
Marionette has to land on m-c, but depends on the remote-debugger bits, and still needs sec review.
We landed marionette temporarily on our b2g fork.  Would like to un-diverge asap :).
Marionette patch queue is here: http://hg.mozilla.org/users/mdas_mozilla.com/marionette-pq

This can be applied directly on top of m-c, using the 'series' file.
> Sec-review completed:
> https://wiki.mozilla.org/Security/Reviews/Marionette

As mentioned in the security review, two things to take note of:

1. We should notify AMO folks to look out for the build pref (or about:config pref) that turns on Marionette, in add-ons hosted on AMO.

2. We should hide this about:config preference by default.
This is the first run of the patch. It will be updated very soon, since all element handling will be moved to another file and chrome support of findElement() will be added.
OS: Mac OS X → All
(In reply to Gary Kwong [:gkw, :nth10sd] from comment #4)
> > Sec-review completed:
> > https://wiki.mozilla.org/Security/Reviews/Marionette
> 
> As mentioned in the security review, two things to take note of:
> 
> 1. We should notify AMO folks to look out for the build pref (or
> about:config pref) that turns on Marionette, in add-ons hosted on AMO.

We will add the 'addon-compat' keyword before we land in aurora and ask to check for the loading of the component.

> 
> 2. We should hide this about:config preference by default.

We're changing this so that we launch from a default preference. We can lock the preference on startup, and if the default pref exists and is set to enabled, we will launch start the server. Otherwise, the server will not start. In either case, the prefs will be locked when we read them, which means we will only use the value set in prefs.js, and will ignore any value set at runtime (by either a user or an addon).  (see: https://developer.mozilla.org/en/nsIPrefBranch#lockPref%28%29).

This way, regardless of crashes, addons, etc. the only way the user can enable marionette is by setting the pref in prefs.js, since default prefs cannot be written by scripts, and the pref is locked.
This is the current working version of Marionette. We'll have more functions being added (https://wiki.mozilla.org/Auto-tools/Projects/Marionette/Marionette_Client_API#HTMLElement_Methods), but the structure of the code is more or less complete.

So the marionette component is added via a build flag, and is enabled via a default pref, which we lock upon startup. 

Unfortunately, as Mossop pointed out, it is possible for add-ons to unlock the preference and set or create the default pref on the fly. During security discussions, the adding of a listener to the preference and listening for changes was proposed, but we didn't follow through with it. How about using that method to prevent unwanted changes to the preference?
Attachment #597488 - Attachment is obsolete: true
Attachment #601354 - Flags: review?(dtownsend+bugmail)
Attachment #601354 - Flags: feedback?(gary)
Gah. Gary, I tagged you for feedback regarding the Comment 8, not the patch itself.
I'll be doing a super-review pass here only, sadly this product doesn't have the flags available. Has this already been code reviewed by anyone?
(In reply to Dave Townsend (:Mossop) from comment #10)
> I'll be doing a super-review pass here only, sadly this product doesn't have
> the flags available. Has this already been code reviewed by anyone?

Not yet reviewed. Do you have any suggestions for a reviewer? We have one person (cjones) interested in looking at part of the code, but not its entirety.
Rob is going to wrangle up a code reviewer for this
Comment on attachment 601354 [details] [diff] [review]
Updated patch for marionette on m-c

taking this review.
Attachment #601354 - Flags: review?(dtownsend+bugmail) → review?(rcampbell)
Attachment #601354 - Flags: review?(dtownsend+bugmail)
Comment on attachment 601354 [details] [diff] [review]
Updated patch for marionette on m-c

> Unfortunately, as Mossop pointed out, it is possible for add-ons to unlock
> the preference and set or create the default pref on the fly. During
> security discussions, the adding of a listener to the preference and
> listening for changes was proposed, but we didn't follow through with it.
> How about using that method to prevent unwanted changes to the preference?

Sounds good to me. feedback+ based on this listener approach, in addition to the safeguard of default pref.
Attachment #601354 - Flags: feedback?(gary) → feedback+
Comment on attachment 601354 [details] [diff] [review]
Updated patch for marionette on m-c

These are just a few drive by comments that I noticed while skimming through the patch quickly... Feel free to ignore the nits and such.

>diff --git a/js/src/config/autoconf.mk.in b/js/src/config/autoconf.mk.in
>diff --git a/js/src/configure.in b/js/src/configure.in

I don't think you need to bother trying to keep these in sync (Marionnette isn't relevant to JS's configure).

>diff --git a/toolkit/devtools/marionette/Makefile.in b/toolkit/devtools/marionette/Makefile.in

Overall, this doesn't really seem like a "devtool" in the commonly-understood meaning of the term (i.e. something that the devtools team would work on, or a peer to the existing "debugger" sub-directory). I don't care too where it lives exactly, but I don't think toolkit/devtools is suitable. testing/marionnette perhaps?

>diff --git a/toolkit/devtools/marionette/components/Makefile.in b/toolkit/devtools/marionette/components/Makefile.in

>+MODULE = MarionetteComponents
>+XPIDL_MODULE = MarionetteComponents

No need to define these since you have no compiled code/IDL.

>diff --git a/toolkit/devtools/marionette/components/marionettemodule.js b/toolkit/devtools/marionette/components/marionettemodule.js

nit: it's a bit odd to use "module" in the name of an XPCOM component.

>+MarionetteModule.prototype = {

>+  observe: function mm_observe(aSubject, aTopic, aData) {

>+      case "profile-after-change":

Can't you do the "enabled" check here, and avoid adding additional listeners if false?

>+  init: function mm_init() {

>+        Services.prefs.lockPref('marionette.defaultPrefs.enabled');

I don't understand the purpose of this.

>+  uninit: function mm_uninit() {
>+    if (Services.prefs.getBoolPref('marionette.defaultPrefs.enabled')) {
>+      DebuggerServer.closeListener();

Seems like this should check a variable that indicates that the server was started (you could repurpose _loaded for that), rather than checking the pref.

>diff --git a/toolkit/devtools/marionette/marionette-actors.js b/toolkit/devtools/marionette/marionette-actors.js

>+loader.loadSubScript("resource:///modules/marionette-simpletest.js");
>+loader.loadSubScript("resource:///modules/marionette-log-obj.js");

It's somewhat confusing to put non-modules under modules/. Normally these would just be normal packaged chrome, and you'd refer to them using chrome:// URLs.

>+if (isB2G) {
>+  // prevent 'slow script' dialogs
>+  prefs.setIntPref("dom.max_script_run_time", 180);
>+}

This seems like something that would ideally be part of the test configuration (whatever script sets up the testing profile), rather than part of the test harness code.

>+function BrowserObj(win) {

>+  setEnvironment: function BO_setEnvironment(win) {

Rather than determining the application per-window, it seems like it would make more sense to hold the "current" app as global state, and base it on the values returned from Services.appinfo.ID.

>diff --git a/toolkit/devtools/marionette/marionette-elements.js b/toolkit/devtools/marionette/marionette-elements.js

Can you give a high-level overview of the purpose of this code? It seems to create a cache of DOM elements, which seems a bit scary from a memory leak point of view. It also seems to add an abstraction layer on top of normal DOM retrieval methods, and it's not clear to me why that abstraction is useful.

>+  inDocument: function EM_inDocument(element, win) {

element.ownerDocument == win.document?

>diff --git a/toolkit/devtools/marionette/marionette-log-obj.js b/toolkit/devtools/marionette/marionette-log-obj.js
>diff --git a/toolkit/devtools/marionette/marionette-logger.jsm b/toolkit/devtools/marionette/marionette-logger.jsm

These seem somewhat similar to log4moz.js. Maybe you could re-use that instead?
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #15)
> Comment on attachment 601354 [details] [diff] [review]
> >diff --git a/toolkit/devtools/marionette/Makefile.in b/toolkit/devtools/marionette/Makefile.in
> 
> Overall, this doesn't really seem like a "devtool" in the
> commonly-understood meaning of the term (i.e. something that the devtools
> team would work on, or a peer to the existing "debugger" sub-directory). I
> don't care too where it lives exactly, but I don't think toolkit/devtools is
> suitable. testing/marionnette perhaps?

I was thinking something along these lines as well.

mdas: Can you move this to testing? I can still review there.

[...]
> >diff --git a/toolkit/devtools/marionette/marionette-log-obj.js b/toolkit/devtools/marionette/marionette-log-obj.js
> >diff --git a/toolkit/devtools/marionette/marionette-logger.jsm b/toolkit/devtools/marionette/marionette-logger.jsm
> 
> These seem somewhat similar to log4moz.js. Maybe you could re-use that
> instead?

Are we still using log4moz.js and is it supported? That came in with sync, did it not?

Thanks for the driveby!

(also setting bug to assigned)
Status: NEW → ASSIGNED
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #15)
> Comment on attachment 601354 [details] [diff] [review]
> Updated patch for marionette on m-c
> 
> These are just a few drive by comments that I noticed while skimming through
> the patch quickly... Feel free to ignore the nits and such.
> 
> >diff --git a/js/src/config/autoconf.mk.in b/js/src/config/autoconf.mk.in
> >diff --git a/js/src/configure.in b/js/src/configure.in
> 
> I don't think you need to bother trying to keep these in sync (Marionnette
> isn't relevant to JS's configure).
> 

Thanks, this has been changed.

> >diff --git a/toolkit/devtools/marionette/Makefile.in b/toolkit/devtools/marionette/Makefile.in
> 
> Overall, this doesn't really seem like a "devtool" in the
> commonly-understood meaning of the term (i.e. something that the devtools
> team would work on, or a peer to the existing "debugger" sub-directory). I
> don't care too where it lives exactly, but I don't think toolkit/devtools is
> suitable. testing/marionnette perhaps?
> 

Moved to testing/marionette

> >diff --git a/toolkit/devtools/marionette/components/Makefile.in b/toolkit/devtools/marionette/components/Makefile.in
> 
> >+MODULE = MarionetteComponents
> >+XPIDL_MODULE = MarionetteComponents
> 
> No need to define these since you have no compiled code/IDL.
> 

Ah good point, thanks

> >diff --git a/toolkit/devtools/marionette/components/marionettemodule.js b/toolkit/devtools/marionette/components/marionettemodule.js
> 
> nit: it's a bit odd to use "module" in the name of an XPCOM component.
> 
> >+MarionetteModule.prototype = {
> 
> >+  observe: function mm_observe(aSubject, aTopic, aData) {
> 
> >+      case "profile-after-change":
> 
> Can't you do the "enabled" check here, and avoid adding additional listeners
> if false?
> 

Changed to MarionetteModule to MarionetteComponent, and we now conditionally set the listeners if enabled.

> >+  init: function mm_init() {
> 
> >+        Services.prefs.lockPref('marionette.defaultPrefs.enabled');
> 
> I don't understand the purpose of this.
> 

This will be changed in the next patch. We had thought that setting a default pref and locking it would prevent addons from setting the pref, but they can just unlock it. We will now add a listener to listen to changes on the pref, and will prevent any change in value.

> >+  uninit: function mm_uninit() {
> >+    if (Services.prefs.getBoolPref('marionette.defaultPrefs.enabled')) {
> >+      DebuggerServer.closeListener();
> 
> Seems like this should check a variable that indicates that the server was
> started (you could repurpose _loaded for that), rather than checking the
> pref.
> 

The check is no longer needed now that the uninit listener is added only if the server is enabled.

> >diff --git a/toolkit/devtools/marionette/marionette-actors.js b/toolkit/devtools/marionette/marionette-actors.js
> 
> >+loader.loadSubScript("resource:///modules/marionette-simpletest.js");
> >+loader.loadSubScript("resource:///modules/marionette-log-obj.js");
> 
> It's somewhat confusing to put non-modules under modules/. Normally these
> would just be normal packaged chrome, and you'd refer to them using
> chrome:// URLs.
> 

fixed.

> >+if (isB2G) {
> >+  // prevent 'slow script' dialogs
> >+  prefs.setIntPref("dom.max_script_run_time", 180);
> >+}
> 
> This seems like something that would ideally be part of the test
> configuration (whatever script sets up the testing profile), rather than
> part of the test harness code.

Good point, and we actually no longer need this as the bug was resolved.

> 
> >+function BrowserObj(win) {
> 
> >+  setEnvironment: function BO_setEnvironment(win) {
> 
> Rather than determining the application per-window, it seems like it would
> make more sense to hold the "current" app as global state, and base it on
> the values returned from Services.appinfo.ID.
> 

Agreed; updated.

> >diff --git a/toolkit/devtools/marionette/marionette-elements.js b/toolkit/devtools/marionette/marionette-elements.js
> 
> Can you give a high-level overview of the purpose of this code? It seems to
> create a cache of DOM elements, which seems a bit scary from a memory leak
> point of view. It also seems to add an abstraction layer on top of normal
> DOM retrieval methods, and it's not clear to me why that abstraction is
> useful.

In the next patch, we will change this cache to use Components.utils.getWeakReference so we don't have active references to the elements. Also, the reason why we add an abstraction layer is because we're implementing a selenium-like test system for users. So a client can ask our server to find an element whose id is 'myId' and can ask to find this element in either chrome or content space. The client doesn't know how we find the elements (whether we use getElementById, or XPATH, or a11y which we will use in the future, etc.); the client is not responsible for the implementation. Since we support both chrome and content searching, then we've put the shared code into this file, so both scopes can access these methods. 

> 
> >+  inDocument: function EM_inDocument(element, win) {
> 
> element.ownerDocument == win.document?
> 
> >diff --git a/toolkit/devtools/marionette/marionette-log-obj.js b/toolkit/devtools/marionette/marionette-log-obj.js
> >diff --git a/toolkit/devtools/marionette/marionette-logger.jsm b/toolkit/devtools/marionette/marionette-logger.jsm
> 
> These seem somewhat similar to log4moz.js. Maybe you could re-use that
> instead?

I'll read up more on this and update the bug with the findings. I wanted to put up a working patch ASAP. Thanks for the drive by ! :)
Attachment #601354 - Attachment is obsolete: true
Attachment #601354 - Flags: review?(rcampbell)
Attachment #601354 - Flags: review?(dtownsend+bugmail)
Attachment #602902 - Flags: review?(rcampbell)
Attachment #602902 - Flags: feedback?(gavin.sharp)
Attachment #602902 - Flags: review?(dtownsend+bugmail)
(In reply to Malini Das [:mdas] from comment #17)
> This will be changed in the next patch. We had thought that setting a
> default pref and locking it would prevent addons from setting the pref, but
> they can just unlock it. We will now add a listener to listen to changes on
> the pref, and will prevent any change in value.

I'm a little confused about what the goal here is - why do you need to worry about add-ons setting the pref?

Is this code going to be built/packaged in release builds?

> Components.utils.getWeakReference so we don't have active references to the
> elements. Also, the reason why we add an abstraction layer is because we're
> implementing a selenium-like test system for users. So a client can ask our
> server to find an element whose id is 'myId' and can ask to find this
> element in either chrome or content space. The client doesn't know how we
> find the elements (whether we use getElementById, or XPATH, or a11y which we
> will use in the future, etc.); the client is not responsible for the
> implementation. Since we support both chrome and content searching, then
> we've put the shared code into this file, so both scopes can access these
> methods.

Hmm, not sure I understand why this requires keeping a cache of elements. Can't you just do the queries in real-time against chrome/content documents, as appropriate?
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #18)
> (In reply to Malini Das [:mdas] from comment #17)
> > This will be changed in the next patch. We had thought that setting a
> > default pref and locking it would prevent addons from setting the pref, but
> > they can just unlock it. We will now add a listener to listen to changes on
> > the pref, and will prevent any change in value.
> 
> I'm a little confused about what the goal here is - why do you need to worry
> about add-ons setting the pref?
> 
> Is this code going to be built/packaged in release builds?

The MarionetteComponent will be packaged with release builds, but it can only be launched if the preference is set at startup.

> 
> > Components.utils.getWeakReference so we don't have active references to the
> > elements. Also, the reason why we add an abstraction layer is because we're
> > implementing a selenium-like test system for users. So a client can ask our
> > server to find an element whose id is 'myId' and can ask to find this
> > element in either chrome or content space. The client doesn't know how we
> > find the elements (whether we use getElementById, or XPATH, or a11y which we
> > will use in the future, etc.); the client is not responsible for the
> > implementation. Since we support both chrome and content searching, then
> > we've put the shared code into this file, so both scopes can access these
> > methods.
> 
> Hmm, not sure I understand why this requires keeping a cache of elements.
> Can't you just do the queries in real-time against chrome/content documents,
> as appropriate?

Ah I get your question. We're implementing the WebDriver spec (http://code.google.com/p/selenium/wiki/JsonWireProtocol), which sends back a reference id to the user when they ask us to locate an element. They then use this reference id to do any actions on the element (click, etc.). So the user can do:

my_link_el = marionette.find_element('tag name', 'a') #finds the first link in the document
my_link_el.click()

The my_link_el variable on the client side just holds the serve-assigned reference id, which gets mapped to the corresponding DOM element on the server side.
(In reply to Malini Das [:mdas] from comment #19)
> The MarionetteComponent will be packaged with release builds, but it can
> only be launched if the preference is set at startup.

Why is this necessary, out of curiosity? It seems like it would be much lower risk to only package/enable this code on our test machines. Apologies if I'm missing something fundamental that makes the answer to this obvious!

Either way, trying to protect against add-ons seems unnecessary - it seems unlikely that an add-on would somehow do this unintentionally, and no amount of effort will truly prevent people from doing it intentionally.
Whiteboard: [secr:gkw]
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #20)
> (In reply to Malini Das [:mdas] from comment #19)
> > The MarionetteComponent will be packaged with release builds, but it can
> > only be launched if the preference is set at startup.
> 
> Why is this necessary, out of curiosity? It seems like it would be much
> lower risk to only package/enable this code on our test machines. Apologies
> if I'm missing something fundamental that makes the answer to this obvious!

We can't package/enable this code on test machines per se; we'd have to instead decide which builds we did/didn't want it in (e.g., nightly, beta, release, unittest, debug, etc), and then we'd be losing the ability to run Marionette tests on those builds which were not selected, which would limit its value somewhat.

It was decided years ago (when --enable-tests was not present for release builds), that it didn't make sense to test one binary but release another, and we'd be departing from that principle if we end up not packaging Marionette in certain builds, but relying on it for tests in others.

> Either way, trying to protect against add-ons seems unnecessary - it seems
> unlikely that an add-on would somehow do this unintentionally, and no amount
> of effort will truly prevent people from doing it intentionally.

This is true.  This is the compromise we worked out during Marionette's security review.  I believe the idea was that Marionette could make exploits "easier" (although any add-on can do what Marionette does without Marionette), so we want to do everything we can to prevent it being enabled by third parties, but as you say, there is no bullet-proof way to completely prevent it if someone is determined to do it.
(In reply to Jonathan Griffin (:jgriffin) from comment #21)
> (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #20)
> > (In reply to Malini Das [:mdas] from comment #19)
> > > The MarionetteComponent will be packaged with release builds, but it can
> > > only be launched if the preference is set at startup.
> > 
> > Why is this necessary, out of curiosity? It seems like it would be much
> > lower risk to only package/enable this code on our test machines. Apologies
> > if I'm missing something fundamental that makes the answer to this obvious!
> 
> We can't package/enable this code on test machines per se; we'd have to
> instead decide which builds we did/didn't want it in (e.g., nightly, beta,
> release, unittest, debug, etc), and then we'd be losing the ability to run
> Marionette tests on those builds which were not selected, which would limit
> its value somewhat.
> 
> It was decided years ago (when --enable-tests was not present for release
> builds), that it didn't make sense to test one binary but release another,
> and we'd be departing from that principle if we end up not packaging
> Marionette in certain builds, but relying on it for tests in others.

I for one believe being able to test arbitrary builds both in production and in the wild is a very valuable thing to have.  As Marionette becomes the way to do testing, I would assume keep this capability on arbitrary builds.  If security is an issue, we should figure out a real fix rather than just disabling the whole component (IMHO).  Otherwise, we will lack the ability of reproducing real failures in the wild
At a higher level, this seems like there may also be a developer platform / product advantage to having this enabled by default, if I'm understanding the current web development landscape correctly.  Which is to say, webdriver support would be appear to be on a track to being the de facto way that people do automated functional testing of webapps.  Being able to run all the tests on a stock build (particularly of mobile firefox, since the alternative functional testing abilities there are fewer) just by hitting "reload" after each webapp change would seem to make Firefox a more comfortable environment for webapp TDD workflows with a lower barrier-to-entry.

That reasoning contains significant suppositions, so CCing dangoor for his better-informed thoughts...
> At a higher level, this seems like there may also be a developer platform /
> product advantage to having this enabled by default, if I'm understanding
> the current web development landscape correctly.

During the security review meeting, it was brought up that Marionette being disabled by default was an acceptable compromise given the benefit of packaging the tool together with release builds of Firefox, and the risks involved in which could result in control of user actions by the tool.

Turning it on by default might warrant more discussion.
(In reply to Gary Kwong [:gkw, :nth10sd] from comment #24)
> > At a higher level, this seems like there may also be a developer platform /
> > product advantage to having this enabled by default, if I'm understanding
> > the current web development landscape correctly.
> 
> During the security review meeting, it was brought up that Marionette being
> disabled by default was an acceptable compromise given the benefit of
> packaging the tool together with release builds of Firefox, and the risks
> involved in which could result in control of user actions by the tool.
> 
> Turning it on by default might warrant more discussion.

Right, having this on by default would definitely warrant more discussion, since it opens up a socket for anyone to connect to and control the gecko environment. Having the component get built into the release but not be enabled is much safer, since it wouldn't start this socket by default. This way, enabling it on a release with marionette built-in is just a matter of setting a pref and restarting the browser.
Comment on attachment 602902 [details] [diff] [review]
Updated patch re: Gavin's comments

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

This patch seems to be missing files. The component claims to implement nsIMarionette but that isn't defined in any idl I can see.
Attachment #602902 - Flags: review?(dtownsend+bugmail) → review-
Attached patch removed old code (obsolete) — Splinter Review
Ah, that's a remnant from an older version of the component, I believe. I updated the patch.
Attachment #602902 - Attachment is obsolete: true
Attachment #602902 - Flags: review?(rcampbell)
Attachment #602902 - Flags: feedback?(gavin.sharp)
Attachment #603511 - Flags: review?(rcampbell)
Attachment #603511 - Flags: review?(dtownsend+bugmail)
(In reply to Malini Das [:mdas] from comment #25)
> (In reply to Gary Kwong [:gkw, :nth10sd] from comment #24)
> > > At a higher level, this seems like there may also be a developer platform /
> > > product advantage to having this enabled by default, if I'm understanding
> > > the current web development landscape correctly.
> > 
> > During the security review meeting, it was brought up that Marionette being
> > disabled by default was an acceptable compromise given the benefit of
> > packaging the tool together with release builds of Firefox, and the risks
> > involved in which could result in control of user actions by the tool.
> > 
> > Turning it on by default might warrant more discussion.
> 
> Right, having this on by default would definitely warrant more discussion,
> since it opens up a socket for anyone to connect to and control the gecko
> environment. Having the component get built into the release but not be
> enabled is much safer, since it wouldn't start this socket by default. This
> way, enabling it on a release with marionette built-in is just a matter of
> setting a pref and restarting the browser.

The command line feature that we've got in progress provides a means to easily toggle prefs and to make the prefs that are relevant to web developers a lot more discoverable (about:config can be a bit daunting ;)
let's not override decisions made during the security review with this release. I think leaving it preffed off initially is the right move, especially with this so close to merge.

I plan on reviewing this today or tomorrow. Sorry for the delay.
Comment on attachment 603511 [details] [diff] [review]
removed old code

build stuff looks good. If I'm reading correctly: we do not build marionette unless it's explicitly enabled currently. Right?

marionettecomponent.js:

+const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

cute. :)

+
+const kMARIONETTE_CONTRACTID

is that a thing we do? k prefix?

+  observe: function mm_observe(aSubject, aTopic, aData) {

the signature mm_ is surprising. Why not mc_ ?

I do find the observer on the preference a little odd. Registering an observer on the pref, then reporting that something attempted to change the pref and resetting it to false feels like a pretty big hammer.

Could you not just watch for profile-after-change and check the pref once without the observer?

in testing/marionette/marionette-actors.js:

functions and methods in MarionetteRootActor and MarionetteDriverActor should be properly documented. Methods with parameters should have an @param javadoc line. Methods and functions that return something should have an @returns line.

I found myself looking for these in a couple of places because I wasn't quite sure what the parameters were and couldn't find any.

Not strictly necessary for /testing code, but it'd be helpful wherever possible.

lots of use of "var" to declare method variables. You should be using "let" in almost all cases. Prevent scope creep.

marionette-elements.js

+function ElementManager(notSupported) {

what's an ElementManager?

This file needs more documentation.

This is great stuff!

I'm not going to be completely sticky about the documentation. I'm curious to know if there's anything that prevents you from using log4js instead of the marionette logging stuff. That does seem to be duplication.

Also, curious to hear what others think of the current preference listener implementation. I think it's somewhat unnecessary.

r- until we clear those up.
Attachment #603511 - Flags: review?(rcampbell) → review-
so, I guess log4moz.js is in browser currently. That might be a bit of a blocker if this is intended to built on other components. I'm ok with leaving in the current marionette logger for now, but possibly worth filing a bug to move log4moz.js into toolkit in the future.

Also, reading back about the preferences stuff in the marionette component, in comment 15, gavin asked:

>+MarionetteModule.prototype = {

>+  observe: function mm_observe(aSubject, aTopic, aData) {

>+      case "profile-after-change":

You replied with:

> Changed to MarionetteModule to MarionetteComponent, and we now conditionally set the listeners if enabled.

Yet you still have the entries for pref change in the observe method. I think those can go now?
(In reply to Rob Campbell [:rc] (robcee) from comment #30)
> Comment on attachment 603511 [details] [diff] [review]
> removed old code
> 
> build stuff looks good. If I'm reading correctly: we do not build marionette
> unless it's explicitly enabled currently. Right?

We will include the component in the builds (with build flag --enable-marionette), but it is preffed off by default. It will only ever start if the 'marionette.defaultPrefs.enabled' pref is true.

> 
> marionettecomponent.js:
> 
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> 
> cute. :)
> 
> +
> +const kMARIONETTE_CONTRACTID
> 
> is that a thing we do? k prefix?

Ah, apparently it was from an MDN example, but we can remove it.

> 
> +  observe: function mm_observe(aSubject, aTopic, aData) {
> 
> the signature mm_ is surprising. Why not mc_ ?
> 

Remnant from renaming marionettecomponent from marionettemodule :) Changing this.

> I do find the observer on the preference a little odd. Registering an
> observer on the pref, then reporting that something attempted to change the
> pref and resetting it to false feels like a pretty big hammer.
> 
> Could you not just watch for profile-after-change and check the pref once
> without the observer?

This is to prevent any addon or script from changing the value of marionette.defaultPrefs.enabled during the gecko runtime. We check the value of this pref at startup to see if we should start marionette. From the secreview, we wanted the user to enable marionette via the pref.js file only and then start their gecko process, and to try to prevent any enabling of the preference during runtime, so this listener was added to make sure that if the pref is tampered with, it will default to false.

> 
> in testing/marionette/marionette-actors.js:
> 
> functions and methods in MarionetteRootActor and MarionetteDriverActor
> should be properly documented. Methods with parameters should have an @param
> javadoc line. Methods and functions that return something should have an
> @returns line.
> 
> I found myself looking for these in a couple of places because I wasn't
> quite sure what the parameters were and couldn't find any.
> 
> Not strictly necessary for /testing code, but it'd be helpful wherever
> possible.

I will update this shortly.

> 
> lots of use of "var" to declare method variables. You should be using "let"
> in almost all cases. Prevent scope creep.

ditto.

> 
> marionette-elements.js
> 
> +function ElementManager(notSupported) {
> 
> what's an ElementManager?
> 
> This file needs more documentation.

Will add documentation. The ElementManager manages dom references. As mentioned in comment #19, the WebDriver spec instructs the sending of element references between client/server, and these references get mapped to the DOM on the server side. The ElementManager keeps track of the references. By the way, I'll be updating this code to use getWeakReference so that it doesn't keep an active reference to each of the dom elements in its cache.

> 
> This is great stuff!
> 
> I'm not going to be completely sticky about the documentation. I'm curious
> to know if there's anything that prevents you from using log4js instead of
> the marionette logging stuff. That does seem to be duplication.
> 
> Also, curious to hear what others think of the current preference listener
> implementation. I think it's somewhat unnecessary.
> 
> r- until we clear those up.

Haven't had the chance to look at log4moz.js yet, will comment on it soon! Related patch will follow.
(In reply to Malini Das [:mdas] from comment #32)
> (In reply to Rob Campbell [:rc] (robcee) from comment #30)

> > I do find the observer on the preference a little odd. Registering an
> > observer on the pref, then reporting that something attempted to change the
> > pref and resetting it to false feels like a pretty big hammer.
> > 
> > Could you not just watch for profile-after-change and check the pref once
> > without the observer?
> 
> This is to prevent any addon or script from changing the value of
> marionette.defaultPrefs.enabled during the gecko runtime. We check the value
> of this pref at startup to see if we should start marionette. From the
> secreview, we wanted the user to enable marionette via the pref.js file only
> and then start their gecko process, and to try to prevent any enabling of
> the preference during runtime, so this listener was added to make sure that
> if the pref is tampered with, it will default to false.
> 

I should also add it's used to prevent the user from accidentally enabling marionette as well
(In reply to Malini Das [:mdas] from comment #33)
> (In reply to Malini Das [:mdas] from comment #32)
> > (In reply to Rob Campbell [:rc] (robcee) from comment #30)
> 
> > > I do find the observer on the preference a little odd. Registering an
> > > observer on the pref, then reporting that something attempted to change the
> > > pref and resetting it to false feels like a pretty big hammer.
> > > 
> > > Could you not just watch for profile-after-change and check the pref once
> > > without the observer?
> > 
> > This is to prevent any addon or script from changing the value of
> > marionette.defaultPrefs.enabled during the gecko runtime. We check the value
> > of this pref at startup to see if we should start marionette. From the
> > secreview, we wanted the user to enable marionette via the pref.js file only
> > and then start their gecko process, and to try to prevent any enabling of
> > the preference during runtime, so this listener was added to make sure that
> > if the pref is tampered with, it will default to false.
> > 
> 
> I should also add it's used to prevent the user from accidentally enabling
> marionette as well

yeah, and sorry to make you repeat all that, but I think you're missing the point. If you only listen to the profile_after_change notification, and never listen to the pref change, you don't have to worry about anything changing the pref during the lifetime of the app. It won't trigger anything.

Does that make sense or am I misunderstanding this?

Regarding ElementManager, that's what I figured it was doing. Just add that description to the file and you're good.

Are you going to replace the ElementManager with a WeakMap in a follow-up or is that something you want to do before the initial landing?

thanks!
(In reply to Rob Campbell [:rc] (robcee) from comment #34)
> (In reply to Malini Das [:mdas] from comment #33)
> > (In reply to Malini Das [:mdas] from comment #32)
> > > (In reply to Rob Campbell [:rc] (robcee) from comment #30)
> > 
> > > > I do find the observer on the preference a little odd. Registering an
> > > > observer on the pref, then reporting that something attempted to change the
> > > > pref and resetting it to false feels like a pretty big hammer.
> > > > 
> > > > Could you not just watch for profile-after-change and check the pref once
> > > > without the observer?
> > > 
> > > This is to prevent any addon or script from changing the value of
> > > marionette.defaultPrefs.enabled during the gecko runtime. We check the value
> > > of this pref at startup to see if we should start marionette. From the
> > > secreview, we wanted the user to enable marionette via the pref.js file only
> > > and then start their gecko process, and to try to prevent any enabling of
> > > the preference during runtime, so this listener was added to make sure that
> > > if the pref is tampered with, it will default to false.
> > > 
> > 
> > I should also add it's used to prevent the user from accidentally enabling
> > marionette as well
> 
> yeah, and sorry to make you repeat all that, but I think you're missing the
> point. If you only listen to the profile_after_change notification, and
> never listen to the pref change, you don't have to worry about anything
> changing the pref during the lifetime of the app. It won't trigger anything.
> 
> Does that make sense or am I misunderstanding this?
> 
> Regarding ElementManager, that's what I figured it was doing. Just add that
> description to the file and you're good.
> 
> Are you going to replace the ElementManager with a WeakMap in a follow-up or
> is that something you want to do before the initial landing?
> 
> thanks!

Ah, I see what you mean, but if we don't listen to the pref changing, then the pref can be changed somehow during runtime, and on next startup, the new pref value will be used, thereby starting marionette. We want to start the service only if the user explicitly puts it in their prefs.js file. It's a way to prevent them from accidentally turning the pref on or from anyone else modifying the pref at runtime.

I'd like to add the weakmap in a follow-up, but if I experience any major blockers, I don't see why we can't add it after the initial landing. It would only cause extra references when an active marionette connection refers to an element. Once the client disconnects, we allow the ElementManager to be GC'd.
(In reply to Malini Das [:mdas] from comment #35)
> (In reply to Rob Campbell [:rc] (robcee) from comment #34)
> > (In reply to Malini Das [:mdas] from comment #33)
> > > (In reply to Malini Das [:mdas] from comment #32)
> > > > (In reply to Rob Campbell [:rc] (robcee) from comment #30)
> > > 
> > > > > I do find the observer on the preference a little odd. Registering an
> > > > > observer on the pref, then reporting that something attempted to change the
> > > > > pref and resetting it to false feels like a pretty big hammer.
> > > > > 
> > > > > Could you not just watch for profile-after-change and check the pref once
> > > > > without the observer?
> > > > 
> > > > This is to prevent any addon or script from changing the value of
> > > > marionette.defaultPrefs.enabled during the gecko runtime. We check the value
> > > > of this pref at startup to see if we should start marionette. From the
> > > > secreview, we wanted the user to enable marionette via the pref.js file only
> > > > and then start their gecko process, and to try to prevent any enabling of
> > > > the preference during runtime, so this listener was added to make sure that
> > > > if the pref is tampered with, it will default to false.
> > > > 
> > > 
> > > I should also add it's used to prevent the user from accidentally enabling
> > > marionette as well
> > 
> > yeah, and sorry to make you repeat all that, but I think you're missing the
> > point. If you only listen to the profile_after_change notification, and
> > never listen to the pref change, you don't have to worry about anything
> > changing the pref during the lifetime of the app. It won't trigger anything.
> > 
> > Does that make sense or am I misunderstanding this?
> > 
> > Regarding ElementManager, that's what I figured it was doing. Just add that
> > description to the file and you're good.
> > 
> > Are you going to replace the ElementManager with a WeakMap in a follow-up or
> > is that something you want to do before the initial landing?
> > 
> > thanks!
> 
> Ah, I see what you mean, but if we don't listen to the pref changing, then
> the pref can be changed somehow during runtime, and on next startup, the new
> pref value will be used, thereby starting marionette. We want to start the
> service only if the user explicitly puts it in their prefs.js file. It's a
> way to prevent them from accidentally turning the pref on or from anyone
> else modifying the pref at runtime.

Oh I see what you did there. Thanks for the explanation. I get it now! :)

> I'd like to add the weakmap in a follow-up, but if I experience any major
> blockers, I don't see why we can't add it after the initial landing. It
> would only cause extra references when an active marionette connection
> refers to an element. Once the client disconnects, we allow the
> ElementManager to be GC'd.

Yeah, I'm fine with adding the weakmap support after the initial landing. Do you have a bug filed for the follow-up work yet?

I'd still like to see a little explanation of what the ElementManager does and how it'll be replaced by Bug XXXXXXX when that's ready in the comments.

I'll R+ with that.

thanks!
(In reply to Jeff Hammel [:jhammel] from comment #22)
> I for one believe being able to test arbitrary builds both in production and
> in the wild is a very valuable thing to have.  As Marionette becomes the way
> to do testing, I would assume keep this capability on arbitrary builds.  If
> security is an issue, we should figure out a real fix rather than just
> disabling the whole component (IMHO).  Otherwise, we will lack the ability
> of reproducing real failures in the wild

There is no "real fix", shipping marionnette to users is inherently risky. It seems to me like there's a disconnect here. I don't really understand which "in the wild" scenarios couldn't be handled by having marionnette packaged as an extension or drop-in component, rather than packaged in the shipped build.

For our test infrastructure specifically, we control the entire configuration, so we aren't tied to shipping something to users just because we want to use it for testing. We already build and run existing test frameworks in release builds but avoid shipping them - I don't really understand why marionnette should be any different.

Maybe we need to get together and sort out the confusion? It's possible I'm missing something important!
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #37)
> (In reply to Jeff Hammel [:jhammel] from comment #22)
> > I for one believe being able to test arbitrary builds both in production and
> > in the wild is a very valuable thing to have.  As Marionette becomes the way
> > to do testing, I would assume keep this capability on arbitrary builds.  If
> > security is an issue, we should figure out a real fix rather than just
> > disabling the whole component (IMHO).  Otherwise, we will lack the ability
> > of reproducing real failures in the wild
> 
> There is no "real fix", shipping marionnette to users is inherently risky.
> It seems to me like there's a disconnect here. I don't really understand
> which "in the wild" scenarios couldn't be handled by having marionnette
> packaged as an extension or drop-in component, rather than packaged in the
> shipped build.
> 
> For our test infrastructure specifically, we control the entire
> configuration, so we aren't tied to shipping something to users just because
> we want to use it for testing. We already build and run existing test
> frameworks in release builds but avoid shipping them - I don't really
> understand why marionnette should be any different.
> 
> Maybe we need to get together and sort out the confusion? It's possible I'm
> missing something important!

I have to agree with this, as far as I can see nothing in here wouldn't work equally well as an extension, perhaps even as a restartless extension. I'd much rather spend time helping make that a reality than doing SR for this feature which hopefully will go unused by the vast majority of our users.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #37)
> There is no "real fix", shipping marionnette to users is inherently risky.
> It seems to me like there's a disconnect here. I don't really understand
> which "in the wild" scenarios couldn't be handled by having marionnette
> packaged as an extension or drop-in component, rather than packaged in the
> shipped build.
> 
> For our test infrastructure specifically, we control the entire
> configuration, so we aren't tied to shipping something to users just because
> we want to use it for testing. We already build and run existing test
> frameworks in release builds but avoid shipping them - I don't really
> understand why marionnette should be any different.
> 
No, there is no physical reason why you can't do all of this in an extension. The reason we would rather build testing into the Gecko platform by default is as follows:
* Everything we build on the platform can be tested - b2g, web app runtimes, thunderbird, firefox, <insert next project here>
* Everything we build can be tested *the same way* using *the same interfaces*. We don't need to keep inventing and maintaining new test frameworks.
* By using the web driver protocol (aka Selenium's interface), we enable any web developer to write these tests, lowering the barrier to entry for potential devs transitioning from web development to core Mozilla development.
* We are free to break requirements around extension interfaces and 3rd party extension installation characteristics that our current testing infrastructure forces us to rely on. (This would have been a huge win if we had had this back in October when we started working on Native Fennec automation). This will free our hands to address future problems (like 3rd party extension installs).
* By having Marionette in Gecko, we can depend on it, and enhance it, and unify our existing test systems to have one driver, with one set of code and one set of interfaces we can all focus on maintaining, instead of the 5-7 systems (depending on how you count them) that we have to maintain currently.

Marionette is adding only a slight change in our attack surface to add test-ability into our platform. (pref'd off, only enabled by physically modifying prefs.js). Any extension can do what Marionette does. For example, MozRepl has been around for over five years and allowed you to do exactly what you can do with Marionette. Mozmill's JsBridge has been around for years, and allows you to do this same style of remote-automation. 

Installing an extension can ruin your life. Marionette really doesn't make that any better or any worse. If anything it gives us a central point to monitor for extensions that misbehave and we can add that to the heuristics that drive the AMO extension test system (which we plan to do), and any browser health heuristics we might choose to implement in the future.

I think you guys are over-rotating on this point. The security team was fine with this decision.
It seems like we're talking past each other here - "in Gecko" vs. "shipped to all of our users" are two largely orthogonal issues, and I'm not sure that the former is relevant at all.

The patch that's attached to this bug is just an XPCOM component - it's not really "in Gecko" in any real sense, and at least until bug 526333 is fixed, still requires every app (Firefox, b2g, etc.) to opt-in to it being shipped.

I don't really understand your points about the test protocol or the consolidation of test frameworks - everyone agrees that you should be able to develop and use Marionnette, so they don't seem relevant to the issue at hand (shipped to normal users or not).

Can we focus on the practical downsides, from your perspective, of having this code not be shipped to users directly? I.e. which specific use cases require that the code be distributed in the builds we ship to users.

I think there are assumptions being made on both sides here, and the only way we can get past that is to talk it out, and focus on specifics - happy to do it here, or elsewhere if it would be more efficient.
Attached patch changes as per robcee's comments (obsolete) — Splinter Review
(In reply to Rob Campbell [:rc] (robcee) from comment #36)

I've switched it to use log4moz.js instead, and updated the documentation in marionette-actors.js/marionette-elements.js. Thanks for your comments!
Attachment #605521 - Flags: review?(rcampbell)
Attachment #603511 - Attachment is obsolete: true
Attachment #603511 - Flags: review?(dtownsend+bugmail)
Howdy, is this discussion progressing towards a resolution?  Marionette is the last big item keeping the b2g fork of m-c alive; we all really want to see it landed asap.
Comment on attachment 605521 [details] [diff] [review]
changes as per robcee's comments

My only outstanding concerns are: Follow-up bug filed for the WeakMap replacement for ElementReference. Whether or not this is enabled by default for all products which seems to be the outstanding issue.

Patchwise, this looks good.
Attachment #605521 - Flags: review?(rcampbell) → review+
Attachment #605521 - Flags: review?(dtownsend+bugmail)
Attachment #605521 - Flags: review?(gavin.sharp)
(In reply to Rob Campbell [:rc] (robcee) from comment #43)
> Comment on attachment 605521 [details] [diff] [review]
> changes as per robcee's comments
> 
> My only outstanding concerns are: Follow-up bug filed for the WeakMap
> replacement for ElementReference. Whether or not this is enabled by default
> for all products which seems to be the outstanding issue.
> 
> Patchwise, this looks good.

Oh, I added the weakmap replacement in the patch, as an example, there's like 69 of marionette-elements.js. There are also changes in marionette-actors so it uses weakref when it can. Thanks for the review!
s/like/line.
oh cool. I missed that in the interdiff.
Attachment #605521 - Flags: review?(gavin.sharp)
Gavin, Jgriffin, Mdas and I met this morning and figured out a good compromise. We will go ahead with the current patch as-is, because the patch doesn't add marionette to the firefox package manifest. This means that marionette *won't* be shipped to firefox users right now, but we will be clear to land on m-c and that will unblock b2g right now.

We will address enabling marionette in Firefox once we start the process of using marionette to drive Firefox Unit Tests. (And we may be able to find a way to enable testing on release builds using marionette and still not ship marionette to users).

This unblocks our concerns for landing right now. And to be abundantly clear we won't be shipping marionette code to users of Firefox at this time, but it will be enabled in b2g by default.
(In reply to Rob Campbell [:rc] (robcee) from comment #46)
> oh cool. I missed that in the interdiff.

I found an intermittent test failure due to the weak reference changes. I rolled it back to the previous version (using live references) and made bug 736592 about it. The revised patch will follow. Only a few lines in marionette-actors.js, marionette-elements.js and marionette-listener.js have changed
Target Milestone: --- → mozilla13
Version: unspecified → Trunk
Here's the revised patch.
Attachment #605521 - Attachment is obsolete: true
Attachment #605521 - Flags: review?(dtownsend+bugmail)
Attachment #606721 - Flags: review?(rcampbell)
Forgot: Added a comment referring to the bug as per your request!
Attachment #606721 - Attachment is obsolete: true
Attachment #606721 - Flags: review?(rcampbell)
Attachment #606724 - Flags: review?(rcampbell)
Attachment #606724 - Flags: review?(dtownsend+bugmail)
Added some b2g specific code (in /b2g directory) that I neglected to add. This code was looked at before by members of the b2g team.

This is patch also includes the rollback of weakref map.
Attachment #606724 - Attachment is obsolete: true
Attachment #606724 - Flags: review?(rcampbell)
Attachment #606724 - Flags: review?(dtownsend+bugmail)
Attachment #607628 - Flags: review?(rcampbell)
Attachment #607628 - Flags: review?(dtownsend+bugmail)
Comment on attachment 607628 [details] [diff] [review]
Added b2g specific code on top of weakref removal

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

From an SR standpoint this seems basically fine. I dislike using log4moz from services but we're planning on moving that to toolkit so I don't think we need to hold up this for that.

::: testing/marionette/Makefile.in
@@ +14,5 @@
> +endif
> +
> +ifdef ENABLE_MARIONETTE
> +    DIRS += components
> +endif

I don't think you want to build the tests when ENABLE_MARIONETTE isn't set

::: testing/marionette/components/marionettecomponent.js
@@ +15,5 @@
> +}
> +
> +MarionetteComponent.prototype = {
> +  classDescription: "Marionette component",
> +  classID: MARIONETTE_CID,

I don't think we use classID anywhere anymore, can probably leave it out.

@@ +48,5 @@
> +        }
> +        break;
> +      case "nsPref:changed":
> +        Services.prefs.setBoolPref("marionette.defaultPrefs.enabled", false);
> +        logger.info("Something tried to change marionette.defaultPrefs.enabled; defaulting to false");

This pseudo-pref-locking seems pointless to me. Not enough that I care to r- because of it but I don't understand the necessity when we currently aren't shipping to Firefox users and I don't believe b2g users have any way to control the pref themselves.
Attachment #607628 - Flags: review?(dtownsend+bugmail) → review+
(In reply to Dave Townsend (:Mossop) from comment #52)
> This pseudo-pref-locking seems pointless to me. Not enough that I care to r-
> because of it but I don't understand the necessity when we currently aren't
> shipping to Firefox users and I don't believe b2g users have any way to
> control the pref themselves.

Thinking more about it, I'm not sure if this buys us anything and basically concur with :Mossop here.
Carrying forward previous r+

(In reply to Dave Townsend (:Mossop) from comment #52)
> Comment on attachment 607628 [details] [diff] [review]
> Added b2g specific code on top of weakref removal
> 
> Review of attachment 607628 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> From an SR standpoint this seems basically fine. I dislike using log4moz
> from services but we're planning on moving that to toolkit so I don't think
> we need to hold up this for that.
> 
> ::: testing/marionette/Makefile.in
> @@ +14,5 @@
> > +endif
> > +
> > +ifdef ENABLE_MARIONETTE
> > +    DIRS += components
> > +endif
> 
> I don't think you want to build the tests when ENABLE_MARIONETTE isn't set
> 

Good point, I added the tests if both ENABLE_MARIONETTE and ENABLE_TESTS is set

> ::: testing/marionette/components/marionettecomponent.js
> @@ +15,5 @@
> > +}
> > +
> > +MarionetteComponent.prototype = {
> > +  classDescription: "Marionette component",
> > +  classID: MARIONETTE_CID,
> 
> I don't think we use classID anywhere anymore, can probably leave it out.

We still need it to register the component (https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_2.0#JavaScript_components). It's used by the generateNSGetFactory call (http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/XPCOMUtils.jsm#182).

> 
> @@ +48,5 @@
> > +        }
> > +        break;
> > +      case "nsPref:changed":
> > +        Services.prefs.setBoolPref("marionette.defaultPrefs.enabled", false);
> > +        logger.info("Something tried to change marionette.defaultPrefs.enabled; defaulting to false");
> 
> This pseudo-pref-locking seems pointless to me. Not enough that I care to r-
> because of it but I don't understand the necessity when we currently aren't
> shipping to Firefox users and I don't believe b2g users have any way to
> control the pref themselves.

Yup, this was was brought up by you, gavin, jhammel and then discussed again by the Sec team (gkw and curtisk), and it was agreed that we can remove it. I updated the patch.
Attachment #607628 - Attachment is obsolete: true
Attachment #607628 - Flags: review?(rcampbell)
Attachment #607991 - Flags: review+
Attachment #607991 - Flags: review?(rcampbell)
Comment on attachment 607628 [details] [diff] [review]
Added b2g specific code on top of weakref removal

yeah, the pref's icky. But R+. <3
Attachment #607628 - Flags: review+
In order to get log4moz.js in b2g, we have modified confvars.sh from:

MOZ_SERVICES_SYNC=

to:

MOZ_SERVICES_SYNC=1

@cjones, are you OK with that change?  The plan is to move log4moz.js into toolkit from services, so eventually we could get rid of this.
Comment on attachment 607991 [details] [diff] [review]
Remove pref listener, update makefile as per Mossop's review

R+ for realz!
Attachment #607991 - Flags: review?(rcampbell) → review+
(In reply to Jonathan Griffin (:jgriffin) from comment #56)
> In order to get log4moz.js in b2g, we have modified confvars.sh from:
> 
> MOZ_SERVICES_SYNC=
> 
> to:
> 
> MOZ_SERVICES_SYNC=1
> 
> @cjones, are you OK with that change?  The plan is to move log4moz.js into
> toolkit from services, so eventually we could get rid of this.

I spoke with cjones on IRC about this and he said it is fine
https://hg.mozilla.org/mozilla-central/rev/e5fc0c1f7adf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: mozilla13 → mozilla14
Depends on: 772310
Flags: sec-review+
blocking-fx: --- → ?
status-b2g18: --- → ?
status-b2g-v1.2: --- → ?
status-b2g-v1.3: --- → ?
status-b2g-v1.4: --- → ?
tracking-b2g18: --- → ?
relnote-firefox: --- → ?
relnote-b2g: --- → ?
Flags: sec-review?
Flags: sec-review+
Flags: sec-bounty?
Flags: needinfo?(jgriffin)
Flags: in-testsuite?
Flags: in-qa-testsuite?(jgriffin)
Flags: needinfo?(jgriffin)
glenp1972 requested a review for a security bounty on this bug, but that doesn't sound right.  should that request be removed?
clearing the bounty flag
Flags: sec-bounty?
Flags: sec-review? → sec-review+
You need to log in before you can comment on or make changes to this bug.