Closed Bug 1134596 Opened 9 years ago Closed 9 years ago

Separate nsIOService's network detection from offlineMode

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

Details

Attachments

(3 files, 6 obsolete files)

      No description provided.
Attachment #8567114 - Attachment is obsolete: true
Comment on attachment 8567480 [details] [diff] [review]
Separate nsIOService's network detection from offlineMode

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

I really like this and the general approach seems fine. I'll take it out for a spin and do some local tests. Do you think it is ready for that or is there more stuff pending to fix first?

::: netwerk/base/nsIOService.cpp
@@ +1199,5 @@
>          }
>      } else if (!strcmp(topic, kProfileChangeNetRestoreTopic)) {
>          if (mOfflineForProfileChange) {
>              mOfflineForProfileChange = false;
> +            OnNetworkLinkEvent(NS_NETWORK_LINK_DATA_UNKNOWN);

I think this OnNetworkLinkEvent() call can be removed now when we split the handling of the "real" connectivity and I don't see how this is useful. This event is really not a reason for the underlying connectivity to have been modified in any way, and this code wouldn't know anyway.
(In reply to Daniel Stenberg [:bagder] from comment #3)
> Comment on attachment 8567480 [details] [diff] [review]
> Separate nsIOService's network detection from offlineMode
> 
> Review of attachment 8567480 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I really like this and the general approach seems fine. I'll take it out for
> a spin and do some local tests. Do you think it is ready for that or is
> there more stuff pending to fix first?
> 

I posted it so I could test it out on my windows machine. It seems right to me, but I want to test it e10s mode too.
This seems ripe for review. I added an IPC mechanism for connectivity, similar to the one for offline.
Attachment #8567748 - Flags: review?(mcmanus)
Attachment #8567480 - Attachment is obsolete: true
Comment on attachment 8567748 [details] [diff] [review]
Separate nsIOService's network detection from offlineMode

I think jason is the best reviewer for this
Attachment #8567748 - Flags: review?(mcmanus) → review?(jduell.mcbugs)
(In reply to Valentin Gosu [:valentin] from comment #5)

> This seems ripe for review. I added an IPC mechanism for connectivity,
> similar to the one for offline.

Isn't this version supposed to switch to "offline" even without manage-offline-status enabled? I've done some basic testing on a Vista machine of mine and I can't seem to get it to anything else than online even if I switch off my wifi (when used wifi-only).

If I enable manage-offline-status however, it will say offline.

I just want to get my expectations right before I start digging!
(In reply to Daniel Stenberg [:bagder] from comment #7)
> (In reply to Valentin Gosu [:valentin] from comment #5)
> 
> > This seems ripe for review. I added an IPC mechanism for connectivity,
> > similar to the one for offline.
> 
> Isn't this version supposed to switch to "offline" even without
> manage-offline-status enabled? I've done some basic testing on a Vista
> machine of mine and I can't seem to get it to anything else than online even
> if I switch off my wifi (when used wifi-only).
> 
> If I enable manage-offline-status however, it will say offline.
> 
> I just want to get my expectations right before I start digging!

I kept that pref for now. I think we might change the default, but I think it's best to have a pref to easily revert the behaviour.
(In reply to Valentin Gosu [:valentin] from comment #8)

> I kept that pref for now. I think we might change the default, but I think
> it's best to have a pref to easily revert the behaviour.

Right, I completely agree that having a pref is good. So with this change the manage-offline-status pref no longer sets the "Work Offline" mode automatically, right?

Then, with this change, it makes it very interesting to add a telemetry probe that counts if we get traffic or otherwise acts while we believe we're offline to get a feel for how accurate our logic is now.

Also, looking into making the Mac and Linux desktop versions be able to say "offline" when there's no non-loopback interfaces being marked as UP.
(In reply to Daniel Stenberg [:bagder] from comment #9)
> (In reply to Valentin Gosu [:valentin] from comment #8)
> 
> > I kept that pref for now. I think we might change the default, but I think
> > it's best to have a pref to easily revert the behaviour.
> 
> Right, I completely agree that having a pref is good. So with this change
> the manage-offline-status pref no longer sets the "Work Offline" mode
> automatically, right?

Yes.

> 
> Then, with this change, it makes it very interesting to add a telemetry
> probe that counts if we get traffic or otherwise acts while we believe we're
> offline to get a feel for how accurate our logic is now.

That would be good to have. I think we can safely hook that telemetry into nsSocketTransport::InitiateSocket. I'll post a patch for it.
Comment on attachment 8567748 [details] [diff] [review]
Separate nsIOService's network detection from offlineMode

Taking the review on Jason's request.
Attachment #8567748 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
Comment on attachment 8567748 [details] [diff] [review]
Separate nsIOService's network detection from offlineMode

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

As I understand (just to confirm) this patch:

- leaves the mIsOffline flag and related notifications entirely up to the user (the File/Work Offline switch on desktop)
- introduces a new state "has connectivity" that:
  - is controlled by the link service (and no longer affects online status), when management is enabled and we have a link service 
  - otherwise believe we are always connected
- there is one central point NS_IsOffline() - already widely used - that combines these two

While the existing "offline" state on io remains intact on link state changes, we still send the existing ONLINE/OFFLINE notifications when the link state changes.  But they are no longer consistent with state of the "offline" indicator on io service.  It's now only consistent with NS_IsOffline() C call.  In the overall result, what is the difference from what we have now?

r- to start a discussion, the patch otherwise doesn't look bad.

::: dom/ipc/ContentChild.cpp
@@ +1780,5 @@
>  
> +bool
> +ContentChild::RecvSetConnectivity(const bool& connectivity)
> +{
> +    nsCOMPtr<nsIIOService> io (do_GetIOService());

io(

::: dom/ipc/ContentParent.cpp
@@ +2878,5 @@
>  #endif
>      }
> +    else if (!strcmp(aTopic, NS_IPC_IOSERVICE_SET_CONNECTIVITY_TOPIC)) {
> +      NS_ConvertUTF16toUTF8 dataStr(aData);
> +      const char *isConnected = dataStr.get();

I'd rather see here compare to UTF16 literal string to save the conversion loop.  but up to you, the file style seems already screwed up...

I'm not sure what is the indention style here either :)

::: netwerk/base/nsIOService.cpp
@@ +288,2 @@
>  
> +    OnNetworkLinkEvent(NS_NETWORK_LINK_DATA_UNKNOWN);

add a comment what all this is doing.

@@ +288,3 @@
>  
> +    OnNetworkLinkEvent(NS_NETWORK_LINK_DATA_UNKNOWN);
> +    SetOffline(false);

if this (call to SetOffline(false)) is still necessary, we should move it somewhere else.  it's dangerous since this method may be called more then once while instantiating the link service might fail last time and the mNetworkLinkServiceInitialized flag is left false.

@@ +940,5 @@
>                  mProxyService->ReloadPAC();
>  
>              // don't care if notification fails
> +            // by checking NS_IsOffline we make sure we only send the
> +            // notification if we have connectivity.

ok, and when we are currently "NS_IsOffline()", when do you send this notification when we actually become (in this case) "connected" or generally "!NS_IsOffline()" again?

@@ +975,5 @@
> +    return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsIOService::SetConnectivity(bool connectivity)

aConnectivity please!

@@ +990,5 @@
> +    }
> +
> +    // This notification sends the connectivity to the child processes
> +    if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +        if (observerService) {

I think this will always pass

@@ +994,5 @@
> +        if (observerService) {
> +            (void)observerService->NotifyObservers(nullptr,
> +                NS_IPC_IOSERVICE_SET_CONNECTIVITY_TOPIC, connectivity ?
> +                MOZ_UTF16("true") :
> +                MOZ_UTF16("false"));

maybe send this notification out only when the state actually changes?

@@ +1006,5 @@
> +            static_cast<nsIIOService *>(this),
> +            NS_IOSERVICE_OFFLINE_STATUS_TOPIC,
> +            NS_LITERAL_STRING(NS_IOSERVICE_ONLINE).get());
> +        return NS_OK;
> +    } else if (!wasOffline && !connectivity) {

else after return

@@ +1012,5 @@
> +        // send the OFFLINE notification
> +        const nsLiteralString offlineString(MOZ_UTF16(NS_IOSERVICE_OFFLINE));
> +        observerService->NotifyObservers(static_cast<nsIIOService *>(this),
> +                                         NS_IOSERVICE_GOING_OFFLINE_TOPIC,
> +                                         offlineString.get());

if something reenters with aConnectivity = true or ends up calling SetOffline, you will break the notification sequence.

I'm afraid you need the same while (mSetConnectivity != mConnectivity) dance as in SetOffline.  If you believe not please put a very good comment why.

Question also is, should we prevent reentered setOffline from posting the notifications?  I think yes.  Hence you need to play with mSettingOffline as well.

This sounds to me like we need a single well controlled place where these notifications are triggered and kept fully consistent when "offline" and "connectivity" attributes change.

Have you considered making 'connectivity' attribute just read only on nsIIOService?  I kinda don't understand why it should be allowed control from outside.  It could save some trouble here.

@@ +1248,5 @@
>      } else if (!strcmp(topic, kProfileChangeNetRestoreTopic)) {
>          if (mOfflineForProfileChange) {
>              mOfflineForProfileChange = false;
> +            SetOffline(false);
> +            OnNetworkLinkEvent(NS_NETWORK_LINK_DATA_UNKNOWN);

note that if you don't have any net link service (may happen!) you don't send any notifications about the "connectivity" state.

but that's probably wanted.

I'm not sure swapping the order of calls to SetOffline and OnNet.. is a good idea.  If you have a reason, please state in a comment.

@@ +1429,2 @@
>      InitializeNetworkLinkService();
> +    OnNetworkLinkEvent(NS_NETWORK_LINK_DATA_UNKNOWN);

it's called from InitializeNetworkLinkService() already

@@ +1473,1 @@
>  #endif

shouldn't this be:

  bool isUp = true;
#if WIN
  isUp = nsNative....Enabled();
#endif
  return SetConnectivity(isUp);

?

@@ +1481,5 @@
>      } else if (!strcmp(data, NS_NETWORK_LINK_DATA_UP)) {
>          isUp = true;
>      } else if (!strcmp(data, NS_NETWORK_LINK_DATA_CHANGED) ||
>                 !strcmp(data, NS_NETWORK_LINK_DATA_UNKNOWN)) {
>          nsresult rv = mNetworkLinkService->GetIsLinkUp(&isUp);

just FYI, not an actual problem:

on win

[Parent 10080] WARNING: CheckLinkStatus called on main thread! No check performed. Assuming link is up, status is unknown.: file c:/Mozilla/src/mozilla-central/netwerk/system/win32/nsNotifyAddrListener.cpp, line 503

This happened to me when I switched the pref to do manage offline status.

It posts "unknown" event, so we end up here again shortly.  I think it keeps posting until the state is know, should be checked if don't loop too much here.

::: netwerk/base/nsIOService.h
@@ +125,5 @@
>                                                       nsIChannel** result);
>  private:
>      bool                                 mOffline;
>      bool                                 mOfflineForProfileChange;
>      bool                                 mManageOfflineStatus;

this member should probably be renamed to mManageLinkStatus.
Attachment #8567748 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #12)

> @@ +1481,5 @@
> >      } else if (!strcmp(data, NS_NETWORK_LINK_DATA_UP)) {
> >          isUp = true;
> >      } else if (!strcmp(data, NS_NETWORK_LINK_DATA_CHANGED) ||
> >                 !strcmp(data, NS_NETWORK_LINK_DATA_UNKNOWN)) {
> >          nsresult rv = mNetworkLinkService->GetIsLinkUp(&isUp);
> 
> just FYI, not an actual problem:

I realize this is not really part of this change but comes from the fix for Bug 1132693, but...

I cannot grasp why we would call GetIsLinkUp() if the event says changed. "Changed" is a signal for exactly that: a change, without implying a change in DOWN/UP. The link service sends individual UP/DOWN events when it detects those so thus this is a totally superfluous call (that is quite expensive at times) then.

The "unknown" part there is basically needed only when nsIOService itself wants to "fake" an event and thus force a readout from the link service. The only time the link service actually sends an unknown is probably as a response to this GetIsLinkUp() when asked to check the status on the main thread and the link status is not yet figured out.

Maybe we could make nsIOService use unknown for the link status until told UP/DOWN (if there's even a point in that instead of just assuming it is up until we figure out something else), and we should never send unknown from the link service since that's sort of unnecessary.
(In reply to Daniel Stenberg [:bagder] from comment #13)
> (In reply to Honza Bambas (:mayhemer) from comment #12)
> 
> > @@ +1481,5 @@
> > >      } else if (!strcmp(data, NS_NETWORK_LINK_DATA_UP)) {
> > >          isUp = true;
> > >      } else if (!strcmp(data, NS_NETWORK_LINK_DATA_CHANGED) ||
> > >                 !strcmp(data, NS_NETWORK_LINK_DATA_UNKNOWN)) {
> > >          nsresult rv = mNetworkLinkService->GetIsLinkUp(&isUp);
> > 
> > just FYI, not an actual problem:
> 
> I realize this is not really part of this change but comes from the fix for
> Bug 1132693, but...
> 
> I cannot grasp why we would call GetIsLinkUp() if the event says changed.

First time it's called artificially with UNKNOWN.  nsNotifyAddrListener::CheckLinkStatus (called from GetIsLinkUp()) posts this again with again UNKNOWN, since the state is really unknown.

> "Changed" is a signal for exactly that: a change, without implying a change
> in DOWN/UP. The link service sends individual UP/DOWN events when it detects
> those so thus this is a totally superfluous call (that is quite expensive at
> times) then.
> 
> The "unknown" part there is basically needed only when nsIOService itself
> wants to "fake" an event and thus force a readout from the link service. The
> only time the link service actually sends an unknown is probably as a
> response to this GetIsLinkUp() when asked to check the status on the main
> thread and the link status is not yet figured out.

Yep!

> 
> Maybe we could make nsIOService use unknown for the link status until told
> UP/DOWN (if there's even a point in that instead of just assuming it is up
> until we figure out something else), and we should never send unknown from
> the link service since that's sort of unnecessary.

I think this is what the link service implementations do for us already.
(In reply to Honza Bambas (:mayhemer) from comment #14)

> > I cannot grasp why we would call GetIsLinkUp() if the event says changed.
> 
> First time it's called artificially with UNKNOWN. 
> nsNotifyAddrListener::CheckLinkStatus (called from GetIsLinkUp()) posts this
> again with again UNKNOWN, since the state is really unknown.

Yes, for the UKNOWN state. But for CHANGED? I'll file a separate bug about it.

> > Maybe we could make nsIOService use unknown for the link status until told
> > UP/DOWN (if there's even a point in that instead of just assuming it is up
> > until we figure out something else), and we should never send unknown from
> > the link service since that's sort of unnecessary.
> 
> I think this is what the link service implementations do for us already.

Not exactly.

I'm suggesting removing the sending of UNKNOWN from the link service since it adds nothing useful. But I'm digressing, that's not about this particular patch. I'll make that a separate bug/patch later
(In reply to Honza Bambas (:mayhemer) from comment #12)
> Comment on attachment 8567748 [details] [diff] [review]
> Separate nsIOService's network detection from offlineMode
> 
> Review of attachment 8567748 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As I understand (just to confirm) this patch:
> 
> - leaves the mIsOffline flag and related notifications entirely up to the
> user (the File/Work Offline switch on desktop)
> - introduces a new state "has connectivity" that:
>   - is controlled by the link service (and no longer affects online status),
> when management is enabled and we have a link service 
>   - otherwise believe we are always connected
> - there is one central point NS_IsOffline() - already widely used - that
> combines these two

Correct.

> 
> While the existing "offline" state on io remains intact on link state
> changes, we still send the existing ONLINE/OFFLINE notifications when the
> link state changes.  But they are no longer consistent with state of the
> "offline" indicator on io service.  It's now only consistent with
> NS_IsOffline() C call.  In the overall result, what is the difference from
> what we have now?

I think this is ok, from the point of view of the callsites of NS_IsOffline: https://dxr.mozilla.org/mozilla-central/search?q=NS_IsOffline&case=false
It is only used to report the value of navigator.onLine

You are correct that the notifications no longer reflect the state of the IOService.
But I think that most of the places listening for it are more interested in connectivity than the state of the IOService.
There should be no functional difference between what this patch does, and the old behaviour (with the manage-offline-status pref set to true).

> @@ +288,3 @@
> >  
> > +    OnNetworkLinkEvent(NS_NETWORK_LINK_DATA_UNKNOWN);
> > +    SetOffline(false);
> 
> if this (call to SetOffline(false)) is still necessary, we should move it
> somewhere else.  it's dangerous since this method may be called more then
> once while instantiating the link service might fail last time and the
> mNetworkLinkServiceInitialized flag is left false.
> 

The comments in SetOffline say it can be called multiple times - although that code seems a bit racy :)
In any case, this needs to be called because the IOService is in offline mode at startup. I added comments to say so.

> @@ +940,5 @@
> >                  mProxyService->ReloadPAC();
> >  
> >              // don't care if notification fails
> > +            // by checking NS_IsOffline we make sure we only send the
> > +            // notification if we have connectivity.
> 
> ok, and when we are currently "NS_IsOffline()", when do you send this
> notification when we actually become (in this case) "connected" or generally
> "!NS_IsOffline()" again?
> 

In this case it's the same place. When mOffline becomes false, we only send the ONLINE notification if mConnectivity is true. Else, the connectivity change will send the notification, and at the same time NS_IsOffline will become true.
I changed the code to query the mConnectivity member directly, to avoid the overhead and to make it a bit clearer.

> 
> @@ +1012,5 @@
> > +        // send the OFFLINE notification
> > +        const nsLiteralString offlineString(MOZ_UTF16(NS_IOSERVICE_OFFLINE));
> > +        observerService->NotifyObservers(static_cast<nsIIOService *>(this),
> > +                                         NS_IOSERVICE_GOING_OFFLINE_TOPIC,
> > +                                         offlineString.get());
> 
> if something reenters with aConnectivity = true or ends up calling
> SetOffline, you will break the notification sequence.
> 
> I'm afraid you need the same while (mSetConnectivity != mConnectivity) dance
> as in SetOffline.  If you believe not please put a very good comment why.
> 
> Question also is, should we prevent reentered setOffline from posting the
> notifications?  I think yes.  Hence you need to play with mSettingOffline as
> well.
> 
> This sounds to me like we need a single well controlled place where these
> notifications are triggered and kept fully consistent when "offline" and
> "connectivity" attributes change.
> 
> Have you considered making 'connectivity' attribute just read only on
> nsIIOService?  I kinda don't understand why it should be allowed control
> from outside.  It could save some trouble here.
> 

The only external call to SetConnectivity should be from ContentChild::RecvSetConnectivity, so there would be no reentrancy. I've now enforced this by making SetConnectivity private, and ContentChild QIs to a new interface called nsIIOServiceInternal, that only contains SetConnectivityInternal. Does this sound OK?

> @@ +1248,5 @@
> >      } else if (!strcmp(topic, kProfileChangeNetRestoreTopic)) {
> >          if (mOfflineForProfileChange) {
> >              mOfflineForProfileChange = false;
> > +            SetOffline(false);
> > +            OnNetworkLinkEvent(NS_NETWORK_LINK_DATA_UNKNOWN);
> 
> note that if you don't have any net link service (may happen!) you don't
> send any notifications about the "connectivity" state.
> 

If there is no net link service, mConnectivity should always be true, meaning all of the notifications would be sent by SetOffline.

> 
> @@ +1429,2 @@
> >      InitializeNetworkLinkService();
> > +    OnNetworkLinkEvent(NS_NETWORK_LINK_DATA_UNKNOWN);
> 
> it's called from InitializeNetworkLinkService() already

If the NetworkLinkService is already initialized, it does not call OnNetworkLinkEvent. This is needed, when mManageOfflineStatus goes from false to true. I added comments to say so.

> 
> @@ +1473,1 @@
> >  #endif
> 
> shouldn't this be:
> 
>   bool isUp = true;
> #if WIN
>   isUp = nsNative....Enabled();
> #endif
>   return SetConnectivity(isUp);
> 
> ?

Yup. Much better. Except it's false by default, if we want the initial behaviour.

> ::: netwerk/base/nsIOService.h
> @@ +125,5 @@
> >                                                       nsIChannel** result);
> >  private:
> >      bool                                 mOffline;
> >      bool                                 mOfflineForProfileChange;
> >      bool                                 mManageOfflineStatus;
> 
> this member should probably be renamed to mManageLinkStatus.

Should we also rename Get/SetManageOfflineStatus?
If yes, I suggest doing this in a later bug. There are some JS components that seem to use it, and I'd like to make sure we don't break any :)
I've also removed calls to NS_IsOffline to make the code clearer. Now it only uses the values of mOffline and mConnectivity
Attachment #8574251 - Flags: review?(honzab.moz)
Attachment #8567748 - Attachment is obsolete: true
Comment on attachment 8574251 [details] [diff] [review]
Separate nsIOService's network detection from offlineMode

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

::: netwerk/base/nsIOService.cpp
@@ +1259,5 @@
>      } else if (!strcmp(topic, kProfileChangeNetRestoreTopic)) {
>          if (mOfflineForProfileChange) {
>              mOfflineForProfileChange = false;
> +            SetOffline(false);
> +            OnNetworkLinkEvent(NS_NETWORK_LINK_DATA_UNKNOWN);

I'm still curious what the motivation is for this call here. There's no change in connectivity on this event, there should be no reason to fake it either?

@@ +1491,5 @@
>      if (!strcmp(data, NS_NETWORK_LINK_DATA_DOWN)) {
>          isUp = false;
>      } else if (!strcmp(data, NS_NETWORK_LINK_DATA_UP)) {
>          isUp = true;
>      } else if (!strcmp(data, NS_NETWORK_LINK_DATA_CHANGED) ||

I've addressed this in another bug, but "CHANGED" is not a reason for changed connectivity and there should be no reason to check it (again) because of this. A need to do that rather indicates a problem in the link service.
(In reply to Daniel Stenberg [:bagder] from comment #18)
> Comment on attachment 8574251 [details] [diff] [review]
> Separate nsIOService's network detection from offlineMode
> 
> Review of attachment 8574251 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsIOService.cpp
> @@ +1259,5 @@
> >      } else if (!strcmp(topic, kProfileChangeNetRestoreTopic)) {
> >          if (mOfflineForProfileChange) {
> >              mOfflineForProfileChange = false;
> > +            SetOffline(false);
> > +            OnNetworkLinkEvent(NS_NETWORK_LINK_DATA_UNKNOWN);
> 
> I'm still curious what the motivation is for this call here. There's no
> change in connectivity on this event, there should be no reason to fake it
> either?
> 

You are correct. I just forgot to remove it in the last review. :)

> @@ +1491,5 @@
> >      if (!strcmp(data, NS_NETWORK_LINK_DATA_DOWN)) {
> >          isUp = false;
> >      } else if (!strcmp(data, NS_NETWORK_LINK_DATA_UP)) {
> >          isUp = true;
> >      } else if (!strcmp(data, NS_NETWORK_LINK_DATA_CHANGED) ||
> 
> I've addressed this in another bug, but "CHANGED" is not a reason for
> changed connectivity and there should be no reason to check it (again)
> because of this. A need to do that rather indicates a problem in the link
> service.

Right.
Blocks: 756364
(In reply to Valentin Gosu [:valentin] from comment #16)
> There should be no functional difference between what this patch does, and
> the old behaviour (with the manage-offline-status pref set to true).

Yeah, and that might the problem.  ***

> The comments in SetOffline say it can be called multiple times - although
> that code seems a bit racy :)
> In any case, this needs to be called because the IOService is in offline
> mode at startup. I added comments to say so.

My point was to move it somewhere else (to the place where from InitNetworkLinkService is called or so).  This should no longer be hidden inside that method.

> > 
> > @@ +1012,5 @@
> > > +        // send the OFFLINE notification
> > > +        const nsLiteralString offlineString(MOZ_UTF16(NS_IOSERVICE_OFFLINE));
> > > +        observerService->NotifyObservers(static_cast<nsIIOService *>(this),
> > > +                                         NS_IOSERVICE_GOING_OFFLINE_TOPIC,
> > > +                                         offlineString.get());
> > 
> > if something reenters with aConnectivity = true or ends up calling
> > SetOffline, you will break the notification sequence.
> > 
> > I'm afraid you need the same while (mSetConnectivity != mConnectivity) dance
> > as in SetOffline.  If you believe not please put a very good comment why.
> > 
> > Question also is, should we prevent reentered setOffline from posting the
> > notifications?  I think yes.  Hence you need to play with mSettingOffline as
> > well.
> > 
> > This sounds to me like we need a single well controlled place where these
> > notifications are triggered and kept fully consistent when "offline" and
> > "connectivity" attributes change.
> > 
> > Have you considered making 'connectivity' attribute just read only on
> > nsIIOService?  I kinda don't understand why it should be allowed control
> > from outside.  It could save some trouble here.
> > 
> 
> The only external call to SetConnectivity should be from
> ContentChild::RecvSetConnectivity, so there would be no reentrancy. I've now
> enforced this by making SetConnectivity private, and ContentChild QIs to a
> new interface called nsIIOServiceInternal, that only contains
> SetConnectivityInternal. Does this sound OK?

Yep, that sounds better.  Can ContentChild refer the IOService directly somehow?  

> > 
> > @@ +1473,1 @@
> > >  #endif
> > 
> > shouldn't this be:
> > 
> >   bool isUp = true;
> > #if WIN
> >   isUp = nsNative....Enabled();
> > #endif
> >   return SetConnectivity(isUp);
> > 
> > ?
> 
> Yup. Much better. Except it's false by default, if we want the initial
> behaviour.

Are you sure?

> 
> > ::: netwerk/base/nsIOService.h
> > @@ +125,5 @@
> > >                                                       nsIChannel** result);
> > >  private:
> > >      bool                                 mOffline;
> > >      bool                                 mOfflineForProfileChange;
> > >      bool                                 mManageOfflineStatus;
> > 
> > this member should probably be renamed to mManageLinkStatus.
> 
> Should we also rename Get/SetManageOfflineStatus?
> If yes, I suggest doing this in a later bug. There are some JS components
> that seem to use it, and I'd like to make sure we don't break any :)

OK.
(In reply to Honza Bambas (:mayhemer) from comment #20)
> (In reply to Valentin Gosu [:valentin] from comment #16)
> > There should be no functional difference between what this patch does, and
> > the old behaviour (with the manage-offline-status pref set to true).
> 
> Yeah, and that might the problem.  ***


Don't worry about this one.. I have to look around first.  Anyway, most of the code checks ioservice.offline, which doesn't change with this patch.
(In reply to Honza Bambas (:mayhemer) from comment #20)
> (In reply to Valentin Gosu [:valentin] from comment #16)
> > There should be no functional difference between what this patch does, and
> > the old behaviour (with the manage-offline-status pref set to true).
> 
> Yeah, and that might the problem.  ***

But the exact "no functional difference" we should maintain is of course interesting.

This task is not primarily about fixing a bug (directly anyway), but to split up two different things internally and that is bound to introduce some new behaviors. So we need to agree on what this new behavior should be.

For example, I think the link service/connectivity should be able to say offline (with the pref set to true) but without that really affecting anything (like preventing attempted network activity).

So to really prevent network activity, the "work offline" mode would have to be set.

This means slightly different than how Firefox works today, as setting the prefs to true today makes Firefox basically set "work offline" mode by itself when it thinks we're offline and that makes false positives very disturbing to users.

Of course, we can play it more conservative and add a new prefs for the new behavior and still maintain the old one.
Attachment #8574251 - Attachment is obsolete: true
Attachment #8574251 - Flags: review?(honzab.moz)
This patch adds another pref to work as a compatibility layer.
when network.offline-mirrors-connectivity is true, ioservice.offline would work just as it did until now
Attachment #8576504 - Flags: review?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #20)
> My point was to move it somewhere else (to the place where from
> InitNetworkLinkService is called or so).  This should no longer be hidden
> inside that method.
> 

I moved it out, just after mNetworkLinkServiceInitialized = true;
Any calls to InitNetworkLinkService that come later would return early anyway.

> > 
> > The only external call to SetConnectivity should be from
> > ContentChild::RecvSetConnectivity, so there would be no reentrancy. I've now
> > enforced this by making SetConnectivity private, and ContentChild QIs to a
> > new interface called nsIIOServiceInternal, that only contains
> > SetConnectivityInternal. Does this sound OK?
> 
> Yep, that sounds better.  Can ContentChild refer the IOService directly
> somehow?  
> 

We QI to nsIIOServiceInternal. It seems best to avoid including unneeded headers.

> > Yup. Much better. Except it's false by default, if we want the initial
> > behaviour.
> 
> Are you sure?
> 

Nope. You're completely right. My bad.


(In reply to Daniel Stenberg [:bagder] from comment #22)
> Of course, we can play it more conservative and add a new prefs for the new
> behavior and still maintain the old one.

I did so with the last patch.
Comment on attachment 8576503 [details] [diff] [review]
Separate nsIOService's network detection from offlineMode

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

::: netwerk/base/nsIOService.cpp
@@ +979,5 @@
> +nsIOService::SetConnectivityInternal(bool aConnectivity)
> +{
> +    // This should only be called from ContentChild to pass the connectivity
> +    // value from the chrome process to the content process.
> +    if (XRE_GetProcessType() != GeckoProcessType_Content) {

I think the condition should more be == GPT_Default, since plugin containing processes could benefit from this as well.

@@ +987,5 @@
> +}
> +
> +
> +nsresult
> +nsIOService::SetConnectivity(bool aConnectivity)

hmm.. nit: the nsresult SetConnectivity should actually have the Internal suffix.  And the public interface method SetConnectivityInternal shouldn't.  It's already in an internal interface, so no need to Internalize it again :)

@@ +1025,5 @@
> +                                         offlineString.get());
> +        observerService->NotifyObservers(static_cast<nsIIOService *>(this),
> +                                         NS_IOSERVICE_OFFLINE_STATUS_TOPIC,
> +                                         offlineString.get());
> +    }

maybe just put:

if (mOffline) {
    return NS_OK;
}

before the block?

Then you can just 

if (aConnectivity) {
...
} else {
...
}


Anyway, I'm curious how this will be broken when someone calls SetOffline from inside of those observer notifications...

@@ +1448,4 @@
>  }
>  
>  NS_IMETHODIMP
>  nsIOService::GetManageOfflineStatus(bool* aManage) {

nit: when you are here, please put the { on a new line please.
Attachment #8576503 - Flags: review?(honzab.moz) → review+
Comment on attachment 8576504 [details] [diff] [review]
imported patch -nsioservice-connectivity-shim.patch

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

We should also think how this is going to affect thunderbird.  I think they want no manage and no mirror.

::: modules/libpref/init/all.js
@@ +1165,5 @@
>  
> +// Whether IOService.connectivity and NS_IsOffline depends on connectivity status
> +pref("network.manage-offline-status", false);
> +// If set to true, IOService.offline depends on IOService.connectivity
> +pref("network.offline-mirrors-connectivity", true);

hmm.. I would expect these two exactly opposite: do manage (the pref name is wrong IMO...) and don't let IO.offline depend on it...

that would bring the new feature and preserve the old behavior for the "offline" member state.

or is the plan different?

::: netwerk/base/nsIOService.cpp
@@ +227,5 @@
>      else
>          NS_WARNING("failed to get observer service");
>  
>      Preferences::AddBoolVarCache(&sTelemetryEnabled, "toolkit.telemetry.enabled", false);
> +    Preferences::AddBoolVarCache(&mOfflineMirrorsConnectivity, OFFLINE_MIRRORS_CONNECTIVITY, true);

initialize the member also in ctor(s) !!

::: netwerk/base/nsIOService.h
@@ +130,5 @@
>      bool                                 mOffline;
>      bool                                 mOfflineForProfileChange;
>      bool                                 mManageLinkStatus;
>      bool                                 mConnectivity;
> +    bool                                 mOfflineMirrorsConnectivity;

weird name deserves a good comment :)
(In reply to Honza Bambas (:mayhemer) from comment #29)
> Comment on attachment 8576504 [details] [diff] [review]
> imported patch -nsioservice-connectivity-shim.patch
> 
> Review of attachment 8576504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We should also think how this is going to affect thunderbird.  I think they
> want no manage and no mirror.

manage=false and mirror=true should lead to the exact same behaviour as before. (Default in all.js) Even changing the manage flag to true, would work the same. I'm thinking that once we have the feature turned on in all products we would change the defaults as well.

It seems thunderbird just calls SetManageOfflineStatus at runtime.
http://mxr.mozilla.org/comm-central/source/mail/base/content/mail-offline.js#51

> ::: modules/libpref/init/all.js
> @@ +1165,5 @@
> >  
> > +// Whether IOService.connectivity and NS_IsOffline depends on connectivity status
> > +pref("network.manage-offline-status", false);
> > +// If set to true, IOService.offline depends on IOService.connectivity
> > +pref("network.offline-mirrors-connectivity", true);
> 
> hmm.. I would expect these two exactly opposite: do manage (the pref name is
> wrong IMO...) and don't let IO.offline depend on it...

I agree. manage-offline-status is a very unfortunate name for a pref. I think it was meant to mean "if I want Firefox to manage the status for me"

Do you think offline-mirrors-connectivity is also ambiguous?
Attachment #8576503 - Attachment is obsolete: true
Attachment #8576504 - Attachment is obsolete: true
Attachment #8576504 - Flags: review?(honzab.moz)
(In reply to Valentin Gosu [:valentin] from comment #30)
> Do you think offline-mirrors-connectivity is also ambiguous?

I think it's OK.
Attachment #8583762 - Flags: review?(honzab.moz) → review+
Ryan, the failures in comment 34 look really suspicious. I am able to build the patches locally on Windows and Linux.
Do you think this is a bug in the automation framework?
Flags: needinfo?(ryanvm)
Yup, look like infra timeouts to me.
Flags: needinfo?(ryanvm)
Turns out the timeouts were caused by a bug in the patches.
Before when calling InitializeNetworkLinkService() in Init(), it would either call SetOffline(false) if mManageOffline was false, or SetOffline(false) would have been called in OnNetworkLinkEvent.

The fix was simple. And the new try build is pretty much green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a163357ee1e
Comment on attachment 8587948 [details] [diff] [review]
SetOffline(false) in nsIOService::Init

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

::: netwerk/base/nsIOService.cpp
@@ +233,5 @@
>  
>      gIOService = this;
>  
>      InitializeNetworkLinkService();
> +    SetOffline(false);

this is what I wanted from the beginning.
Attachment #8587948 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #39)
> Comment on attachment 8587948 [details] [diff] [review]
> SetOffline(false) in nsIOService::Init
> 
> Review of attachment 8587948 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsIOService.cpp
> @@ +233,5 @@
> >  
> >      gIOService = this;
> >  
> >      InitializeNetworkLinkService();
> > +    SetOffline(false);
> 
> this is what I wanted from the beginning.

Also, the call of SetOffline(false) from inside InitializeNetworkLinkService(); should go away.
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8644929&repo=mozilla-inbound which became permanent
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(valentin.gosu)
Depends on: 1179779
You need to log in before you can comment on or make changes to this bug.