1181561 - Exposing a kill switch enabling/disabling API

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[:gerard-majax]

As of bug 1175620 and bug 1181505 we will need to do some job on Gecko to setup the device in kill switch mode. For example, locking recovery access, etc.

Let's wrap this in a small API that will be called by FMD app

Comment 1

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling r=...

Comment 2

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling r=...

Comment 3

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling r=...

Comment 4

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=887a1fef36bc

A first basic setup ...

Comment 5

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling r=...

Comment 6

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=133672aff87a

Comment 7

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c13d7c5f401f

Comment 8

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling r=...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46e6820e1241

Comment 9

[:gerard-majax]

Comment on attachment 8635167 [details] [diff] [review]
Expose a Kill Switch enabling/disabling r=...

Dave, that's a first version on the Gecko side. For now, it's only responsible for switching a setting value against which FMD on Gaia side will react.

Once I have more informations on what exactly Gecko should set (properties? files? partitions?) this will be aded here.

I have hacked inside mozPower because it's where we already have something close: factory reset.

Comment 10

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling r=...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5149d0d27f07

Comment 11

[:gerard-majax]

Cool ... B2G ICS Emulator failure :(

Comment 12

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling r=...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=affa87c90809

Comment 13

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling r=...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57c9cad8eca7

Comment 14

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling r=...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fa4226a48c9

Comment 15

[:gerard-majax]

So there's something wrong with b2g emulator, but I cannot reproduce it locally because running mochitest fails (bug 1146713).

Comment 16

[:gerard-majax]

Logs shows there is something wrong with the observers:

>  02:51:31     INFO -  -*- SettingsManager: Settings::receiveMessage: Settings:Change:Return:OK
>  02:51:31     INFO -  -*- SettingsManager: data:findmydevice.killswitch:true
>  02:51:31     INFO -  -*- SettingsManager: no observers stored!

Comment 17

[:gerard-majax]

My current understanding of the B2G ICS Emulator failure is the following:
 - my mochitest creates a setting from content, named findmydevice.killswitch with a value, let assume "true"
 - we read from content the setting value
 - we install an observer against findmydevice.killswitch
 - we call the API to disable the feature, which will set the setting to false
 - we timeout on the observer

The extra logging I have added shows that all of the logic runs fine. Calling from content instantiates a SettingsManager and we have a connection with SettingsRequestManager being established.

Now, when we do the call to the killswitch API that toggles the setting value to false, we are making use of SettingsSevice code. This code includes a Cu.import("SettingsRequestManager.jsm"). Extra logging added shows that on B2G ICS Emulator, this triggers a new SettingsRequestManager object to be created. Hence this new object has no 'this.children' filled and we fail to trigger the observers.

Comment 18

[:gerard-majax]

And the matching code ... https://hg.mozilla.org/try/rev/bd53b78f3446

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Would adding a guard to only initialize SettingsRequestManager once help? It looks like any time we include the SettingsRequestManager.jsm file, we recreate the manager no matter what. As it /should/ only exist in the chrome compartment of the parent process, I think only creating the manager when this.SettingsRequestManager == undefined might fix it?

Comment 20

[Gregor Wagner [:gwagner]]

Chatted about this with Blake today. In theory we should only get one instance. We also checked if the global sharing is off but we only disable on mulet and debug builds. I am running out of ideas here.

Comment 21

[:gerard-majax]

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #19)
> Would adding a guard to only initialize SettingsRequestManager once help? It
> looks like any time we include the SettingsRequestManager.jsm file, we
> recreate the manager no matter what. As it /should/ only exist in the chrome
> compartment of the parent process, I think only creating the manager when
> this.SettingsRequestManager == undefined might fix it?

So I resent a try for this purpose, with a change like: |!this._loaded| check before doing |this.SettingsRequestManager = SettingsRequestManager| and |this.SettingsRequestManager.init()|. After the init, I do set |this._loaded = true|

The outcome of this is that I still see SettingsRequestManager.jsm being loaded twice, and each time, the |this._loaded| value before setting it to |true| is |undefined|. So we have two different SettingsRequestManager?

Comment 22

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Wow. That is really weird. So is this /only/ happening on the emulator? And are we sure the SettingsRequestManager is getting created in the parent, or is there possibly one in the parent and one in the child (though the one in the child, in which case we should put in a process check?

Comment 23

[:gerard-majax]

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #22)
> Wow. That is really weird. So is this /only/ happening on the emulator? And
> are we sure the SettingsRequestManager is getting created in the parent, or
> is there possibly one in the parent and one in the child (though the one in
> the child, in which case we should put in a process check?

As far as I'm aware:
 - b2g/chrome/content/settings is in the parent,
 - I thought of this and added Is_MainThread() checks on the side using SettingsService, so it should also be on the parent side

Comment 24

[Dave Hylands [:dhylands]]

(In reply to Alexandre LISSY :gerard-majax from comment #23)
> (In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment
> #22)
> > Wow. That is really weird. So is this /only/ happening on the emulator? And
> > are we sure the SettingsRequestManager is getting created in the parent, or
> > is there possibly one in the parent and one in the child (though the one in
> > the child, in which case we should put in a process check?
> 
> As far as I'm aware:
>  - b2g/chrome/content/settings is in the parent,
>  - I thought of this and added Is_MainThread() checks on the side using
> SettingsService, so it should also be on the parent side

Just thought I'd mention that child processes have a main thread. In C++ to test if a process is the main process or a child process, the convention seems to be to use XRE_IsParentProcess().

Comment 25

[:gerard-majax]

I've added an assert with XTR_IsParentProcess() and this is crashing :).

Comment 26

[:gerard-majax]

Kyle, any idea why the NS_ERROR_NOT_INITIALIZED ?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=643b0236e6cd

Basically the Cu.import() of SettingsRequestManager.jsm from SettingsService is done from a child process, and not from the parent. This explains the new instanciation we saw. According to Gregor we should be supporting this.

In the mean time of having a proper way to trigger cross-process observers, I wanted to hack this:
 - make sure a non parent Cu.import() do not instanciate a new SettingsRequestManager
 - make the communication from SettingsService.js to SettingsRequestManager.jsm do not require a direct reference to SettingsRequestManager but also work with sending messages

This last point basically revives the code from before bug 1128489. Instead that we now have two code paths:
 - direct communication when we are in the parent process for both
 - indirect via messages when it is not the case

However, this is failing and I don't get why :(

Comment 27

[:gerard-majax]

Reworked and now all green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dcd50c6cfae

Comment 28

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

https://treeherder.mozilla.org/#/jobs?repo=try&revision=888620743a00

Comment 29

[Dave Hylands [:dhylands]]

Comment on attachment 8638687 [details] [diff] [review]
Expose a Kill Switch enabling/disabling

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

This looks ok to me, but I don't really condsider myself a JS reviewer, so I may have missed some subtleties.

::: b2g/components/KillSwitch.js
@@ +10,5 @@
> +  dump("-*- KillSwitch.js: " + s + "\n");
> +}
> +
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;

I've been seeing it recommended to code this as: const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

@@ +43,5 @@
> +    cpmm.sendAsyncMessage("KillSwitch:Enable");
> +  },
> +
> +  disable: function() {
> +    if (DEBUG) debug("KillSwitch: disable");

nit: I've been told to use this form instead:

DEBUG && debug("KillSwitch: disable");

::: b2g/components/KillSwitchMain.jsm
@@ +8,5 @@
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +const Cr = Components.results;

same comment as  above (for Components)

@@ +42,5 @@
> +  dump("--*-- KillSwitchMain: " + aStr + "\n");
> +}
> +
> +function debug(aStr) {
> +  // dump("--*-- KillSwitchMain: " + aStr + "\n");

This should use const DEBUG = false and DEBUG && like the previous file.

@@ +113,5 @@
> +	if (!this[method]) {
> +          debug("!!! UNABLE TO FIND METHOD FOR: " + aMessage.name);
> +	} else {
> +          this[method]();
> +	}

Any particular reason for doing it this way rather than just having 2 case statements, one that calls doEnable and one that calls doDisable?

Comment 30

[:gerard-majax]

(In reply to Dave Hylands [:dhylands] from comment #29)
> Comment on attachment 8638687 [details] [diff] [review]
> Expose a Kill Switch enabling/disabling
> 
> Review of attachment 8638687 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks ok to me, but I don't really condsider myself a JS reviewer, so I
> may have missed some subtleties.

No problem. If you'd like that I also ask another reviewer to have a look, it's fine :)

> 
> ::: b2g/components/KillSwitch.js
> @@ +10,5 @@
> > +  dump("-*- KillSwitch.js: " + s + "\n");
> > +}
> > +
> > +const Ci = Components.interfaces;
> > +const Cu = Components.utils;
> 
> I've been seeing it recommended to code this as: const {classes: Cc,
> interfaces: Ci, utils: Cu, results: Cr} = Components;

Sure.

> 
> @@ +43,5 @@
> > +    cpmm.sendAsyncMessage("KillSwitch:Enable");
> > +  },
> > +
> > +  disable: function() {
> > +    if (DEBUG) debug("KillSwitch: disable");
> 
> nit: I've been told to use this form instead:
> 
> DEBUG && debug("KillSwitch: disable");

Ok

> 
> ::: b2g/components/KillSwitchMain.jsm
> @@ +8,5 @@
> > +
> > +const Cc = Components.classes;
> > +const Ci = Components.interfaces;
> > +const Cu = Components.utils;
> > +const Cr = Components.results;
> 
> same comment as  above (for Components)

Ditto

> 
> @@ +42,5 @@
> > +  dump("--*-- KillSwitchMain: " + aStr + "\n");
> > +}
> > +
> > +function debug(aStr) {
> > +  // dump("--*-- KillSwitchMain: " + aStr + "\n");
> 
> This should use const DEBUG = false and DEBUG && like the previous file.

Ditto

> 
> @@ +113,5 @@
> > +	if (!this[method]) {
> > +          debug("!!! UNABLE TO FIND METHOD FOR: " + aMessage.name);
> > +	} else {
> > +          this[method]();
> > +	}
> 
> Any particular reason for doing it this way rather than just having 2 case
> statements, one that calls doEnable and one that calls doDisable?

Nope. Do you think it would be more readable?

Comment 31

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be9a4bca3d50

Comment 32

[Dave Hylands [:dhylands]]

(In reply to Alexandre LISSY :gerard-majax from comment #30)
> (In reply to Dave Hylands [:dhylands] from comment #29)
> > Comment on attachment 8638687 [details] [diff] [review]
> > Expose a Kill Switch enabling/disabling
> > 
> > Review of attachment 8638687 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks ok to me, but I don't really condsider myself a JS reviewer, so I
> > may have missed some subtleties.
> 
> No problem. If you'd like that I also ask another reviewer to have a look,
> it's fine :)

I probably would have punted to fabrice, but his profile says he's not reading bugmail. If you know someone who is more familiar with the JS stuff, it probably wouldn't be bad to get them to do a review.

> > @@ +113,5 @@
> > > +	if (!this[method]) {
> > > +          debug("!!! UNABLE TO FIND METHOD FOR: " + aMessage.name);
> > > +	} else {
> > > +          this[method]();
> > > +	}
> > 
> > Any particular reason for doing it this way rather than just having 2 case
> > statements, one that calls doEnable and one that calls doDisable?
> 
> Nope. Do you think it would be more readable?

I think the new vesion (on treeHerder) is much more readable, and should actually be smaller, and faster :)

Comment 33

[:gerard-majax]

Comment on attachment 8638896 [details] [diff] [review]
Expose a Kill Switch enabling/disabling

Paul, can you have a look at this from your security perspective? Thanks!

Comment 34

[:gerard-majax]

Comment on attachment 8638896 [details] [diff] [review]
Expose a Kill Switch enabling/disabling

Fernando, can you also give a look at this to complete Dave's review? Thanks!

Comment 35

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 8638896 [details] [diff] [review]
Expose a Kill Switch enabling/disabling

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

Added a couple comments, but I need to read the rest of the bug comments first since I don't understand what this API is intended to do (afaict it just sets a setting but I think im missing something).

::: b2g/components/KillSwitchMain.jsm
@@ +101,5 @@
> +    }
> +  },
> +
> +  // addMessageListener
> +  receiveMessage: function(aMessage) {

There is no check here to verify that the message came from a certified app? IIRC AvailableIn="CertifiedApps" only creates a check in the child process. If a child process is compromised, I _think_ it could then send the KillSwitch:Enable message to the parent. What would be the consequence of this? Do we need a check here that the message came from a certified app (or even better from an app with a certain permission?)

::: dom/webidl/KillSwitch.webidl
@@ +9,5 @@
> +  */
> +
> +[JSImplementation="@mozilla.org/mozKillSwitch;1",
> + NavigatorProperty="mozKillSwitch",
> + AvailableIn="CertifiedApps"]

In the future, we plan to remove certified apps, so it might be better to better to use a permission here (one that is certified for now). Happy for this to be a separate bug though that is completed in the future.

Comment 36

[Paul Theriault [:pauljt] (no longer reading bugmail)]

So the main risk with something like a "Killswitch" is the risk of DoS due to unauthorised usage. Currently IIUC this API just sets a setting, which FMD observes, and then does something (currently TDB) to "kill" the device. Is that correct?

If so, this seems to me to be a dangerous design: any app with settings access could just set the "findmydevice.killswitch" setting directly? Technically only certified apps have settings API access, but if you are going to go to the trouble of making a separate killswitch API then this API should be the ONLY way to access this funcionality. Maybe we have something like navigator.mozKillSwitch.onchange event, and store the state inside KillSwitchMain.jsm instead of as a setting? 

Ultimately though, it really depends on what we do to KILL a device. If "kill" just means change some gaia Settings so that the device can't be unlocked, we are back to the security of the settings APIs, and there isn't really point of making this API more secure (Do we even need a "killSwitch API at this point? I'm not saying we don't, just unclear of the benefit ?)

Comment 37

[:gerard-majax]

(In reply to Paul Theriault [:pauljt] from comment #35)
> Comment on attachment 8638896 [details] [diff] [review]
> Expose a Kill Switch enabling/disabling
> 
> Review of attachment 8638896 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Added a couple comments, but I need to read the rest of the bug comments
> first since I don't understand what this API is intended to do (afaict it
> just sets a setting but I think im missing something).

It just sets a setting for now but:
 - it will/should set other stuff (android property, files, etc.) to communicate with bootloader/recovery
 - findmydevice app will handle the setting change for gaia-level

> 
> ::: b2g/components/KillSwitchMain.jsm
> @@ +101,5 @@
> > +    }
> > +  },
> > +
> > +  // addMessageListener
> > +  receiveMessage: function(aMessage) {
> 
> There is no check here to verify that the message came from a certified app?
> IIRC AvailableIn="CertifiedApps" only creates a check in the child process.
> If a child process is compromised, I _think_ it could then send the
> KillSwitch:Enable message to the parent. What would be the consequence of
> this? Do we need a check here that the message came from a certified app (or
> even better from an app with a certain permission?)

I think that's a valid concern. We can probably:
 - add a specific permission
 - check here that the callee is from the expected app AND with the permission ?

> 
> ::: dom/webidl/KillSwitch.webidl
> @@ +9,5 @@
> > +  */
> > +
> > +[JSImplementation="@mozilla.org/mozKillSwitch;1",
> > + NavigatorProperty="mozKillSwitch",
> > + AvailableIn="CertifiedApps"]
> 
> In the future, we plan to remove certified apps, so it might be better to
> better to use a permission here (one that is certified for now). Happy for
> this to be a separate bug though that is completed in the future.

Well better to land something future proof now than refactor later ... :)

Comment 38

[:gerard-majax]

(In reply to Paul Theriault [:pauljt] from comment #36)
> So the main risk with something like a "Killswitch" is the risk of DoS due
> to unauthorised usage. Currently IIUC this API just sets a setting, which
> FMD observes, and then does something (currently TDB) to "kill" the device.
> Is that correct?

Yes

> 
> If so, this seems to me to be a dangerous design: any app with settings
> access could just set the "findmydevice.killswitch" setting directly?
> Technically only certified apps have settings API access, but if you are
> going to go to the trouble of making a separate killswitch API then this API
> should be the ONLY way to access this funcionality. Maybe we have something
> like navigator.mozKillSwitch.onchange event, and store the state inside
> KillSwitchMain.jsm instead of as a setting? 

That's another way to do it, and that explains why I decided to go with this little API: Settings is just a mean of storing/communicating, but it's easier to abstract this way.

> 
> Ultimately though, it really depends on what we do to KILL a device. If
> "kill" just means change some gaia Settings so that the device can't be
> unlocked, we are back to the security of the settings APIs, and there isn't
> really point of making this API more secure (Do we even need a "killSwitch
> API at this point? I'm not saying we don't, just unclear of the benefit ?)

Yes, for now it's mostly toggling settings values (will be done on Gaia side in FindMyDevice, in bug 1181505).
But again, we will have also other stuff to toggle like android properties (to kill ADB access for example, block bootloader and recovery also), and prefs maybe to switch.

Comment 39

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Alexandre LISSY :gerard-majax from comment #37)
> (In reply to Paul Theriault [:pauljt] from comment #35)
> > Comment on attachment 8638896 [details] [diff] [review]
> > Expose a Kill Switch enabling/disabling
> > 
> > Review of attachment 8638896 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Added a couple comments, but I need to read the rest of the bug comments
> > first since I don't understand what this API is intended to do (afaict it
> > just sets a setting but I think im missing something).
> 
> It just sets a setting for now but:
>  - it will/should set other stuff (android property, files, etc.) to
> communicate with bootloader/recovery
>  - findmydevice app will handle the setting change for gaia-level
> 
> > 
> > ::: b2g/components/KillSwitchMain.jsm
> > @@ +101,5 @@
> > > +    }
> > > +  },
> > > +
> > > +  // addMessageListener
> > > +  receiveMessage: function(aMessage) {
> > 
> > There is no check here to verify that the message came from a certified app?
> > IIRC AvailableIn="CertifiedApps" only creates a check in the child process.
> > If a child process is compromised, I _think_ it could then send the
> > KillSwitch:Enable message to the parent. What would be the consequence of
> > this? Do we need a check here that the message came from a certified app (or
> > even better from an app with a certain permission?)
> 
> I think that's a valid concern. We can probably:
>  - add a specific permission
>  - check here that the callee is from the expected app AND with the
> permission ?
> 

Sounds good to me.

> > 
> > ::: dom/webidl/KillSwitch.webidl
> > @@ +9,5 @@
> > > +  */
> > > +
> > > +[JSImplementation="@mozilla.org/mozKillSwitch;1",
> > > + NavigatorProperty="mozKillSwitch",
> > > + AvailableIn="CertifiedApps"]
> > 
> > In the future, we plan to remove certified apps, so it might be better to
> > better to use a permission here (one that is certified for now). Happy for
> > this to be a separate bug though that is completed in the future.
> 
> Well better to land something future proof now than refactor later ... :)

Great, thanks.

Comment 40

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Alexandre LISSY :gerard-majax from comment #38)
> (In reply to Paul Theriault [:pauljt] from comment #36)
> > So the main risk with something like a "Killswitch" is the risk of DoS due
> > to unauthorised usage. Currently IIUC this API just sets a setting, which
> > FMD observes, and then does something (currently TDB) to "kill" the device.
> > Is that correct?
> 
> Yes
> 
> > 
> > If so, this seems to me to be a dangerous design: any app with settings
> > access could just set the "findmydevice.killswitch" setting directly?
> > Technically only certified apps have settings API access, but if you are
> > going to go to the trouble of making a separate killswitch API then this API
> > should be the ONLY way to access this funcionality. Maybe we have something
> > like navigator.mozKillSwitch.onchange event, and store the state inside
> > KillSwitchMain.jsm instead of as a setting? 
> 
> That's another way to do it, and that explains why I decided to go with this
> little API: Settings is just a mean of storing/communicating, but it's
> easier to abstract this way.
> 
> > 
> > Ultimately though, it really depends on what we do to KILL a device. If
> > "kill" just means change some gaia Settings so that the device can't be
> > unlocked, we are back to the security of the settings APIs, and there isn't
> > really point of making this API more secure (Do we even need a "killSwitch
> > API at this point? I'm not saying we don't, just unclear of the benefit ?)
> 
> Yes, for now it's mostly toggling settings values (will be done on Gaia side
> in FindMyDevice, in bug 1181505).
> But again, we will have also other stuff to toggle like android properties
> (to kill ADB access for example, block bootloader and recovery also), and
> prefs maybe to switch.

So assuming a permission is added, Ijust want to make sure that apps with settings access can't initiate killswitch without the killswitch permission. I don't have a strong opinion on the best way to do that, but the options I can think of are:

a) an onchange event handler on the killswitch API
b) move the actual settings/pref changes into gecko (IE FMD app doesnt have to handle the settings change at all)

