855948 - [Per App Network Traffic Metering] Collect per app traffic in FTP protocol layer

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Comment 1

[Jason Duell]

We may need on-modify-request style notifications to choke off FTP traffic (maybe), but we don't need them to simply collect traffic stats.

Comment 2

[John Shih]

Hi Jason,
the current ideal is to count the traffic and fire an event(notifyObserver) in the end of transmission.
Than there can be a place to register the event and then get the traffic stats.

Is it match what you thought?

Comment 3

[John Shih]

There is another concern for this bug.
I can not think too much cases (especially for firefox os) that can cause network traffic through
ftp protocol. (As far as I know, any file downloading or uploading is not allowed.)
This may also makes it difficult to have real cases to test..
So, many be this bug will turn to a general bug that expose a event for other use
instead of for network per-app traffic metering.

what do you think?

Comment 4

[John Shih]

I think there are some requirements that can make ftp really work in firefox os:

1. Like what bluetooth did, there may need to support the "download" action in the browser,
and the target file will saved to a specific folder which is restrict in SD card.

2. Moreover, there needs something like FileManager API so that we can examine and access the downloaded file
(same as what android did)

without these requirements, I think the ftp protocol as well as this work can not really mean to firefox os.

Comment 5

[Jason Duell]

> count the traffic and fire an event(notifyObserver) in the end of transmission.

Yes, that sounds like the right architecture.  Much like what you did for HTTP.

> I can not think too much cases (especially for firefox os) that can cause network traffic through ftp protocol. (As far as I know, any file downloading or uploading is not allowed.)

