Closed Bug 786419 Opened 12 years ago Closed 10 years ago

Provide way to "set network offline" per app

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jduell.mcbugs, Assigned: valentin)

References

Details

(Keywords: dev-doc-needed, verifyme)

Attachments

(12 files, 42 obsolete files)

4.95 KB, patch
valentin
: review+
Details | Diff | Splinter Review
2.40 KB, patch
valentin
: review+
Details | Diff | Splinter Review
6.66 KB, patch
valentin
: review+
Details | Diff | Splinter Review
1.99 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
6.85 KB, patch
valentin
: review+
Details | Diff | Splinter Review
5.10 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
4.39 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
4.41 KB, patch
valentin
: review+
Details | Diff | Splinter Review
14.95 KB, patch
valentin
: review+
Details | Diff | Splinter Review
54.41 KB, patch
valentin
: review+
Details | Diff | Splinter Review
2.38 KB, patch
jduell.mcbugs
: review+
marshall
: feedback+
Details | Diff | Splinter Review
16.19 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
For bug 780087 we need to be able to choke off network access for apps that have exceeded their quota.  Sicking suggests that we translate this as acting as if we were offline, so the app will work normally as if it were offline.

To do this, we're going to need some service that keeps track of which appIds are  "offline", and a way for the channel to check them at the places where we currently check for nsIIOService.isOffline.

Fix #1:  change nsIIOService to add a 'setAppOffline(int appId)' function and a "isAppOffline(int appID) function", and check the latter in places like 

   http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp?rev=a16372ce30b5#5902

Fix #2:  if we want this information in both the parent and child, it might be less work to keep the per-app offline info in some JS-based service, and have channels use

   if (IOService->isOffine || YetAnotherJSService->isMyAppOffline(appid))

This is just because IPDL in C++ takes a lot of code.

Do we need this info in the child?  I don't think there's a need for it in B2G v1:  The parent can silently treat requests from a child app as offline.  There's no way for HTML apps to query if they're offline AFAIK, so no need for the child to know it's being throttled.   Long-term I can imagine we may want to provide that info though.  For now I think #1 is enough.
A couple other issues which I don't have time to look into right now:

- What kind of nsIObserver notifications do we want to send out for this?  We may want to send out something to allow existing connections to shut themselves off, but we can't reuse the existing global offline notifications.   We can put the appID in whatever events we send.

- I looked into how websockets, the JS TCP API, and long-lived HTTP connections (watching videos, etc) get handled when we go offline, and the answer is that the nsIIOService.SetOffline calls nsSocketTransportService::Shutdown, which aborts/cancels all existing channels.   For that to work with appIDs, we'd need to make the socket transport know about appIDs and map them to sockets.  Not impossible, but not as trivial a fix as I was hoping for.  Alternative: we could train the various channels to listen for a 'app-offline' event, get the appID from it, compare it with their own appID and cancel themselves as needed.
Hmm, looks like we would also need to take steps to not issue DNS on a per-app basis--right now nsHttpchannel::AsyncOpen tries to do DNS even when we're in offline mode, and the HTML parser will probably try to do DNS prefetch.   That would involve either changing all the DNS clients refrain from issuing them if their app is offline, or change the DNS IDL so that we've got a way to provide appId info to it.  Yuk.
Attached patch IDL change (obsolete) — Splinter Review
Here's a tossed-off IDL strawman to get us started.
Attachment #656281 - Flags: feedback?(mcmanus)
1. Follow the IDL proposed by :jduell, implement the two functions to keep per app offline setting.

2. Add an argument in nsIOService::IsOffline(), the argument is an app id. The function returns true if the app is set offline or offline is set globally, otherwise it returns false. If caller wants to query global offline information, it should pass NO_APP_ID as appid.

3. Update caller of the IsOffline.
Attachment #658396 - Flags: feedback?(jduell.mcbugs)
(In reply to Jason Duell (:jduell) from comment #0)

> 
> To do this, we're going to need some service that keeps track of which
> appIds are  "offline", and a way for the channel to check them at the places
> where we currently check for nsIIOService.isOffline.
> 

I understand the attraction here, but I think a lot of stuff is going to leak. You mention dns.. speculative connects are another possibility, etc.. every possible network activity isn't easily bound to an app. What about responding to an arp query or other os level functions?

If 98% is good enough, it could work.

the idl is fine.
Attachment #656281 - Flags: feedback?(mcmanus)
> every possible network activity isn't easily bound to an app

If we can register IPDL connections with their appID (and we can--just a question of how), then we can choke off pretty much any network request from a given child process--it may just take time to find each case.  Preconnects are covered by having setting LOAD_ONLY_FROM_CACHE on the regular necko channel, and that flag should ideally block all other network access by the channel (i.e. if it doesn't now we can make it so).

The trickiest case right now is DNS prefetch.  I'm going to look into whether we can associate a PNecko protocol with an appID--we could simply not issue the DNS request in the parent.  The main purpose here is to avoid consuming large amount of mobile bandwidth, so DNS, ARP, etc is a possible miss for B2G v1. 

> To do this, we're going to need some service that keeps track of which appIds
> are  "offline"

We may not need a service, if we just have parent channels (HttpChannelParent, etc) listen to offline nsIObserver notifications, they can set an mOffline variable and have the info they need to set LOAD_ONLY_FROM_CACHE on the 'real' necko channel.
Comment on attachment 658396 [details] [diff] [review]
Part 1 WIP: record offline status per app

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

::: netwerk/base/src/nsIOService.cpp
@@ +1247,5 @@
>                                                    aTarget);
>  }
> +
> +NS_IMETHODIMP
> +nsIOService::SetAppOffline(uint32_t aAppId, bool aIsOffline) {

Your SetAppOffline/isAppOffline functions are ok but I don't think we'll need them--I think we're going to block things at a higher level than nsIOService.  We'll have the {Http,Ftp,WebSocket}ParentChannel classes listen for 'quota-exceeded' notifications, note whether the channel is offline/online in a member variable, and then set LOAD_ONLY_FROM_CACHE on the nsHttpChannel as I described in my previous bug comment (or fail the channel for FTP/Websockets/TCP).  Let's try that approach--ask me here or on IRC if you've got questions.

::: netwerk/base/src/nsIOService.h
@@ +78,5 @@
>      const nsCOMArray<nsIContentSniffer>& GetContentSniffers() {
>        return mContentSniffers.GetEntries();
>      }
>  
> +    bool IsOffline(uint32_t aAppId);

isOffline needs to be kept a non-appID function--it's used in non-B2G to turn off the entire browser, and it needs to stay that way.

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +427,5 @@
>          return NS_ERROR_UNEXPECTED;
>  
>      // Don't initialize inside the offline mode
> +    if (gIOService->IsOffline(nsIScriptSecurityManager::NO_APP_ID)
> +        && !gIOService->IsComingOnline())

This isn't going to work--sorry!  We have only one instance of the socket transport service in the parent, so we can't fail init per appID.

::: netwerk/cache/nsCacheService.cpp
@@ +2167,1 @@
>  

Same thing here--we can't fail this per-appID
Attachment #658396 - Flags: feedback?(jduell.mcbugs)
> {Http,Ftp,WebSocket}ParentChannel

To be precise, {Http,FTP,WebSocket}ChannelParent.cpp.   In the RecvAsyncOpen function have them register for app-offline notifications (and they'll need to check if the app is already offline in case the notification already happened--so we do need an AppIsOffline service of some kind--I assume you can get the info from whatever service fires the app-offline notification--NetworkTrafficService?), and de-register for notifications in OnStopRequest.
another thing to give a few minutes of thought to is bug 421128 - where we'd like to partition connection manager resources (conns per host, etc..) on a per tab basis.. maybe this work could overlap (make a tab equivalent in some relationship to an appid.. add online/offline to the data) and accomplish two goals.
(In reply to Jason Duell (:jduell) from comment #8)
> To be precise, {Http,FTP,WebSocket}ChannelParent.cpp.   In the RecvAsyncOpen
> function have them register for app-offline notifications (and they'll need
> to check if the app is already offline in case the notification already
> happened--so we do need an AppIsOffline service of some kind--I assume you
> can get the info from whatever service fires the app-offline
> notification--NetworkTrafficService?), and de-register for notifications in
Just want to clarify. 
Do you mean the service in bug 780087, or we should add a new service to store per-app offline setting? If offline can only set by network stats, will it be less general? (I mean... maybe there is some other service/app wants to set app offline?) 

> OnStopRequest.
> If offline can only set by network stats, will it be less general?

:kk1fff: yes, you are right.  In particular, we are going to want to turn off the network the exact same way if a user has restricted App XYZ to only use the network on wifi, etc.  I.e bug 746074.

It seems like the plan you laid out in bug 746074 comment 7 is good for this.  I don't know how many different services need to exist, but from necko it would be nice to call a single service and ask "is this app offline or not?".  That could be NetworkPolicyApp, which could query a separate NetworkTrafficHistoryService to check if quota is exceeded. Or we could combine both functions into a single service--I don't have an opinion at this point.

I'm not sure of the right thing to do if we've launched a network load and then we get a notification that the app is now offline.  We can't change a channel to use HTML5 offline app fallback logic at that point (too late), so we need to either let the load finish or cancel it.  I think setting a timeout and additional traffic limit (say 500kb) is the way to go here:  if the channel is not done loading after 60 seconds or 500kb (whichever comes sooner), we cancel it.  That'll allow the vast majority of channels to finish w/o allowing endless youtube video, etc to continue forever past quota limit.
I agree that also fixing bug bug 421128 with same infrastructure if possible would be swell here.  I don't have time to think about it now but I'll look into it soon.
(In reply to Jason Duell (:jduell) from comment #12)
> I agree that also fixing bug bug 421128 with same infrastructure if possible
> would be swell here.  I don't have time to think about it now but I'll look
> into it soon.

maybe we can put our heads together at the necko meetup and see if a grand unified theory of network management is possible.
Please note, this bug is not a basecamp blocker. I.e. it's not something that is urgent for the B2G team.
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
Attachment #656281 - Attachment is obsolete: true
Attachment #658396 - Attachment is obsolete: true
Comment on attachment 8394200 [details] [diff] [review]
Provide way to _set network offline_ per app

f? for the HTTP part of the patch.
I tested it on a B2G. Changing offline status for the browser app from gdb seems to inhibit creation of new connections, cancels active channels and still loads available data from cache.
Dispatching to the main thread for the observer part isn't as clean as I'd like.

Currently working on FTP/Websocket patches.
Attachment #8394200 - Flags: feedback?(jduell.mcbugs)
Jonas--some API questions:

> For network what I'm envisioning is the ability to use an API for setting
> a per-app network policy. I.e. say "only wifi", "however-you-want" or "no
> network at all". We could then let gaia use this to implement more complex
> higher-level policies like "don't allow 3g connectivity when running in
> background", "don't allow more than 5MB of 3g per month", "don't allow
> background 3g when below 30% battery".

Am I right in guessing that at the necko level we only need a

  nsIIOService.setAppOffline(appID, bool isOffline)

call here?  I.e. Gaia/gonk stuff will determine whether we're on wifi and toggle online/offline appropriately?  At the moment I think that the upper B2G levels have a better understanding of what type of network we're using than necko does. And necko has no understanding of what processes are in the background.  I'm open to trying to push that info into necko if we think it's the right way to go.

Some more issues:

1) Do we want to cancel() channels that are in existence when SetOffline() gets called? (and also Websockets and JS UDP/TCP channels?) Or is it enough to stop new channels from being created? If we don't want to be too abrupt we could 1) stop new channels and 2) set a timer that kills existing ones after 1 minute or something like that.  I do think  we'll want to stop existing network connections at some point, or they could consume considerable traffic (and have confusing semantics, ex: websocket-driven updates keep   arriving after app is "offline".

2) My understanding is that the IOService.SetAppOffline() call will be called in the parent process only?  Besides keeping a hashtable so we know which apps' channels need to be  considered offline, my understanding is that we also want to send an IPC to the child and essentially call IOService.SetOffline() (i.e. the global offline/online call that we   currently use in desktop firefox, that fire off "network:offline-status-changed" notifications.

Question: do we really want to use the global setOffline() in the child (and the global offline notifications), or do we want to use setAppOffline() and some new app-specific   notifications?  My concern is with single-process B2G and/or processes that run more than one appID (if there is such a thing, or will be).  We can't just go calling setOffline() globally in these cases, or we'll turn off network access for all apps.  Note that desktop B2G still runs single-process by default IIRC.  So I suspect we're better off using per-app calls here and training all current callsites of IsOffline() to also check IsAppOffline() too.

3) Even if an app is offline we'll still support loading URIs from HTTP cache or appache.  There are various semantics to choose from here regarding cached URIs that would normally require revalidation from the server to use.

It looks like when we're offline, Navigator.cpp at least sets nsIRequest::LOAD_FROM_CACHE

  http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#897

For history navigations, docshell uses VALIDATE_NEVER (which actually does validation sometimes :) so is actually weaker than LOAD_FROM_CACHE):

  http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10161

I'm guessing that we want LOAD_FROM_CACHE semantics?  Do we want those set automatically within necko if an app is marked offline, or do we want to just change existing isOffline() callsites to check isAppOffline() too?
Flags: needinfo?(jonas)
Comment on attachment 8394200 [details] [diff] [review]
Provide way to _set network offline_ per app

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

Valentin: some feedback--I haven't looked over the HTTPChannel parts of the patch yet though.

::: netwerk/base/public/nsNetUtil.h
@@ +1488,5 @@
> + * A convenience function to get app id of a channel. Returns app id or
> + * NO_APP_ID if app id is not available.
> + */
> +inline uint32_t
> +NS_GetAppId(nsIChannel *aChannel) {

put this function next to NS_GetAppInfo in this file.

::: netwerk/base/src/nsIOService.cpp
@@ +1266,5 @@
> +            return NS_ERROR_OUT_OF_MEMORY;
> +        }
> +        nsRefPtr<nsIRunnable> event = NS_NewRunnableMethodWithArg<uint32_t>
> +            (this, &nsIOService::NotifyOffline, aAppId);
> +        NS_DispatchToMainThread(event);

So we need to figure out if this function can be called off-main thread.  My read of nsIOService.SetOffline() is that it only gets called on the main thread (though we're lacking in assertions to that effect, we're not doing any locking so it's not thread safe). A quick search through callsites in the tree seem to back that up (though I can't say I'm 100% sure).  SocketTransport.SetOffline() on the other hand does do locking, so it's designed to be thread safe.

here you're doing some of the work for off-main (dispatching notifyOffline to main), but not doing any locking around the hashtable, which means this code isn't thread safe.

I'm guessing all calls to SetAppOffline will be main thread only, but let's double-check with the B2G folks.
Attachment #8394200 - Flags: feedback?(jduell.mcbugs) → feedback+
> Am I right in guessing that at the necko level we only need a
> 
>   nsIIOService.setAppOffline(appID, bool isOffline)
> 
> call here?  I.e. Gaia/gonk stuff will determine whether we're on wifi and
> toggle online/offline appropriately?

I think we want something like

enum NetworkMode {
  eOffline,
  eWifiOnly,
  eOnline
};

nsIIOService.setAppNetwork(appID, NetworkMode mode)

(Or we could make the second parameter a bitfield, with individual bits for wifi and mobile, but that seems like a minor detail)

If we make it gaia's responsibility to switch apps to offline when we switch from a wifi connection to a mobile connection I would be worried about races. I.e. that we'd accidentally manage to squeeze through a few requests on the mobile connection before all the gaia events are processed and tell necko to turn the application to offline.

But I totally agree that things like foreground/background, number of MBs transferred, phase of moon etc isn't something that necko needs to bother with. We can leave it up to Gaia to track that.

Does this make sense? Does it seem implementable? Should we get the RIL team involved?

> 1) Do we want to cancel() channels that are in existence when SetOffline()
> gets called? (and also Websockets and JS UDP/TCP channels?) Or is it enough
> to stop new channels from being created?

I think we should cancel existing channels as well as prevent additional channels. It's more important that bytes don't go over the wrong connection than that apps behave perfectly.

> Question: do we really want to use the global setOffline() in the child (and
> the global offline notifications), or do we want to use setAppOffline() and
> some new app-specific   notifications?  My concern is with single-process
> B2G and/or processes that run more than one appID (if there is such a thing,
> or will be).  We can't just go calling setOffline() globally in these cases,
> or we'll turn off network access for all apps.  Note that desktop B2G still
> runs single-process by default IIRC.  So I suspect we're better off using
> per-app calls here and training all current callsites of IsOffline() to also
> check IsAppOffline() too.

We could *possibly* get away with doing this on a per-process basis. The only time that multiple apps run in the same process is due to us still running several apps in the parent process. However all those apps are built-in gaia apps (like keyboard and browser). We could possibly simply be really careful and have those apps honor whatever policies that we use without needing any necko support.

However if it's not too hard it would be great to come up with some new notification and make sure that navigator.onLine honors that for the appid it's running inside of. We've definitely discussed having apps share process in order to save memory on the Tarako device.

I'm not sure what else needs to change behavior in response to going offline. Other than actual network requests of course.

> I'm guessing that we want LOAD_FROM_CACHE semantics?

That would be great.

> Do we want those set
> automatically within necko if an app is marked offline

I think so yes.

> or do we want to
> just change existing isOffline() callsites to check isAppOffline() too?

I'm not sure I understand the implications of the two options here. What would the behavior differences be?
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #19)

> enum NetworkMode {
>   eOffline,
>   eWifiOnly,
>   eOnline
> };
> 
> nsIIOService.setAppNetwork(appID, NetworkMode mode)
> 
> Does this make sense? Does it seem implementable? Should we get the RIL team
> involved?
> 

I think this should work. The only change needed would be to make isAppOffline to take this into account and have the RIL expose connection info if it doesn't already. 

> > I'm guessing that we want LOAD_FROM_CACHE semantics?
> 
> That would be great.
>

The LOAD_ONLY_FROM_CACHE flag seems to do the job in this case. Cached websites are still available in the browser after switching it to offline mode.
> If we make it gaia's responsibility to switch apps to offline when we switch
> from a wifi connection to a mobile connection I would be worried about races.
> I.e. that we'd accidentally manage to squeeze through a few requests on the
> mobile connection before all the gaia events are processed and tell necko to
> turn the application to offline.

So right now AFAICT we've got two ways for necko to know if we're on wifi on
B2G: either 2) have gaia call nsIOService.setAppOffline, or 2) having necko
listen to nsINetworkInterface's 'network-interface-state-changed' nsIObserver
notification.  I wouldn't think there'd be much difference as far as races go
(plus we're going to kill open channels anyway).  If there's some way to get the
info faster (particularly on the socket thread) via the RIL, etc. then that might make a difference. Is that what you're suggesting, Jonas?

> enum NetworkMode {
>   eOffline,
>   eWifiOnly,
>   eOnline
> };

The only thing that's gross here is that we have different ways of getting wifi/mobile, depending on platform (right now we also support android):

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp#1912

So I have slight preference, all things equal, to put gaia in charge of just setting a binary offline/online switch (i.e. have it keep track of wifi vs mobile), since it will already need to do this for foreground/background and quotas.  But we can totally support the the eWifiOnly option in necko too if you think getting info from the RIL more quickly would be a significant benefit.

> it would be great to come up with some new notification and make sure that
> navigator.onLine honors that for the appid it's running inside of.

Valentin's patch adds the new app-specific offline notification.  Is this the code that controls navigator.onLine?

  http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#543

If so it should be a matter of getting the appID within Navigator.cpp (I don't see how to get a nsILoadContext from the Navigator--Jonas, do you know?) and adding a new NS_AppOffLine(appID) that it also checks.  And we'll probably want to do that for other calls sites to NS_Offline:

  http://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#3568

  http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#10392

  // Have this code also observe the new app-offline notification and call
  // SendOfflineStatusChangeEventToAllWorkers if the appId matches? Not
  // sure how to check appID here though...
  http://mxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#2310

Valentin, also take a look at the rest of Navigator.cpp for the other references to online/offline stuff.  I didn't look at them carefully.  They may all just work with the above changes.

> set LOAD_FROM_CACHE automatically within necko 

Valentin's patch does that too.  I think that makes sense.

> I think we should cancel existing channels 

Valentin: I'm not sure if the best fix here is to 1) add some hashtable of open channels by appID, or 2) have all channels listen for app-offline notifications.  #1 gives us an easy way to cancel channels after some time period instead of immediately (just one timer needed instead of one per-channel), so let's do that.  I'm guessing the easiest way to do this is to add channels to the hash during AsyncOpen and remove them during OnStopRequest.  We could potentially try to be more precise and remove channels when their network (or cache) I/O is all done, but I'm not sure it's worth it.
Flags: needinfo?(jonas)
(In reply to Jason Duell (:jduell) from comment #21)
> > If we make it gaia's responsibility to switch apps to offline when we switch
> > from a wifi connection to a mobile connection I would be worried about races.
> > I.e. that we'd accidentally manage to squeeze through a few requests on the
> > mobile connection before all the gaia events are processed and tell necko to
> > turn the application to offline.
> 
> So right now AFAICT we've got two ways for necko to know if we're on wifi on
> B2G: either 2) have gaia call nsIOService.setAppOffline, or 2) having necko
> listen to nsINetworkInterface's 'network-interface-state-changed' nsIObserver
> notification.  I wouldn't think there'd be much difference as far as races go
> (plus we're going to kill open channels anyway).  If there's some way to get
> the
> info faster (particularly on the socket thread) via the RIL, etc. then that
> might make a difference. Is that what you're suggesting, Jonas?

I don't know. I suggest reaching out to the RIL team.

The only goal that I have with this is to make sure that there aren't any races which could cause us to make a few requests on the "wrong" interface. As long as we're guaranteed that won't happen then I'm happy.

But I worry that it's very easy to insert an asynchronous step in the Gaia front-end code and now we're suddenly racing with that.

> > enum NetworkMode {
> >   eOffline,
> >   eWifiOnly,
> >   eOnline
> > };
> 
> The only thing that's gross here is that we have different ways of getting
> wifi/mobile, depending on platform (right now we also support android):

I don't think that we care about this feature set on anything but FirefoxOS.

Android already has OS-level features which allow disabling connectivity on a per-app basis.

I.e. the feature we're talking about here is a OS-level feature, not something we're planning on exposing to webapps.

> > it would be great to come up with some new notification and make sure that
> > navigator.onLine honors that for the appid it's running inside of.
> 
> Valentin's patch adds the new app-specific offline notification.  Is this
> the code that controls navigator.onLine?
> 
>   http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#543

That appears to be the case yes. But please do test. But there's also the "online" and "offline" events here:

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#10954
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#10387

These need to line up with navigator.online. I.e. after firing "online" navigator.onLine should return true, and after firing "offline" navigator.onLine should return false.

> If so it should be a matter of getting the appID within Navigator.cpp (I
> don't see how to get a nsILoadContext from the Navigator--Jonas, do you
> know?) and adding a new NS_AppOffLine(appID) that it also checks.  And we'll
> probably want to do that for other calls sites to NS_Offline:

The appid is available off of nsIPrincipal.
Flags: needinfo?(jonas)
I guess navigator.onLine should return the state of nsIOService::mOffline in the main process, and the state of isAppOffline in the child process.

As for the racing issue, does the RIL code still allow traffic on 3G once the wifi connection is active? If it doesn't then this should not be an issue.
> I guess navigator.onLine should return the state of nsIOService::mOffline in 
> the main process, and the state of isAppOffline in the child process.

When we're using multi-process B2G that should be right.  But for desktop e10s we use single process, so I think grabbing the appID from the navigator (via nsIPrincipal, as Jonas mentioned) is probably best.  It will be appid=0 most of the time, but might not.

> As for the racing issue, does the RIL code still allow traffic on 3G once the wifi 
> connection is active? If it doesn't then this should not be an issue.

That's true, but I don't know if that's what the RIL does.  I'm also not sure who knows the RIL stuff.  Vicamo shows up in some of the hg logs, so let's start with him :)
Flags: needinfo?(vyang)
Actually, in the main process is the only place where we currently run multiple apps. It would be great to there too do the same thing as in the child process and check the appid.

But yes, when appid=0 then we can use the current code paths.
(In reply to Valentin Gosu [:valentin] from comment #23)
> As for the racing issue, does the RIL code still allow traffic on 3G once
> the wifi connection is active? If it doesn't then this should not be an
> issue.

The part that I'm worried about is when we switch from wifi to 3G. When that happens we don't want to make it possible for an app which should only have wifi-access to be able to get a request in before gaia calls setAppOffline.
(In reply to Jonas Sicking (:sicking) from comment #26)
> (In reply to Valentin Gosu [:valentin] from comment #23)
> > As for the racing issue, does the RIL code still allow traffic on 3G once
> > the wifi connection is active? If it doesn't then this should not be an
> > issue.
> 
> The part that I'm worried about is when we switch from wifi to 3G. When that
> happens we don't want to make it possible for an app which should only have
> wifi-access to be able to get a request in before gaia calls setAppOffline.

Oh, right, I was thinking in reverse. I'll look into this, but hopefully it shouldn't be to hard to avoid this case.
(In reply to Jason Duell (:jduell) from comment #24)
> > As for the racing issue, does the RIL code still allow traffic on 3G once the wifi 
> > connection is active? If it doesn't then this should not be an issue.
> 
> That's true, but I don't know if that's what the RIL does.  I'm also not
> sure who knows the RIL stuff.  Vicamo shows up in some of the hg logs, so
> let's start with him :)

There might be multiple 3G data connection of different purpose being set up at the same time.  WiFi network and the data connection that provides internet ability are mutually exclusive, but there can still be WiFi/mms or WiFi/supl or even WiFi/mms/supl.  Besides, despite the fact that WiFi & internet data connection should be mutually exclusive, there could still be a short period that they are both in connected state.
Flags: needinfo?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #28)
> (In reply to Jason Duell (:jduell) from comment #24)
> > > As for the racing issue, does the RIL code still allow traffic on 3G once the wifi 
> > > connection is active? If it doesn't then this should not be an issue.
> > 
> > That's true, but I don't know if that's what the RIL does.  I'm also not
> > sure who knows the RIL stuff.  Vicamo shows up in some of the hg logs, so
> > let's start with him :)
> 
> There might be multiple 3G data connection of different purpose being set up
> at the same time.  WiFi network and the data connection that provides
> internet ability are mutually exclusive, but there can still be WiFi/mms or
> WiFi/supl or even WiFi/mms/supl.  Besides, despite the fact that WiFi &
> internet data connection should be mutually exclusive, there could still be
> a short period that they are both in connected state.

Thanks for the info. Do we have a way of knowing which data connection is being used at any time?

Regarding Jonas's concerns, I think that for already established connections, these should remain bound to their original interface (in this case Wifi). It would be ideal if we were able to specify when creating a connection that we only want it to succed on Wifi.
Flags: needinfo?(vyang)
(In reply to Valentin Gosu [:valentin] from comment #29)
> Thanks for the info. Do we have a way of knowing which data connection is
> being used at any time?

Yes.  "@mozilla.org/network/manager;1".active

See:
http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsINetworkManager.idl#140
http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsINetworkManager.idl#29

> Regarding Jonas's concerns, I think that for already established
> connections, these should remain bound to their original interface (in this
> case Wifi). It would be ideal if we were able to specify when creating a
> connection that we only want it to succeed on Wifi.

Sounds like bug 871452?
Flags: needinfo?(vyang)
How do we handle some cases which connect to network through socket directly? (e.g., TCP API, UDP API, WebRTC) I think there could be more edge cases in B2G.
In those cases too we need to make sure that we track which app-id is making the connection and then kill those connections as appropriate.
In the architecture I have at the moment, there is a class called OfflineObserver which others could extend, and it will provide callbacks to cut off internet access when necessary.
Attachment #8394200 - Attachment is obsolete: true
Attachment #8399645 - Attachment is obsolete: true
Comment on attachment 8400747 [details] [diff] [review]
Provide way to _set network offline_ per app

I got the patch in a pretty functional state.
So far, it seems to work, except for cutting off FTP downloads (are those done in a different app than the browser?)
I also tested wifi/3g and that seems to work as well.

Jonas, can you take a look at ContentParent.cpp and see if GetAppId seems correct? I pretty much moved the code from NeckoParent::GetValidatedAppInfo
Attachment #8400747 - Flags: review?(jonas)
Attachment #8400747 - Flags: review?(jduell.mcbugs)
Is the reason that FTP doesn't work that we don't track which App-id each requests comes from? And don't enforce that the appid is correct in the parent process?

If so, that's something that we should do. Doesn't need to be done in this bug of course, but it's something we should do as a followup.
Comment on attachment 8400747 [details] [diff] [review]
Provide way to _set network offline_ per app

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

::: dom/ipc/ContentParent.cpp
@@ +2072,5 @@
> +        }
> +
> +        bool inBrowser = false;
> +        uint32_t appId = nsIScriptSecurityManager::NO_APP_ID;
> +        nsresult rv = GetAppId(&appId, &inBrowser, true);

Sadly we can't count on there being only one appid per process. We need to determine on a per window basis if we should make navigator.onLine return true/false (and fire the accompanying events).

::: netwerk/base/public/nsIIOService2.idl
@@ +17,5 @@
> +%{C++
> +  enum NetworkMode {
> +    kOnline,
> +    kOffline,
> +    kWifiOnly

Please use idl "const"ants instead so that script can access these too.

::: netwerk/ipc/NeckoCommon.h
@@ +79,5 @@
>      }                                                                          \
>    } while (0)
>  
> +enum OfflineInfoType {
> +  kOfflineStatus,

This is a bit too hacky. This assumes that all fields are one 16 bit word long, but doesn't make that very clear.

This is especially bad since the appid is a 32bit rather than 16bit.

It's also a bit risky to count on that you can stick any 16bit value in a string. There's a bit of a risk that if someone reencodes through utf8 you'll get dataloss.

A better solution would be to create functions that convert from a string to a bool+appid and back.
Attachment #8400747 - Flags: review?(jonas) → review-
Hrm.. I see that I previously said that we could get away with doing this per-process rather than per-window. Which is true. It's just not very future proof.

Would it be at all possible to try to do this per window instead?
Comment on attachment 8400747 [details] [diff] [review]
Provide way to _set network offline_ per app

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

OK, we're on the right track here, but I've got long list of tweaks...

0) The architecture for NS_IPC_IOSERVICE_SET_OFFLINE_TOPIC (from bug 552829) is silly, and I think we can make it simpler. Instead of having nsIOService fire off NS_IPC_IOSERVICE_SET_OFFLINE_TOPIC, ContentParent listen for it, and then sending an IPDL message via PContent to PContentChild, which then calls nsIOService.SetOffline() in the child, we should just be having PNecko listen to the notification and use PNecko to do the IPDL.  PContent doesn't need to be involved.  Also, I suspect we could just listen to NS_IOSERVICE_OFFLINE_STATUS_TOPIC and get rid of NS_IPC_IOSERVICE_SET_OFFLINE_TOPIC.  But we don't need any of those changes for this bug, so let's leave all this for a followup.

1) As Jonas mentioned, the kAppId hack for the observer notification is dubious.  I think there's a simpler way to do this.  Create a new notification that's only for app-offline events: let's call it 

   "network:app-offline-status-changed"

Define it in nsIIOService.idl, in this section of code.

  http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIIOService.idl#122

(I don't see any reason yet why we also need a "going offline" message in the child--if we do, then we can also create a

  "network:app-offline-about-to-go-offline"

notification.  But let's see if we need it.)

2) Create a new XPCOM interface (nsIAppOfflineInfo) that just has an appId and 'networkType' (i.e. wifi, cellular, offline) (and maybe a 'bool offline') in it (or we can leave that as the string argument).  The interface can go in nsIIOService.idl.  Make it the 'subject' of "network:app-offline-status-changed" notifications.  Then you won't have to play around with printf-ing the appID into a string and back.

2) Have nsIOService.SetAppOffline() send "network:app-offline-status-changed" instead of NS_IPC_IOSERVICE_SET_OFFLINE_TOPIC, with an nsIAppOfflineInfo containing the appId that's going offline.  When we change from wifi to cellular, have it send multiple notifications, one per wifi-only app (we could potentially optimize this to one notification, but I don't think it'll be a big performance hit.) And also of course when SetAppOffline() is called with args that turn off an app's network.

3) Change {ftp|http/websocket}ChannelParents to listen to "network:app-offline-status-changed" instead of NS_IPC_IOSERVICE_SET_OFFLINE_TOPIC.  You've already got the logic to cancel the channel when the appID == the appID mentioned in the notification, which is what needs to happen, so that's good.

4) we need to also train the Javascript TCP and UDP socket APIs to listen to "network:app-offline-status-changed", and cancel when appropriate.  Here's the bug that introduced them, which should give you pointers to the code to hook Observers in:

     https://bugzilla.mozilla.org/show_bug.cgi?id=869869
     https://bugzilla.mozilla.org/show_bug.cgi?id=770778

If you can't figure it out, I believe Honza reviewed that code when it was created.
   
5) I'm not sure if we need anything additional to cancel WebRTC connections (ask :jesup or :ekr for help with this: I don't know that code well enough).

6) Also have NeckoParent observe "network:app-offline-status-changed": when it gets it, we need to check the list of apps that are in the process (sometimes it's more than one app per process), and if any of the appIds match, we should send an IPDL message to the child (and NeckoChild should call nsIOService::SetAppOffline with the appId).  Some example code for how to iterate over the list of appIDs in a process is here:

     http://mxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoParent.cpp#540

(I discovered bug 1029352 while I was looking at this, so this code will change, but the logic to loop over appIds is right).

7) You've already got IsAppOffline checks in NeckoParent to make sure we fail channels if the app is offline. Good.

