746074 - Allow enforcing network-usage policies on "web apps"

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Again, going with "web apps" because they have a strong identity.  Other solutions are possible for the general case.)

The idea here is to allow a user to say, "my Facebook app can only use Wifi", or "Twitter can only use up to 10MB of 3g data."  Like bug 746073, the multi-process setup probably makes this simpler because there's a choke point at which ~sockets are opened, and at a point where we can find a domain/origin for the web app.  The basic idea of the impl would be to check the currently-selected interface and see if the app's policy allows opening a channel on that interface, along with live monitoring to see when quotas are exceeded.  (And then, close the channel?  Pop an alert?  Not so clear.  But a UX problem :) .)

I think initially, we can keep things pretty simple and allow two kinds of policy bits
 - app allowed/disallowed to use network interface "foo"
 - app has quota of X bytes on interface "foo"

We'll need bug 746073 implemented to enforce the second kind of policy above.  We can do the first in parallel though, and split the second into a separate bug.

Comment 1

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

I think the best way is to offer a global "firewall-like" API.

For each application and interface, I think we should offer a way to assign it as mandatory, forbidden or preferred, so, if both (wifi & 3g) networks are available, depending on the rules, we can route through the user desired one.

So, to summarize:

The API should deal:

* Recover List of installed applications
* Recover List of available interfaces (connection types)
* Get/Set new application policies
* Get/Set default policies for new & not configured apps.

WDYT?

Is necessary to define a completely new API? Where do you think this API can fit?

I think we can join to the IDL defined in the bug 746069 (https://bugzilla.mozilla.org/attachment.cgi?id=630128)

Comment 2

[Andrew Overholt [:overholt]]

Nomming for basecamp.

Comment 3

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

We propose a possible API for this bug:

Our proposal is to define an API to manage policies for all the installed applications and also default rules for new ones and unconfigured ones:

The use of the API could be something like:

// Get the policies for an app
var policyData = navigator.mozAppPolicies.get(uri)

// Create new policy
var policyData = new mozPolicy();
policyData.init({app: appURI, allowNetworkAccess: true, policies: { c3g: { enabled: false, max: 1000 }, wifi: {enabled: true, prefered: true, max: 1000} } });

// Store for the policy for an app
navigator.mozAppPolicies.save(policyData)

// Retreive / Store default policies

var defaultPol = navigator.mozAppPolicies.defaultPolicy;

defaultPol = {allowNetworkAccess: true, policies: { c3g: { enabled: false, max: 1000 }, wifi: {enabled: true, prefered: true, max: 1000} } }

navigator.mozAppPolicies.defaultPolicy = defaultPol;

--------------------------------

Suggested interfaces


interface nsIDOMNetworkPoliciesConnectionTypes : nsISupports
{
  attribute boolean enabled;             // Is allowed the use of this Connection type
  attribute boolean prefered;            // If more than one connection type allowed, this is the prefered one
  attribute long? max;                   // Max amount of MB allowed to transmit through this connection type
};

interface nsIDOMNetworkPoliciesApplicationPolicies : nsISupports
{
  attribute DOMString connectionType;    // Connection type name associated to this policy
  attribute boolean allowNetworkAccess;                                         // The application is allowed to access to the network
  attribute nsIDOMNetworkPoliciesConnectionTypes policiesPerConnectionType;     // policies per connection type

  DOMRequest init(in nsIDOMNetworkPoliciesApplicationPolicies policy);      // Init policy
};

interface nsIDOMNetworkPolicies : nsISupports
{
  readonly attribute nsIArray installedApplications;  // Array of DOMString (IDs of the installed applications)
  readonly attribute nsIArray connectionTypes;        // Array of DOMString (IDs of the available connection types)

  attribute nsIDOMNetworkPoliciesApplicationPolicies defaultPolicies;  // Default policies for new installed applications & for unconfigured ones

  DOMRequest get(in DOMString uri, out nsIDOMNetworkPoliciesApplicationPolicies policies);  // Retreive policies of one application
  DOMRequest save(in nsIDOMNetworkPoliciesApplicationPolicies policies);      // Store new policies to an application
   DOMRequest applicationPolicy(in DOMString application, in nsIDOMNetworkPoliciesApplicationPolicies policies);      // Store new policies to an application
};

Comment 4

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

I think that we may also define an event to notify app when it exceed the limit and is banned by policy.

Comment 5

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Patrick Wang from comment #4)
> I think that we may also define an event to notify app when it exceed the
> limit and is banned by policy.

sounds nice :)

Comment 6

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Proposed IDL

First proposal

Comment 7

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