Comment 41

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

Switching to all the logic within Gecko, adding a permission and killing the
"findmydevice.killswitch" setting.

Comment 42

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a727a854db7

Next steps would include:
 - permission checking on the parent process
 - xpcshell unit testing of the parent process activity
 - gonk xpcshell unit testing of setting low-level stuff

Comment 43

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

https://treeherder.mozilla.org/#/jobs?repo=try&revision=818b8cdd0a42

Let's get this one green and build upon this basics.

Comment 44

[:gerard-majax]

Looking at dom/power mochitest they document that permissions are only working after a document reload(). That would explain why the first test fails and subsequent do works.

This try should make more failure by force-removing the permission after the end of the test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ca07b4fbae0

Comment 45

[:gerard-majax]

That's mochitest-specific, this code is running fine on my Z3c.

Comment 46

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ca07b4fbae0
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10eb90d92717

Comment 47

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

Adding a permission check
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e1e2835f65b

Comment 48

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #46)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ca07b4fbae0
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=10eb90d92717

green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=10eb90d92717
So we needed just to reload after adding the perm

Comment 49

[:gerard-majax]

green https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5630b6daf8b
that's with permission checking on parent side, and xpcshell tests to verify this

Comment 50

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff4ef3ff6ef3

Comment 51

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