8) We should not do DNS or speculative HTTP connections when an app is marked offline.  It looks like we're already covered for speculative connections (we skip if LOAD_ONLY_FROM_CACHE is set), but I think we need to skip the DNSPrefetch calls in BeginConnect (unless I'm missing something we get to this codepath even if LOAD_ONLY_FROM_CACHE is set? I could easily be wrong about that). Given that the HTML parser and other code also issue DNS prefetches, make sure that whatever content/html/content/src/nsHTMLDNSPrefetch.cpp does also prevents network traffic if an app is offline.  (Given that DNS is very small network traffic, I'm fine with making this a followup bug if needed).

9) We need to make sure that all callers of NS_Offline() also check for IsAppOffline if needed.  As Jonas mentioned you should hopefully be able to get an appId to pass in from the nsIPrincipal.  I haven't looked to see how easy it is to get the principal in all the call sites.

10) We also need to train any code that lives in the child that currently handles offline/online notifications (i.e.  "network:offline-about-to-go-offline" and/or "network:offline-status-changed") to observe the new app-specific "network:app-offline-status-changed" notification.  I grepped through the tree and the only code I saw that seemed to need changing was dom/media/PeerConnection.js.  But take a look yourself and see if I missed anything.

11) We must make sure that when an app goes online/offline, we fire nsGlobalWindow::FireOfflineStatusEvent() and after that Navigator.Online must match.  Hopefully #9 and #10 take care of this already, but we need to check to make sure.  

12) So far my understanding is that nsIOService::SetAppOffline can only be called from the parent process.  We would need additional IPDL to make it work if it can be called from the child.  So let's make it fail on the child (and add some DoSetAppOffline function that we can call from NeckoChild when it receives a message from the parent telling it that setAppOffline has been called in the parent).  Also, let's fail if we're not called on the main thread.

::: netwerk/base/public/nsIIOService2.idl
@@ +45,5 @@
> +  void setAppOffline(in uint32_t appId, in unsigned long state);
> +
> +  /**
> +   * Checks if app with given appId has been set to offline mode by
> +   * setAppOffline.

Change wording to:  "Returns true iff the given appID is currently allowed to make network connections".  Because it's not whether app was set to 'offline', it also includes set to 'wifi only' and we're not on wifi.

::: netwerk/base/src/nsIOService.cpp
@@ +150,5 @@
>      , mShutdown(false)
>      , mNetworkLinkServiceInitialized(false)
>      , mChannelEventSinks(NS_CHANNEL_EVENT_SINK_CATEGORY)
>      , mAutoDialEnabled(false)
> +    , mOfflineApps()

Wouldn't the hashtable get initialized automatically without this? Seems like you don't need it.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +200,5 @@
> +  }
> +
> +  uint32_t loadFlags = aLoadFlags;
> +  if (app_offline) {
> +    loadFlags |= nsICachingChannel::LOAD_NO_NETWORK_IO;

Why LOAD_NO_NETWORK_IO?  I thought we'd decided to use LOAD_ONLY_FROM_CACHE (which returns cached data w/o revalidating, whereas NO_NETWORK_IO fails if validation is needed.  Offline mode generally maps to LOAD_ONLY_FROM_CACHE).
Attachment #8400747 - Flags: review?(jduell.mcbugs) → review-
Attachment #8400747 - Attachment is obsolete: true
Comment on attachment 8451537 [details] [diff] [review]
Modify calls to NS_IsOffline to check for NS_IsAppOffline as well

This patch changes the call sites of NS_IsOffline to also check NS_IsAppOffline.
r?-ing jonas for the dom/ bits, and jduell for the netwerk/ bits
Attachment #8451537 - Flags: review?(jonas)
Attachment #8451537 - Flags: review?(jduell.mcbugs)
Comment on attachment 8451718 [details] [diff] [review]
Have the JS TCP sockets check if the app is offline

No unit tests for this yet.
I tested it by opening tcp sockets in the email app, and setting it offline with marionette commands. Everything seems to work.
Attachment #8451718 - Flags: review?(jonas)
https://tbpl.mozilla.org/?tree=Try&rev=6564b4ddfa55

Some B2G test failures, which I am currently investigating, but hopefully aren't related to these patches.
Attachment #8447961 - Attachment is obsolete: true
Attachment #8452166 - Flags: review?(jduell.mcbugs)
Comment on attachment 8453486 [details] [diff] [review]
Have UDP sockets check if the app is offline

I'm having trouble testing this, since Bug 870660 limits its uses in the content process. Implementation is mostly based on the TCPSocketParent patch.
Attachment #8453486 - Flags: review?(jonas)
Comment on attachment 8453501 [details] [diff] [review]
Check app offline status in PeerConnection.js

I tested this with the http://apprtc.appspot.com/ demo, and it seems to do the right thing, but I'm not really sure this kills the signalling connection too.
Attachment #8453501 - Flags: review?(rjesup)
Comment on attachment 8453501 [details] [diff] [review]
Check app offline status in PeerConnection.js

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

r+ with the extra test removed, unless you think I'm wrong - if so, please comment

::: dom/media/PeerConnection.js
@@ +321,5 @@
>          JSON.parse(Services.prefs.getCharPref("media.peerconnection.default_iceservers"));
>      }
>      this._mustValidateRTCConfiguration(rtcConfig,
>          "RTCPeerConnection constructor passed invalid RTCConfiguration");
> +    if (_globalPCList._networkdown || !this._win.navigator.onLine) {

This shouldn't be needed - we set _networkdown on app-offline-status-changed
Attachment #8453501 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #52)
> Comment on attachment 8453501 [details] [diff] [review]
> Check app offline status in PeerConnection.js
> 
> Review of attachment 8453501 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the extra test removed, unless you think I'm wrong - if so, please
> comment
> 
> ::: dom/media/PeerConnection.js
> @@ +321,5 @@
> >          JSON.parse(Services.prefs.getCharPref("media.peerconnection.default_iceservers"));
> >      }
> >      this._mustValidateRTCConfiguration(rtcConfig,
> >          "RTCPeerConnection constructor passed invalid RTCConfiguration");
> > +    if (_globalPCList._networkdown || !this._win.navigator.onLine) {
> 
> This shouldn't be needed - we set _networkdown on app-offline-status-changed

After setting the app to the "offline" state, the network might flip on-off several times, and "network:offline-status-changed" would change _networkdown, so navigator.onLine needs to make sure it has the permission to open new connections.
Hope you agree.
Comment on attachment 8452166 [details] [diff] [review]
Provide way to "set network offline" per app

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

Lots of progress.  Still some stuff to fix.  If you turn this around while I'm on PTO give the review to Honza--I'd like him to look at this anyway, it covers a lot of code and it's not unlikely I've missed something.

::: netwerk/base/public/nsIIOService.idl
@@ +125,5 @@
> +    const long ONLINE = 1;
> +    const long OFFLINE = 2;
> +    const long WIFI_ONLY = 3;
> +
> +    readonly attribute long mode;

any reason for this to be signed?  Generally for flags/modes/enum-like settings I'm used to seeing unsigned.

@@ +145,5 @@
>   * NS_IOSERVICE_ONLINE.
>   */
>  #define NS_IOSERVICE_GOING_OFFLINE_TOPIC  "network:offline-about-to-go-offline"
>  #define NS_IOSERVICE_OFFLINE_STATUS_TOPIC "network:offline-status-changed"
> +#define NS_IOSERVICE_APP_OFFLINE_STATUS   "network:app-offline-status-changed"

Add comment:

 // when network:app-offline-status-changed is fired, the 'Subject' argument is a nsIOfflineAppInfo.

::: netwerk/base/public/nsIIOService2.idl
@@ +30,5 @@
>    /**
> +   * Set whether network appears to be offline for network connections from
> +   * a given appID.
> +   *
> +   * Calling this function may fire observer notifications ... see below.

Calling this function may fire the "network:app-offline-status-changed" notification.

@@ +36,5 @@
> +  void setAppOffline(in uint32_t appId, in long state);
> +
> +  /**
> +   * Returns true if given appId is currently
> +   * allowed to make network connections.

is currently /not/ allowed to make network connections.

::: netwerk/base/public/nsNetUtil.h
@@ +74,5 @@
>  #include "nsISocketProvider.h"
>  #include "nsIRedirectChannelRegistrar.h"
>  #include "nsIMIMEHeaderParam.h"
>  #include "nsILoadContext.h"
> +#include "nsIScriptSecurityManager.h"

Do you need this?  Fine if you do, but weird to see it with no other changes in the file.

::: netwerk/base/src/OfflineObserver.h
@@ +20,5 @@
> +  NS_DECL_NSIOBSERVER
> +
> +  virtual ~OfflineObserver() { }
> +  virtual void CancelChannel() { };
> +  virtual uint32_t GetChannelAppId() { return 0; };

shouldn't this be a pure virtual function?  Is there a case where you're using the 0 value and not overriding the function?

@@ +48,5 @@
> +                        const char *aTopic,
> +                        const char16_t *aData) MOZ_OVERRIDE;
> +
> +private:
> +  ParentObserver * mParent;

I'm not sure I'm a fan of the complexity of this whole Observer/ParentObserver/ParentOfflineObserver setup (and the names make it hard to follow).  You've got lots of fancy features like handling registering observers on the main thread if the register function is called off the main thread, but do you need that?  I appreciate the code consolidation here, but I wonder if you could get the same effect by making a single template class that has all your HandleAppOfflineNotification() logic, and then the HttpChannelParent/etc classes could inherit from that and call it from their own Observe functions. (Or maybe not even a template: just a "shouldCancel()" that takes the app-offline notification and returns true/false)  But I haven't thought about it too hard.  See what Honza thinks of it in the next rev of your patch.

::: netwerk/base/src/nsIOService.cpp
@@ +27,5 @@
>  #include "nsTArray.h"
>  #include "nsIConsoleService.h"
>  #include "nsIUploadChannel2.h"
>  #include "nsXULAppAPI.h"
> +#include "nsIScriptSecurityManager.h"

shouldn't need same include in .h and .cpp file (if it's in .h file .cpp will see it too)

@@ +977,5 @@
> +            return NS_ERROR_FAILURE;
> +        }
> +        if (state == nsINetworkInterface::NETWORK_STATE_CONNECTED) {
> +            SendAppOfflineNotification(nsIScriptSecurityManager::NO_APP_ID,
> +                                       nsIAppOfflineInfo::ONLINE);

So you're passing a "magic" value of NO_APP here.  This isn't going to work for a couple of reasons:

1) it's valid for an app to have the NO_APP appid, or at least it might be soon (see bug 820712).  I'm not clear from sicking's comment whether the B2G browser runs as {appID=0, inBrowserElement=true}.  If that's the case and it can't be changed, then we're going to need to change our whole API for this to also key based on InBrowserElement as well as appID.  Ask bent or fabrice.  I've never understood why we'd allow appid=0, inBrowser=true--it seems silly and makes our necko logic harder for no benefit I know of.

2) You could try to fudge this by using NECKO_UNKNOWN_APP_ID instead, but we also want to only fire the app-offline notification when there's an actual app that's changing offline state (see my comment below).

It seems like it would be simpler/easier to just have NeckoParent (and ParentOfflineObservers) listen for kNetworkStateChanged.  You're not doing anything here in nsIOService that they can't do too, and this would save us trying to overload the meaning of the app-offline event.

@@ +1277,5 @@
>      return pps->AsyncResolve(aURI, 0, callback, getter_AddRefs(cancelable));
>  }
> +
> +void
> +nsIOService::SendAppOfflineNotification(uint32_t appId, int32_t state)

Don't use "Send" here: we reserve that mostly for IPDL SendFoo, etc, so it's confusing. How about "NotifyAppOffline()"?

@@ +1288,5 @@
> +    if (observerService) {
> +        NS_ASSERTION(NS_IsMainThread(),
> +            "Should be called on the main thread");
> +        nsRefPtr<nsAppOfflineInfo> info = new nsAppOfflineInfo(appId, state);
> +        (void)observerService->NotifyObservers(

No need for the (void).  That's very old school K&R style C :)

@@ +1321,5 @@
> +NS_IMETHODIMP
> +nsIOService::SetAppOffline(uint32_t aAppId, int32_t aState)
> +{
> +    // TODO: remove this
> +    // This is for debugging purposes only

Why remove it?  If it's this easy to make these calls thread-safe, I'm fine with keeping.

@@ +1332,5 @@
> +                   NS_ERROR_FAILURE);
> +    NS_ENSURE_TRUE(aAppId != nsIScriptSecurityManager::NO_APP_ID,
> +                   NS_ERROR_INVALID_ARG);
> +    NS_ENSURE_TRUE(aAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID,
> +                   NS_ERROR_INVALID_ARG);

might as well put these checks before the dispatch to main thread, so off-main callers fail synchronously and see the error return.

@@ +1342,5 @@
> +
> +void
> +nsIOService::DoSetAppOffline(uint32_t aAppId, int32_t aState)
> +{
> +    NS_ENSURE_TRUE_VOID(NS_IsMainThread());

add a MOZ_ASSERT too.  It's best to catch big errors like that with a hard fail in debug mode (but keep the error return patch for release).

@@ +1355,5 @@
> +        SendAppOfflineNotification(aAppId, nsIAppOfflineInfo::ONLINE);
> +        break;
> +    case nsIAppOfflineInfo::ONLINE:
> +        mOfflineApps.Remove(aAppId);
> +        SendAppOfflineNotification(aAppId, nsIAppOfflineInfo::ONLINE);

So you're always firing notifications here, even if the situation hasn't changed. but in regular SetOffline() we only fire NS_IOSERVICE_GOING_OFFLINE_TOPIC/NS_IOSERVICE_OFFLINE_STATUS_TOPIC if mOffline is changing.  I think we should do the same here--only fire notification if it's a change of existing state.

@@ +1364,5 @@
> +
> +}
> +
> +bool
> +nsIOService::IsWifiActive()

I still find it weird that we consider an ethernet connection "wifi".  But whatever :)

It also seems weird that this function returns true if mOffline = true, i.e. the browser has been set to offline.  Not intuitive.  But maybe not a problem?  We could probably add a line at the top that returns false if mOffline?

@@ +1399,5 @@
> +    if (aAppId == nsIScriptSecurityManager::NO_APP_ID
> +        || aAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID
> +        || mOffline) {
> +        // App id is not available.
> +        *aResult = mOffline;

As mentioned earlier we need to figure out if we'll be handling the {appid=0, inBrowser=true} case.

@@ +1403,5 @@
> +        *aResult = mOffline;
> +    } else {
> +        if (mOfflineApps.Contains(aAppId)) {
> +            switch (mOfflineApps.Get(aAppId)) {
> +                case nsIAppOfflineInfo::OFFLINE:

put "case" at same indent as "switch" (i.e. unindent by 4)

@@ +1427,5 @@
> +nsAppOfflineInfo::nsAppOfflineInfo(uint32_t aAppId,
> +                                   int32_t aMode)
> +{
> +    mAppId = aAppId;
> +    mMode = aMode;

Why init here and not in constructor initialization list in the .h file?  I guess for simple types it doesn't make any difference efficiency-wise.

::: netwerk/base/src/nsIOService.h
@@ +17,5 @@
>  #include "nsIChannelEventSink.h"
>  #include "nsCategoryCache.h"
>  #include "nsISpeculativeConnect.h"
> +#include "nsDataHashtable.h"
> +#include "nsIScriptSecurityManager.h"

what do you need the scriptSecMgr.h for in this file, versus putting it in the cpp file?

@@ +49,5 @@
>                              , public nsINetUtil
>                              , public nsISpeculativeConnect
>                              , public nsSupportsWeakReference
>  {
> +friend class mozilla::net::NeckoChild;

I generally avoid using 'friend' unless classes are really interwoven with each other.  Better to just make DoSetAppOffline() public.

::: netwerk/ipc/NeckoChild.cpp
@@ +320,5 @@
>  
> +bool
> +NeckoChild::RecvAppOfflineStatus(const uint32_t& aId, const bool& aOffline)
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "This should be called on the main thread");

MOZ_ASSERT (hard fail in debug mode) is much more useful than NS_ASSERTION.  I find NS_ASSERTION pointless (just puts error msg in console, not all our automated tools notice it).

::: netwerk/ipc/NeckoParent.cpp
@@ +824,5 @@
> +    nsRefPtr<TabParent> tabParent =
> +      static_cast<TabParent*>(Manager()->ManagedPBrowserParent()[i]);
> +    uint32_t appId = tabParent->OwnOrContainingAppId();
> +
> +    // TODO: can the tab's appId be NO_APP_ID ?

Yes, it can be NO_APP_ID: see bug 820712.  So for your wildcard, "notify all apps" value, I think you should use NECKO_UNKNOWN_APP_ID instead.

::: netwerk/ipc/NeckoParent.h
@@ +7,5 @@
>  
>  #include "mozilla/net/PNeckoParent.h"
>  #include "mozilla/net/NeckoCommon.h"
> +#include "nsIObserver.h"
> +#include "mozilla/net/OfflineObserver.h"

OfflineObserver.h already includes nsIObserver.h, so don't need to include nsIObserver.h in this file?

::: netwerk/protocol/ftp/FTPChannelParent.cpp
@@ +31,5 @@
>    , mStatus(NS_OK)
>    , mDivertingFromChild(false)
>    , mDivertedOnStartRequest(false)
>    , mSuspendedForDiversion(false)
> +  , mAppId(NECKO_NO_APP_ID)

init to UNKNOWN_APP_ID any time you need a bogus initial value, not NO_APP.

@@ +44,5 @@
>  {
>    gFtpHandler->Release();
> +  if (mObserver) {
> +    mObserver->RemoveObserver();
> +    mObserver = nullptr;

destructor of mObserver should release automatically w/o assignment to nullptr needed?

@@ +111,5 @@
>    LOG(("FTPChannelParent DoAsyncOpen [this=%p uri=%s]\n",
>         this, uriSpec.get()));
>  #endif
>  
> +  if (mLoadContext) {

mLoadContext is always true: we set it in the constructor.  So that's probably the best place to set mAppId, unless I'm missing something.

@@ +620,5 @@
>    }
>  }
>  
> +void
> +FTPChannelParent::Disconnect()

I don't like the name Disconnect() here--it reminds me of closing a socket.  How about "GoOffline()"?

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +68,5 @@
>    } else {
>      mNestedFrameId = iframeEmbedding.get_uint64_t();
>    }
> +
> +  if (mLoadContext) {

mLoadContext always set.

@@ +312,3 @@
>        if (mLoadContext) {
>          mLoadContext->GetIsInBrowserElement(&inBrowser);
> +        mLoadContext->GetAppId(&mAppId);

if you set mAppId in the constructor it will never change so no need to keep getting it.

::: netwerk/protocol/websocket/WebSocketChannelParent.cpp
@@ +83,5 @@
> +    gIOService->IsAppOffline(mAppId, &app_offline);
> +  }
> +
> +  if (app_offline) {
> +    goto fail;

You can nest this inside the above 'if' statement (i.e. just put it right after IsAppOffline check).

@@ +286,5 @@
> +void
> +WebSocketChannelParent::Disconnect()
> +{
> +  if (mChannel) {
> +    mChannel->Close(nsIWebSocketChannel::CLOSE_NO_STATUS,

from the websocketchannel code it looks like CLOSE_GOING_AWAY might be better--otherwise we'll leave an error in the log.
Attachment #8452166 - Flags: review?(jduell.mcbugs) → feedback+
Valentin: also we need tests for this.  You should be able to write an xpcshell test for HTTP at least.  I'm thinking something that does a run_test_in_child() once (while app is online), to populate the cache with URI_1 (using a nsILoadContext that you create in JS with appID=345) then have the callback to run_test_in_child setAppOffline(345) and then runs another test in the child, which should see 1) URI_1 succeed (from cache) and URI_2 fail with NS_ERROR_OFFLINE.  You can use the LoadContextCallback class in head_channels.js to create a loadContext for channels--see how test_cookiejars does it.

Once we figure out the multi-app mochitest setup (for bug 1029352) we can probably re-use that for this too for websockets and TCP/UDP.
Comment on attachment 8452166 [details] [diff] [review]
Provide way to "set network offline" per app

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

Another thing we need to think about here: Jonas has told me he wants channels that have been diverted back to the parent to *not* get canceled if an app goes offline.  AFAICT we're fine with this patch as long as the divert() is complete, but I'm not sure what happens if the divert is still in progress.  Ideally we could allow the divert to complete.

From Jonas:

For "downloads" of the type where the user saves a resource to the SD
card (i.e. triggered by the download API, or by a
content-disposition:attachment response), we should not stop the
download even if the app goes offline.

Conceptually I think we should think of those as being "owned" by the
system app, even though they use the cookiejar of a different app.

But "downloads" that are performed by a page, for example an XHR
request or an <img> request, should be aborted yes.
Comment on attachment 8451537 [details] [diff] [review]
Modify calls to NS_IsOffline to check for NS_IsAppOffline as well

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

Necko bits look good.  I don't know the DOM stuff well enough to say if the way you're getting appID is right.  Probably is, but good to have sicking or someone else take a look.

::: dom/workers/RuntimeService.cpp
@@ +2419,5 @@
> +    if (!principal) {
> +      return NS_OK;
> +    }
> +
> +    uint32_t appId = nsIScriptSecurityManager::NO_APP_ID;

use UNKNOWN_APP_ID for init of all temp vars.

@@ +2422,5 @@
> +
> +    uint32_t appId = nsIScriptSecurityManager::NO_APP_ID;
> +    principal->GetAppId(&appId);
> +
> +    uint32_t notificationAppId = nsIScriptSecurityManager::NO_APP_ID;

same

::: netwerk/base/public/nsNetUtil.h
@@ +1635,5 @@
> +{
> +    if (!principal) {
> +        return NS_IsOffline();
> +    }
> +    uint32_t appId = nsIScriptSecurityManager::NO_APP_ID;

UNKNOWN_APP_ID

::: netwerk/protocol/ftp/nsFtpConnectionThread.cpp
@@ +2514,5 @@
> +    uint32_t appId;
> +    bool isInBrowser;
> +    NS_GetAppInfo(mChannel, &appId, &isInBrowser);
> +
> +    if (NS_IsOffline() || NS_IsAppOffline(appId)) {

IIRC from the main patch you'll cancel the channel if appOffline() before we ever get here?  Unless we go offline after AsyncOpen (which is of course entirely possible).  So this is fine I guess.
Attachment #8451537 - Flags: review?(jduell.mcbugs) → review+
It would be nice to have a patch that also drops DNS prefetches if an app is offline.
Jason asked me to look at the patches, all of them.  Do you need review or feedback?
I think I need review for the ones Jonas is supposed to review (since he is on PTO), and feedback on the others, in case there's something terribly wrong I'm doing.
(In reply to Jason Duell (:jduell) (on PTO until July 24th) from comment #54)
> Comment on attachment 8452166 [details] [diff] [review]
> > +    const long WIFI_ONLY = 3;
> > +
> > +    readonly attribute long mode;
> 
> any reason for this to be signed?  Generally for flags/modes/enum-like
> settings I'm used to seeing unsigned.

So appId and mode would have different types and there would be no mistaking them.
Making it unsigned wouldn't be too big of an inconvenience.

> ::: netwerk/base/src/OfflineObserver.h
> @@ +48,5 @@
> > +  ParentObserver * mParent;
> 
> I'm not sure I'm a fan of the complexity of this whole
> Observer/ParentObserver/ParentOfflineObserver setup (and the names make it
> hard to follow).  You've got lots of fancy features like handling
> registering observers on the main thread if the register function is called
> off the main thread, but do you need that?  I appreciate the code
> consolidation here, but I wonder if you could get the same effect by making
> a single template class that has all your HandleAppOfflineNotification()
> logic, and then the HttpChannelParent/etc classes could inherit from that
> and call it from their own Observe functions. (Or maybe not even a template:
> just a "shouldCancel()" that takes the app-offline notification and returns
> true/false)  But I haven't thought about it too hard.  See what Honza thinks
> of it in the next rev of your patch.
> 

It's a bit complicated because NeckoParent and TCPSocketParent for example can't inherit from nsIObserver, and some parent objects are created off-main-thread, so we can't always add the observers from the constructor.

I've refactored it a bit so that we now have OfflineObserver - the observer class, and DisconnectableParent - ParentChannel which gets disconnected. I renamed the disconnection method to OfflineDisconnect, so there's no confusion about what it does.

> 2) You could try to fudge this by using NECKO_UNKNOWN_APP_ID instead, but we
> also want to only fire the app-offline notification when there's an actual
> app that's changing offline state (see my comment below).

Yup, I think that's better.

> It seems like it would be simpler/easier to just have NeckoParent (and
> ParentOfflineObservers) listen for kNetworkStateChanged.  You're not doing
> anything here in nsIOService that they can't do too, and this would save us
> trying to overload the meaning of the app-offline event.

NeckoParent doesn't inherit from ISUPPORTS, so it can't be the actual observer.
I can extend OfflineObserver to do that, I think but that would complicate it too much.

> > +bool
> > +nsIOService::IsWifiActive()
> 
> I still find it weird that we consider an ethernet connection "wifi".  But
> whatever :)
> 
> It also seems weird that this function returns true if mOffline = true, i.e.
> the browser has been set to offline.  Not intuitive.  But maybe not a
> problem?  We could probably add a line at the top that returns false if
> mOffline?

I made this a static function since it's only used in nsIOSevice, and added some comments to explain the decision. I think it should return true if a wifi-only app should have connectivity, In this case, desktop should always return true.

> Yes, it can be NO_APP_ID: see bug 820712.  So for your wildcard, "notify all
> apps" value, I think you should use NECKO_UNKNOWN_APP_ID instead.

Ok.
  
> > +void
> > +FTPChannelParent::Disconnect()
> 
> I don't like the name Disconnect() here--it reminds me of closing a socket. 
> How about "GoOffline()"?

Changed it to OfflineDisconnect(). Let me know if you think GoOffline is better suited.

> > +  if (mLoadContext) {
> 
> mLoadContext always set.

Some places were checking if it was null, so I expected that might be the case.
Attached patch per_app.interdiff (obsolete) — Splinter Review
Comment on attachment 8458434 [details] [diff] [review]
per_app.interdiff

This is the interdiff for the changes addressing Jason's comments.
Attachment #8458434 - Attachment is patch: true
Attachment #8452166 - Attachment is obsolete: true
Comment on attachment 8458435 [details] [diff] [review]
Provide way to "set network offline" per app

This is the updated patch.

It touches {Http/Ftp/Websocket/Necko}Parent which now have an OfflineObserver member and extend DisconnectableParent. The Offline observer observes the app-offline notification, and calls OfflineDisconnect on the parent to cancel that channel some feedback on this mechanism is very welcome. See comment 61 for why I did it this way.

When SetOffline(appId,state) is called on the ioservice, the app state will be saved to a hashtable, and a notification will be emited. Channels that have that appId are expected to disconnect if needed. The notification is also passed to the child processes, by NeckoParent. Since more than one app may be in a child process, the ioservice in the child will also save the state/appid and return them when the apps need it.
Attachment #8458435 - Flags: review?(honzab.moz)
Comment on attachment 8458435 [details] [diff] [review]
Provide way to "set network offline" per app

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

the separate class approach is not bad, solves some problems.  making the target classes be also the direct listener would be complicated.

but, I think we should not instantiate the intermediate class when the channel is not bound to an app.  it just means to do bonding a bit later.

Please mainly think of better names, add proper comments (!!!) and outline code paths in a bugzilla comment when submitting the patch.  In this state it's hard to track if the patch concept is correct.

::: netwerk/base/public/nsIIOService.idl
@@ +149,4 @@
>   */
>  #define NS_IOSERVICE_GOING_OFFLINE_TOPIC  "network:offline-about-to-go-offline"
>  #define NS_IOSERVICE_OFFLINE_STATUS_TOPIC "network:offline-status-changed"
> +#define NS_IOSERVICE_APP_OFFLINE_STATUS   "network:app-offline-status-changed"

NS_IOSERVICE_APP_OFFLINE_STATUS_TOPIC

::: netwerk/base/public/nsIIOService2.idl
@@ +37,5 @@
> +  void setAppOffline(in uint32_t appId, in long state);
> +
> +  /**
> +   * Returns true if given appId is currently not
> +   * allowed to make network connections.

add to the comment this 'calculates' the result for wifi-only

::: netwerk/base/src/OfflineObserver.cpp
@@ +39,5 @@
> +  }
> +}
> +
> +void
> +OfflineObserver::RegisterOfflineObserverMT()

MT sounds like "multi-thread".  Please don't use non-clear abbreviations.

@@ +64,5 @@
> +  }
> +}
> +
> +nsresult
> +DisconnectableParent::OfflineNotification(nsISupports *aSubject)

keep class definitions together, don't jump from one class to another and then back

@@ +68,5 @@
> +DisconnectableParent::OfflineNotification(nsISupports *aSubject)
> +{
> +  nsCOMPtr<nsIAppOfflineInfo> info(do_QueryInterface(aSubject));
> +  if (!info) {
> +    return NS_OK;

NS_ERROR() this?

@@ +113,5 @@
> +                         const char16_t *aData)
> +{
> +  if (mParent &&
> +      !strcmp(aTopic, NS_IOSERVICE_APP_OFFLINE_STATUS)) {
> +    mParent->OfflineNotification(aSubject);

do you have special reasons why to pass the object here?  If you don't have any counter-arguments, I would rather change the OfflineNotification signature to:
(uint32_t aAppID, bool aOffline)

::: netwerk/base/src/OfflineObserver.h
@@ +14,5 @@
> +
> +/**
> + * Parents should extend this class and have a nsRefPtr<OfflineObserver> member.
> + * The constructor should initialize the member to new OfflineObserver(this)
> + * and the destructor should call RemoveObserver on the member.

why cannot the base class do it?

@@ +19,5 @@
> + */
> +class DisconnectableParent
> +{
> +public:
> +  virtual nsresult OfflineNotification(nsISupports *aSubject);

should be OnAppOfflineStatusChanged or something.
why is this virtual when it already has an implementation that seems like doesn't need to be overloaded?
IMO should be private, at least I would expect it.

@@ +20,5 @@
> +class DisconnectableParent
> +{
> +public:
> +  virtual nsresult OfflineNotification(nsISupports *aSubject);
> +  virtual uint32_t GetAppId() { return 0; }

what's this for?

@@ +21,5 @@
> +{
> +public:
> +  virtual nsresult OfflineNotification(nsISupports *aSubject);
> +  virtual uint32_t GetAppId() { return 0; }
> +  virtual void     OfflineDisconnect() { }

comments!  what is the expected implementation?  who is calling these methods?  on what thread?  when exactly?

OfflineDisconnect() is not the best name, but comments would make it clearer.

@@ +25,5 @@
> +  virtual void     OfflineDisconnect() { }
> +};
> +
> +/**
> + * This class extends observes the "network:app-offline-status-changed" topic

extends observes ?

@@ +28,5 @@
> +/**
> + * This class extends observes the "network:app-offline-status-changed" topic
> + * and calls OfflineNotification on the DisconnectableParent with the subject.
> + */
> +class OfflineObserver

some more descriptive name would be appropriate.

::: netwerk/base/src/nsIOService.cpp
@@ +975,5 @@
> +        if (NS_FAILED(interface->GetState(&state))) {
> +            return NS_ERROR_FAILURE;
> +        }
> +        if (state == nsINetworkInterface::NETWORK_STATE_CONNECTED) {
> +            NotifyAppOffline(NECKO_UNKNOWN_APP_ID, nsIAppOfflineInfo::ONLINE);

comment why you are passing UNKNOWN_APP_ID here.

@@ +1275,5 @@
>      return pps->AsyncResolve(aURI, 0, callback, getter_AddRefs(cancelable));
>  }
> +
> +void
> +nsIOService::NotifyAppOffline(uint32_t appId, int32_t state)

call this NotifyAppOfflineStatus (or State, please chose one and go with it)

@@ +1284,5 @@
> +    NS_ASSERTION(observerService, "The observer service should not be null");
> +
> +    if (observerService) {
> +        NS_ASSERTION(NS_IsMainThread(),
> +            "Should be called on the main thread");

it should be even instantiated on the main thread, move this to the top of the method

@@ +1289,5 @@
> +        nsRefPtr<nsAppOfflineInfo> info = new nsAppOfflineInfo(appId, state);
> +        observerService->NotifyObservers(
> +            info,
> +            NS_IOSERVICE_APP_OFFLINE_STATUS,
> +            state == nsIAppOfflineInfo::OFFLINE ?

and what about the wifi-only state?

@@ +1294,5 @@
> +            MOZ_UTF16("offline") : MOZ_UTF16("online"));
> +    }
> +}
> +
> +class MainThreadOffline : public nsRunnable

close to anonymous namespace and give this a bit more descriptive name
also could be defined inside the SetAppOffline function

@@ +1305,5 @@
> +    }
> +
> +    NS_IMETHOD Run()
> +    {
> +        gIOService->SetAppOffline(mAppId, mState);

assert main thread (assuming this is what you want)

call DoSetAppOffline?

@@ +1335,5 @@
> +}
> +
> +// This returns true if wifi-only apps should have connectivity.
> +bool
> +IsWifiActive()

static or in anon namespace or member of nsIOService

@@ +1340,5 @@
> +{
> +#ifdef MOZ_WIDGET_GONK
> +    // On B2G we query the network manager for the active interface
> +    int32_t type;
> +    nsCOMPtr<nsINetworkInterface> active;

declare before use

@@ +1347,5 @@
> +    if (!networkManager) {
> +        return false;
> +    }
> +    networkManager->GetActive(getter_AddRefs(active));
> +    if (!active) {

GetActive may throw, right?

@@ +1350,5 @@
> +    networkManager->GetActive(getter_AddRefs(active));
> +    if (!active) {
> +        return false;
> +    }
> +    active->GetType(&type);

this as well..

@@ +1361,5 @@
> +    }
> +#else
> +    // On anything else than B2G we return true so than wifi-only
> +    // apps don't think they are offline.
> +    return true;

is there anything for mobile?  can be tho well done in a followup.

@@ +1372,5 @@
> +    MOZ_ASSERT(NS_IsMainThread());
> +    NS_ENSURE_TRUE_VOID(NS_IsMainThread());
> +
> +    if (mOfflineApps.Contains(aAppId) &&
> +        mOfflineApps.Get(aAppId) == aState) {

isn't there |bool Get(key, *result)| ?  I think it is, since your way you have to do two lookups.

@@ +1378,5 @@
> +        return;
> +    }
> +
> +    bool offline = false;
> +    IsAppOffline(aAppId, &offline);

hmm... 4 lookups.  I can see a way to reduce to just a single one... use the state you find in the hashtable (or synthesize) instead of result of this relatively expensive method.

@@ +1389,5 @@
> +    case nsIAppOfflineInfo::WIFI_ONLY:
> +        mOfflineApps.Put(aAppId, nsIAppOfflineInfo::WIFI_ONLY);
> +        if (offline && IsWifiActive()) {
> +            NotifyAppOffline(aAppId, nsIAppOfflineInfo::ONLINE);
> +        } else if (!offline && !IsWifiActive()) {

cache IsWifiActive() result

@@ +1407,5 @@
> +NS_IMETHODIMP
> +nsIOService::IsAppOffline(uint32_t aAppId, bool* aResult)
> +{
> +    NS_ENSURE_ARG(aResult);
> +    if (aAppId == nsIScriptSecurityManager::NO_APP_ID

blank like before this line

@@ +1409,5 @@
> +{
> +    NS_ENSURE_ARG(aResult);
> +    if (aAppId == nsIScriptSecurityManager::NO_APP_ID
> +        || aAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID
> +        || mOffline) {

put || at the end of the line before

@@ +1413,5 @@
> +        || mOffline) {
> +        // App id is not available.
> +        *aResult = mOffline;
> +    } else {
> +        if (mOfflineApps.Contains(aAppId)) {

} else if () {, but I can see the same problem with contains()+get() calls.  the previous block should just return NS_OK, avoid else when you can.  prefilling *aResult = false would make the code simpler as well.

@@ +1454,5 @@
> +nsAppOfflineInfo::GetAppId(uint32_t *aAppId)
> +{
> +    *aAppId = mAppId;
> +    return NS_OK;
> +}

inline these to the helper class declaration

::: netwerk/base/src/nsIOService.h
@@ +40,5 @@
>  class nsPISocketTransportService;
>  
> +namespace mozilla { namespace net {
> +    class NeckoChild;
> +}}

should be 

namespace mozilla {
namespace net {

}
}

@@ +80,5 @@
>        return mOffline && mSettingOffline && !mSetOfflineValue;
>      }
>  
> +    // Should only be called from NeckoParent. Use SetAppOffline instead.
> +    void DoSetAppOffline(uint32_t appId, int32_t status);

SetAppOfflineStatusInternal ?

@@ +111,5 @@
>      void LookupProxyInfo(nsIURI *aURI, nsIURI *aProxyURI, uint32_t aProxyFlags,
>                           nsCString *aScheme, nsIProxyInfo **outPI);
>  
> +    // notify content processes of offline status
> +    void NotifyAppOffline(uint32_t appId, int32_t status);

NotifyAppOfflineStatus ?

@@ +141,5 @@
>      nsTArray<int32_t>                    mRestrictedPortList;
>  
>      bool                                 mAutoDialEnabled;
> +
> +    nsDataHashtable<nsUint32HashKey, int32_t> mOfflineApps;

mAppsOfflineStatus ?

comments.

@@ +148,5 @@
>      static uint32_t   gDefaultSegmentSize;
>      static uint32_t   gDefaultSegmentCount;
>  };
>  
> +class nsAppOfflineInfo : public nsIAppOfflineInfo

what is this used for?  comments.

::: netwerk/ipc/NeckoChild.cpp
@@ +323,5 @@
> +bool
> +NeckoChild::RecvAppOfflineStatus(const uint32_t& aId, const bool& aOffline)
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread");
> +  if (gIOService) {

maybe instantiate the service first?

::: netwerk/ipc/NeckoParent.cpp
@@ +83,5 @@
>  NeckoParent::~NeckoParent()
>  {
> +  if (mObserver) {
> +    mObserver->RemoveObserver();
> +    mObserver = nullptr;

no need to nullify.

@@ +823,5 @@
> +
> +  uint32_t targetAppId = NECKO_UNKNOWN_APP_ID;
> +  info->GetAppId(&targetAppId);
> +
> +  for (uint32_t i = 0; i < Manager()->ManagedPBrowserParent().Length(); i++) {

++i

@@ +831,5 @@
> +
> +    // targetAppId is NECKO_UNKNOWN_APP_ID when the network comes online again
> +    // and wifi-only apps might need to recompute their state.
> +    if (appId == targetAppId || targetAppId == NECKO_UNKNOWN_APP_ID) {
> +      bool offline = false;

declare before use

@@ +834,5 @@
> +    if (appId == targetAppId || targetAppId == NECKO_UNKNOWN_APP_ID) {
> +      bool offline = false;
> +      if (gIOService) {
> +        gIOService->IsAppOffline(appId, &offline);
> +        if (!SendAppOfflineStatus(appId, offline))

so if one of the children is down, you break the complete cycle?  rather just warn

::: netwerk/protocol/ftp/FTPChannelParent.cpp
@@ +13,5 @@
>  #include "mozilla/ipc/URIUtils.h"
>  #include "mozilla/unused.h"
>  #include "SerializedLoadContext.h"
> +#include "nsIOService.h"
> +

no new blank line

::: netwerk/protocol/ftp/FTPChannelParent.h
@@ +106,5 @@
>    // Set if we successfully suspended the nsHttpChannel for diversion. Unset
>    // when we call ResumeForDiversion.
>    bool mSuspendedForDiversion;
> +
> +  uint32_t mAppId;

again...

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +26,5 @@
>  #include "mozilla/ipc/URIUtils.h"
>  #include "SerializedLoadContext.h"
>  #include "nsIAuthInformation.h"
>  #include "nsIAuthPromptCallback.h"
> +#include "nsIIOService2.h"

probably no need to include?

@@ +212,5 @@
>    nsCOMPtr<nsIIOService> ios(do_GetIOService(&rv));
>    if (NS_FAILED(rv))
>      return SendFailedAsyncOpen(rv);
>  
> +  bool app_offline = false;

appOffline

@@ +220,5 @@
> +
> +  uint32_t loadFlags = aLoadFlags;
> +  if (app_offline) {
> +    loadFlags |= nsICachingChannel::LOAD_ONLY_FROM_CACHE;
> +  }

The correct flag is NO_NETWORK_IO (and maybe also LOAD_ONLY_FROM_CACHE to double force..)

@@ +359,5 @@
> +    gIOService->IsAppOffline(mAppId, &app_offline);
> +  }
> +
> +  if (app_offline) {
> +    mChannel->Cancel(NS_ERROR_OFFLINE);

and why not to set the NO_NET_IO flag as in case of opening a channel only?  You just have to do it a bit sooner then here, AsyncOnChannelRedirect is the place (in HttpChannelParentListener).

@@ +361,5 @@
> +
> +  if (app_offline) {
> +    mChannel->Cancel(NS_ERROR_OFFLINE);
> +    mStatus = NS_ERROR_OFFLINE;
> +    return true;

no need to return?

::: netwerk/protocol/http/HttpChannelParent.h
@@ +152,5 @@
>    bool mSentRedirect1Begin          : 1;
>    bool mSentRedirect1BeginFailed    : 1;
>    bool mReceivedRedirect2Verify     : 1;
>  
> +  uint32_t mAppId;

maybe always get from load context?  I assume it's non-null all the time.  I don't see a point in caching this, it's redundant.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2550,5 @@
>      else {
>          openURI = mURI;
>      }
>  
> +    uint32_t appid = info->AppId();

appID

@@ +2551,5 @@
>          openURI = mURI;
>      }
>  
> +    uint32_t appid = info->AppId();
> +    bool app_offline = false;

appOffline

@@ +2556,5 @@
> +
> +    if (appid != NECKO_NO_APP_ID)
> +        gIOService->IsAppOffline(appid, &app_offline);
> +
> +    LOG(("nsHttpChannel::OpenCacheEntry appid: %u, offline: %d\n", appid, app_offline));

please log only that an app (not no-app-id) is offline, this would pollute desktop logs.

::: netwerk/protocol/websocket/WebSocketChannelParent.cpp
@@ +76,5 @@
> +  if (mLoadContext) {
> +    mLoadContext->GetAppId(&mAppId);
> +  }
> +
> +  bool app_offline = false;

appOffline, please check all places in you patch.  we don't use underscores in names, just lowerCamelCase.

::: netwerk/protocol/websocket/WebSocketChannelParent.h
@@ +62,4 @@
>    nsCOMPtr<nsIAuthPromptProvider> mAuthProvider;
>    nsCOMPtr<nsIWebSocketChannel> mChannel;
>    nsCOMPtr<nsILoadContext> mLoadContext;
> +  uint32_t mAppId;

again, get this from load context.
Attachment #8458435 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #66)
Thanks for the review!

> > +    mParent->OfflineNotification(aSubject);
> 
> do you have special reasons why to pass the object here?  If you don't have
> any counter-arguments, I would rather change the OfflineNotification
> signature to:
> (uint32_t aAppID, bool aOffline)

The subject (nsAppOfflineInfo) contains the appId and offline status.
In the case of an 3g-wifi switch, the offline would be irrelevant in NeckoParent, as it would need to recompute the status for each app anyway. 

> ::: netwerk/base/src/OfflineObserver.h
> @@ +14,5 @@
> > +
> > +/**
> > + * Parents should extend this class and have a nsRefPtr<OfflineObserver> member.
> > + * The constructor should initialize the member to new OfflineObserver(this)
> > + * and the destructor should call RemoveObserver on the member.
> 
> why cannot the base class do it?

Because after registering the observer, there will be 2 references to it - one in the parent, and the other in the observerService. The parent's destructor will only remove its own reference, so it also needs to manually call RemoveObserver, so that the observerService reference will be removed, and so that the observer will not keep a pointer to the parent even if it gets dispatched to the main thread.

> @@ +19,5 @@
> > + */
> > +class DisconnectableParent
> > +{
> > +public:
> > +  virtual nsresult OfflineNotification(nsISupports *aSubject);
> 
> should be OnAppOfflineStatusChanged or something.
> why is this virtual when it already has an implementation that seems like
> doesn't need to be overloaded?
> IMO should be private, at least I would expect it.

It provides a default implementation, but the parent can also override it for different purpose (Such as the one in NeckoParent).

> > +  virtual uint32_t GetAppId() { return 0; }
> what's this for?

It's used to get the appId for the parentChannel and called in the default implementation of OfflineNotification. NeckoParent for example, would not use this, so it gave it a default value -  NECKO_UNKNOWN_APP_ID is probably more appropriate.

> > +class OfflineObserver
> 
> some more descriptive name would be appropriate.

> @@ +1289,5 @@
> > +        nsRefPtr<nsAppOfflineInfo> info = new nsAppOfflineInfo(appId, state);
> > +        observerService->NotifyObservers(
> > +            info,
> > +            NS_IOSERVICE_APP_OFFLINE_STATUS,
> > +            state == nsIAppOfflineInfo::OFFLINE ?
> 
> and what about the wifi-only state?

The consumer should call IsAppOffline, so that shouldn’t matter.
 
> > +    networkManager->GetActive(getter_AddRefs(active));
> > +    if (!active) {
> 
> GetActive may throw, right?

It's implemented in JS, and sometimes may be null. It doesn't throw, AFAIK.

> @@ +1361,5 @@
> > +    }
> > +#else
> > +    // On anything else than B2G we return true so than wifi-only
> > +    // apps don't think they are offline.
> > +    return true;
> 
> is there anything for mobile?  can be tho well done in a followup.

That depends a lot on what access we have on Android. At the moment there are a lot of MOZ_B2G_RIL ifdefs in NetworkManager.

> > +  if (app_offline) {
> > +    loadFlags |= nsICachingChannel::LOAD_ONLY_FROM_CACHE;
> > +  }
> 
> The correct flag is NO_NETWORK_IO (and maybe also LOAD_ONLY_FROM_CACHE to
> double force..)

Comment 40 seems to favour LOAD_ONLY_FROM_CACHE. Jason's comment seems fair. Let me know if I should still change it.

> 
> @@ +359,5 @@
> > +    gIOService->IsAppOffline(mAppId, &app_offline);
> > +  }
> > +
> > +  if (app_offline) {
> > +    mChannel->Cancel(NS_ERROR_OFFLINE);
> 
> and why not to set the NO_NET_IO flag as in case of opening a channel only? 
> You just have to do it a bit sooner then here, AsyncOnChannelRedirect is the
> place (in HttpChannelParentListener).
> 

How about doing mChannel->SetLoadFlags(loadFlags | LOAD_ONLY_FROM_CACHE) ? Is that allowed in ConnectChannel? If so, then would it also be allowed to do that in OfflineDisconnect?
Comment on attachment 8460039 [details] [diff] [review]
xpcshell test for HTTP and mochitest for WebSocket per-app offline

I was unable to add permission to the iframe for mozTCPSocket (even if the app already has that permission). If app A goes to http://example.com would it be allowed to use mozTCPSocket, even thought it's on another domain?

Also, there seem to be no JS tests for UDP sockets, and I see no way to identify the appId.
Attachment #8460039 - Flags: review?(jduell.mcbugs)
Attachment #8458435 - Attachment is obsolete: true
Attached patch per_app.interdiff (obsolete) — Splinter Review
Attachment #8458434 - Attachment is obsolete: true
Comment on attachment 8460675 [details] [diff] [review]
Provide way to "set network offline" per app

I have addressed Honza's comments, except for those in comment 67.
Another relevant change - we now listen for "network-active-changed" instead of "network-interface-state-changed". This way, we only get apps to recompute their status when the interface actually changes, not every time when it flips on-off.

Some feedback on the code in HttpChannelParent::ConnectChannel would be useful, as I'm not sure it's correct. I added it because apps would still have connectivity after setting them offline by reusing that channel.
Attachment #8460675 - Flags: review?(jduell.mcbugs)
I have run into an issue:

When we set an app offline, we send a notification, and the child process holds its own hashtable in nsIOService to determine if it's offline.
When I open a new browser tab, it creates a new process. It has the same id, but its tables are empty, so it doesn't know it's offline. The same happens if I kill the app, and open it again.

Question: Is there a way to notify it when it gets created of which apps are offline? Sync ipdl calls are bad, so we can't get the status that way.

A hacky mechanism I have in mind:
NeckoChild is forked, with mFirst = true;
It makes a SendRequestOfflineStatus() call to NeckoParent and sets mFirst = false
NeckoParent calls NeckoParent::OfflineNotification(NECKO_UNKNOWN_APP_ID) which gets all the apps contained in the process to recompute their status, so the hashtable gets to the right state.

What do you think?
Flags: needinfo?(jduell.mcbugs)
bent:

This is just about comment 73: we need to somehow inform new B2G processes about which apps are currently marked offline. (Valentin: IIUC this will be the case not just for new tabs, but also at app startup).

Do we have a "best practice" for sending info to apps at fork() time?  The ways that come to mind for me are

0) Pass the info as part of the fork, somehow.  Not sure if we've got such a frob in the API, though.

1) Set an environment variable in the parent to the list of the appIds and then the newly forked process can see it.  But I'm not sure that's going to work if we fork from a pristine NUWA process?  (My knowledge of B2G process creation is rusty)

2) Send an IPDL message to NeckoChild right after forking, with the list of appIds.  But this sets up a possible race condition: if we don't process that msg fast enough we could report document.offline incorrectly.
Flags: needinfo?(jduell.mcbugs) → needinfo?(bent.mozilla)
As far as I know we don't have a successfully loaded app process until we sent the "LoadURL" message here:

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#505

Andrea is adding some additional parameters that will be sent along there in bug 984050 (see the changes in PBrowser.ipdl). Maybe you could add something there?
Flags: needinfo?(bent.mozilla)
Val/Jason, 

As you know, this bug is a dependency for one of our Smart Data Consumption user stories/feature targeted for 2.1 fxos release. Do you have an ETA on when this work will land? 

Thanks
Hema
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(jduell.mcbugs)
Comment on attachment 8451537 [details] [diff] [review]
Modify calls to NS_IsOffline to check for NS_IsAppOffline as well

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

::: dom/base/Navigator.cpp
@@ +590,5 @@
>  
>  bool
>  Navigator::OnLine()
>  {
> +  if (mWindow && mWindow->GetDoc() && mWindow->GetDoc()->NodePrincipal()) {

NodePrincipal() is always non-null. So no need to check that here.
Attachment #8451537 - Flags: review?(jonas) → review+
Comment on attachment 8451718 [details] [diff] [review]
Have the JS TCP sockets check if the app is offline

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

Punting this to jdm who knows this code better
Attachment #8451718 - Flags: review?(jonas) → review?(josh)
Comment on attachment 8453486 [details] [diff] [review]
Have UDP sockets check if the app is offline

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

I don't know this code well enough. Looks like Patrick has done most of the reviewing of this code.
Attachment #8453486 - Flags: review?(jonas) → review?(mcmanus)
Comment on attachment 8453486 [details] [diff] [review]
Have UDP sockets check if the app is offline

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

lgtm; I can't actually vouch for all of the appid details, but it seems right. The socket bits are good.

::: dom/network/src/UDPSocketParent.cpp
@@ +61,5 @@
>  }
>  
>  NS_IMPL_ISUPPORTS(UDPSocketParent, nsIUDPSocketListener)
>  
> +UDPSocketParent::UDPSocketParent(nsIUDPSocketFilter* filter)

nsidupsocketfilter[space]*filter
Attachment #8453486 - Flags: review?(mcmanus) → review+
Comment on attachment 8453486 [details] [diff] [review]
Have UDP sockets check if the app is offline

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

::: dom/network/src/UDPSocketParent.cpp
@@ +85,5 @@
> +  if (!info) {
> +    return NS_OK;
> +  }
> +
> +  uint32_t targetAppId = NECKO_NO_APP_ID;

I still prefer UNKNOWN_APP when you're init'ing variable, though it really shouldn't make any difference here.

@@ +98,5 @@
> +  const PContentParent *content = Manager()->Manager();
> +  const InfallibleTArray<PBrowserParent*>& browsers = content->ManagedPBrowserParent();
> +  if (browsers.Length() > 0) {
> +    TabParent *tab = static_cast<TabParent*>(browsers[0]);
> +    appId = tab->OwnAppId();

So you're only looking at the 1st browser in the array.  Any reason why not check if any of the browser's appIDs match?  I.e. like we're doing in bug 1029352?   We should probably be figuring out which appId created the UDP socket on the child and store it in the parent at creation time, so we know exactly which app the UDP socket belongs to in the multi-app per process case.  I've emailed Shih-Chiang about that.
Comment on attachment 8460675 [details] [diff] [review]
Provide way to "set network offline" per app

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

::: netwerk/base/public/nsIIOService.idl
@@ +144,5 @@
>   * with topic NS_IOSERVICE_OFFLINE_STATUS_TOPIC and data
>   * NS_IOSERVICE_ONLINE.
> + *
> + * When network:app-offline-status-changed is fired,
> + * the 'Subject' argument is a nsIOfflineAppInfo.

Put this description right above the line that defines NS_IOSERVICE_APP_OFFLINE_STATUS_TOPIC.  The farther away it is, the less likely anyone is to notice it.

::: netwerk/base/public/nsIIOService2.idl
@@ +33,5 @@
> +   *
> +   * Calling this function may fire the "network:app-offline-status-changed"
> +   * notification, which is also sent to child processes containing this appId.
> +   */
> +  void setAppOffline(in uint32_t appId, in long state);

add

  // 'state' must one of nsIAppOfflineInfo::{ONLINE|OFFLINE|WIFI_ONLY}.

@@ +40,5 @@
> +   * Returns true if given appId is currently not allowed to make network
> +   * connections. It will return true if the app is in the wifi-only state
> +   * and we are currently on a 3G connection.
> +   */
> +  boolean isAppOffline(in uint32_t appId);

It's a little odd to have the nsIAppOfflineInfo struct and events in nsIIOService, but then these two functions in nsIIOService2.  There's really no reason for nsIIOService to exist any more (it was originally because nsIIOService was a 'frozen' IDL, but there's no such thing any more).  Just stick all IDL changes in nsIIOService.idl.

::: netwerk/base/src/OfflineObserver.cpp
@@ +103,5 @@
> +  // Obtain App ID
> +  uint32_t appId = GetAppId();
> +
> +  if (appId != targetAppId &&
> +      targetAppId != NECKO_UNKNOWN_APP_ID) {

Add comment above 'if' statement:

   // NECKO_UNKNOWN_APP_ID means global network interface has changed, so all apps must check NS_IsAppOffline() in case they are wifi-only.

(if we keep using UNKNOWN_APP this way)

::: netwerk/base/src/nsIOService.cpp
@@ +976,5 @@
> +        if (NS_FAILED(interface->GetState(&state))) {
> +            return NS_ERROR_FAILURE;
> +        }
> +        // Send an online notification to get wifi-only apps
> +        // to recompute their status.

// Send a 'magic' notification with UNKNOWN_APP_ID: tells wifi-only apps to recompute their status.

But this has a flaw:  we'll never send out a APP_OFFLINE notification if a wifi-only app goes offline because we've switched to 3G, for instance.  (No code relies on that now, but someday something might want to use the notifications to observe this, and it would make sense for it to be consistent).  So instead I suggest that you change this code so that when network-active-changed occurs, nsIOService iterates through mAppOfflineStatuses and figures out itself which apps will be changing offline/online status, and then have it send out 1 app-offline-status notification per app that has changed.  Then we'll truly see one notification per app that changes status.  We could also completely get rid of the use of UNKNOWN_APP as a magic value.  Make sense?

@@ +1280,5 @@
> +
> +void
> +nsIOService::NotifyAppOfflineStatus(uint32_t appId, int32_t state)
> +{
> +    NS_ASSERTION(NS_IsMainThread(),

NS_ASSERTIONS are useless.  MOZ_RELEASE_ASSERT() is better.

@@ +1285,5 @@
> +            "Should be called on the main thread");
> +
> +    nsCOMPtr<nsIObserverService> observerService =
> +        mozilla::services::GetObserverService();
> +    NS_ASSERTION(observerService, "The observer service should not be null");

MOZ_ASSERT.  Or nothing--it should be exceedingly rare that we can't get the observer svc (and if we can't things are probably hopelessly broken).

@@ +1293,5 @@
> +        observerService->NotifyObservers(
> +            info,
> +            NS_IOSERVICE_APP_OFFLINE_STATUS_TOPIC,
> +            state == nsIAppOfflineInfo::OFFLINE ?
> +            MOZ_UTF16("offline") : MOZ_UTF16("online"));

Not sure it helps to duplicate the state in both the nsAppOfflineInfo and in the string arg.  I'd be fine leaving string empty, or even set to "all data in nsIAppOfflineInfo status argument".  But if there's any reason to keep the string as 'offline/online' that's fine. Not a big deal.

@@ +1407,5 @@
> +        break;
> +    case nsIAppOfflineInfo::ONLINE:
> +        mAppsOfflineStatus.Remove(aAppId);
> +        NotifyAppOfflineStatus(aAppId, nsIAppOfflineInfo::ONLINE);
> +        break;

This code will still send out notifications when there's no change in ONLINE/OFFLINE state.  Example: we're set to WIFI_ONLY and we're on a 3G network.  Then something calls SetAppOffline(OFFLINE).  This code will fire off a notification for an 'offline' change, even though IsAppOffline() was true before and afterwards, i.e. no delta.  Should be easy to fix by checking your 'bool offline' variable in the OFFLINE (and ONLINE) cases to make sure there's an actual state change before firing notification.

@@ +1415,5 @@
> +
> +}
> +
> +NS_IMETHODIMP
> +nsIOService::IsAppOffline(uint32_t aAppId, bool* aResult)

I still think we should return true if mOffline, i.e. if the browser as a whole is offline.

@@ +1422,5 @@
> +
> +    *aResult = false;
> +    int32_t state;
> +    if (aAppId == nsIScriptSecurityManager::NO_APP_ID ||
> +        aAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID ||

Might as well keep using NECKO_{NO|UNKNOWN}_APP_ID, instead of switching back and forth between them and the nsIScriptSecMgr equivalents? (That said, we copied them to the NECKO_* versions only because we wanted to keep necko ignorant of the scriptSecMgr, which we care less and less about, so one of these days we should get rid of the NECKO_* constants and just use the SecMgr ones.  But let's be consistent either way, and for now, that's easiest by using the NECKO_* ones).

It doesn't make sense to ask for UNKNOWN_APPs status--have that return NS_ERROR_UNAVAILABLE.

::: netwerk/base/src/nsIOService.h
@@ +21,2 @@
>  #include "mozilla/Attributes.h"
> +#include "nsThreadUtils.h"

what do you need from ThreadUtils.h?  I don't see anything that's need in this .h file.

@@ +112,5 @@
>      // consolidated helper function
>      void LookupProxyInfo(nsIURI *aURI, nsIURI *aProxyURI, uint32_t aProxyFlags,
>                           nsCString *aScheme, nsIProxyInfo **outPI);
>  
> +    // notify content processes of offline status

// 'status' must be a nsIAppOfflineInfo mode constant.

@@ +143,5 @@
>      nsTArray<int32_t>                    mRestrictedPortList;
>  
>      bool                                 mAutoDialEnabled;
> +
> +    // Hashtable of (appId, status) pairs used especially in IsAppOffline

// Hashtable of (appId, nsIAppOffineInfo::mode) pairs used especially in IsAppOffline

::: netwerk/protocol/ftp/FTPChannelParent.cpp
@@ +42,5 @@
>  FTPChannelParent::~FTPChannelParent()
>  {
>    gFtpHandler->Release();
> +  if (mObserver) {
> +    mObserver->RemoveObserver();

Is there a reason we can't have mObserver's destructor take care of removing itself from the nsIObserverService?

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +214,5 @@
> +  }
> +
> +  uint32_t loadFlags = aLoadFlags;
> +  if (appOffline) {
> +    loadFlags |= nsICachingChannel::LOAD_ONLY_FROM_CACHE;

I need to ask Jonas whether we want this or NO_NETWORK_IO.   I'll needinfo him.

@@ +357,5 @@
> +
> +  if (appOffline) {
> +    mChannel->Cancel(NS_ERROR_OFFLINE);
> +    mStatus = NS_ERROR_OFFLINE;
> +  }

So I've been looking over how e10s redirects work (what a mess: I'm always amazed at how hard that code is to understand) and I don't *think* you need this code.  When we redirect to the new channel (which can be another HTTP channel or an FTP channel), that channel will check 

   NS_IsOffline() || NS_IsAppOffline(appId)

(as you added in your other patch) and the channel will behave correctly (fail if FTP or use cache-only if HTTP).  

That said, I think we should make sure we test this case, because it's really easy to miss something in all this logic.
Attachment #8460675 - Flags: review?(jduell.mcbugs) → feedback+
Jonas,

So there's some question as to which load flag we should use for channels when their app is offline, LOAD_NO_NETWORK_IO or LOAD_ONLY_FROM_CACHE:

  LOAD_NO_NETWORK_IO : This flag differs from LOAD_ONLY_FROM_CACHE in that
     * this flag fails the load if validation is required while
     * LOAD_ONLY_FROM_CACHE skips validation where possible

Do we want to skip validation and deliver cached content when we're offline, or should unvalidated content just fail?

Oh, and LOAD_NO_NETWORK_IO apparently fails channels with NS_ERROR_DOCUMENT_NOT_CACHED instead of NS_ERROR_OFFLINE.  Do we care which error code gets returned?  Oh, hmm, looks like that's the error code that gets returned when we use LOAD_ONLY_FROM_CACHE, too.  So we'll need more changes to the patch if NOT_CACHED is not the error we want.
Flags: needinfo?(jonas)
:hema, when is the deadline for 2.1?  This bug is making progress but I can't say just yet when it will be finished, as it's complex.  But it's a top priority.
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(hkoka)
Valentin:  we still need to turn off DNS prefetch too.  Though that might be fine as a followup--that's not a lot of bytes.  But it would good to have.  Remember that in the multiple-app-per-process case Jonas wants us to only turn off DNS prefetching if all apps in the process are offline (unless we can figure out a way to turn it off per app).
Valentin: also, it looks like the best way to send new processes/tabs the list of appIds that are offline is to pass them along in SendLoadURI, per comment 75.  I don't think we need to wait for bug 984050 (which changes that call site too) to land--if we bitrot their patch, tough luck :)

I suppose ideally we could pass along just the appIds that are in the process.  There should be a way to figure that out but I'm not sure exactly (the lines below SendLoadURI() in the link in comment 75 show getting it for a mozIApplication, but that might work only for packaged apps?  If not that, maybe a call to OwnOrContainingAppId()? But I'm guessing).
Comment on attachment 8451718 [details] [diff] [review]
Have the JS TCP sockets check if the app is offline

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

The only thing that I think we need to answer is my question about why we're not checking for appId != UNKNOWN_APP like OfflineObserver.cpp does.

::: dom/network/src/TCPSocketParent.cpp
@@ +81,5 @@
> +  const PContentParent *content = Manager()->Manager();
> +  const InfallibleTArray<PBrowserParent*>& browsers = content->ManagedPBrowserParent();
> +  if (browsers.Length() > 0) {
> +    TabParent *tab = static_cast<TabParent*>(browsers[0]);
> +    appId = tab->OwnAppId();

I see this is how we're getting the appID in other parts of the file (RecvOpen).  But I still wonder how well this will work for multi-app processes.  Probably best if we pass the appID in during socket IPDL construction (and trust the child as long as it's one of the appIDs listed for the process).  That can be a followup.

@@ +84,5 @@
> +    TabParent *tab = static_cast<TabParent*>(browsers[0]);
> +    appId = tab->OwnAppId();
> +  }
> +
> +  if (appId != targetAppId) {

So I notice that in OfflineObserver.cpp this 'if' statement also checks appId != UNKNOWN_APP, which forces wifi-only to recheck.  Don't we need that here?  We're also missing from the UDP socket patch.

(This is moot if we wind up getting rid of the magic use of UNKNOWN_APP, as I suggested in my review of the main patch).

@@ +148,5 @@
>    }
>  
> +  if (NS_IsAppOffline(appId)) {
> +    NS_ERROR("Can't open socket because app is offline");
> +    FireInteralError(this, __LINE__);

Not the most graceful error--this isn't really an internal error, the way 'out of memory", etc, is.  But I don't see a better way to add an error, and hopefully "invalid state" is good enough for the JS on the child to see.
Attachment #8451718 - Flags: review?(josh) → feedback+
(In reply to Jason Duell (:jduell) from comment #84)
> :hema, when is the deadline for 2.1?  This bug is making progress but I
> can't say just yet when it will be finished, as it's complex.  But it's a
> top priority.

Ideally we would have this landed before the end of this week (8/22). Feature Landing for 2.1 is 9/1 IIRC (the week after, which we'd need to test and land our changes on top of this at minimum)
Flags: needinfo?(hkoka)
(In reply to Jason Duell (:jduell) from comment #82)

> ::: netwerk/protocol/ftp/FTPChannelParent.cpp
> @@ +42,5 @@
> >  FTPChannelParent::~FTPChannelParent()
> >  {
> >    gFtpHandler->Release();
> > +  if (mObserver) {
> > +    mObserver->RemoveObserver();
> 
> Is there a reason we can't have mObserver's destructor take care of removing
> itself from the nsIObserverService?

The observer service holds a reference to the observer, so its destructor will not get called until we remove it manually.
@@ +357,5 @@
> > +
> > +  if (appOffline) {
> > +    mChannel->Cancel(NS_ERROR_OFFLINE);
> > +    mStatus = NS_ERROR_OFFLINE;
> > +  }
> 
> So I've been looking over how e10s redirects work (what a mess: I'm always
> amazed at how hard that code is to understand) and I don't *think* you need
> this code.  When we redirect to the new channel (which can be another HTTP
> channel or an FTP channel), that channel will check 
> 
>    NS_IsOffline() || NS_IsAppOffline(appId)
> 
> (as you added in your other patch) and the channel will behave correctly
> (fail if FTP or use cache-only if HTTP).  
> 
> That said, I think we should make sure we test this case, because it's
> really easy to miss something in all this logic.

I added this because I noticed some pages would still load when the app was offline. I didn't fully understand how channels get redirected, but with this change they would behave properly.
(In reply to Marshall Culpepper [:marshall_law] from comment #88)
> (In reply to Jason Duell (:jduell) from comment #84)
> > :hema, when is the deadline for 2.1?  This bug is making progress but I
> > can't say just yet when it will be finished, as it's complex.  But it's a
> > top priority.
> 
> Ideally we would have this landed before the end of this week (8/22).
> Feature Landing for 2.1 is 9/1 IIRC (the week after, which we'd need to test
> and land our changes on top of this at minimum)

Yes thats correct. We need this work landed by 8/22 at the latest (it is already pretty tight to wrap up the remaining work on top of things and get everything in a landable state by 8/29). We want to avoid too many complex landings in the last sprint of the dev cycle. So Jason, can you please let us know the ETA for this bug and based on it we can decide the plan for user story: https://bugzilla.mozilla.org/show_bug.cgi?id=1035553

Thanks
Hema
Flags: needinfo?(jduell.mcbugs)
I think we may have +r patches by 8/22, but this is a fairly complex bug so there's definitely some risk involved that things won't be perfect or possibly even landable.  Looking at bug 1035553, I'm not clear if we will have the UI ready to allow users to use this code, even if this bug lands?
Flags: needinfo?(jduell.mcbugs)
Using nsIScriptSecurityManager::UNKNOWN_APP_ID instead of NECKO_NO_APP_ID
Attachment #8474825 - Flags: review?(jduell.mcbugs)
Attachment #8451718 - Attachment is obsolete: true
Using nsIScriptSecurityManager::UNKNOWN_APP_ID instead of NECKO_NO_APP_ID. Already reviewed by :mcmanus
Attachment #8453486 - Attachment is obsolete: true
Using UNKNOWN_APP_ID instead of NO_APP_ID.
NodePrincipal() is always non-null, so we're not checking it anymore.
nsIIOService2 is no longer necessary (isAppOffline was moved to nsIOService.idl)
Already reviewed by :sicking and :jduell
Attachment #8451537 - Attachment is obsolete: true
Blocks: 1035553
Moved is/setAppOffline to nsIOService.idl
Only sending notification when app state actually changes.
Using EnumerateWifiAppsChangingState to iterate through the hashtable and notify WifiOnly apps of a Wifi-3G switch.
Addressed other comments in previous review
Attachment #8474936 - Flags: review?(jduell.mcbugs)
Attachment #8460675 - Attachment is obsolete: true
Attached patch interdiff.patch (obsolete) — Splinter Review
Interdiff for main patch.
Attachment #8460676 - Attachment is obsolete: true
Comment on attachment 8475609 [details] [diff] [review]
Disable HTMLDNSPrefetches for offline apps

This patch disables HTMLDNSPrefetches. It is enforced in the child process.
The offline state is passed to the child process in TabParent::LoadURL.
I introduced a new ipdl call to pass the status to the child, which basically copies the one in PNecko.ipdl

I also encountered an issue we might want to fix - we seem to still resolve DNS names even if the LOAD_ONLY_FROM_CACHE or LOAD_NO_NETWORK_IO flags are set. This is the stack trace: http://pastebin.mozilla.org/6061524
I'm looking into it, and I think it would be better if we fixed it in a different bug.
Attachment #8475609 - Flags: review?(jduell.mcbugs)
Comment on attachment 8474936 [details] [diff] [review]
Provide way to "set network offline" per app

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

> The observer service holds a reference to the observer, so its destructor will
> not get called until we remove it manually.

So the way this is generally handled is to use a nsIWeakReference, so that the ObserverService ref doesn't contribute to an object's refcount (and there won't be a crash if the object goes away and forgets to RemoveObserver).  But I think I'm ok with keeping OfflineObserver as a separate object and manually removing the observers in the HTTP/FTP/etc channel destructors.  The code does share a bunch of logic.  So you don't need to change anything in this patch for this--just FYI.

::: netwerk/base/src/OfflineObserver.cpp
@@ +103,5 @@
> +  // Obtain App ID
> +  uint32_t appId = GetAppId();
> +
> +  // NECKO_UNKNOWN_APP_ID means global network interface has changed,
> +  // so all apps must check NS_IsAppOffline() in case they are wifi-only.

UNKNOWN not needed any more, per my later comment.  Remove comment and the if condition that tests for it.

::: netwerk/base/src/OfflineObserver.h
@@ +28,5 @@
> +  // aSubject is of type nsAppOfflineInfo and contains appId and offline mode.
> +  virtual nsresult OfflineNotification(nsISupports *aSubject);
> +
> +  // GetAppId returns the appId for the app associated with the parent
> +  virtual uint32_t GetAppId() { return 0; }

So it's only NeckoParent that doesn't override this?  Seems better to make this a pure virtual function, and have NeckoParent either return UNKNOWN (if it shouldn't ever get called) or 0 if there's some reason why it needs to be 0.  (Is there?)

::: netwerk/base/src/nsIOService.h
@@ +159,5 @@
> + * This class is passed as the subject to a NotifyObservers call for the
> + * "network:app-offline-status-changed" topic.
> + * Observers will use the appId and mode to get the offline status of an app.
> + * The appId may be NECKO_UNKNOWN_APP_ID to get all observers to recompute their
> + * status.

Remove comment about UNKNOWN.

::: netwerk/ipc/NeckoChild.cpp
@@ +327,5 @@
> +
> +  // Instantiate the service to make sure gIOService is initialized
> +  nsCOMPtr<nsIIOService> ioService = do_GetIOService();
> +  if (gIOService) {
> +    gIOService->SetAppOfflineInternal(aId, aOffline);

This looks really broken.  

For one, you're passing a bool to GetAppOfflineInternal, which takes an int enum.  Even if the conversion from bool -> int doesn't give you a compiler error/warning, the value is going to not match up from true/false to the 1,2,3 values of the enum.

Next, I don't think we want to run the same logic as GetAppOfflineInternal.  I'm not sure if we can get the nsINetworkManager on the child to calculate isWifiOn, and we also shouldn't need it? (and IsWifiOn() short-circuits in your code if it's called in the child, so we'd get the wrong answer half the time).  We have the end result (app is offline or online), so we don't necessarily need to keep info about which apps are marked WIFI_ONLY (if we ever need to query that info on the child, we would need to keep that info, but AFAICT there's only IsAppOnline, which is a simple online/offline answer).

I can see two pathways here:

1) we keep a different data structure than mAppsOfflineStatus (that has only bool online/offline values for each app, not the int32 status.

2) keep the same mAppsOfflineStatus data structure, but only store OFFLINE in it in the child (i.e. WIFI_ONLY apps would simply be OFFLINE if we get a message saying they're offline).  We'd set those values using some other function (horrible name: SetAppOfflineInternalFromChild(appId, bool)) that just sets (or removes) the OFFLINE value in mAppsOfflineStatus, and fires off a app-offline notification (unconditionaly: we could MOZ_ASSERT the case where we have the app marked as OFFLINE and we get an incoming 'offline' IPDL message, since if the code is correct we should only get IPDL messages when there's a change in status.

3) We could extend mAppsOfflineStatus to include {appId, ONLINE|OFFLINE|WIFI_ONLY, online|offline).  We'd only need the 3rd parameter (Boolean online/offline) for the child, but if we ever need to know if an app is set Wifi-only, we'd be able to query that too.  And of course we'd have to keep the parent/child in sync.

I think #2 sounds the best.  There's no need to add the ability to query the enum on the child--it seems like the boolean online/offline is all we need for now.  And #2 lets us keep the same data structure.  I guess doing #1 and splitting nsIOService into separate Base/Parent/Child versions might be be most pure way of doing this.  But I'm not sure it's worth it.  Think about it and let me know if you have an opinion.

::: netwerk/ipc/NeckoParent.cpp
@@ +827,5 @@
> +    nsRefPtr<TabParent> tabParent =
> +      static_cast<TabParent*>(Manager()->ManagedPBrowserParent()[i]);
> +    uint32_t appId = tabParent->OwnOrContainingAppId();
> +
> +    // targetAppId is NECKO_UNKNOWN_APP_ID when the network comes online again

// when the network interface has changed

@@ +829,5 @@
> +    uint32_t appId = tabParent->OwnOrContainingAppId();
> +
> +    // targetAppId is NECKO_UNKNOWN_APP_ID when the network comes online again
> +    // and wifi-only apps might need to recompute their state.
> +    if (appId == targetAppId || targetAppId == NECKO_UNKNOWN_APP_ID) {

So we don't need to check for UNKNOWN_APP here any more, and we shouldn't ever SendAppOfflineStatus(UNKNOWN_APP), right?  The change you made to IOService to enumerate over the list of statuses will mean that we'll get a OfflineNotification for each WIFI_ONLY app that needs a notification.  We don't need to use UNKNOWN_APP as a wildcard any more.  Just check for targetAppId being a match with an app in the process.

@@ +832,5 @@
> +    // and wifi-only apps might need to recompute their state.
> +    if (appId == targetAppId || targetAppId == NECKO_UNKNOWN_APP_ID) {
> +      if (gIOService) {
> +        bool offline = false;
> +        gIOService->IsAppOffline(appId, &offline);

check for success from IsAppOffline() and if it fails, LOG an "Unexpected: appId not found by isAppOffline()" message (with the appID printed) and don't drop down to SendAppOfflineStatus()

@@ +836,5 @@
> +        gIOService->IsAppOffline(appId, &offline);
> +        if (!SendAppOfflineStatus(appId, offline)) {
> +          printf_stderr("NeckoParent: "
> +                        "SendAppOfflineStatus failed for appId: %u\n", appId);
> +        }

nit: once you've found a match where targetAppId == appId, you can break;

::: netwerk/ipc/PNecko.ipdl
@@ +99,5 @@
>     * that was passed to the PBrowserOrId param in to the PHttpChannel constructor
>     */
>    AsyncAuthPromptForNestedFrame(uint64_t nestedFrameId, nsCString uri,
>                                  nsString realm, uint64_t callbackId);
> +  AppOfflineStatus(uint32_t id, bool offline);

call the parameter 'appId', just for consistency.  And add comment

  // Notifies child that a given app is now offline (or online)

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +208,5 @@
>      return SendFailedAsyncOpen(rv);
>  
> +  bool appOffline = false;
> +  uint32_t appId = GetAppId();
> +  if (appId != NECKO_UNKNOWN_APP_ID) {

So in regular single-process mode (or B2G when we launch channels in the parent only, which we might do for things like updates and fetching phishing lists, etc), there may still be a loadContext, and its appId would be 0 (NO_APP).  So check for both != UNKNOWN and != NO_APP here (and in the same place for FTP/websockets and the other places in HTTP where you do similar things)

@@ +350,5 @@
>    }
>  
> +  bool appOffline = false;
> +  uint32_t appId = GetAppId();
> +  if (appId != NECKO_UNKNOWN_APP_ID) {

ditto

@@ +356,5 @@
> +  }
> +
> +  if (appOffline) {
> +    mChannel->Cancel(NS_ERROR_OFFLINE);
> +    mStatus = NS_ERROR_OFFLINE;

What we should really be doing here is setting 

  loadFlags |= nsICachingChannel::LOAD_ONLY_FROM_CACHE

not canceling the channel. (do it by GetLoadFlags, |= the value, then SetLoadFlags).

Try that and see if it works.

Then try a http->ftp redirect and see what happens.  Hopefully the 

   NS_IsOffline() || NS_IsAppOffline(appId)

you added to FTPConnectionThread.cpp in your other patch will fail the FTP channel.  If not, you can Cancel() it in the FTPParent::ConnectChannel function.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2573,2 @@
>      uint32_t cacheEntryOpenFlags;
> +    bool offline = gIOService->IsOffline() || appOffline;

This is the line that I would have expected would make redirects fail, i.e. we'd detect here that we're offline and we wouldn't load over the network (as you report redirected-to channels are still doing).  Could be worth stepping through in a debugger.
Attachment #8474936 - Flags: review?(jduell.mcbugs) → review-
Comment on attachment 8474825 [details] [diff] [review]
Have the JS TCP sockets check if the app is offline

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

I think this is mostly ready.  +r with nit fixed.

::: dom/network/src/TCPSocketParent.cpp
@@ +70,5 @@
> +
> +  uint32_t targetAppId = nsIScriptSecurityManager::UNKNOWN_APP_ID;
> +  info->GetAppId(&targetAppId);
> +
> +  if (targetAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID) {

I don't think we're going to support NO_APP as the target, either. It's not an app.  So check for that too.
Attachment #8474825 - Flags: review?(jduell.mcbugs) → review+
(In reply to Jason Duell (:jduell) from comment #99)
> Comment on attachment 8474936 [details] [diff] [review]
> 
> ::: netwerk/ipc/NeckoChild.cpp
> @@ +327,5 @@
> > +
> > +  // Instantiate the service to make sure gIOService is initialized
> > +  nsCOMPtr<nsIIOService> ioService = do_GetIOService();
> > +  if (gIOService) {
> > +    gIOService->SetAppOfflineInternal(aId, aOffline);
> 
> This looks really broken.  
> 
> For one, you're passing a bool to GetAppOfflineInternal, which takes an int
> enum.  Even if the conversion from bool -> int doesn't give you a compiler
> error/warning, the value is going to not match up from true/false to the
> 1,2,3 values of the enum.
> 

I actually fixed this some time ago, but qrefreshed it onto the wrong patch. The child ends up being aware of an online/offline state ( it shouldn't care about wifi-only, as nsINetworkManager actually crashes if it tries to get the interface in the child )
>  actually fixed this some time ago

Splendid.  We're getting close then.
Honza, do you have anything to say about comment 83?  Do you know what validation semantics apps should get?  I assume we need to hear from Jonas to resolve this, but maybe you have some info that would make the choice clear.
Flags: needinfo?(honzab.moz)
Traditionally if the user uses the "work offline" menuitem on desktop we treat that as LOAD_ONLY_FROM_CACHE, right? I.e. we use resources from cache even if they normally would be considered expired.

If so, I think LOAD_ONLY_FROM_CACHE is the way to go.

In fact, I could probably be convinced that LOAD_ONLY_FROM_CACHE is the way to go no matter what we do for the "work offline" menuitem.
Flags: needinfo?(jonas)
Attachment #8474936 - Attachment is obsolete: true
Attached patch per_app_interdiff.patch (obsolete) — Splinter Review
I have addressed most of the comments.

I only have a few concerns left. Setting the flags in ConnectChannel doesn't seem to work. I fired up wireshark, immediately after setting the browser offline, it still makes network requests. If I cancel the channel (as I do now), it fails, and a refresh is needed to load it from the cache.
Is there a simple way of stopping requests from doing network requests?

Another issue that is only obvious in wireshark, is that although FTP connections don't load in the browser, some FTP requests are made over the network. Not really sure how to stop them.
Attachment #8474938 - Attachment is obsolete: true
Attachment #8476401 - Flags: review?(jduell.mcbugs)
(In reply to Jason Duell (:jduell) from comment #83)
> Jonas,
> 
> So there's some question as to which load flag we should use for channels
> when their app is offline, LOAD_NO_NETWORK_IO or LOAD_ONLY_FROM_CACHE:
> 
>   LOAD_NO_NETWORK_IO : This flag differs from LOAD_ONLY_FROM_CACHE in that
>      * this flag fails the load if validation is required while
>      * LOAD_ONLY_FROM_CACHE skips validation where possible

It doesn't.  LOAD_FROM_CACHE does.  I think we can combine these all (maybe except load_only_from_cache) together to get what we want.  I think NO_NET_IO is a must for this work.  Only this way you ensure the app doesn't go out.

> 
> Do we want to skip validation and deliver cached content when we're offline,
> or should unvalidated content just fail?
> 
> Oh, and LOAD_NO_NETWORK_IO apparently fails channels with
> NS_ERROR_DOCUMENT_NOT_CACHED instead of NS_ERROR_OFFLINE.  Do we care which
> error code gets returned?  Oh, hmm, looks like that's the error code that
> gets returned when we use LOAD_ONLY_FROM_CACHE, too.  So we'll need more
> changes to the patch if NOT_CACHED is not the error we want.
Flags: needinfo?(honzab.moz)
Removed check for unknown app notification
Attachment #8474834 - Attachment is obsolete: true
Valentin: for future reference, when you've got multiple patches in a bug, it's handy to label them "part 1, v3", etc (with part 0 being the first thing that would need to land hg-wise).
>>   LOAD_NO_NETWORK_IO : This flag differs from LOAD_ONLY_FROM_CACHE in that
>>      * this flag fails the load if validation is required while
>>      * LOAD_ONLY_FROM_CACHE skips validation where possible
>
> It doesn't.  LOAD_FROM_CACHE does.

Honza:

So does that mean we should change the comment in the IDL?

>  I think we can combine these all (maybe except load_only_from_cache) together
>  to get what we want.  I think NO_NET_IO is a must for this work.  Only this
>  way you ensure the app doesn't go out.

Will this give us the semantics that Jonas wants in comment 104? I.e we'll use resources from cache even if they normally would be considered expired?  I'm not clear on why we can't just use LOAD_FROM_CACHE, but I'll trust you (our caching loadflags are really confusing and have very misleading names).
Flags: needinfo?(honzab.moz)
Comment on attachment 8476401 [details] [diff] [review]
Provide way to "set network offline" per app

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

One more revision, hopefully we can land tomorrow...

> Setting the flags in ConnectChannel doesn't seem to work.

Try setting the flags Honza seems to be recommending (NO_NETWORK_IO | LOAD_FROM_CACHE and maybe also LOAD_ONLY_FROM_CACHE) and see if it still happens?  Ideally we'd like to not cancel, because we'd like to load from cache if possible.  But I expect few apps are going to be deliberately storing things they want to use offline behind a URI that redirects every time, so it's not a huge deal if we wind up cancel()ing if that's all that works (though we should figure out what's going on in a followup).

>  is that although FTP connections don't load in the browser, some FTP requests are made over the network. Not really sure how to stop them.

Weird.  Look into it if you have time. But we don't really have time. Fortunately I expect the % of traffic that uses FTP on B2G to be vanishingly small (and most would be downloads, which we're not canceling anyway), so this doesn't need to block progress in this bug.

::: netwerk/base/src/OfflineObserver.cpp
@@ +91,5 @@
> +
> +uint32_t
> +DisconnectableParent::GetAppId()
> +{
> +  return NECKO_UNKNOWN_APP_ID;

Move to NeckoParent::GetAppId()

::: netwerk/base/src/OfflineObserver.h
@@ +28,5 @@
> +  // aSubject is of type nsAppOfflineInfo and contains appId and offline mode.
> +  virtual nsresult OfflineNotification(nsISupports *aSubject);
> +
> +  // GetAppId returns the appId for the app associated with the parent
> +  virtual uint32_t GetAppId();

I'd still like to see this be a pure virtual function, i.e.

    virtual uint32_t GetAppId() = 0;

with the version that returns UNKNOWN in NeckoParent.

::: netwerk/base/src/nsIOService.cpp
@@ +917,5 @@
> +IsWifiActive()
> +{
> +    // We don't need to do this check inside the child process
> +    if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +        return false;

It's most likely just going to confuse things (as it already has) if this function can be called by the child but always returns 'false'.  Make it MOZ_RELEASE_ASSERT that we're not in the child, and never call it when we're in the child.

@@ +1021,5 @@
>          if (!mOfflineForProfileChange && mManageOfflineStatus) {
>              TrackNetworkLinkStatusForOffline();
>          }
>      }
> +    else if (!strcmp(topic, kNetworkActiveChanged)) {

Are we sure that network-active-changed never gets called in the child?  Because if it does, and we run the logic below, things are going to be messed up.  Why don't you guard all this code in this block with 

  if(!IsNeckoChild())

to make sure we don't do anything if at some point this gets fired on the child.  Or a MOZ_RELEASE_ASSERT(!IsNeckoChild()) if you know that we're not supposed to get this notification in the child.

@@ +1389,5 @@
> +
> +NS_IMETHODIMP
> +nsIOService::SetAppOffline(uint32_t aAppId, int32_t aState)
> +{
> +    NS_ENSURE_TRUE(XRE_GetProcessType() == GeckoProcessType_Default,

!IsNeckoChild() is slightly faster (caches the result), easier to read, and shorter.

@@ +1418,5 @@
> +        // The app is already in this state. Nothing needs to be done.
> +        return;
> +    }
> +
> +    bool wifiActive = IsWifiActive();

only call IsWifiActive() if !IsNeckoChild().

(or, given my next comment, just stick a MOZ_RELEASE_ASSERT(!IsNeckoChild()) at the top of the function.)

@@ +1426,5 @@
> +    switch (aState) {
> +    case nsIAppOfflineInfo::OFFLINE:
> +        mAppsOfflineStatus.Put(aAppId, nsIAppOfflineInfo::OFFLINE);
> +        if (!offline) {
> +            NotifyAppOfflineStatus(aAppId, nsIAppOfflineInfo::OFFLINE);

so... this is still broken.  As I mentioned in my last review, on the child we want to call the notifyAppOfflineStatus() unconditionally (without checking 'offline', which is bogus on the child)

> 2) keep the same mAppsOfflineStatus data structure, but only store OFFLINE in
> it in the child (i.e. WIFI_ONLY apps would simply be OFFLINE if we get a
> message saying they're offline).  We'd set those values using some other
> function (horrible name: SetAppOfflineInternalFromChild(appId, bool)) that
> just sets (or removes) the OFFLINE value in mAppsOfflineStatus, and fires off
> a app-offline notification (unconditionaly: we could MOZ_ASSERT the case where
> we have the app marked as OFFLINE and we get an incoming 'offline' IPDL
> message, since if the code is correct we should only get IPDL messages when
> there's a change in status.

So you can either change SetAppOfflineInternal to behave correctly when called
from the child, or add a new function that doesn't check isWifiOffline() nor
'offline' before firing a notification.   I think the code will be cleaner if
you use a separate function (and let's also change SetAppOfflineInternal's name
to SetAppOfflineInParent(), so we have that and SetAppOfflineInChild(),
since they are quite different and the names will make it clearer what's going
on).

::: netwerk/ipc/NeckoChild.cpp
@@ +325,5 @@
> +{
> +  // Instantiate the service to make sure gIOService is initialized
> +  nsCOMPtr<nsIIOService> ioService = do_GetIOService();
> +  if (gIOService) {
> +    gIOService->SetAppOfflineInternal(aId, aOffline ?

Per other comments: this will wind up calling SetAppOfflineInChild()

::: netwerk/ipc/NeckoParent.cpp
@@ +833,5 @@
> +        bool offline = false;
> +        nsresult rv = gIOService->IsAppOffline(appId, &offline);
> +        if (NS_FAILED(rv)) {
> +          printf_stderr("Unexpected - NeckoParent: "
> +                        "appId not found by isAppOffline(): %u\n", appId);

Why is this unexpected?  If the transition is from ONLINE to OFFLINE or WIFI-ONLY, we won't have an entry in IsAppOffline, right?

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +215,5 @@
> +  }
> +
> +  uint32_t loadFlags = aLoadFlags;
> +  if (appOffline) {
> +    loadFlags |= nsICachingChannel::LOAD_ONLY_FROM_CACHE;

Sounds like Honza wants us to use 

   LOAD_NO_NETWORK_IO | LOAD_FROM_CACHE

and maybe LOAD_ONLY_FROM_CACHE too (check in the http code to see if adding it makes sense if you have time, else let's wait for what Honza says).
Attachment #8476401 - Flags: review?(jduell.mcbugs) → review-
Comment on attachment 8475609 [details] [diff] [review]
Disable HTMLDNSPrefetches for offline apps

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

I don't see why you need anything other than the nsHTMLDNSPrefetch.cpp change here.

::: content/html/content/src/nsHTMLDNSPrefetch.cpp
@@ +93,5 @@
>  
>  bool
>  nsHTMLDNSPrefetch::IsAllowed (nsIDocument *aDocument)
>  {
> +  if (NS_IsAppOffline(aDocument->NodePrincipal())) {

This code looks fine.

::: dom/ipc/PBrowser.ipdl
@@ +523,5 @@
>  
> +    /**
> +     * Tell the child of an app's offline status
> +     */
> +    AppOfflineStatus(uint32_t id, bool offline);

I don't see why you need an IPDL message to TabChild.  Doesn't the app-offline notification that NeckoChild listens to populate the info we need for NS_IsAppOffline() to work?

::: dom/ipc/TabParent.cpp
@@ +505,5 @@
> +    uint32_t appId = OwnOrContainingAppId();
> +    if (NS_IsAppOffline(appId)) {
> +      // If the app is offline in the parent process
> +      // pass that state to the child process as well
> +      unused << SendAppOfflineStatus(appId, true);

I think we're fine with the PNecko AppOfflineStatus IPDL message only.  But even if we did need one for the Tabchild, this would only send the status when the next URI is loaded, which is not deterministic.
Attachment #8475609 - Flags: review?(jduell.mcbugs) → review-
Comment on attachment 8475609 [details] [diff] [review]
Disable HTMLDNSPrefetches for offline apps

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

::: content/html/content/src/nsHTMLDNSPrefetch.cpp
@@ +93,5 @@
>  
>  bool
>  nsHTMLDNSPrefetch::IsAllowed (nsIDocument *aDocument)
>  {
> +  if (NS_IsAppOffline(aDocument->NodePrincipal())) {

Oh wait--we were originally talking about only disabling DNSPrefetch is all apps in the process were offline.  I assume this code is smart enough to know the exact appID of the prefetch?  It looks like it.
(In reply to Jason Duell (:jduell) from comment #113)
> Comment on attachment 8475609 [details] [diff] [review]
> Disable HTMLDNSPrefetches for offline apps
> 
> Review of attachment 8475609 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/nsHTMLDNSPrefetch.cpp
> @@ +93,5 @@
> >  
> >  bool
> >  nsHTMLDNSPrefetch::IsAllowed (nsIDocument *aDocument)
> >  {
> > +  if (NS_IsAppOffline(aDocument->NodePrincipal())) {
> 
> Oh wait--we were originally talking about only disabling DNSPrefetch is all
> apps in the process were offline.  I assume this code is smart enough to
> know the exact appID of the prefetch?  It looks like it.

This was a problem for new processes, which did not have the offline info right away. Now that they do, we can just chech if it's offline (in the child) and simply not do the prefetch.
(In reply to Jason Duell (:jduell) from comment #112)
> Comment on attachment 8475609 [details] [diff] [review]
> Disable HTMLDNSPrefetches for offline apps
> 
> Review of attachment 8475609 [details] [diff] [review]:
> >  
> > +    /**
> > +     * Tell the child of an app's offline status
> > +     */
> > +    AppOfflineStatus(uint32_t id, bool offline);
> 
> I don't see why you need an IPDL message to TabChild.  Doesn't the
> app-offline notification that NeckoChild listens to populate the info we
> need for NS_IsAppOffline() to work?
> 

It only populates it when the SetAppOffline API is called.
Per comment 86 we need to send the app status when initializing the app/process. I duplicated the code from neckoParent/child because I didn't find a way to get the NeckoParent in TabParent. If there's a way, that would make it all a bit better.

> ::: dom/ipc/TabParent.cpp
> @@ +505,5 @@
> > +    uint32_t appId = OwnOrContainingAppId();
> > +    if (NS_IsAppOffline(appId)) {
> > +      // If the app is offline in the parent process
> > +      // pass that state to the child process as well
> > +      unused << SendAppOfflineStatus(appId, true);
> 
> I think we're fine with the PNecko AppOfflineStatus IPDL message only.  But
> even if we did need one for the Tabchild, this would only send the status
> when the next URI is loaded, which is not deterministic.

According to comment 75, this is sent every time a process is initialized (and probably other times as well). We need this to keep the child process table in sync with the one in the parent. Future changes to the app offline status would be delivered to the child via the PNecko ipdl message.
(In reply to Jason Duell (:jduell) from comment #110)
> >>   LOAD_NO_NETWORK_IO : This flag differs from LOAD_ONLY_FROM_CACHE in that
> >>      * this flag fails the load if validation is required while
> >>      * LOAD_ONLY_FROM_CACHE skips validation where possible
> >
> > It doesn't.  LOAD_FROM_CACHE does.
> 
> Honza:
> 
> So does that mean we should change the comment in the IDL?

Absolutely not.  It conforms with what I say, if I of course express myself correctly :D

LOAD_FROM_CACHE: force load from the cache even for a stall content if available, otherwise - go to the server freely

LOAD_ONLY_FROM_CACHE: if validation of a cached content (if present) is needed, fail the load


So, I'm for combo of all the 3 flags.  This will make web content/caching work and we have something like an "offline cache" that way - similar to comment 104.  But depends, sometimes web apps may want let the chrome show "sorry, you are offline, we don't have anything to show you".  

Could be preferable...


> 
> >  I think we can combine these all (maybe except load_only_from_cache) together
> >  to get what we want.  I think NO_NET_IO is a must for this work.  Only this
> >  way you ensure the app doesn't go out.
> 
> Will this give us the semantics that Jonas wants in comment 104? I.e we'll
> use resources from cache even if they normally would be considered expired? 
> I'm not clear on why we can't just use LOAD_FROM_CACHE, but I'll trust you
> (our caching loadflags are really confusing and have very misleading names).

He means LOAD_FROM_CACHE.  And, it's actually different, but with the same result: when we are offline, we open the cache entry as read-only at [1], since there is no way to go out and get potentially a new content anyway (also prevents creation of empty entries).  And then, based on the "read-only" flag set, we bypass validation completely at [2].

Hence, if gIOService->IsOffline() may return TRUE for a channel belonging to an app that is set offline, we win.  Still NO_NET_IO IMO should apply too.

However, LOAD_FROM_CACHE eventually does the same thing as gIOService->IsOffline() == true.


[1] http://hg.mozilla.org/mozilla-central/annotate/0b9dd32d1e16/netwerk/protocol/http/nsHttpChannel.cpp#l2566
[2] http://hg.mozilla.org/mozilla-central/annotate/0b9dd32d1e16/netwerk/protocol/http/nsHttpChannel.cpp#l2746
Flags: needinfo?(honzab.moz)
Comment on attachment 8475609 [details] [diff] [review]
Disable HTMLDNSPrefetches for offline apps

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

Sorry, didn't read comments clearly enough to realize the PBrowser parts of this were for initialing app offline/online status.

As mentioned in IRC, we only need to set this once, at first LoadURI, so it would be nice to add a bool to TabParent to only send it to app the 1st time.  Otherwise this looks good.

> I also encountered an issue we might want to fix - we seem to still resolve
> DNS names even if the LOAD_ONLY_FROM_CACHE or LOAD_NO_NETWORK_IO flags are
> set. This is the stack trace: http://pastebin.mozilla.org/6061524 I'm looking
> into it, and I think it would be better if we fixed it in a
> different bug.

Yes, it's very little traffic, so given the time frame for landing let's follow up in another bug.
Attachment #8475609 - Flags: review- → review+
Attachment #8476401 - Attachment is obsolete: true
Attachment #8474862 - Attachment is obsolete: true
Attachment #8474825 - Attachment is obsolete: true
Attachment #8476998 - Attachment is obsolete: true
Attachment #8453501 - Attachment is obsolete: true
Attachment #8475609 - Attachment is obsolete: true
Make GetAppId() pure-virtual and add implementation for TCPSocketParent, UDPSocketParent, and one for Necko parent that returns UNKNOWN_APP_ID
Change nsIOService to use IsNeckoChild instead of XRE_ method.
Include nsThreadUtils.h (for nsRunnable and dispatchToMainThread) and NeckoCommon for IsNeckoChild.
Make HttpChannelParent set LOAD_ONLY_FROM_CACHE, LOAD_FROM_CACHE and LOAD_NO_NETWORK_IO when offline.
Attachment #8477739 - Flags: review?(jduell.mcbugs)
Attachment #8476406 - Attachment is obsolete: true
Comment on attachment 8477744 [details] [diff] [review]
Part 7 - xpcshell test for HTTP and mochitest for WebSocket per-app offline

bzexport malfunctioned and uploaded the patch twice.
Attachment #8477744 - Attachment is obsolete: true
Attachment #8477744 - Flags: review?(jduell.mcbugs)
Attachment #8460039 - Attachment is obsolete: true
Attachment #8460039 - Flags: review?(jduell.mcbugs)
Comment on attachment 8477739 [details] [diff] [review]
Part 8 - Several improvements to per-app-offline behaviour

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

looks good
Attachment #8477739 - Flags: review?(jduell.mcbugs) → review+
Added cleanup in case the test times out
Attachment #8477797 - Flags: review?(jduell.mcbugs)
Attachment #8477743 - Attachment is obsolete: true
Attachment #8477743 - Flags: review?(jduell.mcbugs)
Changed IsAppOffline to always return true if mOffline is set
Attachment #8477724 - Attachment is obsolete: true
Attachment #8477731 - Attachment is obsolete: true
Refreshed on top of part 1
Comment on attachment 8477807 [details] [diff] [review]
Part 9 - Add better comments to nsIOService

Another bzexport error.
Attachment #8477807 - Attachment is obsolete: true
Rebased on top of Part 1
Attachment #8477739 - Attachment is obsolete: true
Comment on attachment 8477804 [details] [diff] [review]
Part 1 - Provide way to "set network offline" per app

Reviewed by :jduell
Attachment #8477804 - Flags: review+
Comment on attachment 8477805 [details] [diff] [review]
Part 2 - Modify calls to NS_IsOffline to check for NS_IsAppOffline as well

Reviewed by :jduell and :sicking
Attachment #8477805 - Flags: review+
Comment on attachment 8477734 [details] [diff] [review]
Part 3 - Have the JS TCP sockets check if the app is offline

Reviewed by :jduell
Attachment #8477734 - Flags: review+
Comment on attachment 8477735 [details] [diff] [review]
Part 4 - Have UDP sockets check if the app is offline

Reviewed by :mcmanus and :jduell
Attachment #8477735 - Flags: review+
Comment on attachment 8477736 [details] [diff] [review]
Part 5 - Check app offline status in PeerConnection.js

Reviewed by :rjesup
Attachment #8477736 - Flags: review+
Comment on attachment 8477737 [details] [diff] [review]
Part 6 - Disable HTMLDNSPrefetches for offline apps

Reviewed by :jduell
Attachment #8477737 - Flags: review+
Comment on attachment 8477811 [details] [diff] [review]
Part 8 - Several improvements to per-app-offline behaviour

Reviewed by :jduell
Attachment #8477811 - Flags: review+
Comment on attachment 8477739 [details] [diff] [review]
Part 8 - Several improvements to per-app-offline behaviour

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

looks good
Attachment #8477797 - Attachment is obsolete: true
Attachment #8477797 - Flags: review?(jduell.mcbugs)
Attachment #8477817 - Flags: review?(jduell.mcbugs) → review+
Attachment #8477796 - Flags: review?(jduell.mcbugs) → review+
Blocks: 1057686
Blocks: 1057687
Blocks: 1057688
Blocks: 1057689
We'd like the following from QA at some point (once there's UI for turning on/off offline per app, i.e. once bug 1042745 lands):

- Test navigator.online to make sure it report online/offline in sync with actual online/offline state per app

- Test that apps are indeed offline: they should be able to load cached resources (and their own resource from their application.zip file) but nothing from the network.
  - Test HTTP connections
  - Test websockets
  - Test UDP/TCP sockets
  - Test WebRTC

- Try marking an app offline when it's not running, then launch it and verify that no network activity occurs for it.

- Same for new browser tab (which launches new process: it should have correct online/offline status)
Keywords: qawanted
https://hg.mozilla.org/mozilla-central/rev/1ed271ffb59c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1058718
Switching to verifyme since the request above is a post landing verification need.
Keywords: qawantedverifyme
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla34 → ---
Comment on attachment 8479547 [details] [diff] [review]
Part 10 - Fix app wrongly reporting an offline status

Description of the bug:
In order for the content process to have an up-to-date table of which apps are
offline, we send that status in TabParent::LoadURL. However, the fact that
IsAppOffline returned true when the browser was offline, caused us to send
the offline status to content processes, and caused Bug 1058718.
This also updates the UUID for nsIOService.
Attachment #8479547 - Attachment description: Fix app wrongly reporting an offline status → Part 10 - Fix app wrongly reporting an offline status
Part 2 of this bug adds nsGlobalWindow as an observer for the app-offline
notification. There are however a few corner cases we haven't handled.
For example: If the browser is offline, and an app is made offline,
there should be no offline event dispatched.
Also, WorkerPrivate should ignore offline events that cause no change
in its offline state.
Attachment #8479550 - Flags: review?(jonas)
Attachment #8479547 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 8479550 [details] [diff] [review]
Part 11 - Address possible issues with offline notifications in nsGlobalWindow and WorkerPrivate

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

looks fine to me FWIW
Attachment #8479550 - Flags: feedback+
Blocks: 1060108
Attachment #8477735 - Attachment is obsolete: true
Attachment #8477811 - Attachment is obsolete: true
Rebased on top of m-c
Attachment #8477804 - Attachment is obsolete: true
Comment on attachment 8479550 [details] [diff] [review]
Part 11 - Address possible issues with offline notifications in nsGlobalWindow and WorkerPrivate

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

These changes look correct, but I think you'll also need to change ::Observe in dom/workers/RuntimeService.cpp

http://mxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#2476
Attachment #8479550 - Flags: review?(jonas) → review+
Attachment #8477825 - Attachment is obsolete: true
Attachment #8480977 - Attachment is obsolete: true
Part 2 of this bug adds nsGlobalWindow as an observer for the app-offline
notification. There are however a few corner cases we haven't handled.
For example: If the browser is offline, and an app is made offline,
there should be no offline event dispatched.
Also, WorkerPrivate should ignore offline events that cause no change
in its offline state.
Attachment #8479550 - Attachment is obsolete: true
Comment on attachment 8481741 [details] [diff] [review]
Part 11 - Address possible issues with offline notifications in nsGlobalWindow and WorkerPrivate

Reviewed by :sicking in comment 159
Attachment #8481741 - Flags: review+
Attachment #8481739 - Flags: review+
Attachment #8480976 - Flags: review+
Attachment #8480975 - Flags: review+
Part 2 of this bug adds nsGlobalWindow as an observer for the app-offline
notification. There are however a few corner cases we haven't handled.
For example: If the browser is offline, and an app is made offline,
there should be no offline event dispatched.
Also, WorkerPrivate should ignore offline events that cause no change
in its offline state.
Attachment #8481741 - Attachment is obsolete: true
Comment on attachment 8481779 [details] [diff] [review]
Part 11 - Address possible issues with offline notifications in nsGlobalWindow and WorkerPrivate

The change Jonas suggested in comment 159 proved fair, but implementing it uncovered a bug in handling the APP_OFFLINE_STATUS_TOPIC in RuntimeService.cpp
In turn, I changed it so that when receiving the OFFLINE_STATUS or the APP_OFFLINE_STATUS, the runtime service would send an offline change event with the value of NS_IsOffline, and in WorkerPrivateParent<Derived>::OfflineStatusChangeEvent I also check NS_IsAppOffline(mLoadInfo.mPrincipal).

I validated that everything works well with carefully placed printfs :)
Re-requesting review.

https://tbpl.mozilla.org/?tree=Try&rev=e67f98773da2
Attachment #8481779 - Flags: review?(jonas)
PS. The bug was in RuntimeService::Observe, when calling GetPrincipalForAsmJSCacheOp. It asserts that it's not on the main thread. Having the workers figure out their appOffline status based on the principal seems like a better idea.
Comment on attachment 8481779 [details] [diff] [review]
Part 11 - Address possible issues with offline notifications in nsGlobalWindow and WorkerPrivate

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

Bent should review this. Unless he wants me to :)
Attachment #8481779 - Flags: review?(jonas) → review?(bent.mozilla)
Comment on attachment 8481739 [details] [diff] [review]
Part 1 - Provide way to "set network offline" per app

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

::: netwerk/base/public/nsIIOService.idl
@@ +102,5 @@
> +     * Returns true if given appId is currently not allowed to make network
> +     * connections. It will return true if the app is in the wifi-only state
> +     * and we are currently on a 3G connection.
> +     */
> +    boolean isAppOffline(in uint32_t appId);

Can you add a function that returns the offline state, or replace this one? Something like..

> long getAppOfflineState(in uint32_t appId);

With the API exposed this way, we have no way of knowing from chrome if an app is just online because there is a wifi connection (i.e. WIFI_ONLY), or if it is online because it was enabled for both kinds of connections (ONLINE)..
Attachment #8485312 - Flags: review?(jduell.mcbugs)
Attachment #8485312 - Flags: feedback?(marshall)
We could potentially replace isAppOffline with getAppOfflineState, as this duplicates a very small portion of code, but IMO it's better to have them separate as they perform slightly different functions. Unless Jason disagrees. :)
Comment on attachment 8481779 [details] [diff] [review]
Part 11 - Address possible issues with offline notifications in nsGlobalWindow and WorkerPrivate

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

::: dom/base/nsGlobalWindow.cpp
@@ -1078,5 @@
>      mHavePendingClose(false),
>      mHadOriginalOpener(false),
>      mIsPopupSpam(false),
>      mBlockScriptedClosingFlag(false),
> -    mFireOfflineStatusChangeEventOnThaw(false),

Nit: Maybe just put a comment in the initializer list (in the right spot) saying that you didn't forget |mWasOffline| but it gets initialized later in the body.

@@ +1128,5 @@
>        !aOuterWindow) {
>      SetIsDOMBinding();
>    }
>  
> +  mWasOffline = NS_IsOffline() || NS_IsAppOffline(GetPrincipal());

Please ensure that nothing in GetPrincipal() can call AddRef (including QueryInterface) on 'this' nsGlobalWindow because you're still in the constructor and don't yet have a valid (non-zero) refcount.

::: dom/workers/WorkerPrivate.cpp
@@ +2964,5 @@
>    AssertIsOnParentThread();
>  
>    nsRefPtr<OfflineStatusChangeRunnable> runnable =
> +    new OfflineStatusChangeRunnable(ParentAsWorkerPrivate(), aIsOffline ||
> +      NS_IsAppOffline(mLoadInfo.mPrincipal));

Unfortunately mLoadInfo.mPrincipal is a main-thread-only object: calling any methods on it (and, really, calling any functions that use it) must only happen on the main thread.

In the case of a single non-nested worker this will be fine since the "parent thread" of a non-nested worker is the main thread. However, nested workers have non-main threads as "parent threads" so this would not be ok in those cases.

I would suggest just doing this NS_IsAppOffline check in the RuntimeService. Any nested workers will have to be part of the same app anyway. Then you'll need to keep track of which workers you should actually send the notification to...
Attachment #8481779 - Flags: review?(bent.mozilla) → review-
Comment on attachment 8485312 [details] [diff] [review]
Add getAppOfflineState to nsIIOService

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

Thanks Val!
Attachment #8485312 - Flags: feedback?(marshall) → feedback+
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #171)
> Comment on attachment 8481779 [details] [diff] [review]
> Part 11 - Address possible issues with offline notifications in
> nsGlobalWindow and WorkerPrivate
> 
> Please ensure that nothing in GetPrincipal() can call AddRef (including
> QueryInterface) on 'this' nsGlobalWindow because you're still in the
> constructor and don't yet have a valid (non-zero) refcount.
> 

It seems, if I'm not mistaken, that NS_ADDREF could be be called on this via
GetPrincipal -> GetParentInternal -> GetParent -> GetRealParent -> http://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#3907

Is there any other way I could get the appId or principal in the constructor? / may I assume that it is online by default?

> I would suggest just doing this NS_IsAppOffline check in the RuntimeService.
> Any nested workers will have to be part of the same app anyway. Then you'll
> need to keep track of which workers you should actually send the
> notification to...

The thinking was that I would always send the notification, and I'd return early in WorkerPrivate::OfflineStatusChangeEventInternal if there wasn't any change in offline status.

> >  
> >    nsRefPtr<OfflineStatusChangeRunnable> runnable =
> > +    new OfflineStatusChangeRunnable(ParentAsWorkerPrivate(), aIsOffline ||
> > +      NS_IsAppOffline(mLoadInfo.mPrincipal));
> 
> Unfortunately mLoadInfo.mPrincipal is a main-thread-only object: calling any
> methods on it (and, really, calling any functions that use it) must only
> happen on the main thread.

I could save the the appId in the constructor for WorkerPrivateParent, but that could also be either main-thread or worker-thread.
I could add an mAppId member to WorkerPrivateParent, which would be initialized from LoadInfo when on the main thread, or would get its parent's appId when nested. Does that seem correct?
Ben, does my approach in Comment 173 seem correct?
Flags: needinfo?(bent.mozilla)
Comment on attachment 8485312 [details] [diff] [review]
Add getAppOfflineState to nsIIOService

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

I'm fine with keeping isAppOffline too--it would be tedious and error-prone to make people get the state and then have to check if wifi is active, etc.

::: netwerk/base/src/nsIOService.cpp
@@ +1476,5 @@
> +    *aResult = nsIAppOfflineInfo::ONLINE;
> +
> +    if (aAppId == NECKO_NO_APP_ID ||
> +        aAppId == NECKO_UNKNOWN_APP_ID) {
> +        return NS_ERROR_NOT_AVAILABLE;

I'd return here before setting *result to ONLINE.  If it's not available, don't mess with the output arg.
Attachment #8485312 - Flags: review?(jduell.mcbugs) → review+
Just curious if there's anything I can do to help this along to land? We'd still like to land the ability to toggle mobile data per app upstream in Gaia for 2.2, and this looks so close :)
I implemented the changes described in comment 173. Try is green: https://tbpl.mozilla.org/?tree=Try&rev=4641a7868972.
GlobalWindow is assumed to be online in its constructor.
WorkerPrivateParent initializes mAppId by accessing the principal, if it's on the main thread, otherwise by getting it from its parent.
Calling NS_IsAppOffline(mAppId) instead of using the principal.

Let me know if this seems ok.
Attachment #8491884 - Flags: review?(bent.mozilla)
Attachment #8481779 - Attachment is obsolete: true
Comment on attachment 8491884 [details] [diff] [review]
Part 11 - Address possible issues with offline notifications in nsGlobalWindow and WorkerPrivate

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

Looks good to me, though I don't know enough to judge whether it's ok to assume GlobalWindow is online in its constructor.

::: dom/workers/WorkerPrivate.h
@@ +250,5 @@
>    // in Construct() and emptied by WorkerFinishedRunnable, and conditionally
>    // traversed by the cycle collector if the busy count is zero.
>    nsRefPtr<WorkerPrivate> mSelfRef;
> +  // The worker's appId needs to be cached, so we can access it on any thread
> +  uint32_t mAppId;

init this to APP_UNKNOWN in the .cpp constructor please.
Attachment #8491884 - Flags: feedback+
Comment on attachment 8491884 [details] [diff] [review]
Part 11 - Address possible issues with offline notifications in nsGlobalWindow and WorkerPrivate

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

::: dom/base/nsGlobalWindow.cpp
@@ +1082,5 @@
>      mHavePendingClose(false),
>      mHadOriginalOpener(false),
>      mIsPopupSpam(false),
>      mBlockScriptedClosingFlag(false),
> +    mWasOffline(false),

I don't think it's a good idea to assume this... The simplest fix is to add a static nsGlobalWindow::Create function (and nsGlobalChromeWindow::Create) and then make both classes have private constructors. Then add a private Init() function that figures out the offline status safely once the refcount has been stabilized.

::: dom/workers/WorkerPrivate.cpp
@@ +3021,5 @@
>    AssertIsOnParentThread();
>  
>    nsRefPtr<OfflineStatusChangeRunnable> runnable =
> +    new OfflineStatusChangeRunnable(ParentAsWorkerPrivate(), aIsOffline ||
> +      NS_IsAppOffline(mAppId));

You can't call NS_IsAppOffline off the main thread since it access a hashtable that may be mutating.

What about trying my suggestion?
Attachment #8491884 - Flags: review?(bent.mozilla) → review-
Flags: needinfo?(bent.mozilla)
Thanks for your suggestions Ben. I think addressed all the issues.
Attachment #8498109 - Flags: review?(bent.mozilla)
Attachment #8491884 - Attachment is obsolete: true
Comment on attachment 8498109 [details] [diff] [review]
Part 11 - Address possible issues with offline notifications in nsGlobalWindow and WorkerPrivate

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

This is looking much better! Only a few fixes needed:

::: dom/base/nsGlobalWindow.cpp
@@ +13377,5 @@
>  
> +/* static */ nsGlobalChromeWindow*
> +nsGlobalChromeWindow::Create(nsGlobalWindow *aOuterWindow)
> +{
> +  nsGlobalChromeWindow* window = new nsGlobalChromeWindow(aOuterWindow);

The whole point to moving this stuff into a static Create() function was so that you could guarantee a stable refcount when calling GetPrincipal(). You're not using a nsRefPtr here so you're still calling GetPrincipal() with a refcount of 0...

@@ +13810,5 @@
> +/* static */ nsGlobalModalWindow*
> +nsGlobalModalWindow::Create(nsGlobalWindow *aOuterWindow)
> +{
> +  nsGlobalModalWindow* window = new nsGlobalModalWindow(aOuterWindow);
> +  window->mWasOffline = NS_IsOffline() || NS_IsAppOffline(window->GetPrincipal());

How about you move all the duplicated code into a protected function on nsGlobalWindow?

::: dom/base/nsGlobalWindow.h
@@ +486,5 @@
>    void GetOwnPropertyNames(JSContext* aCx, nsTArray<nsString>& aNames,
>                             mozilla::ErrorResult& aRv);
>  
>    // Object Management
> +  static nsGlobalWindow* Create(nsGlobalWindow *aOuterWindow);

All of these Create() functions should return already_AddRefed<T> so that ownership is explicit (and sane).

::: dom/workers/RuntimeService.cpp
@@ +2600,5 @@
>      SendOfflineStatusChangeEventToAllWorkers(NS_IsOffline());
>      return NS_OK;
>    }
>    if (!strcmp(aTopic, NS_IOSERVICE_APP_OFFLINE_STATUS_TOPIC)) {
> +    // Code inspired by the BROADCAST_ALL_WORKERS macro

You shouldn't have to unpack the macro yourself here... Won't it work to just do:

  BROADCAST_ALL_WORKERS(
    OfflineStatusChangeEvent,
    NS_IsOffline() ||  NS_IsAppOffline(workers[index]->GetPrincipal()));
Attachment #8498109 - Flags: review?(bent.mozilla) → review-
Part 2 of this bug adds nsGlobalWindow as an observer for the app-offline
notification. There are however a few corner cases we haven't handled.
For example: If the browser is offline, and an app is made offline,
there should be no offline event dispatched.
Also, WorkerPrivate should ignore offline events that cause no change
in its offline state.
Attachment #8500665 - Flags: review?(bent.mozilla)
Attachment #8498109 - Attachment is obsolete: true
Comment on attachment 8500665 [details] [diff] [review]
Part 11 - Address possible issues with offline notifications in nsGlobalWindow and WorkerPrivate

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

Looks good, thanks!
Attachment #8500665 - Flags: review?(bent.mozilla) → review+
\o/ great work Val!
This is not yet exposed to Users, is it?
Keywords: dev-doc-needed
See Bug 1042745 for the WebAPI for this feature.
Verifying this issue seems like it'll require tools such as a packet sniffer that can detect offline mode per app. Would that be app suspending in developer mode ?  At this point verification steps are not clear.
QA Whiteboard: [QAExclude]
Flags: needinfo?(jmercado)
Flags: needinfo?(jmercado)
See Also: → 1307491
You need to log in before you can comment on or make changes to this bug.