I think this bug can be resolved by:
1. Implement nsDOMNetworkPolicies that stores the network policies and implements the interface to set/query network policy.
2. Before an application attempts to make a network connection (perhaps when it tries to initialize an nsHttpTransaction), we use the network usage data collected by meter of Bug 746073, connection type (3G or Wifi, can be obtained from NetworkManager) and nsDOMNetworkPolicies to check whether it exceed its maximum allowed bytes. If the application exceed its maximum allowed bytes, we block application's require (let nsHttpTransaction::Init() fail) and issue an event to the app to let it know it is blocked due to traffic limit.

Comment 8

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Patrick Wang [:patrickwang] from comment #7)
> I think this bug can be resolved by:
> 1. Implement nsDOMNetworkPolicies that stores the network policies and
> implements the interface to set/query network policy.
> 2. Before an application attempts to make a network connection (perhaps when
> it tries to initialize an nsHttpTransaction), we use the network usage data
> collected by meter of Bug 746073, connection type (3G or Wifi, can be
> obtained from NetworkManager) and nsDOMNetworkPolicies to check whether it
> exceed its maximum allowed bytes. If the application exceed its maximum
> allowed bytes, we block application's require (let nsHttpTransaction::Init()
> fail) and issue an event to the app to let it know it is blocked due to
> traffic limit.

Yeap, I agree

Comment 9

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Patrick Wang [:patrickwang] from comment #7)
> I think this bug can be resolved by:
> 1. Implement nsDOMNetworkPolicies that stores the network policies and
> implements the interface to set/query network policy

Hi, I've a dummy component (false data ;) which implements the interface:

frsela@yoghurtu:~/Datos/ProyectosTID/OpenWebDevice/mozilla-central.git$ telnet localhost 9999
Trying ::1...
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
JS> a = navigator.mozNetworkPolicies
[xpconnect wrapped nsIDOMMozNetworkPoliciesManager @ 0x9d293340 (native @ 0x9bd93b60)]
JS> b = a.get("facebook")
[object Object]
JS> JSON.stringify(b)
{"_app":"facebook","_allowNetworkAccess":true,"_policies":[{"_connectionType":"wifi","_allowed":true,"_prefered":true,"_max":1000},{"_connectionType":"mobile","_allowed":true,"_prefered":false,"_max":400}]}
JS> b.policies[1].allowed
true

I'll do it async. and we'll include the database to store this information. We'll upload it ASAP

> 2. Before an application attempts to make a network connection (perhaps when
> it tries to initialize an nsHttpTransaction), we use the network usage data
> collected by meter of Bug 746073, connection type (3G or Wifi, can be
> obtained from NetworkManager) and nsDOMNetworkPolicies to check whether it
> exceed its maximum allowed bytes. If the application exceed its maximum
> allowed bytes, we block application's require (let nsHttpTransaction::Init()
> fail) and issue an event to the app to let it know it is blocked due to
> traffic limit.

I don't have Necko expertise, so probably other guys could implement this part to get it implemented in short time. Any volunteer? ;)

Comment 10

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Patrick Wang [:patrickwang] from comment #7)
> 2. Before an application attempts to make a network connection (perhaps when
> it tries to initialize an nsHttpTransaction), 

Patrick: One consideration: This shall work with any kind of connection ... so probably nsHttpTransaction is not the best place, is it?

Comment 11

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Fernando R. Sela from comment #10)
> (In reply to Patrick Wang [:patrickwang] from comment #7)
> > 2. Before an application attempts to make a network connection (perhaps when
> > it tries to initialize an nsHttpTransaction), 
> 
> Patrick: One consideration: This shall work with any kind of connection ...
> so probably nsHttpTransaction is not the best place, is it?

nsHttpTransaction seems only used by Http(s) and spdy. For all kind of connections, there may be a better place to execute the policy..

Comment 12

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Fernando R. Sela from comment #9)
> > fail) and issue an event to the app to let it know it is blocked due to
> > traffic limit.
> 
> I don't have Necko expertise, so probably other guys could implement this
> part to get it implemented in short time. Any volunteer? ;)

I am interested by it. If you don't mind, I can work on the network part. I'll file another bug to track this work.

Comment 13

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Patrick Wang [:patrickwang] from comment #12)
> (In reply to Fernando R. Sela from comment #9)
> I am interested by it. If you don't mind, I can work on the network part.

Cool ;)

I suppose you'll use the service we're developing (the one which recovers the info of each app from indexedDB)

Now this is implemented into "NetworkPoliciesService" class (I'll share the patch with you)

I want to ask you: This service works in an async way, I don't know if you'll need a sync call to recover the app policies from necko side. If you need a sync method, please, tell me.

> I'll file another bug to track this work.

Create it and put this one as a blocker/dependency ;)

Thank you !

Comment 14

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Proposed Interface

New release:

* Removed defaultPolicy since it can be retrieved with app as empty string
* Removed installedApplications as other API returns this information (mozApps)