With passing XPCShell unit tests and fake property manipulation on non Gonk.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=451cfa8a6d91

Comment 52

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f9297b38811

Comment 53

[:gerard-majax]

Comment on attachment 8642182 [details] [diff] [review]
Expose a Kill Switch enabling/disabling

So that should be much better looking from a security point of view :)

All the big bits are in place, now we just need to set the proper settings and android properties values.

Comment 54

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a881969c3eba

Comment 55

[:gerard-majax]

Right the last one is with a dom.mozKillSwitch.enabled pref disabled by default for now.

Comment 56

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8642303 [details] [diff] [review]
Expose a Kill Switch enabling/disabling

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

Thank you Alexandre!

It would help a comment explaining what kill switch does exactly on the device. What happens when FMD calls mozKillSwitch.enable? What do we show to the user? How can the user disable the kill switch?

::: b2g/components/B2GComponents.manifest
@@ +119,5 @@
>  contract @mozilla.org/presentation-device/prompt;1 {4a300c26-e99b-4018-ab9b-c48cf9bc4de1}
>  
> +# KillSwitch.js
> +component {5edd4348-3435-43b2-a273-f51d2449a057} KillSwitch.js
> +contract @mozilla.org/mozKillSwitch;1 {5edd4348-3435-43b2-a273-f51d2449a057}

