Closed Bug 312793 Opened 19 years ago Closed 18 years ago

Support DBUS and listen to NetworkManager events

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

(Keywords: fixed1.8.1)

Attachments

(4 files, 4 obsolete files)

Updated patch
62.05 KB, patch
darin.moz
: review+
shaver
: review+
mconnor
: review+
caillon
: review+
darin.moz
: superreview+
shaver
: approval-branch-1.8.1+
Details | Diff | Splinter Review
This landed on the 1.8 branch
32.55 KB, patch
Details | Diff | Splinter Review
updated patch to trunk
30.11 KB, patch
benjamin
: review-
Details | Diff | Splinter Review
forceful configure.in
1.75 KB, patch
benjamin
: review+
Details | Diff | Splinter Review 
The new Linux NetworkManager tool can inform applications via DBUS when network
connectivity changes. We can use this to automatically go online or offline.

I have created a small DBUS connection component that listens for network status
messages and does the right thing. It's always dynamic so if installed on a
system without DBUS, it will just fail to load and nothing bad will happen. I'd
like to have this in our tree. We could add support for other kinds of daemons
and events.
Attached patch patch (obsolete) — — Splinter Review 
As described
Attachment #199896 - Flags: superreview?(darin)
Attachment #199896 - Flags: review?(caillon)
Comment on attachment 199896 [details] [diff] [review]
patch

This can be handled without pulling in D-Bus by using libnm.  See the test app
http://cvs.gnome.org/viewcvs/NetworkManager/test/libnm_glib_test.c?rev=1.5&view
=markup
Although note that there has been talk about renaming the types to fit more with
other GLib bindings InterCaps as opposed to underscore_delimited.  The physical
layout of the structures and the method names should be stable, which makes this
not an issue AFAIK for us as long as we dynamically load libnm.

So, why not put this directly in necko?  Bug 76111 has a patch that does
something like this for Windows.  Also, how is this supposed to interact with
the user's choice to set the browser offline?
The advantage of having a dedicated DBUS connector component is that when DBUS
libraries are not available, the component fails to load and the app carries on.

Also, with this component we could listen for other events on the same DBUS
connection. I don't know if libnm is implemented in a way that permits that.

Also^2, this is implemented in a way that would also be usable with Qt with very
minor modifications.

The issue of user control vs automated switching we might want to address in a
higher bandwidth forum...
Events from other daemons, I mean.
I think my point is that libnm is less likely to change than the dbus signal
names emitted by NM.
They're not frozen API?
I know that right now the values are not frozen API, but are you saying they're
not going to be frozen?
Attached patch patch #2 (obsolete) — — Splinter Review 
this one fixes the dynamic creation of the shared lib and added some needed
additional changes as entries in autoconf.mk and packages-static
Is this a Gecko 1.9 / Firefox 2.0 thing?
Yeah, although Novell might ship something like this as a patch to FF1.5.

Taking the lack of feedback to my email about online/offline philosophy as an
invitation to forge ahead :-), I'm going to rework this patch to extend
nsIIOService (via unfrozen nsIIOService2) with new methods for setting/getting
system connectivity status, with an associated observer event for changes, and
make nsIIOService::SetOffline(PR_FALSE) only set the user preference; it will
not change the status to online if the system is disconnected. And I'll patch
the Firefox UI to disable toggling of "Work Offline" while the system is
disconnected.
Another choice would be to have your extension dispatch an event that can be
observed via nsIObserverService.  Then, make nsIOService listen to that event.

If you want to get this in for FF 1.5, then it would be best to involve the
folks driving this release (aka deerpark@mozilla.org).
I don't intend to submit anything to Mozilla's 1.8/FF1.5 branch.

> Another choice would be to have your extension dispatch an event that can be
> observed via nsIObserverService.  Then, make nsIOService listen to that event.