Comment 15

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: IDLs

nsIDOMNetworkPolicies IDLs

Comment 16

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: WIP v1 - JavaScript implementation of mozNetworkPolicies

Comment 17

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Patrick Wang [:kk1fff] from comment #12)

> I am interested by it. If you don't mind, I can work on the network part.
> I'll file another bug to track this work.

Hi Patrick,

I uploaded the IDLs and the WIP patch (without database integration, so always returns the same policies for any app ;) in order to give you the interfaces to deal with them in your patch.

As told before, you should use NetworkPoliciesService to recover the real information.

Comment 18

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 648653 [details] [diff] [review]
WIP v1 - JavaScript implementation of mozNetworkPolicies

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

Patch error

::: b2g/installer/package-manifest.in
@@ +377,5 @@
>  @BINPATH@/components/amWebInstallListener.js
>  @BINPATH@/components/nsBlocklistService.js
>  
> +@BINPATH@/components/NetworkPolicies.js
> +@BINPATH@/components/NetworkPolicies.manifest

s/NetworkPolicies/NetworkPoliciesManager

Comment 19

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 648652 [details] [diff] [review]
IDLs

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

::: dom/network/interfaces/nsIDOMNetworkPoliciesManager.idl
@@ +39,5 @@
> +
> +  /**
> +   * Recover a policy
> +   */
> +  jsval get(in DOMString application);

s/jsval/nsIDOMDOMRequest

Comment 20

[Albert [:albert]]

Attachment: WIP v2 - JavaScript implementation of mozNetworkPolicies

Added IndexedDB support

Comment 21

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Fernando R. Sela from comment #13)
> (In reply to Patrick Wang [:patrickwang] from comment #12)
> > (In reply to Fernando R. Sela from comment #9)
> > I am interested by it. If you don't mind, I can work on the network part.
> 
> Cool ;)
> 
> I suppose you'll use the service we're developing (the one which recovers
> the info of each app from indexedDB)
> 
> Now this is implemented into "NetworkPoliciesService" class (I'll share the
> patch with you)
> 
> I want to ask you: This service works in an async way, I don't know if
> you'll need a sync call to recover the app policies from necko side. If you
> need a sync method, please, tell me.
> 
> > I'll file another bug to track this work.
> 
> Create it and put this one as a blocker/dependency ;)
> 
> Thank you !

Hi Fernando,

I think synchronous methods would be needed by implementation in necko side. Could you also provide them?

Thank you.

Comment 22

[Jason Duell]

There's been some discussion about whether we need a synchronous API for necko here.  I've got some questions:

1) where is the CSP check (and the call to get the network policy) happening?  From talking to cjones it sounds like CSP check needs to be on parent, so the network policy would be gotten there too.  So I don't know that we need all the code to make it happen on the child? (perhaps there are other uses we need it for on the child?)

2) it sounds like the Network Policy for an app is the same for all channels (so we don't need to get a new policy check for each channel).  We only need to get a new policy when the app's network policy as a whole changes ("facebook app must now only uses wifi network", etc.)  right?

In that case, I can see two models for getting the policy info to the CSP in such a way that it will have the most up-to-date values:

1) like for cookies, the policy manager can offer a synchronous API which usually does not block.  For nsICookieService, this is because it keeps all policy info in RAM, and only ever blocks if it's called early in startup (when it's reading info from a database).  Blocking once at startup is probably OK.