s/mozKillSwitch/moz-kill-switch please

::: b2g/components/KillSwitch.js
@@ +21,5 @@
> +                                   "@mozilla.org/childprocessmessagemanager;1",
> +                                   "nsIMessageSender");
> +
> +const KILLSWITCH_CID = "{5edd4348-3435-43b2-a273-f51d2449a057}";
> +const KILLSWITCH_CONTRACTID = "@mozilla.org/mozKillSwitch;1";

moz-kill-switch

@@ +24,5 @@
> +const KILLSWITCH_CID = "{5edd4348-3435-43b2-a273-f51d2449a057}";
> +const KILLSWITCH_CONTRACTID = "@mozilla.org/mozKillSwitch;1";
> +
> +function KillSwitch() {
> +  this.innerWindowID = null;

It seems that innerWindowID is unused.

@@ +38,5 @@
> +  },
> +
> +  enable: function() {
> +    DEBUG && debug("KillSwitch: enable");
> +    cpmm.sendAsyncMessage("KillSwitch:Enable");

It's usually a good thing to get the result of the request that we make to the parent, so we can know on the child side if it succeeded or not. But it's up to you. If you think that progressing the result to the child is useless in this case, just leave it this way.

@@ +46,5 @@
> +    DEBUG && debug("KillSwitch: disable");
> +    cpmm.sendAsyncMessage("KillSwitch:Disable");
> +  },
> +
> +  receiveMessage: function(message) {

You are not listening for any message coming from the parent, so it seems that this function can be removed.

@@ +62,5 @@
> +      let wId = aSubject.QueryInterface(Ci.nsISupportsPRUint64).data;
> +      if (wId != this.innerWindowID) {
> +        return;
> +      }
> +      Services.obs.removeObserver(this, "inner-window-destroyed");

You are basically doing nothing on this handler, so it seems that this can also be removed as long with the observer part on |init| as well.

@@ +70,5 @@
> +  classID : Components.ID(KILLSWITCH_CID),
> +  contractID : KILLSWITCH_CONTRACTID,
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIKillSwitch,
> +                                         Ci.nsIDOMGlobalPropertyInitializer,
> +                                         Ci.nsIObserver,

No need to implement nsIObserver if you remove the observer part. Same thing with nsIMessageListener, cause you are not listening for any message sent from the parent.

::: b2g/components/KillSwitchMain.jsm
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = [ "KillSwitchMain" ];

nit: no space before or after []

@@ +34,5 @@
> +#endif
> +
> +const kEnableKillSwitch   = "KillSwitch:Enable";
> +const kEnableKillSwitchOK = "KillSwitch:Enable:OK";
> +const kEnableKillSwitchKO = "KillSwitch:Enable:KO";

kEnableKillSwitchOK and kEnableKillSwitchKO are unused

@@ +38,5 @@
> +const kEnableKillSwitchKO = "KillSwitch:Enable:KO";
> +
> +const kDisableKillSwitch   = "KillSwitch:Disable";
> +const kDisableKillSwitchOK = "KillSwitch:Disable:OK";
> +const kDisableKillSwitchKO = "KillSwitch:Disable:KO";

Same thing with kDisableKillSwitchOK and kDisableKillSwitchKO

@@ +40,5 @@
> +const kDisableKillSwitch   = "KillSwitch:Disable";
> +const kDisableKillSwitchOK = "KillSwitch:Disable:OK";
> +const kDisableKillSwitchKO = "KillSwitch:Disable:KO";
> +
> +const kMessages = [ kEnableKillSwitch, kDisableKillSwitch ];

Same nit about [] and spaces before and after

@@ +45,5 @@
> +
> +const kXpcomShutdownObserverTopic = "xpcom-shutdown";
> +
> +let inParent = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime)
> +                  .processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;