If that's the case, then this bug is low priority. (i.e. we don't need it now, but might someday if we support downloading).  Do you know of any bug for allowing B2G downloads? We would want to make this block that (so we don't forget to count FTP downloads if they start happening).

Comment 6

[John Shih]

(In reply to Jason Duell (:jduell) from comment #5)
> > count the traffic and fire an event(notifyObserver) in the end of transmission.
> 
> Yes, that sounds like the right architecture.  Much like what you did for
> HTTP.
> 
> > I can not think too much cases (especially for firefox os) that can cause network traffic through ftp protocol. (As far as I know, any file downloading or uploading is not allowed.)
> 
> If that's the case, then this bug is low priority. (i.e. we don't need it
> now, but might someday if we support downloading).  Do you know of any bug
> for allowing B2G downloads? We would want to make this block that (so we
> don't forget to count FTP downloads if they start happening).

As far as I know that there is no planning on downloading support for b2g (after asking several people)
but I'll keep my attention on any related progress and put this bug in my tracking list.

Comment 7

[John Shih]

Attachment: (wip) Bug 855948 - Network Per-App Metering on FTP Layer.

This is the wip patch for network per-app metering on FTP patch.
Similar to how it did on HTTP and Websocket, the downloading traffic through data transport is counted and then an event will be fired before the data transport is closing.

But also differently, in this case the metering information is carried by FtpChannel (which is the subject of the event).

not sure this design works for you, needs your feedback on it

Thanks

Comment 8

[John Shih]

Attachment: (wip) Bug 855948 - Network Per-App Metering on FTP Layer.

Comment 9

[Patrick McManus [:mcmanus]]

Comment on attachment 815719 [details] [diff] [review]
(wip) Bug 855948 - Network Per-App Metering on FTP Layer.

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

::: netwerk/base/public/nsINetworkStats.idl
@@ +9,5 @@
> + * This interface is used for keeping the data in network
> + * per-app statistics.
> + */
> +[scriptable, uuid(17a67f50-7b09-4407-bb85-fcc1fb34029a)]
> +interface nsINetworkStats : nsISupports

didn't you already introduce a similar structure in a different bug?

at the very least, a generic nsINetworkStats should contain bidirectional counters (even if FTP only populates downstream ones) but I'm worried about unnecessary proliferation

::: netwerk/protocol/ftp/nsFTPChannel.h
@@ +27,5 @@
>                       public nsIFTPChannel,
>                       public nsIUploadChannel,
>                       public nsIResumableChannel,
> +                     public nsIProxiedChannel,
> +                     public nsINetworkStats

how does this work on a platform where you didn't include nsINetworkStats?

::: netwerk/protocol/ftp/nsFtpConnectionThread.cpp
@@ +1679,2 @@
>  nsresult
>  nsFtpState::Init(nsFtpChannel *channel)

this should assert main thread

@@ +2184,5 @@
>      // input stream instead of mDataStream.
>  
>      if (mDataStream) {
> +        // network metering
> +        mChannel->CountRecvBytes(count);

I'm pretty sure this needs to be the value of result after ->readsegments and only if NS_SUCCEEDED..

Comment 10

[John Shih]

(In reply to Patrick McManus [:mcmanus] from comment #9)
> Comment on attachment 815719 [details] [diff] [review]
> (wip) Bug 855948 - Network Per-App Metering on FTP Layer.
> 
> Review of attachment 815719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/public/nsINetworkStats.idl
> @@ +9,5 @@
> > + * This interface is used for keeping the data in network
> > + * per-app statistics.
> > + */
> > +[scriptable, uuid(17a67f50-7b09-4407-bb85-fcc1fb34029a)]
> > +interface nsINetworkStats : nsISupports
> 
> didn't you already introduce a similar structure in a different bug?
> 
> at the very least, a generic nsINetworkStats should contain bidirectional
> counters (even if FTP only populates downstream ones) but I'm worried about
> unnecessary proliferation
> 

as Jason suggested to use on-modify-request style on-modify-request style notifications on
FTP part, so I'm trying to do the same thing.
One of problems is how to carry the network stats (i.e., appid, rxbytes, txbytes) with the notification.
In this patch, I use the FtpChannel (the Subject in notification) to carry these stats
that's why I created this idl for bundling the stats to FtpChannel.

I'm not sure this way is right, or I can just change to what I did in Http/Websocket part.

> ::: netwerk/protocol/ftp/nsFTPChannel.h
> @@ +27,5 @@
> >                       public nsIFTPChannel,
> >                       public nsIUploadChannel,
> >                       public nsIResumableChannel,
> > +                     public nsIProxiedChannel,
> > +                     public nsINetworkStats
> 
> how does this work on a platform where you didn't include nsINetworkStats?
> 
> ::: netwerk/protocol/ftp/nsFtpConnectionThread.cpp
> @@ +1679,2 @@
> >  nsresult
> >  nsFtpState::Init(nsFtpChannel *channel)
> 
> this should assert main thread
> 
> @@ +2184,5 @@
> >      // input stream instead of mDataStream.
> >  
> >      if (mDataStream) {
> > +        // network metering
> > +        mChannel->CountRecvBytes(count);
> 
> I'm pretty sure this needs to be the value of result after ->readsegments
> and only if NS_SUCCEEDED..

Comment 11

[Patrick McManus [:mcmanus]]

(In reply to John Shih from comment #10)
> (In reply to Patrick McManus [:mcmanus] from comment #9)
> > Comment on attachment 815719 [details] [diff] [review]
> > (wip) Bug 855948 - Network Per-App Metering on FTP Layer.
> > 
> 
> 
> I'm not sure this way is right, or I can just change to what I did in
> Http/Websocket part.
> 

before I can answer this I need to understand the background on why you wouldn't do it the http/websocket way. pros/cons..

Comment 12

[John Shih]

(In reply to Patrick McManus [:mcmanus] from comment #11)
> (In reply to John Shih from comment #10)
> > (In reply to Patrick McManus [:mcmanus] from comment #9)
> > > Comment on attachment 815719 [details] [diff] [review]
> > > (wip) Bug 855948 - Network Per-App Metering on FTP Layer.
> > > 
> > 
> > 
> > I'm not sure this way is right, or I can just change to what I did in
> > Http/Websocket part.
> > 
> 
> before I can answer this I need to understand the background on why you
> wouldn't do it the http/websocket way. pros/cons..

the original thought seems what it said on bug 782085.
that is, in order further to support the network policy,
the notify style way is more likely to do that.

so maybe we need to figure out more detail on how we are going to
implement the network policy (it also will be the following bug of network metering)

Comment 13

[John Shih]

Here is the previous discussion about using notify-style way:
https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.tech.network/5lsJ90U5VEQ

also, we also need to get back to the discussion on network-policy related bugs:
bug 780087, bug 786419, bug 746074

there are still some undetermined ways on these works, the brief summaries are:
1. we intended to use notify-style way to choke off network access (channel) according to some policies on
different protocols. (bug 780087)

2. provide the network offline status for apps so that protocols can check the status before connection (bug 784619).
if following this work, notify-style way may not be necessary (please correct me if i'm wrong).

3. where to check the policy is another question. there are two provided ways:
   a. a service (named nsTrafficBlocker) actively query the app stats from NetworkStatsService
      and to check if there is an app exceeds its limitation.
   b. NetworkStatsService checks the app stats itself and fired an event when there is an app
      exceeds its limitation.

Comment 14

[Patrick McManus [:mcmanus]]

(In reply to John Shih from comment #12)

> > before I can answer this I need to understand the background on why you
> > wouldn't do it the http/websocket way. pros/cons..
> 
> the original thought seems what it said on bug 782085.
> that is, in order further to support the network policy,
> the notify style way is more likely to do that.
> 
> so maybe we need to figure out more detail on how we are going to
> implement the network policy (it also will be the following bug of network
> metering)

given what I've read I think you should report transfer events in the same manner as http and worry about the enforcement of new ftp sessions via an ftp-on-modify-request event that we'll need to add in the other bug.

make sense?

Comment 15

[John Shih]

(In reply to Patrick McManus [:mcmanus] from comment #14)
> (In reply to John Shih from comment #12)
> 
> > > before I can answer this I need to understand the background on why you
> > > wouldn't do it the http/websocket way. pros/cons..
> > 
> > the original thought seems what it said on bug 782085.
> > that is, in order further to support the network policy,
> > the notify style way is more likely to do that.
> > 
> > so maybe we need to figure out more detail on how we are going to
> > implement the network policy (it also will be the following bug of network
> > metering)
> 
> given what I've read I think you should report transfer events in the same
> manner as http and worry about the enforcement of new ftp sessions via an
> ftp-on-modify-request event that we'll need to add in the other bug.
> 
> make sense?

sure. so I'll focus just on metering part first
thanks for this helpful suggestion

Comment 16

[Jason Duell]

Comment on attachment 815719 [details] [diff] [review]
(wip) Bug 855948 - Network Per-App Metering on FTP Layer.

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

>  I think you should report transfer events in the same manner as http and worry 
> about the enforcement of new ftp sessions via an ftp-on-modify-request event 
> that we'll need to add in the other bug.

That sounds right to me.

::: netwerk/base/public/nsINetworkStats.idl
@@ +9,5 @@
> + * This interface is used for keeping the data in network
> + * per-app statistics.
> + */
> +[scriptable, uuid(17a67f50-7b09-4407-bb85-fcc1fb34029a)]
> +interface nsINetworkStats : nsISupports

I agree here too--like with websocket/http/TCP, we can just fire a nsINetworkStatsServiceProxy.saveAppStats() with 0 for transmitted bytes.  Doesn't look like we need nsINetworkStats.

::: netwerk/protocol/ftp/nsFtpConnectionThread.cpp
@@ +1690,5 @@
> +    // Obtain app information
> +    uint32_t appId;
> +    bool isInBrowser;
> +    NS_GetAppInfo(mChannel, &appId, &isInBrowser);
> +    mChannel->SetAppId(appId);

No need for mAppId member variable in nsFTPChannel--we can always use NS_GetAppInfo to get it when needed.

Comment 17

[John Shih]

Attachment: Bug 855948 - Network Per-App Metering on FTP Layer. r=mcmanus

I've rewrite the patch to use the similar way as http part.
there are only few differences including:
1. nsFtpConnectionThread seems doesn't run in the main thread
so the code of obtaining network interface is also dispatched
to the main thread.
2. refers to Jason's comment, remove the mAppId member because it
can easily be obtained through mChannel.

Comment 18

[John Shih]

Attachment: Bug 855948 - Network Per-App Metering on FTP Layer. r=mcmanus

oops... I think I was wrong
nsFtpConnectionThread is actually running in the mainthread
step back to make the code of obtaining network interface as
a independent function.

Comment 19

[John Shih]

Attachment: bug_855948_v2.patch

patch update.

Comment 20

[Patrick McManus [:mcmanus]]

Comment on attachment 822085 [details] [diff] [review]
bug_855948_v2.patch

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

::: netwerk/protocol/ftp/nsFtpConnectionThread.cpp
@@ +2221,5 @@
> +nsFtpState::SaveNetworkStats(bool enforce)
> +{
> +#ifdef MOZ_WIDGET_GONK
> +    // Obtain app id
> +    uint32_t mAppId;

don't name stack variable m*

@@ +2222,5 @@
> +{
> +#ifdef MOZ_WIDGET_GONK
> +    // Obtain app id
> +    uint32_t mAppId;
> +    bool mIsInBrowser;

again - don't name m*

@@ +2224,5 @@
> +    // Obtain app id
> +    uint32_t mAppId;
> +    bool mIsInBrowser;
> +    NS_GetAppInfo(mChannel, &mAppId, &mIsInBrowser);
> +

I would like you to do a lazy-init of mActiveNetwork right inline here..

i.e. if (!mActivenetwork) {
 ALL THE STUFF IN  GetActiveNetwork()
}

and then remove the GetActiveNetwork function.. I just want to minimize the number of gonk specific members and methods. I presume you're on the main thread here? Defintiely assert that.

::: netwerk/protocol/ftp/nsFtpConnectionThread.h
@@ +266,5 @@
> +    const static uint64_t NETWORK_STATS_THRESHOLD = 65536;
> +
> +private:
> +    uint64_t                           mCountRecv;
> +    uint32_t                           mAppId;

mappid isn't used (you have another variable currently named the same thing used on the stack)

Comment 21

[John Shih]

Attachment: Bug 855948 - Network Per-App Metering on FTP Layer. r=mcmanus

refine base on Comment 20

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/46d196169a48

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/46d196169a48