2) We can have the CSP get all apps policies (asynchronously or sync, doesn't matter much) at startup, and then have the policy manager issue a notification when an app's policy changes (or a new app is installed).  This would use nsIObserverService to send out the notification, and then the CSP that Patrick is writing could query the policy manager to get the new policy.  In this case, we could probably have an async API (unless we really need to enforce the policy 100% immediately, which I doubt we do: i.e. if there's .5 seconds before the app sees the policy change, that's probably OK--we're not going to be able to do things like stop ongoing downloads anyway--if you tell an app to only update when on wifi, and it's already 50% through a download, it won't stop).

#2 basically means CSP needs to keep around the whole network policy database in memory, so I'd guess #1 is better?  Note that I'm only talking about offering a sync API on the parent, not the child.

I hope this is clear--I've poked around the patches here but it's unfamiliar code to me, so I might be missing important design issues.

Comment 23

[Albert [:albert]]

Attachment: WIP v3 - JavaScript implementation of mozNetworkPolicies

Added simple cache

Comment 24

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: IDLs - V2

Added "defaultPolicyName" attribute to nsIDOMMozNetworkPoliciesManager.

The idea is: Default policies will be stored and recovered using the "name" given by this attribute. Also If you query an nonexistent app, the component will return the default policies.

For recover ALL the stored policies into an array, use an empty string in the get method:

navigator.mozNetworkPolicies.get("") -> Will return an array of stored policies
navigator.mozNetworkPolicies.get(navigator.mozNetworkPolicies.defaultPolicyName) -> Will return the default policy

Comment 25

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: WIP v4 - JavaScript implementation of mozNetworkPolicies

Added a method to recover all stored policies (getAllPolicies). Used when the policy name is an empty string:

  navigator.mozNetworkPolicies.get("")

Added a way to get default policy when the queried app is not stored into the policy DB

  navigator.mozNetworkPolicies.get("facebook")

will return st like:

  {
    "app":"facebook",
    "allowNetworkAccess":true,
    "policies": [
      {
        "connectionType":"wifi",
        "allowed":true,
        "prefered":true,
        "max":1000
      },{
        "connectionType":"mobile",
        "allowed":true,
        "prefered":false,
        "max":400
      }
    ]
  }

Finally if the app is not yet stored, will return the "__default" app name as a result (ie, the default policy):


  navigator.mozNetworkPolicies.get("no_stored_app")

  {
    "app":"_default",
    "allowNetworkAccess":true,
    "policies": [
      {
        "connectionType":"wifi",
        "allowed":true,
        "prefered":true,
        "max":1000
      },{
        "connectionType":"mobile",
        "allowed":true,
        "prefered":false,
        "max":400
      }
    ]
  }

Note: The defaultPolicyName can be retrieved using the "defaultPolicyName" attribute:

  navigator.mozNetworkPolicies.defaultPolicyName

will return: _default

----

TODO: Cache the "All policies" method to offer a sync method to recover it by Necko side. @see: https://bugzilla.mozilla.org/show_bug.cgi?id=746074#c21

Comment 26

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: IDLs - V3

Added a new method (getSync) to recover policies synchronously

Comment 27

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: WIP v5 - JavaScript implementation of mozNetworkPolicies

Improved cache (in a new class)
Added method to recover cached policies synchronously

Comment 28

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Patrick Wang [:kk1fff] from comment #21)
> I think synchronous methods would be needed by implementation in necko side.
> Could you also provide them?

Hi Patrick, the WIPv5 includes a new getSync method to recover policies synchronous. (only cached ones), so use get("") to cache ALL stored policies before the new one.

Regards.

Comment 29

[Philipp von Weitershausen [:philikon]]

Comment on attachment 652414 [details] [diff] [review]
IDLs - V3

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

::: dom/network/interfaces/nsIDOMNetworkPoliciesManager.idl
@@ +7,5 @@
> +#include "nsISupports.idl"
> +
> +interface nsIDOMDOMRequest;
> +
> +dictionary NetworkPoliciesPolicyConnection

Why not just `NetworkPoliciesConnection`?

@@ +15,5 @@
> +  boolean prefered;
> +  long max;
> +};
> +
> +dictionary NetworkPoliciesPolicy

Why not just `NetworkPolicy`?

@@ +29,5 @@
> +  readonly attribute jsval connectionTypes;             // DOMString[]
> +  readonly attribute DOMString defaultPolicyName;       // Name for the default settings (to be used instead the application name)
> +
> +  /**
> +   * Assing new policy

Did you mean "Assign"? If so, assign to what? Maybe "enable", "create" or "set" are better verbs here.

@@ +33,5 @@
> +   * Assing new policy
> +   *
> +   * typeof policies = NetworkPoliciesPolicy
> +   *
> +   * If policy.app is not provided, assing default policy

Please use JavaDoc-style comments here, e.g.:

  * @param policy [optional]
  *        A NetworkPolicy object that describes the policy to put
  *        in effect. If `policy.app` is not provided, the default
  *        network policy is affected.

@@ +38,5 @@
> +   */
> +  nsIDOMDOMRequest set(in jsval policy);
> +
> +  /**
> +   * Recover a policy

"Recover" is not the verb you want here. I suggest "retrieve"

@@ +45,5 @@
> +
> +  /**
> +   * Recover a policy synchronously
> +   */
> +  jsval getSync(in DOMString application);

Why does the asynchronous `get` method even exist if this exists? Or, to ask differently, why do we need a synchronous method?

Comment 30

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Philipp von Weitershausen [:philikon] from comment #29)
> Comment on attachment 652414 [details] [diff] [review]
> IDLs - V3
> 
> Review of attachment 652414 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/interfaces/nsIDOMNetworkPoliciesManager.idl
> @@ +7,5 @@
> > +#include "nsISupports.idl"
> > +
> > +interface nsIDOMDOMRequest;
> > +
> > +dictionary NetworkPoliciesPolicyConnection
> 
> Why not just `NetworkPoliciesConnection`?
> 

Agree, I'll change it for next release.

> @@ +15,5 @@
> > +  boolean prefered;
> > +  long max;
> > +};
> > +
> > +dictionary NetworkPoliciesPolicy
> 
> Why not just `NetworkPolicy`?
> 

