Closed Bug 1181561 Opened 5 years ago Closed 5 years ago

Exposing a kill switch enabling/disabling API

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
FxOS-S6 (04Sep)
feature-b2g 2.2r+
Tracking Status
firefox43 --- fixed
b2g-v2.2r --- fixed
b2g-master --- fixed

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [systemsfe])

Attachments

(4 files, 36 obsolete files)

3.67 KB, text/plain
Details
4.48 KB, text/plain
Details
56.62 KB, patch
gerard-majax
: review+
gerard-majax
: superreview+
gerard-majax
: feedback+
Details | Diff | Splinter Review
56.73 KB, patch
gerard-majax
: review+
gerard-majax
: superreview+
gerard-majax
: ui-review+
Details | Diff | Splinter Review
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
Whiteboard: [systemsfe]
Attachment #8631036 - Attachment is obsolete: true
Attachment #8631606 - Attachment is obsolete: true
Attachment #8634090 - Attachment is obsolete: true
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.
Attachment #8635167 - Flags: feedback?(dhylands)
Cool ... B2G ICS Emulator failure :(
So there's something wrong with b2g emulator, but I cannot reproduce it locally because running mochitest fails (bug 1146713).
Depends on: 1146713
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!
feature-b2g: --- → 2.2r+
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.
Flags: needinfo?(kyle)
Flags: needinfo?(anygregor)
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?
Flags: needinfo?(kyle)
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.
Flags: needinfo?(anygregor)
(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?
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?
(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
(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().
I've added an assert with XTR_IsParentProcess() and this is crashing :).
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 :(
Flags: needinfo?(kyle)
Reworked and now all green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dcd50c6cfae
Flags: needinfo?(kyle)
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?
Attachment #8638687 - Flags: review?(dhylands) → review+
(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?
Flags: needinfo?(dhylands)
Blocks: 1181505
(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 :)
Flags: needinfo?(dhylands)
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!
Attachment #8638896 - Flags: feedback?(ptheriault)
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!
Attachment #8638896 - Flags: review?(ferjmoreno)
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.
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 ?)
(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 ... :)
(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.
(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.
(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)
Switching to all the logic within Gecko, adding a permission and killing the
"findmydevice.killswitch" setting.
Attachment #8638896 - Attachment is obsolete: true
Attachment #8638896 - Flags: review?(ferjmoreno)
Attachment #8638896 - Flags: feedback?(ptheriault)
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
Attachment #8641051 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=818b8cdd0a42

Let's get this one green and build upon this basics.
Attachment #8641083 - Attachment is obsolete: true
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
That's mochitest-specific, this code is running fine on my Z3c.
green https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5630b6daf8b
that's with permission checking on parent side, and xpcshell tests to verify this
With passing XPCShell unit tests and fake property manipulation on non Gonk.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=451cfa8a6d91
Attachment #8641668 - Attachment is obsolete: true
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.
Attachment #8642182 - Flags: feedback?(ptheriault)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a881969c3eba
Attachment #8642145 - Attachment is obsolete: true
Attachment #8642182 - Attachment is obsolete: true
Attachment #8642182 - Flags: feedback?(ptheriault)
Attachment #8642303 - Flags: feedback?
Right the last one is with a dom.mozKillSwitch.enabled pref disabled by default for now.
Attachment #8642303 - Flags: feedback? → feedback?(ptheriault)
Attachment #8642303 - Flags: review?(ferjmoreno)
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.
Attachment #8642303 - Flags: review?(ferjmoreno)
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 ...
I think we lack one piece when disabling this: restoring the settings, prefs and android properties values of the user.
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
Attachment #8645679 - Attachment is obsolete: true
That should cover everything and be green everywhere. We don't kill adb there.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e9593e77dbf
Attachment #8645741 - Attachment is obsolete: true
And it looks like changing the sys.usb properties also cut off ADB
https://treeherder.mozilla.org/#/jobs?repo=try&revision=889099cdc2c1
Attachment #8645970 - Attachment is obsolete: true
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.
Attachment #8646038 - Attachment is obsolete: true
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 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.
Attachment #8646482 - Flags: review?(dhylands)
Attachment #8646482 - Flags: feedback?(ptheriault)
Added abort and error handlers, gets green on B2G ICS Emulator debug:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=048244ad1a83
Attachment #8646482 - Attachment is obsolete: true
Attachment #8646482 - Flags: review?(dhylands)
Attachment #8646482 - Flags: feedback?(ptheriault)
Attachment #8646763 - Flags: review?(dhylands)
Attachment #8646763 - Flags: feedback?(ptheriault)
Depends on: 1193677
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 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.
Attachment #8646763 - Flags: review?(dhylands) → review+
(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.
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.
Actually for disable it should not be a big deal ...
Cleanup of some debug and adding checking for enforcing only one |enable()| call
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c35ae5ca3e1
Attachment #8646763 - Attachment is obsolete: true
Attachment #8646763 - Flags: feedback?(ptheriault)
Attachment #8648032 - Flags: review+
Attachment #8648032 - Flags: feedback?(ptheriault)
Went too hard on the cleanup :)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccb4b7f1a6a9
Attachment #8648032 - Attachment is obsolete: true
Attachment #8648032 - Flags: feedback?(ptheriault)
Attachment #8648093 - Flags: review+
Attachment #8648093 - Flags: feedback?(ptheriault)
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!
Attachment #8648093 - Flags: superreview?(jonas)
Target Milestone: --- → FxOS-S5 (21Aug)
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?
Attachment #8648093 - Flags: feedback?(ptheriault) → feedback+
(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.
Attachment #8648093 - Attachment is obsolete: true
Attachment #8648093 - Flags: superreview?(jonas)
Attachment #8648750 - Flags: superreview?(jonas)
Attachment #8648750 - Flags: review+
Attachment #8648750 - Flags: feedback+
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 ?
Flags: needinfo?(ptheriault)
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.
Attachment #8648750 - Attachment is obsolete: true
Attachment #8648750 - Flags: superreview?(jonas)
Attachment #8648835 - Flags: review+
Attachment #8648835 - Flags: feedback+
Attachment #8648835 - Flags: superreview?(jonas)
Vishy, can you check comment 83 ? Thanks!
Flags: needinfo?(vkrishnamoorthy)
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?
Attachment #8648835 - Flags: superreview?(jonas) → superreview+
(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 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.
I do have a null principal while running the mochitest, but maybe I can fix it on the way I run those tests :)
Paul, that try should be green and with the assertPermission() call:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a98411ad68c6
(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
WebIDL on B2G37 has no "CheckAnyPermissions" but "CheckPermissions"
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27b8eeb85c41
Attachment #8650354 - Attachment is obsolete: true
Looks like the uplifted patch will be green.
Rebased.
Attachment #8650343 - Attachment is obsolete: true
Attachment #8652777 - Flags: superreview+
Attachment #8652777 - Flags: review+
Attachment #8652777 - Flags: feedback+
Rebased.
Attachment #8650372 - Attachment is obsolete: true
Attachment #8652780 - Flags: superreview+
Attachment #8652780 - Flags: review+
Attachment #8652780 - Flags: feedback+
comment 83 covers all the usecases for the first release
Flags: needinfo?(vkrishnamoorthy)
Paul confirmed it was okay on IRC.
Flags: needinfo?(ptheriault)
Keywords: checkin-needed
Blocks: 1084540
https://hg.mozilla.org/mozilla-central/rev/a0279bffee7d
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: FxOS-S5 (21Aug) → FxOS-S6 (04Sep)
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
Attachment #8652780 - Flags: approval‑mozilla‑b2g37_v2_2r?
Rebased
Attachment #8652780 - Attachment is obsolete: true
Attachment #8652780 - Flags: approval‑mozilla‑b2g37_v2_2r?
Attachment #8654490 - Flags: superreview+
Attachment #8654490 - Flags: review+
Attachment #8654490 - Flags: feedback+
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
Attachment #8654490 - Flags: approval‑mozilla‑b2g37_v2_2r?
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.
Attachment #8654490 - Flags: approval‑mozilla‑b2g37_v2_2r?
(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.
Flags: needinfo?(ryanvm)
checkin-needed for v2.2r
Keywords: checkin-needed
The uplift queries still work fine. No need to get attention two other ways.
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
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
Attachment #8654490 - Attachment is obsolete: true
Attachment #8655948 - Flags: ui-review+
Attachment #8655948 - Flags: superreview+
Attachment #8655948 - Flags: review+
(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 :)
Flags: needinfo?(lissyx+mozillians) → needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.