nit: align ".processType" with "["

@@ +52,5 @@
> +  dump("--*-- KillSwitchMain: " + aStr + "\n");
> +}
> +
> +function debug(aStr) {
> +  dump("--*-- KillSwitchMain: " + aStr + "\n");

I see no difference between "error" and "debug"

@@ +65,5 @@
> +    if (libcutils) {
> +      debug("Has libcutils ...");
> +      this._libcutils = libcutils;
> +    } else {
> +      debug("HAS NO libcutils ...");

If we have no libcutils, we can probably early return here.

@@ +85,5 @@
> +
> +    Services.obs.removeObserver(this, kXpcomShutdownObserverTopic);
> +  },
> +
> +  checkLibcUtils: function() {

Just a suggestion. I would do something like:

ensureLibCUtils: function() {
  if (this._libcutils) {
    return Promise.resolve();
  } else {
    dump("No libcutils\n");
    return Promise.reject();
  }
}

This way you can do on readStateProperty and others something like:

ensureLibCUtils().then(() => { // do whatever with libcutis });

@@ +99,5 @@
> +  readStateProperty: function() {
> +    debug("readStateProperty");
> +    try {
> +      this.checkLibcUtils();
> +    } catch (ex) {

If you choose to ignore the "ensureLibCUtils" suggestion, dump ex here, please

@@ +103,5 @@
> +    } catch (ex) {
> +      return;
> +    }
> +
> +    this._ksState = this._libcutils.property_get("persist.moz.killswitch", "false") === "true";

nit: lines shorter than 80 chars, please.

@@ +111,5 @@
> +    debug("writeStateProperty");
> +    try {
> +      this.checkLibcUtils();
> +    } catch (ex) {
> +      return;

Dump ex here as well, please.

@@ +114,5 @@
> +    } catch (ex) {
> +      return;
> +    }
> +
> +    let value = this._ksState ? "true" : "false";

Remove this line and simply do |this._ksState.toString()| in the following line.

@@ +119,5 @@
> +    this._libcutils.property_set("persist.moz.killswitch", value);
> +  },
> +
> +  getLock: function() {
> +    return settings.createLock();

One line function feels useless to me.

@@ +143,5 @@
> +
> +  // Settings Callbacks
> +  handle: function(aName, aValue) {
> +    debug("handle: aName=" + aName + " ; aValue=" + aValue);
> +    // We don't have to do anything for now.

Then we better remove this :)

@@ +154,5 @@
> +
> +  handleError: function(aMessage) {
> +    error("handleError: " + JSON.stringify(aMessage));
> +    throw Cr.NS_ERROR_ABORT;
> +  },

These two functions are not used, so it seems that we can remove them

@@ +176,5 @@
> +    debug("Using principal=" + principal);
> +    if (principal && !permMgr.testPermissionFromPrincipal(principal, "killswitch")) {
> +      error("Message " + aMessage.name + " from a process with no killswitch perm.");
> +      aMessage.target && aMessage.target.killChild();
> +      return null;

return;

@@ +190,5 @@
> +
> +      default:
> +        error("Unsupported message: " + aMessage.name);
> +        aMessage.target && aMessage.target.killChild();
> +        return null;

return;

::: b2g/components/test/mochitest/file_loadserver.js
@@ +4,5 @@
> +
> +// Stolen from SpecialPowers, since at this point we don't know we're in a test.
> +let isMainProcess = function() {
> +  try {
> +    return Cc["@mozilla.org/xre/app-info;1"].

nit: move the "." at the begining of the following line as you do in the rest of the patch, please

::: b2g/components/test/mochitest/test_killswitch_basics.html
@@ +44,5 @@
> +
> +  SpecialPowers.pushPrefEnv({"set": [
> +    ["dom.ignore_webidl_scope_checks", true],
> +    ["dom.mozKillSwitch.enabled", true],
> +  ]}, addPermissions);

Could you also check that mozKillSwitch is not exposed when the permission is not set, please?

::: b2g/components/test/mochitest/test_killswitch_enable.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

This test is pretty much the same as test_killswitch_disable.html. If you want to keep the mochitests separated in to html files, that's fine, but could we somehow share the common code in a helper script, please?

::: dom/webidl/KillSwitch.webidl
@@ +1,1 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

You need a DOM peer review for this

@@ +4,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +/**
> +  * Kill switch will make sure the device has no value.

Could you explain a little more what kill switch does exactly, please.

Comment 57

[:gerard-majax]

Ok, everything addressed except the messaging stuff because I plan to make use of it to at least notify content of error cases. Yet what we should do in this context is not clear ...

Comment 58

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling API

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21ac767292bf

Comment 59

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

Added toggling prefs
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e50900e53cfd

Comment 60

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

Fixing mochitest
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee04b52414aa

Comment 61

[:gerard-majax]

I think we lack one piece when disabling this: restoring the settings, prefs and android properties values of the user.

Comment 62

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87870783a7e7

Comment 63

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

With saveUserValues() method and its testing. X and M green locally, this
should also pass on try. Next target will be making doDisable() read the saved
values and then apply then.

We add a setTimeout() hack in mochitest because reading the settings values
might return improper values: |enable()| and |disable()| methods of the API
should maybe changed into Promises so that we track their real completion
(and not try to read settings values before) and also errors

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2550a098b52f

Comment 64

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

That should cover everything and be green everywhere. We don't kill adb there.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e9593e77dbf

Comment 65

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

And it looks like changing the sys.usb properties also cut off ADB
https://treeherder.mozilla.org/#/jobs?repo=try&revision=889099cdc2c1

Comment 66

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

Hopefully a full green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcf75af56c6b

After being able to build an emuator and run this locally, I found that it was
just the test_killswitch.js that was broken on Gonk/Emulator. The file
test_killswitch_gonk.js does pass successfully and was intended for this
purpose. It contains the same set of tests, just differs in init.

Comment 67

[:gerard-majax]

Attachment: local xpcshell success

Comment 68

[:gerard-majax]

Attachment: xpcshell ics emulator debug error

So this is badly failing on B2G ICS Emulator because of errors at IndexedDB level: "Received NS_ERROR_STORAGE_BUSY when attempting to start write transaction, retrying for up to 10 seconds"

Comment 69

[:gerard-majax]

Comment on attachment 8646482 [details] [diff] [review]
Expose a Kill Switch enabling/disabling

There are still some debug left, but I think it's in shape now.

Comment 70

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

Added abort and error handlers, gets green on B2G ICS Emulator debug:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=048244ad1a83

Comment 71

[Dave Hylands [:dhylands]]

Not sure how relevant this is, but when I run b2g-desktop, I see the following warning inthe log:

> PermissionsTable.jsm: expandPermissions: Unknown Permission: killswitch
> PermissionsInstaller.jsm: 'killswitch' is not a valid Webapps permission name.Error getting tracking ID [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getIntPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://b2g/content/settings.js :: setUpdateTrackingId :: line 242"  data: no]

Comment 72

[Dave Hylands [:dhylands]]

Comment on attachment 8646763 [details] [diff] [review]
Expose a Kill Switch enabling/disabling

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

::: b2g/components/KillSwitchMain.jsm
@@ +357,5 @@
> +          this.setPref(key, values.prefs[key]);
> +        }
> +
> +        for (let key of Object.keys(values.properties)) {
> +          this._libcutils.property_set(key, values.properties[key]);

Normally, setting persist.sys.usb.config has a side-effect of also setting sys.usb.config, so the order that you restore these in is significant.

See: https://git.mozilla.org/?p=b2g/platform_system_core.git;a=blob;f=rootdir/init.rc;h=236c97b184f12b62c8bec485c2ffa7e8e4ba342d;hb=HEAD#l339

Similarly, setting sys.usb.config will cause sys.usb.state to be set.

It is valid for persist.sys.usb.config to be different from sys.usb.config, so you need to set persist.sys.usb.config first, and then set sys.usb.config, and you probably shouldn't set sys.usb.state at all (or set it first)

@@ +391,5 @@
> +            }
> +          }
> +        };
> +
> +        // For settings we have to wait all the callbacks to come

nit: comment seems to be missing a couple of workd to make the sentence complete.

Comment 73

[:gerard-majax]

(In reply to Dave Hylands [:dhylands] from comment #71)
> Not sure how relevant this is, but when I run b2g-desktop, I see the
> following warning inthe log:
> 
> > PermissionsTable.jsm: expandPermissions: Unknown Permission: killswitch
> > PermissionsInstaller.jsm: 'killswitch' is not a valid Webapps permission name.Error getting tracking ID [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getIntPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://b2g/content/settings.js :: setUpdateTrackingId :: line 242"  data: no]

Maybe you need to make sure to have a clean profile. Thanks for the other comments, I'll keep just setting the "persist" properties when there are dupes.

Comment 74

[:gerard-majax]

I think we might also want to block from doing |enable()| when the kill switch mode is already enabled or |disable()| when it is already disabled.

Comment 75

[:gerard-majax]

Actually for disable it should not be a big deal ...

Comment 76

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

Cleanup of some debug and adding checking for enforcing only one |enable()| call
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c35ae5ca3e1

Comment 77

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

Went too hard on the cleanup :)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccb4b7f1a6a9

Comment 78

[:gerard-majax]

Comment on attachment 8648093 [details] [diff] [review]
Expose a Kill Switch enabling/disabling

Jonas, would you mind check the new API ? I think it should be okay now, further changes will be on the values we toggle to set the device into killswitch mode, and fixing tests on emulator debug. Thanks!

Comment 79

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 8648093 [details] [diff] [review]
Expose a Kill Switch enabling/disabling

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

Overall looks OK, I can't see issues with the, maybe one minor improvement, but that's up to you really I think.

::: b2g/components/KillSwitchMain.jsm
@@ +356,5 @@
> +        }
> +
> +        for (let key of Object.keys(values.properties)) {
> +          this._libcutils.property_set(key, values.properties[key]);
> +        }

Here it would be better to read ONLY the value that you wrote to the file, rather than just adding all the prefs from the file. If an attacker could modify this file, and then cause it to be read, then the attacker could set prefs/settings. Of course if the attacker has access to a file in the profile, then they can probably just changes prefs directly (assuming they can stop/start the b2g process). So up to you if you think it is worth adding this check or not.

TBH I am not completely sure why you use a separate file here at all. Why not just set a setting or pref, which contains all the information you want to preserve? I had thought you were using a separate file in case the profile gets reset, but you since you are putting this file in the profile dir, Im not sure I understand the point of it. Maybe Im missing something though. Can you explain why you are creating a seperate file here?

Comment 80

[:gerard-majax]

(In reply to Paul Theriault [:pauljt] from comment #79)
> Comment on attachment 8648093 [details] [diff] [review]
> Expose a Kill Switch enabling/disabling
> 
> Review of attachment 8648093 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks OK, I can't see issues with the, maybe one minor improvement,
> but that's up to you really I think.
> 
> ::: b2g/components/KillSwitchMain.jsm
> @@ +356,5 @@
> > +        }
> > +
> > +        for (let key of Object.keys(values.properties)) {
> > +          this._libcutils.property_set(key, values.properties[key]);
> > +        }
> 
> Here it would be better to read ONLY the value that you wrote to the file,
> rather than just adding all the prefs from the file. If an attacker could
> modify this file, and then cause it to be read, then the attacker could set
> prefs/settings. Of course if the attacker has access to a file in the
> profile, then they can probably just changes prefs directly (assuming they
> can stop/start the b2g process). So up to you if you think it is worth
> adding this check or not.

That was the same reasoning I had: if the attacker has access to the profile, it's already too late.

> 
> TBH I am not completely sure why you use a separate file here at all. Why
> not just set a setting or pref, which contains all the information you want
> to preserve? I had thought you were using a separate file in case the
> profile gets reset, but you since you are putting this file in the profile
> dir, Im not sure I understand the point of it. Maybe Im missing something
> though. Can you explain why you are creating a seperate file here?

Mostly because it was easier and less messy than copying settings, prefs or android properties around.
Also, pushing all this values in a setting or in a pref, I was not sure of the limit for the amount of data. And manipulating a file for this feels simpler and safer.

That being said maybe we should remove the file once we are done with restoring the values.

Comment 81

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

Comment 82

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b558beddb9b

Comment 83

[:gerard-majax]

So my current set of settings is:
> "debugger.remote-mode": "disabled",
> "developer.menu.enabled": false,
> "lockscreen.enabled": true,
> "lockscreen.locked": true,
> "lockscreen.lock-immediately": true,
> "tethering.usb.enabled": false,
> "tethering.wifi.enabled": false,
> "ums.enabled": false

Paul, would you see any other setting, gecko pref and/or Android properties to switch ?

Comment 84

[:gerard-majax]

So after checking on a real device with a VARIANT=user build, this is working as expected: used WebIDE from FindMyDevice app to call the API, and this effectively locked me out of the device.

Comment 85

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

Comment 86

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8ed33f8aa66

Comment 87

[:gerard-majax]

Vishy, can you check comment 83 ? Thanks!

Comment 88

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The API looks fine if it meets the desired use cases. I assume that there's some protection in place such that the attacker can't simply call .disable() in order to unlock the phone?

Comment 89

[:gerard-majax]

(In reply to Jonas Sicking (:sicking) from comment #88)
> The API looks fine if it meets the desired use cases. I assume that there's
> some protection in place such that the attacker can't simply call .disable()
> in order to unlock the phone?

Yep, the same level of protections as for the .enable(). If anyone sees a shortcoming on this, feel free to notify me :)

Comment 90

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 8648835 [details] [diff] [review]
Expose a Kill Switch enabling/disabling

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

::: b2g/components/KillSwitchMain.jsm
@@ +442,5 @@
> +  // addMessageListener
> +  receiveMessage: function(aMessage) {
> +    let principal = aMessage.principal;
> +    DEBUG && debug("Using principal=" + principal);
> +    if (principal && !permMgr.testPermissionFromPrincipal(principal, "killswitch")) {

Oh I just noticed the logic here. If principal is ever null, the permission check will be bypassed I think which isnt good? (can it be null? I dont know) I think we have a better idiom for permission checking, I'll find out what that is.

Other code just does something like:
aMessage.target.assertPermission("engineering-mode")

But Im not sure what the _correct_ way to check this is these days, I'll have to check.

Comment 91

[:gerard-majax]

I do have a null principal while running the mochitest, but maybe I can fix it on the way I run those tests :)

Comment 92

[:gerard-majax]

Paul, that try should be green and with the assertPermission() call:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a98411ad68c6

Comment 93

[:gerard-majax]

same with emulator debug disabled:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=347603c27721

Comment 94

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #93)
> same with emulator debug disabled:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=347603c27721

The skip-if was wrong. Proper one is in:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de9642c7a977

Comment 95

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0cf9fd4b5aa

Comment 96

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling on v2.2r

That is the uplifted patch for v2.2r

https://treeherder.mozilla.org/#/jobs?repo=try&revision=252ace14e1eb

Comment 97

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling on v2.2r

WebIDL on B2G37 has no "CheckAnyPermissions" but "CheckPermissions"
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27b8eeb85c41

Comment 98

[:gerard-majax]

Looks like the uplifted patch will be green.

Comment 99

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling

Rebased.

Comment 100

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling on v2.2r

Rebased.

Comment 101

[vkrishnamoorthy]

comment 83 covers all the usecases for the first release

Comment 102

[:gerard-majax]

Paul confirmed it was okay on IRC.

Comment 103

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0279bffee7d

Comment 104

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a0279bffee7d

Comment 105

[:gerard-majax]

Comment on attachment 8652780 [details] [diff] [review]
Expose a Kill Switch enabling/disabling on v2.2r

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1170629
User impact if declined: No killswitch feature on v2.2r
Testing completed: on device, xpcshell unit tests and mochitest
Risk to taking this patch (and alternatives if risky): low, we can disable the API in the worst case.
String or UUID changes made by this patch: new API

Comment 106

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling on v2.2r

Rebased

Comment 107

[:gerard-majax]

Comment on attachment 8654490 [details] [diff] [review]
Expose a Kill Switch enabling/disabling on v2.2r

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1170629
User impact if declined: No killswitch feature on v2.2r
Testing completed: on device, xpcshell unit tests and mochitest
Risk to taking this patch (and alternatives if risky): low, we can disable the API in the worst case.
String or UUID changes made by this patch: new API

Comment 108

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8654490 [details] [diff] [review]
Expose a Kill Switch enabling/disabling on v2.2r

RelMan decided to auto-approve bugs with feature-b2g:2.2r+

That said, I'm assuming this is blocked on bug 1181505 and will therefore hold off on trying to push it in the mean time.

Comment 109

[:gerard-majax]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #108)
> Comment on attachment 8654490 [details] [diff] [review]
> Expose a Kill Switch enabling/disabling on v2.2r
> 
> RelMan decided to auto-approve bugs with feature-b2g:2.2r+
> 
> That said, I'm assuming this is blocked on bug 1181505 and will therefore
> hold off on trying to push it in the mean time.

No, we can land the Gecko part even if Gaia part is not there.

Comment 110

[:gerard-majax]

checkin-needed for v2.2r

Comment 111

[Ryan VanderMeulen [:RyanVM]]

The uplift queries still work fine. No need to get attention two other ways.

Comment 112

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/3aa144580779

Comment 113

[Ryan VanderMeulen [:RyanVM]]

Backed out from v2.2r for test_filepicker_path.html failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=5172&repo=mozilla-b2g37_v2_2r

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/43dc38631fec

Comment 114

[:gerard-majax]

Attachment: Expose a Kill Switch enabling/disabling. r=dhylands, sr=sicking, a=2.2r+

Rebased and fixed the test skip list: the test was not expected to be run at
all, hence the failure.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ce15ad8597f

Comment 115

[:gerard-majax]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #113)
> Backed out from v2.2r for test_filepicker_path.html failures.
> https://treeherder.mozilla.org/logviewer.html#?job_id=5172&repo=mozilla-
> b2g37_v2_2r
> 
> https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/43dc38631fec

Should be good with the new patch and the try in comment 114. Sorry for missing this :)

Comment 116

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/c39fac7c081e