Same. I'll change it for next release.

> @@ +29,5 @@
> > +  readonly attribute jsval connectionTypes;             // DOMString[]
> > +  readonly attribute DOMString defaultPolicyName;       // Name for the default settings (to be used instead the application name)
> > +
> > +  /**
> > +   * Assing new policy
> 
> Did you mean "Assign"? If so, assign to what? Maybe "enable", "create" or
> "set" are better verbs here.
> 

That's true, is assign, sorry for the mistake :p

The main idea is to create/modify the policy. As you can see, the method name is "SET" so as you suggested, "set" is a better verb.

> @@ +33,5 @@
> > +   * Assing new policy
> > +   *
> > +   * typeof policies = NetworkPoliciesPolicy
> > +   *
> > +   * If policy.app is not provided, assing default policy
> 
> Please use JavaDoc-style comments here, e.g.:
> 
>   * @param policy [optional]
>   *        A NetworkPolicy object that describes the policy to put
>   *        in effect. If `policy.app` is not provided, the default
>   *        network policy is affected.
> 

Ok.

> @@ +38,5 @@
> > +   */
> > +  nsIDOMDOMRequest set(in jsval policy);
> > +
> > +  /**
> > +   * Recover a policy
> 
> "Recover" is not the verb you want here. I suggest "retrieve"
> 

or "GET"? (in order to be aligned with the recover one ("SET")

> @@ +45,5 @@
> > +
> > +  /**
> > +   * Recover a policy synchronously
> > +   */
> > +  jsval getSync(in DOMString application);
> 
> Why does the asynchronous `get` method even exist if this exists? Or, to ask
> differently, why do we need a synchronous method?

Well, this is because the async method is used by the main API but Necko guys require a sync method to recover it while creating a new connection (see comment #21)

Since the indexedDb is used async. I think the async method for recover policies is better in order to avoid block webapps calls.

Comment 31

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

We've been discussing to include the billing cycle into this interface to be able to determinate the consume of an application (see bug #746073 & #746069)

What do you think?

Comment 32

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: IDLs - V4

Changed Philikon's suggestion.
Also added a new const to define a global interface policy (to be used by bug #746069)

Comment 33

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: WIP v6 - JavaScript implementation of mozNetworkPolicies

Changed to be compatible with the new IDL (v4)

Comment 34

[Philipp von Weitershausen [:philikon]]

Comment on attachment 652416 [details] [diff] [review]
WIP v5 - JavaScript implementation of mozNetworkPolicies

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

::: dom/network/src/NetworkPoliciesDB.jsm
@@ +77,5 @@
> +    this.newTxn("readwrite", function(txn, store) {
> +      debug("Going to store " + JSON.stringify(policy));
> +
> +      if (!txn.result)
> +        txn.result = {};

Braces. Please address this here and everywhere else.

::: dom/network/src/NetworkPoliciesManager.js
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict"
> +
> +/* static functions */

This comment makes no sense in JS. Please remove.

@@ +6,5 @@
> +
> +/* static functions */
> +let DEBUG = true;
> +if (DEBUG) {
> +  debug = function (s) { dump("-*- NetworkPoliciesManager: " + s + "\n"); }

Missing semicolon at the end of the line. Also, please explicitly declare `debug` as a global variable.

I know you copies these lines of code from other places. That doesn't mean you need to copy their mistakes. :)

(Please address this in other files, too.)

@@ +13,5 @@
> +}
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;

Shorter:

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

@@ +22,5 @@
> +Cu.import("resource://gre/modules/NetworkPoliciesService.jsm");
> +
> +XPCOMUtils.defineLazyGetter(Services, "DOMRequest", function() {
> +  return Cc["@mozilla.org/dom/dom-request-service;1"].getService(Ci.nsIDOMRequestService);
> +});

Get rid of this, please. Services.DOMRequest already exists. And even if it didn't, it shouldn't be monkey-patched like that.

@@ +30,5 @@
> +                .getService(Ci.nsIFrameMessageManager)
> +                .QueryInterface(Ci.nsISyncMessageSender);
> +});
> +
> +const nsIClassInfo                      = Ci.nsIClassInfo;

whitespace much? :)

@@ +32,5 @@
> +});
> +
> +const nsIClassInfo                      = Ci.nsIClassInfo;
> +
> +// NetworkPoliciesPolicyConnection & NetworkPoliciesPolicy are not directly instantiated. They are used as interfaces.

I don't understand this sentence. Please clarify. And also please wrap at 79 characters.

@@ +50,5 @@
> +
> +NetworkPoliciesPolicyConnection.prototype = {
> +  get connectionType() {
> +    return this._connectionType;
> +  },

These getters and setters are not necessary. Please remove them.

@@ +160,5 @@
> +  __proto__: DOMRequestIpcHelper.prototype,
> +
> +  get connectionTypes() {
> +    debug("get connectionTypes");
> +    if(this.hasPrivileges) {

Nit: space between `if` and parenthesis. Please address this here and everywhere else.

@@ +170,5 @@
> +
> +  get defaultPolicyName() {
> +    debug("get defaultPolicyName: " + JSON.stringify(NetworkPoliciesService.defaultPolicyName));
> +    if(this.hasPrivileges) {
> +      return cpmm.sendSyncMessage("NetworkPolicies:GetDefaultPolicyName")[0];

Better: bail out early style for every method here:

  if (!this.hasPrivileges) {
    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
  }

and even better: make this a helper method that you just invoke at the top of every public method.

@@ +226,5 @@
> +            debug("firing success: " + JSON.stringify(_policy));
> +            Services.DOMRequest.fireSuccess(req, _policy);
> +          } else {
> +            let _policies = [];
> +            for(let _policy in msg.policies) {

Why the leading underscores? Remove, please. (Here and everywhere else, please.)

@@ +264,5 @@
> +    this.initHelper(aWindow, ["NetworkPolicies:Get:Return:OK", "NetworkPolicies:Get:Return:KO",
> +                              "NetworkPolicies:Set:Return:OK", "NetworkPolicies:Set:Return:KO"]);
> +
> +    let principal = aWindow.document.nodePrincipal;
> +    let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"].getService(Ci.nsIScriptSecurityManager);

Please use `Services.scriptSecurityManager`.

::: dom/network/src/NetworkPoliciesService.jsm
@@ +83,5 @@
> +  free: function free() {
> +    debug("NetworkPoliciesCache: free");
> +    debug(JSON.stringify(this.policiesCache));
> +
> +    let time = new Date().getTime();

let time = Date.now();

@@ +85,5 @@
> +    debug(JSON.stringify(this.policiesCache));
> +
> +    let time = new Date().getTime();
> +    debug("NetworkPoliciesCache: Free time threshold: " + time);
> +    let alfa = 0;

What is "alfa" supposed to mean?

@@ +129,5 @@
> +    this.messages.forEach(function(msgName) {
> +      ppmm.addMessageListener(msgName, this);
> +    }, this);
> +
> +    var idbManager = Components.classes["@mozilla.org/dom/indexeddb/manager;1"].getService(Ci.nsIIndexedDatabaseManager);

Wrap overlong line, please.

@@ +142,5 @@
> +          Services.perms.add(Services.io.newURI(aHost, null, null), "networkpolicies-manage",
> +                             Ci.nsIPermissionManager.ALLOW_ACTION);
> +      });
> +    } catch(e) { debug(e); }
> +*/

Um, no commented code in patches, please.

@@ +224,5 @@
> +    if(typeof(_policy) != "object") {
> +      aErrorMsg = "Policy shall be a NetworkPoliciesPolicy object";
> +    }
> +    if(aErrorMsg != "") {
> +      ppmm.sendAsyncMessage("NetworkPolicies:Set:Return:KO", { id: msg.id, errorMsg: aErrorMsg });

Don't broadcast the response to all content processes, please. Use message.target instead. (See e.g. bug 777202 for an example.)

@@ +236,5 @@
> +
> +        // Update cache
> +        NetworkPoliciesCache.write(_policy.app, _policy);
> +      }.bind(this),
> +      function(aErrorMsg) { ppmm.sendAsyncMessage("NetworkPolicies:Set:Return:KO", { id: msg.id, errorMsg: aErrorMsg }); }

The (..., successCb, errorCB) signature pattern in your DB methods makes for some really ugly code here in the consumers. I would suggest going to a single callback with the signature (error, result) (also known as the node.js convention.) Then your DB calls can look like this:

  this._db.addPolicy(_policy, function (error, result) {
    if (error) {
      // notify error, etc.
      return;
    }
    // handle success case, etc.
  });

That's much cleaner and easier to read than passing two function expressions as parameters.

@@ +249,5 @@
> +
> +    // TODO: Validate input data
> +    if(typeof(_appName) != "string") {
> +      aErrorMsg = "Application name shall be a string";
> +    }_appName

?!?

Comment 35

[Philipp von Weitershausen [:philikon]]

Comment on attachment 652416 [details] [diff] [review]
WIP v5 - JavaScript implementation of mozNetworkPolicies

Sorry, I had started reviewing this (v5) while v6 was uploaded.

Comment 36

[Philipp von Weitershausen [:philikon]]

Comment on attachment 652721 [details] [diff] [review]
WIP v6 - JavaScript implementation of mozNetworkPolicies

Please see my review comments for v5.

Comment 37

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Thank you Philipp. Now I'm working on this changes, anyway I would like to ask a couple of points:

(In reply to Philipp von Weitershausen [:philikon] from comment #34)
> Comment on attachment 652416 [details] [diff] [review]
> WIP v5 - JavaScript implementation of mozNetworkPolicies
> 
> Review of attachment 652416 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +85,5 @@
> > +    debug(JSON.stringify(this.policiesCache));
> > +
> > +    let time = new Date().getTime();
> > +    debug("NetworkPoliciesCache: Free time threshold: " + time);
> > +    let alfa = 0;
> 
> What is "alfa" supposed to mean?
> 

Is a cache invalidator factor, I renamed it ;)

> 
> @@ +236,5 @@
> > +
> > +        // Update cache
> > +        NetworkPoliciesCache.write(_policy.app, _policy);
> > +      }.bind(this),
> > +      function(aErrorMsg) { ppmm.sendAsyncMessage("NetworkPolicies:Set:Return:KO", { id: msg.id, errorMsg: aErrorMsg }); }
> 
> The (..., successCb, errorCB) signature pattern in your DB methods makes for
> some really ugly code here in the consumers. I would suggest going to a
> single callback with the signature (error, result) (also known as the
> node.js convention.) Then your DB calls can look like this:
> 
>   this._db.addPolicy(_policy, function (error, result) {
>     if (error) {
>       // notify error, etc.
>       return;
>     }
>     // handle success case, etc.
>   });
> 
> That's much cleaner and easier to read than passing two function expressions
> as parameters.
> 

We don't like this "2 callbacks" way but the indexedDb requires different callback functions ... :(

Comment 38

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: WIP v7 - JavaScript implementation of mozNetworkPolicies

Applied v6 feedback suggestions.

Comment 39

[Philipp von Weitershausen [:philikon]]

(In reply to Fernando R. Sela from comment #37)
> We don't like this "2 callbacks" way but the indexedDb requires different
> callback functions ... :(

Huh? IndexedDBHelper.prototype.newTxn() might, but that's no reason for your database's interface to do the same. E.g.:

  newTransaction: newTransaction(txn_type, callback, txnCb) {
    function successCb(result) {
      txnCb(null, result);
    }
    function errorCb(error) {
      txnCb(error, null);
    }
    return this.newTxn(txn_type, callback, successCb, errorCb);
  },

We could even add this variant (perhaps under a better name) to IndexedDBHelper.

Comment 40

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: JavaScript implementation of mozNetworkPolicies - V8

Rebased & only 1 callback for DB operations.

Comment 41

[Philipp von Weitershausen [:philikon]]

I will have to de-prioritize the review for this since it's not a basecamp blocker anymore.

Comment 42

[Philipp von Weitershausen [:philikon]]

Comment on attachment 656522 [details] [diff] [review]
JavaScript implementation of mozNetworkPolicies - V8

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

This patch seems to be missing a bunch of files.

Comment 43

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: JavaScript implementation of mozNetworkPolicies - V8.1

Sorry, now it's complete.

Comment 44

[Philipp von Weitershausen [:philikon]]

Comment on attachment 657224 [details] [diff] [review]
JavaScript implementation of mozNetworkPolicies - V8.1

Removing this from my review queue since it's not a basecamp blocker. Let's come back to this after v1.

Comment 45

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

Comment on attachment 652720 [details] [diff] [review]
IDLs - V4

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

Same as philikon.

Comment 46

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Important content use case (content doesn't throttle itself), gives us advantage over competition.

Comment 47

[John Shih]

Hi Fernando,
will you still work on this bug after the network per-app metering in done?
because there have already some progress in network per-app metering (bug 784575, bug 746073, ..etc)
and network per-app policies is likely the following bug.

another thing, since there is a work for usage alarm for new version of NetworkStats API (bug 858005)
do you think there is any duplicated/analogue function between this bug and that?
That is, it this API still suitable to be created independently? 

Need your feedback! Thanks :)

Comment 48

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to John Shih from comment #47)
> Hi Fernando,
> will you still work on this bug after the network per-app metering in done?
> because there have already some progress in network per-app metering (bug
> 784575, bug 746073, ..etc)
> and network per-app policies is likely the following bug.
> 
> another thing, since there is a work for usage alarm for new version of
> NetworkStats API (bug 858005)
> do you think there is any duplicated/analogue function between this bug and
> that?
> That is, it this API still suitable to be created independently? 
> 
> Need your feedback! Thanks :)

Hi John,

Yes, Albert informed me about these advanced in related bugs so will be great to resume the works on this bug.

Currently I'm working in some urgent issues but I would like to work on this too. If this is really urgent I can assign another guy of my team to work on this.

Which is the expected delivery date? :p

Comment 49

[John Shih]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #48)
> (In reply to John Shih from comment #47)
> > Hi Fernando,
> > will you still work on this bug after the network per-app metering in done?
> > because there have already some progress in network per-app metering (bug
> > 784575, bug 746073, ..etc)
> > and network per-app policies is likely the following bug.
> > 
> > another thing, since there is a work for usage alarm for new version of
> > NetworkStats API (bug 858005)
> > do you think there is any duplicated/analogue function between this bug and
> > that?
> > That is, it this API still suitable to be created independently? 
> > 
> > Need your feedback! Thanks :)
> 
> Hi John,
> 
> Yes, Albert informed me about these advanced in related bugs so will be
> great to resume the works on this bug.
> 
> Currently I'm working in some urgent issues but I would like to work on this
> too. If this is really urgent I can assign another guy of my team to work on
> this.
> 
> Which is the expected delivery date? :p

it's definitely great to know that you can still get back on this work!
and I think it won't be that urgent so you don't have to worry about that ;)

so basically I'll working on the network part which is previously owned by Patrick
and there are going to have some discussion on implementation detail about it.

I'll make sure you can know about the progress and also may need your feedback on
API part.

it's my precious to work with you! I'm looking forward it :)

Comment 50

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to John Shih from comment #49)
> (In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from
> comment #48)
> > (In reply to John Shih from comment #47)
> > > Hi Fernando,
> > > will you still work on this bug after the network per-app metering in done?
> > > because there have already some progress in network per-app metering (bug
> > > 784575, bug 746073, ..etc)
> > > and network per-app policies is likely the following bug.
> > > 
> > > another thing, since there is a work for usage alarm for new version of
> > > NetworkStats API (bug 858005)
> > > do you think there is any duplicated/analogue function between this bug and
> > > that?
> > > That is, it this API still suitable to be created independently? 
> > > 
> > > Need your feedback! Thanks :)
> > 
> > Hi John,
> > 
> > Yes, Albert informed me about these advanced in related bugs so will be
> > great to resume the works on this bug.
> > 
> > Currently I'm working in some urgent issues but I would like to work on this
> > too. If this is really urgent I can assign another guy of my team to work on
> > this.
> > 
> > Which is the expected delivery date? :p
> 
> it's definitely great to know that you can still get back on this work!
> and I think it won't be that urgent so you don't have to worry about that ;)
> 
> so basically I'll working on the network part which is previously owned by
> Patrick
> and there are going to have some discussion on implementation detail about
> it.
> 
> I'll make sure you can know about the progress and also may need your
> feedback on
> API part.
> 
> it's my precious to work with you! I'm looking forward it :)

Great !

So as soon as I finish one urgent work, I'll re-study this bug to remember all the details ;)

We'll be in touch.

Comment 51

[John Shih]

Hi Fernando,

It has been a while since our last contact! Recently, I just finished my previous work and was trying to find some new works to do. Then, this bug, which greatly matches my knowledge and my team, comes to my mind. If you don't mind, I'd like to take this bug and start to work on new API proposal (based on the existing one). Not sure if you can agree with that.

Thanks!

Comment 52

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to John Shih from comment #51)
> Hi Fernando,
> 
> It has been a while since our last contact! Recently, I just finished my
> previous work and was trying to find some new works to do. Then, this bug,
> which greatly matches my knowledge and my team, comes to my mind. If you
> don't mind, I'd like to take this bug and start to work on new API proposal
> (based on the existing one). Not sure if you can agree with that.
> 
> Thanks!

Hi John,

Thank you for your interest in this ;)

Highest priorities in this project moved me to other topics so this bug hadn't been changed in a time :(

I'm not against new proposals. The current patch proposed a simple network policy management but I'm happy to see new ones ;)

In my team started to rebase it but we can leave it to you; we'll help in our possibilities

Regards.

Comment 53

[John Shih]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #52)
> (In reply to John Shih from comment #51)
> > Hi Fernando,
> > 
> > It has been a while since our last contact! Recently, I just finished my
> > previous work and was trying to find some new works to do. Then, this bug,
> > which greatly matches my knowledge and my team, comes to my mind. If you
> > don't mind, I'd like to take this bug and start to work on new API proposal
> > (based on the existing one). Not sure if you can agree with that.
> > 
> > Thanks!
> 
> Hi John,
> 
> Thank you for your interest in this ;)
> 
> Highest priorities in this project moved me to other topics so this bug
> hadn't been changed in a time :(
> 
> I'm not against new proposals. The current patch proposed a simple network
> policy management but I'm happy to see new ones ;)
> 
> In my team started to rebase it but we can leave it to you; we'll help in
> our possibilities
> 
> Regards.

Thanks for your response! I'll start to work on the proposal on a basis of the existing one. It definitely needs your feedback then ;)