There still needs to be a new API somewhere so people can discover the current
system connectivity status.
Perhaps the solution is a change at the UI level.  Let nsIIOService::offline be a boolean to control necko's offline state as it is today, but introduce some high-level setting that knows how to properly mix the user requested state with the actual OS state.
You mean like, have a system network status component, and then have UI-level code monitor that status, mix the status with their UI setting, and control changes to Necko offline/online status? I suppose that's an option, but it means more per-application work ... it would be nice if xulrunner apps had some reasonable automatic online/offline switching by default.
(In reply to comment #17)
> You mean like, have a system network status component, and then have UI-level
> code monitor that status, mix the status with their UI setting, and control
> changes to Necko offline/online status? I suppose that's an option, but it
> means more per-application work ... it would be nice if xulrunner apps had some
> reasonable automatic online/offline switching by default.

So, implement a default mixer that monitors a preference in addition to the network status.  If that preference is undefined, then assume that the UI is not interested in trumping the online/offline state.
OK, I can do something like that.
Attachment #199896 - Flags: superreview?(darin) → superreview-
See also http://www.whatwg.org/specs/web-apps/current-work/#scs-browser ... this should just track nsIIOService::offline, I guess.
> See also http://www.whatwg.org/specs/web-apps/current-work/#scs-browser ...
> this should just track nsIIOService::offline, I guess.

navigator.onLine is currently hooked up to the IO Service.
Attached patch new patch (obsolete) — — Splinter Review 
This patch takes Darin's approach, basically.

It introduces a new service interface nsINetworkLinkService. This can be used to query the current link status. It notifies observers when the link status changes. By default, it also updates nsIIOService::offline to match the link status, but this behaviour can be disabled by apps that don't want it, via nsINetworkLinkService::manageIOServiceOfflineStatus. Those apps can then implement whatever policy they want.

I've reworked the code to talk to DBus/NetworkManager to implement this interface. It separates the DBus and NetworkManager-specific parts better. This version of the code handles DBus disconnect and reconnect, so you can stop and start dbus (e.g., during a package upgrade) without Firefox crashing or even having to restart.

The Firefox "Work Offline" UI works like this: when a nsINetworkLinkService component is not available, we behave exactly as we always did. When an nsINetworkLinkService is available, we ignore the browser.offline preference. The nsINetworkLinkService is allowed to manage nsIIOService::offline until the user toggles the status using "Work Offline"; then we turn management off and ignore link changes for the rest of the lifetime of this Firefox process. Seamonkey works similarly.

I make Thunderbird and Seamonkey mailnews disable management on startup and just ignore link status changes so there's no change for them. They need more work to integrate link status with their online/offline processing.

In a followup bug I want to change Firefox's "offline" error page message to take into account whether there is a network link or not, when that information is available. it doesn't help to say "Uncheck 'Work Offline' in the File menu, then try again." if there is no network link. I also plan to implement the DOM-event-firing part of the above-mentioned WHATWG spec.

I would like to get review of the dbus/networkmanager related code from caillon, and the rest from darin. Maybe I should get some sort of ui review too...
Attachment #199896 - Attachment is obsolete: true
Attachment #200058 - Attachment is obsolete: true
Attachment #213805 - Flags: superreview?(darin)
Attachment #213805 - Flags: review?(caillon)
Attachment #199896 - Flags: review?(caillon)
Comment on attachment 213805 [details] [diff] [review]
new patch

mconnor, would you take a look at the slight UI change here?
Attachment #213805 - Flags: review?(mconnor)
Attached patch updated patch (obsolete) — — Splinter Review 
This enhances the previous patch slightly by adding "linkStatusKnown" to nsINetworkLinkService and firing "unknown" notifications if we lose track of the network link status. nsNetworkManagerListener reports "unknown" while dbus is down, and also if it is unable to contact NetworkManager. This ensures that Firefox will revert to existing behaviour if dbus is running but NetworkManager is not running.
Attachment #213805 - Attachment is obsolete: true
Attachment #213829 - Flags: superreview?(darin)
Attachment #213829 - Flags: review?(caillon)
Attachment #213805 - Flags: superreview?(darin)
Attachment #213805 - Flags: review?(mconnor)
Attachment #213805 - Flags: review?(caillon)
Hey Roc, so how does nsINetworkLinkStatus work with the windows auto-dialer code?  We initiate the autodial when the socket transport fails to establish a connection or fails to resolve a hostname.  But, if the IO service is offline, then we will never even attempt to autodial.  Hmm...
> then we will never even attempt to autodial.
                                 ... use the socket transport service.


Right now there is no Windows implementation of nsINetworkLinkStatus so the dialer isn't affected :-)

I guess your question is what the user experience should be and how it should be implemented in the future assuming someone gets around to implementing network link sensing for Windows. One simple approach would be: if autodialing is configured, then just pretend the network link is always up. That will basically get you back to the current behaviour for those users.

Another option would be to take the browser offline when the user is not dialed up, but provide an extra button in the network error page: "Dial". This would require an additional interface+component to control dialing.

Take your pick.
> Right now there is no Windows implementation of nsINetworkLinkStatus so the
> dialer isn't affected :-)

Of course, but when designing a cross-platform API, we need to consider how it might be implemented on the important platforms at least.


> I guess your question is what the user experience should be and how it should
> be implemented in the future assuming someone gets around to implementing
> network link sensing for Windows. One simple approach would be: if autodialing
> is configured, then just pretend the network link is always up. That will
> basically get you back to the current behaviour for those users.
> 
> Another option would be to take the browser offline when the user is not dialed
> up, but provide an extra button in the network error page: "Dial". This would
> require an additional interface+component to control dialing.

The first option sounds better.  Right now, launching Firefox results in the browser automatically dialing.  If windows is configured properly this all happens without the user having to do anything more than double click the Firefox icon.

I'm not too happy about having to change the autodial code to cope with this new API.  It'd be nice if there were some way for it to work without having to rework the windows autodialer code.
(In reply to comment #28)
> I'm not too happy about having to change the autodial code to cope with this
> new API.

You don't. That's up to whoever implements nsINetworkLinkStatus on Windows. They'll have to detect that autodialing is configured and pretend that the link is up in that case. Is that hard? Aren't there just some prefs or registry entries to check?
Even if someone else contributes a patch, I'll be on the hook to review it and to ensure that it works as well as before if not better.  That means I need to have a modem setup to test it out, and so on.  Not happy about that! :-)
I don't see how some dialer-specific work can be avoided if you want Firefox to sense the link status on Windows and go into offline mode automatically for non-dialer users.
> I don't see how some dialer-specific work can be avoided if you want Firefox to
> sense the link status on Windows and go into offline mode automatically for
> non-dialer users.

My concern is that we might prefer to check the link status on network failure in addition to doing so asynchronously.  Right now, when DNS failure occurs, we call res_ninit on linux to pick up new /etc/resolv.conf settings.  Network failure at the socket level seems like an important exception point worth handling cleanly.  So, IMO we might want to do something at the socket transport layer to propogate errors out to listeners that can act on them.

Related to this is the problem of needing to reload PAC files when users switch networks.  This is best handled IMO by trapping network connection errors, reloading PAC, and retrying the connection (or something like that).  Autodial is very similar.

So, I'm concerned about how all these things fit together.  I think we might want a different link status API, but I'm not sure.  Let me think about this for a little bit.
My intent was to just harvest information the OS already has ... which is basically whether a network interface is configured or not.

I think we can treat this as somewhat orthogonal to the propagation and handling of network failures. Those can occur for a variety of reasons, including transient burps, and I think should be treated as a different kind of information. For example, we probably don't want to change gross application state such as offline mode just because a connection timed out. On the other hand, a harmless refresh of resolv.conf, or PAC reloading, seems like a fine thing to do there.

We probably *should* refresh DNS and PAC (and whatever else you do on network failures) whenever the link status changes to "up", even if you haven't had a network failure yet. That way an idle browser will be ready to go at all times.

So perhaps the right setup here is to define a new observer service event "network:refresh" (or whatever) which fires when the link comes up *and* when a network failure is detected. DNS, PAC and other services can respond to this event by doing any sort of harmless (idempotent) action.

If you agree, that could be forked off into a different bug.
Yeah, I agree... we don't have to fix all of those issues here, and we can always change nsINetworkLinkService and/or the observer events later if need be.
Hey roc, the patch in bug 76111 is interesting from a windows perspective.  It doesn't seem to try to muck with the autodialer stuff too much.  I'm going to review the API here first.  Is caillon going to review the dbus stuff?  Or, do you want me to review that as well?
Comment on attachment 213829 [details] [diff] [review]
updated patch

>Index: extensions/dbus/Makefile.in

>+# The Initial Developer of the Original Code is Novell, Inc.

What happened to the rest of the License goop?


>Index: extensions/dbus/nsDBusService.h

>+/* vim:expandtab:shiftwidth=4:tabstop=4:
>+ */

Or this:

/* vim:set expandtab:shiftwidth=4:tabstop=4: */

With "set" vim is magically okay with the trailing "*/"


I skipped over most of the dbus specific code.


>Index: netwerk/base/public/nsIIOService.idl

>+%{C++
>+#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_OFFLINE              "offline"
>+#define NS_IOSERVICE_ONLINE               "online"
>+%}

Please move these below the interface definition, and document them
as much as you can.


>+     * shut down DNS and socket services (although the network link may

Hmm... do we want to reveal this implementation detail in this
frozen API.  It could be something we might wish to change in
the future.  For example, maybe we'd want the DNS and socket
services to remain initialized, but we could just have them
error out with NS_ERROR_OFFLINE.  Maybe it is best to keep this
detail out of the API docs.


>Index: netwerk/base/public/nsINetworkLinkService.idl

>+%{C++
>+#define NS_NETWORK_LINK_TOPIC "network:link-status-changed"
>+#define NS_NETWORK_LINK_DATA_UP      "up"
>+#define NS_NETWORK_LINK_DATA_DOWN    "down"
>+#define NS_NETWORK_LINK_DATA_UNKNOWN "unknown"
>+
>+#define NS_NETWORK_LINK_SERVICE_CONTRACTID \
>+    "@mozilla.org/network/network-link-service;1"
>+%}

Please move the topic strings down below the interface definition,
and move the ContractID into nsNetCID.h with appropriate comments
about what interfaces that ContractID implies.  Please document
the topic strings.


>+interface nsINetworkLinkService : nsISupports
...
>+  /**
>+   * This is set to true when we believe that isLinkUp is accurate. When
>+   * it changes, we notify observers of topic "network:link-status-changed"
>+   * with data either "up", "down" or "unknown".
>+   */
>+  readonly attribute boolean linkStatusKnown;

So, when this field changes to true, we notify with either "up" or "down",
and when this field changes to false, we notify with "unknown", right?
Maybe the comment should spell this out for clarity.


>+   * Note
>+   * that this means during application startup, IOService may be offline
>+   * if there is no link, until application code runs and can turn off
>+   * this management.
>+   */
>+  attribute boolean manageIOServiceOfflineStatus;

Hmm... the startup timing concerns me.  If an extension wanted to
get in during startup to do some operations before the browser
started, like fetch some document off a server, process it, and
do stuff with it, it would have to either know about this API, or
it would have to wedge itself into the startup process right after
the browser initialized itself.

I'm also worried about cases where a PAC file must be loaded before
any HTTP requests can be loaded.  The PAC file is often loaded very
early during startup.  If it gets an immediate failure from necko
that the system is offline, then it will just give up.

Perhaps we should startup not managing the IO service.  Or, if you
like perhaps we should define a preference that applications can
set to control this behavior.  That might be ideal in a xulrunner
world.


>Index: browser/base/content/browser.js

>+      var netLinkService = Components.classes["@mozilla.org/network/network-link-service;1"].
>+        getService(Components.interfaces.nsINetworkLinkService);
>+      haveNetLinkService =
>+        netLinkService != null && netLinkService.linkStatusKnown;
>+    } catch (ex) {
>     }

netLinkService will be non-null if getService did not throw an
exception, so no need to check it.


>+    // if we have a netLinkService, then ioservice.offline is already set
>+    // correctly. We will continue to allow the netLinkService to manage
>+    // ioService.offline for us, until the user uses the "Work Offline" UI.

NOTE: The work offline UI state is something that is persisted.
Does this new logic handle that case correctly given that it
only reads the pref if !haveNetLinkService?


>Index: mail/base/content/commandglue.js
>Index: mailnews/base/resources/content/commandglue.js

You're going to want to get mscott or bienvenu to review these changes.
(In reply to comment #35)
> Hey roc, the patch in bug 76111 is interesting from a windows perspective.
> It doesn't seem to try to muck with the autodialer stuff too much.

Interesting, didn't know about that bug. It seems to just turn online/offline state changes off when the autodialer is configured.

> I'm going to review the API here first.  Is caillon going to review the dbus
> stuff?  Or, do you want me to review that as well?

I'd like caillon to review the DBUS stuff.

The patch in bug 76111 puts automatic online/offline tracking into IOService, controlled by a pref, whereas I have put it in the network link status service implementation, controlled by a IDL attribute. I actually lean towards the former approach since we can then centralize the tracking code and autodialer checking, and applications like Thunderbird can disable automanagement in their prefs. What do you think?

If we do that (or even if we don't) then my interface is very much like the interface in bug 76111 and hopefully that Windows code can be put behind nsINetworkLinkService (although I think that Windows code can be improved a bit).

(In reply to comment #36)
> (From update of attachment 213829 [details] [diff] [review] [edit])
> >Index: extensions/dbus/Makefile.in
> 
> >+# The Initial Developer of the Original Code is Novell, Inc.
> 
> What happened to the rest of the License goop?

"oops"

> >Index: extensions/dbus/nsDBusService.h
> 
> >+/* vim:expandtab:shiftwidth=4:tabstop=4:
> >+ */
> 
> Or this:
> 
> /* vim:set expandtab:shiftwidth=4:tabstop=4: */
> 
> With "set" vim is magically okay with the trailing "*/"

OK :-) I neither know nor care about vim, I just copied the text from elsewhere.

> Please move these below the interface definition, and document them
> as much as you can.

OK

> Maybe it is best to keep this detail out of the API docs.

Agreed.

> Please move the topic strings down below the interface definition,
> and move the ContractID into nsNetCID.h with appropriate comments
> about what interfaces that ContractID implies.  Please document
> the topic strings.

Sure.

> >+  readonly attribute boolean linkStatusKnown;
> 
> So, when this field changes to true, we notify with either "up" or "down",
> and when this field changes to false, we notify with "unknown", right?
> Maybe the comment should spell this out for clarity.

Sure

> Hmm... the startup timing concerns me.  If an extension wanted to
> get in during startup to do some operations before the browser
> started, like fetch some document off a server, process it, and
> do stuff with it, it would have to either know about this API, or
> it would have to wedge itself into the startup process right after
> the browser initialized itself.

During startup, the browser will be offline only if there is no network link and autodialer is not configured. In this situation it seems unlikely to be useful for an extension to request a document from a server.

> I'm also worried about cases where a PAC file must be loaded before
> any HTTP requests can be loaded.  The PAC file is often loaded very
> early during startup.  If it gets an immediate failure from necko
> that the system is offline, then it will just give up.

Okay, but again, isn't that the right thing here, since there is no network connectivity? When the browser goes into online mode the PAC file will be reloaded, won't it?

The important thing is that, as documented, the reported link status errs on the side of being up.

> netLinkService will be non-null if getService did not throw an
> exception, so no need to check it.

OK

> >+    // if we have a netLinkService, then ioservice.offline is already set
> >+    // correctly. We will continue to allow the netLinkService to manage
> >+    // ioService.offline for us, until the user uses the "Work Offline" UI.
> 
> NOTE: The work offline UI state is something that is persisted.
> Does this new logic handle that case correctly given that it
> only reads the pref if !haveNetLinkService?

See the fourth paragraph of comment #22. By design, the pref is ignored and therefore online/offline settings are effectively not persisted when a netlinkservice is available.

> >Index: mail/base/content/commandglue.js
> >Index: mailnews/base/resources/content/commandglue.js
> 
> You're going to want to get mscott or bienvenu to review these changes.

If we go the route of using a pref to control automanagement, then there will be just one pref setting here so not much to review :-).
(In reply to comment #37)
> See the fourth paragraph of comment #22. By design, the pref is ignored and
> therefore online/offline settings are effectively not persisted when a
> netlinkservice is available.

Let me explain a little bit... If we adopt my suggested model where toggling the "Work Offline" menu item implicitly disables automanagement, then for persisting the online/offline setting to be meaningful, we have to start up next time with automanagement disabled. Then I'm not sure how to empower normal users to enable it again.
> > See the fourth paragraph of comment #22. By design, the pref is ignored and
> > therefore online/offline settings are effectively not persisted when a
> > netlinkservice is available.
> 
> Let me explain a little bit... If we adopt my suggested model where toggling
> the "Work Offline" menu item implicitly disables automanagement, then for
> persisting the online/offline setting to be meaningful, we have to start up
> next time with automanagement disabled. Then I'm not sure how to empower normal
> users to enable it again.

It seems like the application might like to re-enable automanagement when the user unchecks Work Offline.
roc: On my work today, I thought about this patch some more, and it occured to me that it might be nice to move the management feature from nsINetworkLinkService to nsIIOService2 (new interface).  Instead of making the NLS manage the IOS, provide an API on the IOS that an application may use to make the IOS tune into the network link state.  One advantage at least of doing this is that we would no longer need to replicate the management code for NLS implementations on other operating systems.  A secondary advantage is that the application can now control online/offline state and state management from one interface (make nsIIOService2 extend nsIIOService).  Thirdly, this would give Necko the ability to poll the NLS at various points, like on network connection failure, instead of only being able to receive notifications asynchronously.  Thoughts?
(In reply to comment #39)
> It seems like the application might like to re-enable automanagement when the
> user unchecks Work Offline.

Then it becomes impossible for the user to forcibly go online when there is no network interface, which is actually quite useful (for accessing a local Web server if you're a Web developer, for example).
(In reply to comment #40)
> roc: On my work today, I thought about this patch some more, and it occured
> to me that it might be nice to move the management feature from
> nsINetworkLinkService to nsIIOService2 (new interface).  Instead of making
> the NLS manage the IOS, provide an API on the IOS that an application may use
> to make the IOS tune into the network link state.  One advantage at least of
> doing this is that we would no longer need to replicate the management code
> for NLS implementations on other operating systems.  A secondary advantage is
> that the application can now control online/offline state and state
> management from one interface (make nsIIOService2 extend nsIIOService).

Sounds reasonable. I'll do that.

> Thirdly, this would give Necko the ability to poll the NLS at various points,
> like on network connection failure, instead of only being able to receive
> notifications asynchronously. 

Necko can already poll the NLS via nsINetworkLinkStatus, or just track the NLS state perfectly by listening to notifications, so I don't think this is a big deal.
> Necko can already poll the NLS via nsINetworkLinkStatus, or just track the NLS
> state perfectly by listening to notifications, so I don't think this is a big
> deal.

Yeah, but necko would have check to see if the NLS is configured to manage the IOS.  While that could be done, it seems a bit awkward to me.
Attached patch Updated patch — — Splinter Review 
Okay Darin, over to you.
Attachment #213829 - Attachment is obsolete: true
Attachment #215964 - Flags: superreview?
Attachment #215964 - Flags: review?
Attachment #213829 - Flags: superreview?(darin)
Attachment #213829 - Flags: review?(mconnor)
Attachment #213829 - Flags: review?(caillon)
Comment on attachment 215964 [details] [diff] [review]
Updated patch

caillon, I still need you to review the dbus/networkmanager bits, thanks.
Attachment #215964 - Flags: superreview?(darin)
Attachment #215964 - Flags: superreview?
Attachment #215964 - Flags: review?(darin)
Attachment #215964 - Flags: review?(caillon)
Attachment #215964 - Flags: review?
Comment on attachment 215964 [details] [diff] [review]
Updated patch

mconnor, I need your review for this Firefox UI change.
Attachment #215964 - Flags: review?(mconnor)
BTW, this patch does almost everything we talked about here. It doesn't move to a pref though, management is controlled directly via nsIIOService2.

I've also tweaked the previous patches so that dbus support is enabled by default when we build on a platform that supports it (those builds will still run on a system without dbus, though).
Comment on attachment 215964 [details] [diff] [review]
Updated patch

So, I think this doesn't quite work the way I'd like, but ultimately I'd like to kill this UI entirely, at least by default.


-    if (!this._uiElement)
-      this._uiElement = document.getElementById("goOfflineMenuitem");
+    var ioService = Components.classes["@mozilla.org/network/io-service;1"].
+      getService(Components.interfaces.nsIIOService2);
 
   toggleOfflineStatus: function ()
   {
+    var ioService = Components.classes["@mozilla.org/network/io-service;1"].
+      getService(Components.interfaces.nsIIOService2);
+

nit: correct style is like this: (and yes, its 80 chars, though we tend to ignore that more and more in browser code)

    var ioService = Components.classes["@mozilla.org/network/io-service;1"]
                              .getService(Components.interfaces.nsIIOService2);

+    try {
+      ioService.manageOfflineStatus = false;
+    } catch (ex) {
+    }
+  

nit: style in new code should be like this (Ben's working on slowly fixing browser.js's mismash of 17 different styles)

try {
  foo()
}
catch (ex) {
}
Attachment #215964 - Flags: review?(mconnor) → review+
Thunderbird would love to use auto-detect of online/offline state.
That's good to hear. Once we've landed this infrastructure it'll be easy for you to get the information. You will need to change your offline strategy though, because by the time we report loss of the network, the network really is gone and it's too late to download anything. It's fundamentally difficult to detect that the user is about to yank the ethernet cable or drive into a tunnel :-).
bienvenu: keep in mind that this patch only provides linux support (on recent distros), so we still have much work to do to provide support on windows and osx.  I'm hoping for help with those platforms :-)
the user can do a file | offline sync  to download before going offline, or they can use the mode where we download everything for offline use as they go. I'm not sure that we have to change our offline model, as least as far as the user can tell - we can still make clicking the toilet plunger icon download for offline use and then set the io service offline, if we so choose, right?

Re linux-only, yeah, I noticed that, but with this framework in place, adding this support for windows and mac shouldn't be too hard - I bet we can find volunteers somewhere - people are always pointing out how easy this is in windows :-)
(In reply to comment #51)
> bienvenu: keep in mind that this patch only provides linux support (on recent
> distros), so we still have much work to do to provide support on windows and
> osx.  I'm hoping for help with those platforms :-)

It shouldn't be much work to resurrect the Windows code in bug 76111 and fit it into this interface.
(In reply to comment #52)
> or they can use the mode where we download everything for offline use as
> they go.

That's what you want to do while automatic offline management is enabled.

> I'm not sure that we have to change our offline model, as least as far as the
> user can tell - we can still make clicking the toilet plunger icon download
> for offline use and then set the io service offline, if we so choose, right?

Yes. All I meant was is that automatic offline management is only useful in conjunction with a "download everything for offline use as we go" policy. If you already support that (I didn't know you did), you're in good shape. However you will still have to figure out how users will switch between automatic and manual selection of offline mode.
Comment on attachment 215964 [details] [diff] [review]
Updated patch

>Index: netwerk/base/public/nsINetworkLinkService.idl

>+[scriptable, uuid(61618a52-ea91-4277-a4ab-ebe10d7b9a64)]
>+interface nsINetworkLinkService : nsISupports
>+{
>+  /**
>+   * This is set to true when the system is believed to have a usable
>+   * network connection.
>+   *
>+   * The link is only up when network connections can be established. For
>+   * example, the link is down during DHCP configuration (unless there
>+   * is another usable interface already configured).
>+   *
>+   * If the link status is not currently known, we generally assume that
>+   * it is up.
>+   */
>+  readonly attribute boolean isLinkUp;
>+  
>+  /**
>+   * This is set to true when we believe that isLinkUp is accurate.
>+   */
>+  readonly attribute boolean linkStatusKnown;
>+};

A couple comments about this interface:

It's sort of like a tri-state thing that you are representing, except that
by providing linkStatusKnown you don't have to make consumers know to treat
!linkStatusKnown as "link status up".  That seems like a good thing to me.

nit: The name "linkStatusKnown" seems like it should correspond to
an attribute called "linkStatus", but calling it "isLinkUpKnown"
doesn't make sense either.  "isLinkStatusKnown" could work, but it
feels too verbose.  It'd be nice if there were more symmetry between
the names of these two attributes.


>+%{C++
>+/**
>+ * We send notifications through nsIObserverService with topic
>+ * NS_NETWORK_LINK_TOPIC whenever one of isLinkUp or linkStatusKnown
>+ * changes. We pass one of the NS_NETWORK_LINK_DATA_ constants below
>+ * as the aData parameter of the notification.
>+ */
>+#define NS_NETWORK_LINK_TOPIC "network:link-status-changed"
>+
>+/**
>+ * isLinkUp is now true, linkStatusKnown is true.
>+ */
>+#define NS_NETWORK_LINK_DATA_UP      "up"
>+/**
>+ * isLinkUp is now false, linkStatusKnown is true.
>+ */
>+#define NS_NETWORK_LINK_DATA_DOWN    "down"
>+/**
>+ * linkStatusKnown is now false.
>+ */
>+#define NS_NETWORK_LINK_DATA_UNKNOWN "unknown"
>+%}

Here, you're representing the link status as a tri-state thing, but
on the interface it's a bit different.  Perhaps the observer "data"
should just be null for NS_NETWORK_LINK_TOPIC.  Then, just have
consumers check the subject's attributes to determine the state.
Otherwise the consumer might be tempted to interpret the "data"
value instead of asking the NetworkLinkStatus object.

Also, I'd strongly encourage you to implement nsIClassInfo so that
script consumers will not have to QI to nsINetworkLinkService.


>Index: netwerk/base/src/nsIOService.cpp

>+nsresult
>+nsIOService::TrackNetworkLinkStatusForOffline()
>+{
>+    NS_ASSERTION(mManageOfflineStatus,
>+                 "Don't call this unless we're managing the offline status");
>+    if (!mNetworkLinkService)
>+        return NS_ERROR_FAILURE;

You could also just do this:

  NS_ENSURE_STATE(mNetworkLinkStatus);


>Index: netwerk/build/nsNetCID.h

Shouldn't NS_NETWORK_LINK_SERVICE_CONTRACTID have been defined in this file?


>Index: extensions/dbus/nsDBusService.cpp

>+nsDBusService::nsDBusService() {
>+  mConnection = nsnull;
>+  mSingleClient = nsnull;
>+}

nit: prefer C++ member initializer list


>+class nsNetworkManagerListener : public nsINetworkLinkService,
>+                                 public DBusClient {
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSINETWORKLINKSERVICE
>+
>+  nsNetworkManagerListener();
>+  virtual ~nsNetworkManagerListener();

Does this dtor really need to be public and virtual?  Since objects
of this type are always deleted through Release, it could be made
private and non-virtual.


r+sr=darin
Attachment #215964 - Flags: superreview?(darin)
Attachment #215964 - Flags: superreview+
Attachment #215964 - Flags: review?(darin)
Attachment #215964 - Flags: review+
roc: how about landing the non-dbus parts of this patch (aka the API) sooner rather than later?  it'd be nice to get this API in so we can begin implementing it on windows and osx.  thanks!
OK, I've checked in everything except the dbus stuff.
(In reply to comment #57)
>OK, I've checked in everything except the dbus stuff.
Including some xpfe changes for which I don't even see a diff. Before I clean up after you, is there a good reason why you used a try/catch block?
I had some xpfe changes in an earlier patch, and they dropped out somewhere along the way, so I just copied the Thunderbird change before checking in, sorry.

I used a try/catch block merely to minimize the chance of accidentally horking the app, if, for example, the app JS changes were copied back to a branch.
Blocks: 333065
Sorry for the delay here.

Well, I guess this patch uses dbus correctly, but I still strongly prefer to use libnm as I originally suggested in comment 2.  The arguments to not use libnm directly were: 

> The advantage of having a dedicated DBUS connector component is that when DBUS
libraries are not available, the component fails to load and the app carries on.

Sure.  But When DBUS libraries ARE available, you are loading the module for nothing if NM isn't around, though.  If you look for NM, then you know for a fact that you'll be getting meaningful stuff out of it since NM has a requirement on DBUS.


> Also, with this component we could listen for other events on the same DBUS
connection. I don't know if libnm is implemented in a way that permits that.

libnm has its own DBUS connection, but it will do that anyway regardless of whether Firefox is listening onto it.  We may well need a DBUS handler later for  something else, but I don't see a real reason why Firefox should require it now.

> Also^2, this is implemented in a way that would also be usable with Qt with very minor modifications.

You are requiring the DBUS glib bindings.  I'm not sure how that makes libnm-glib any less desirable.
Blocks: 216490
(In reply to comment #60)
> > The advantage of having a dedicated DBUS connector component is that when
> > DBUS libraries are not available, the component fails to load and the app
> > carries on.
> 
> Sure.  But When DBUS libraries ARE available, you are loading the module for
> nothing if NM isn't around, though.  If you look for NM, then you know for a
> fact that you'll be getting meaningful stuff out of it since NM has a
> requirement on DBUS.

Not quite, because using libnm-glib, if NM is installed but not running, we'll still load the module. But the component is small and loading it when NM is not installed or not running is not a big deal.

> > Also, with this component we could listen for other events on the same DBUS
> > connection. I don't know if libnm is implemented in a way that permits
> > that.
> 
> libnm has its own DBUS connection, but it will do that anyway regardless of
> whether Firefox is listening onto it.

Not if we don't load it, I presume...

> We may well need a DBUS handler later
> for  something else, but I don't see a real reason why Firefox should require
> it now.

I don't have any other immediate use, but I don't want to go down a path that needs to be rewritten later.

(One potential use: Epiphany uses dbus for remoting command line args to a running instance, and we might want to follow suit. Our current setup is somewhat crappy.)

> > Also^2, this is implemented in a way that would also be usable with Qt with 
> > very minor modifications.
> 
> You are requiring the DBUS glib bindings.  I'm not sure how that makes
> libnm-glib any less desirable.

I would remove the dbus-glib dependency, but this is a very minor point.

About libnm-glib: Does it continue to work correctly across dbus and NM restarts?

Also, Christian Persch says that libnm-glib sleeps for 50ms during startup waiting for a thread to start, is that true? (Epiphany has NM integration partly based on my code, BTW, they decided not to use libnm-glib.) Hmm, an extra thread seems worth avoiding, actually.

I guess you won't be surprised that I like what I have here :-). It's simple and small, and avoids depending on one build-time package and one run-time package; the latter is a real difference for KDE users running firefox. It's efficient now, and lets us add new dbus users efficiently later.
The main reason epiphany stopped using it was because it started crashing on printing.  The crash is fixed, and I'm fixing the threading issue.  By the time any Firefox release is done with this code, it will be long since fixed.  The libnm-qt port (which nobody has stepped up to do) would be very minimal code.  Things such as evolution already use libnm-glib and I'd like to move things to use that where possible.  When the libnm stuff is fixed, I plan on moving epiphany back to using it, because I'd like to keep dependence on unstable dbus signals to a minimum.  Epiphany has already been hit by the actual dbus signals that NM sends out changing.  For them, it's no big deal to simply change things since it's the GNOME stack; people tend to upgrade GNOME wholesale.  But this will cause problems after a Firefox release is made since people will need a different version of the Firefox component based on the NM version.

I'm taking ownership of the libnm stuff and will work to resolve any issues that you have with it.
(In reply to comment #62)
> The main reason epiphany stopped using it was because it started crashing on
> printing.  The crash is fixed, and I'm fixing the threading issue.  By the
> time any Firefox release is done with this code, it will be long since fixed.
> The libnm-qt port (which nobody has stepped up to do) would be very minimal
> code.

But it would require a new Firefox component, and until those are both written, we will need NetworkManager-gnome to be installed for KDE Firefox users.

> When the libnm stuff is fixed, I plan on moving epiphany back to using it,
> because I'd like to keep dependence on unstable dbus signals to a minimum.
> Epiphany has already been hit by the actual dbus signals
> that NM sends out changing. 

That's an issue. So there are no plans to stabilize the signals?

> I'm taking ownership of the libnm stuff and will work to resolve any issues
> that you have with it.

Will we be able to share a dbus connection? Or is GNOME heading down a road where there's one dbus connection, libfoo-glib shared library, and foo-gnome package for each service you want to deal with?
(In reply to comment #63)
> (In reply to comment #62)
> > The main reason epiphany stopped using it was because it started crashing on
> > printing.  The crash is fixed, and I'm fixing the threading issue.  By the
> > time any Firefox release is done with this code, it will be long since fixed.
> > The libnm-qt port (which nobody has stepped up to do) would be very minimal
> > code.
> 
> But it would require a new Firefox component, and until those are both written,
> we will need NetworkManager-gnome to be installed for KDE Firefox users.

Or some ifdefs.  Regardless, NetworkManager without NetworkManager-gnome is effectively useless for a desktop install anyway.  There's no way to tell NM to connect to a network without the applet (well, the user could do dbus-send but that's not a really good way to do it) so network status changes won't happen really.  NM won't even connect in this situation unless you're on a wired connection and NM is able to simply pull up a network (which isn't really the target for offline/online mode).  I am going to attempt to fix this situation but it may not make the next firefox release.

> 
> > When the libnm stuff is fixed, I plan on moving epiphany back to using it,
> > because I'd like to keep dependence on unstable dbus signals to a minimum.
> > Epiphany has already been hit by the actual dbus signals
> > that NM sends out changing. 
> 
> That's an issue. So there are no plans to stabilize the signals?

There are.  But the fact is still that dbus is just an implementation detail of NetworkManager.  It so happens that this implementation detail is a cool piece of the desktop, and the signals we send publically accessible if you connect to the bus, but its still just an implementation detail.


> > I'm taking ownership of the libnm stuff and will work to resolve any issues
> > that you have with it.
> 
> Will we be able to share a dbus connection? Or is GNOME heading down a road
> where there's one dbus connection, libfoo-glib shared library, and foo-gnome
> package for each service you want to deal with?


With libnm you mean?  I can certainly add support for supplying your own DBusConnection* for libnm to use (I was talking about this with a few people the other day as something I'd like to do), but the current shipped API doesn't yet have this.
(In reply to comment #64)
> Or some ifdefs.  Regardless, NetworkManager without NetworkManager-gnome is
> effectively useless for a desktop install anyway.  There's no way to tell NM
> to connect to a network without the applet (well, the user could do dbus-send
> but that's not a really good way to do it) so network status changes won't
> happen really.  NM won't even connect in this situation unless you're on a
> wired connection and NM is able to simply pull up a network (which isn't
> really the target for offline/online mode).  I am going to attempt to fix
> this situation but it may not make the next firefox release.

I don't think that's right, there is knetworkmanager.

> There are.  But the fact is still that dbus is just an implementation detail
> of NetworkManager.  It so happens that this implementation detail is a cool
> piece of the desktop, and the signals we send publically accessible if you
> connect to the bus, but its still just an implementation detail.

NetworkManager exposes a lot of stuff through signals that you can't get through libnm-glib. Are you going to be exposing all that? Do you expect people to write libnm-glib bindings for Python etc instead of just using the dbus bindings that already exist?

Making a set of dbus signals be the basic supported API seems to have major advantages over making a C wrapper over dbus be the only supported API: desktop independence, language independence, and late binding without XPCOM or dlopen hacks. I don't understand what you gain in exchange for throwing those away.

> With libnm you mean?  I can certainly add support for supplying your own
> DBusConnection* for libnm to use (I was talking about this with a few people
> the other day as something I'd like to do), but the current shipped API
> doesn't yet have this.

OK, that'd be nice to have.
Blocks: 336285
(In reply to comment #65)
> I don't think that's right, there is knetworkmanager.
> 

It requires Qt4, and a whole bunch of other "brand spanking new" stuff.  This was not even being worked on as far as I knew because it was in a state of "impossible to do in Qt 3, and planned for Qt4, but not implemented yet", but I just looked at it, and it appears to have traction now which is awesome.  I'll look into that, and see what we can do here.


> > There are.  But the fact is still that dbus is just an implementation detail
> > of NetworkManager.  It so happens that this implementation detail is a cool
> > piece of the desktop, and the signals we send publically accessible if you
> > connect to the bus, but its still just an implementation detail.
> 
> NetworkManager exposes a lot of stuff through signals that you can't get
> through libnm-glib. Are you going to be exposing all that? Do you expect people
> to write libnm-glib bindings for Python etc instead of just using the dbus
> bindings that already exist?

Mozilla exposes stuff that people need too, but isn't available via XPCOM for example.  NetworkManager exposes more than Firefox would ever need: you could send some signals and cause the box to switch to a different network.  But that's rather irrelevant to the discussion.

Is there something specific you want exposed?  I am going to extend the API to support giving out SSIDs of the connected wireless network.  What else would you potentially want?


> Making a set of dbus signals be the basic supported API seems to have major
> advantages over making a C wrapper over dbus be the only supported API: desktop
> independence, language independence, and late binding without XPCOM or dlopen
> hacks. I don't understand what you gain in exchange for throwing those away.

Dbus has changed a bit over time.  I assume you're just going to ignore bug reports on old API dbus's.  Which exist in RHEL4 still.  So, you'd have to do dlopening anyway, lest things break.  And I don't see what language indepdence gains you if you are going to use the language anyway.  We use glib and qt.  No need to pretend we don't.  If you want a pure C API, I can provide that, just as easily.  I've not gotten a request for one yet, but I don't see how it's a bad thing.

> 
> > With libnm you mean?  I can certainly add support for supplying your own
> > DBusConnection* for libnm to use (I was talking about this with a few people
> > the other day as something I'd like to do), but the current shipped API
> > doesn't yet have this.
> 
> OK, that'd be nice to have.
> 

Cool.  Stay tuned.
> Dbus has changed a bit over time.  I assume you're just going to ignore bug
> reports on old API dbus's.  Which exist in RHEL4 still.  So, you'd have to do
> dlopening anyway, lest things break.

No, we get that for free with this component; RHEL4 users won't load the component and nothing will break.

Can you tell me straight-out whether the dbus signals are a supported API to NetworkManager? Because if they are, I want you to review this patch, and if they are not, I think someone's made a colossal mistake.
From the NetworkManager architecture page at
http://people.redhat.com/dcbw/NetworkManager/architecture.html:

"All other programs on the system interact with NetworkManager over dbus, sending messages and receiving replies via the NetworkManager dbus API. This provides a flexible interface to user programs, without having to rely upon binary linking and deal with issues of binary compatibility."

I would like to shake that man's hand!

Another right-thinker appears to have worked on http://www.gnome.org/projects/NetworkManager/, which says

"And since the NetworkManager components communicate with each other using dbus, it's easy to to build network-aware applications in languages like C, C++, and Python."

and then goes on to add

"Using the awesome power and flexibility of dbus and hal, NetworkManager provides facilities for other desktop applications like browsers and email clients to be aware of the network's state and adjust their operation accordingly for features like offline operation."

I would like some awesome power and flexibility please!

If the dbus events aren't stable by the time we code-freeze Fx2, and there isn't compatibility support for the old ones, then some Linux users on hyper-modern desktops will lose out until Fx3.  I think it would behoove the NM maintainers to view that prospect with some significant concern, but that's not my call.    Maybe Portland will save us, in time.  Otherwise, we can add new dbus event names and such until quite late in the release cycle.  What we want right now is to get this stuff exposed for test and developer feedback, in Bon Echo Alpha 2, which we're freezing ASAP.

(And a dbus-accessing component is a virtuous thing to provide as part of our platform on Linux, since I hear that's how fashionable Linux apps talk to other fashionable Linux apps these days.)

Comment on attachment 215964 [details] [diff] [review]
Updated patch

Let's get the API bits of this (at least) on the 1.8 branch.
Attachment #215964 - Flags: approval-branch-1.8.1?(shaver)
Comment on attachment 215964 [details] [diff] [review]
Updated patch

r=shaver and branch-approval for the API changes.
Attachment #215964 - Flags: review?(caillon)
Attachment #215964 - Flags: review+
Attachment #215964 - Flags: approval-branch-1.8.1?(shaver)
Attachment #215964 - Flags: approval-branch-1.8.1+
Here's the part of the patch that I landed on the 1.8 branch for FF2a2.
(I also copied nsIIOService2.idl from the trunk to the 1.8 branch.)
Comment on attachment 215964 [details] [diff] [review]
Updated patch

Chris agreed to review this after checking that there are no currently-planned changes to this NM API. I'm holding out for that!
Attachment #215964 - Flags: review?(caillon)
Attachment #215964 - Flags: review?(caillon)
Comment on attachment 215964 [details] [diff] [review]
Updated patch

Sorry, I shouldn't have removed the caillon review? flag.
Attachment #215964 - Flags: review?(caillon)
Comment on attachment 215964 [details] [diff] [review]
Updated patch

Okay.  Sorry for the long delay, here.  The patch looks pretty good.  DBus use looks pretty good.

The API will be stable at least for these guys so we're safe.  But there is a potential pitfall coming up: the semantics will slightly change when we move to supporting multiple connections.  This shouldn't affect things terribly, but it might have some effect in this scenario:

eth0 has a connection
a keepalive connection is made
eth1 is brought up
eth0 is brought down

Not sure what the best solution is here offhand, since there's a case for always having state be NM_STATE_CONNECTED, and also a strong case for continuing to issue NM_STATE_CONNECTING, etc for the new interface...

In any case, retroactive r=caillon.
Attachment #215964 - Flags: review?(caillon) → review+
Updated to trunk. I moved the dbus code to toolkit/system/dbus in line with the new directory structure for system components. bsmedberg, can you review the build system changes? thanks.
Attachment #243281 - Flags: review?
Attachment #243281 - Flags: review? → review?(benjamin)
Blocks: 359134
Comment on attachment 243281 [details] [diff] [review]
updated patch to trunk

>Index: configure.in

>+    MOZ_ENABLE_DBUS=1
>+
>+    MOZ_ARG_DISABLE_BOOL(dbus,
>+    [  --disable-dbus       Disable dbus support (default: auto, optional at runtime) ],
>+        MOZ_ENABLE_DBUS=,
>+        MOZ_ENABLE_DBUS=force)

I know that linux-style configures use "auto" options a lot, but I would like our build system to be more determinite. I'd like the default to be "force" and have builders explicitly --disable-dbus if they don't have the right libs available.
Attachment #243281 - Flags: review?(benjamin) → review-
Attachment #246742 - Flags: superreview?(benjamin)
Attachment #246742 - Flags: superreview?
Attachment #246742 - Flags: review?(benjamin)
Attachment #246742 - Flags: review?
Attachment #246742 - Flags: superreview?(benjamin)
Attachment #246742 - Flags: review?(benjamin) → review+
checked in.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
This set Tinderbox on fire because various Tinderboxen need to have their configurations changed to explicitly --disable-dbus. I fixed that by checking in the earlier version of my patch with the autodetection.

I can file a followup bug to restore the forcing behaviour, but we'll need to change tinderbox configs first. Actually I'd like to understand why autodetection is deprecated. Isn't it useful for people to be able to configure and build and get *something* that works without spending a lot of time messing with .mozconfig? Lowers barriers to entry and all that.
If we're going to autodetect, then I think that at least --enable-official-branding should suppress the autodetection: if you're building something branded, you should be explicit about what's in it, if only so that it's easier/possible to analyze the configuration without access to the build machine.

Also: is there a way for about:buildconfig to say which path it took for autodetected options?  I think that'll be necessary for tracking down reports related to missing DBUS support. 
(In reply to comment #81)
> If we're going to autodetect, then I think that at
> least --enable-official-branding should suppress the
> autodetection: if you're building something branded,
> you should be explicit about what's in it, if only so
> that it's easier/possible to analyze the configuration
> without access to the build machine.

That sounds like a good idea.

> Also: is there a way for about:buildconfig to say which
> path it took for autodetected options?  I think that'll
> be necessary for tracking down reports related to
> missing DBUS support.

Good question, I don't know how hard that would be.
(In reply to comment #80)
> I can file a followup bug to restore the forcing behaviour, but we'll need to
> change tinderbox configs first. Actually I'd like to understand why
> autodetection is deprecated. Isn't it useful for people to be able to configure
> and build and get *something* that works without spending a lot of time messing
> with .mozconfig? Lowers barriers to entry and all that.

One approach that might solve the main problems people have with autodetection is if:
 (1) autodetection never happens when there's an explicit --enable-* or --disable-*
 (2) any autodetection case recorded its result in a text file somewhere as --enable-* or --disable-* so that we can add an "Autodetected build options" section to about:buildconfig, improving both reproducability and documentation of build configuration

Maybe I should file a bug on that if others think it makes sense...
Keywords: fixed1.8.1
Blocks: 388743
Component: XP Miscellany → General
QA Contact: brendan → general
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: