495674 - Internet connection should be initiated if needed

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Claudius Henrichs]

User-Agent:       Opera/9.64 (Macintosh; Intel Mac OS X; U; de) Presto/2.1.1
Build Identifier: 

The vast majority of maemo applications try to initiate the device's internet connection if they need it and it's currently disable. Fennec doesn't do that yet but it should.

Reproducible: Always

Steps to Reproduce:
1. Disconnect any internet connection on your Maemo device
2. Start Fennec
3a. Open the extension manager
3b. Try to access a website
Actual Results:  
3a => "Fennec couldn't retrieve add-ons"
3b => "Page load error"

Expected Results:  
Fennec should initiate the device's internet connection

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Code from maemo microb that does this?

https://garage.maemo.org/svn/browser/mozilla/trunk/microb-eal/src/gmozillaconnectivity.c

Comment 2

[Jeremias Bosch (:jbos)]

Attachment: Patch to add use of connectivity to fennec for maemo

This patch makes use of the build-in autodial function of Fennec!
It handles the mode quite different than the windows versions do, since it switch fennec in its offline mode when the user abort the connection process. 

The Connection Library of Maemo Diablo does work asynchronously, that made it necessary to add an Maemo Network Manager which synchronize this process.

You need to have the Library: libconic installed to get this compiling. Libconic is installed by default in Maemo Diablo.

Comment 3

[Tobias Hunger]

Patch works for me, but I am not able to set review+ here and have it stick:-)

Comment 4

[Antonio Gomes (tonikitoo)]

patch looks great ! dougt could be your reviewer i guess.

a few nit comments:

+ifdef MOZ_PLATFORM_HILDON
+	LIBS += -lconic
+	OS_INCLUDES += $(GLIB_CFLAGS)
+	LOCAL_INCLUDES += -I/usr/include/dbus-1.0 -I/usr/lib/dbus-1.0/include -I/usr/include/conic

you could probably add an entry in configure.in and have LIBCONIC_CFLAGS | _LIBS here.


i am wondering why is this required:
ifdef MOZ_ENABLE_DBUS
+ifndef MOZ_PLATFORM_HILDON
 tier_toolkit_dirs    += toolkit/system/dbus
+endif


there could be possible followup bugs:
* listen to connection failures and pause downloads when connection failures.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 387156 [details] [diff] [review]
Patch to add use of connectivity to fennec for maemo

>diff --git a/netwerk/base/src/nsIOService.cpp b/netwerk/base/src/nsIOService.cpp

>+#if defined( MOZ_PLATFORM_HILDON )
>+    if (mSocketTransportService)
>+        mSocketTransportService->SetAutodialEnabled(true);
>+#endif

PR_TRUE instead of true ?

However, maybe we should be using the preference to set this value. PrefsChanged is called in Init too and reads "network.autodial-helper.enabled" from preferences.

We should just add that to mobile.js in the Fennec code and not force it here.

>diff --git a/netwerk/base/src/nsMaemoNetworkManager.cpp b/netwerk/base/src/nsMaemoNetworkManager.cpp

>+    void notifyNetworkLinkObservers()

Use leading capital for method names

A few things with the entire file:
 - No need for space padding inside parens
 - The convention is to use an "a" prefix for method arguments: "event" -> "aEvent"


>diff --git a/netwerk/base/src/nsMaemoNetworkManager.h b/netwerk/base/src/nsMaemoNetworkManager.h

>+class nsMaemoNetworkManager
>+{
>+public:
>+    static nsMaemoNetworkManager* GetInstance();
>+    ~nsMaemoNetworkManager();
>+
>+    PRBool openConnectionSync();
>+    void closeConnection();
>+
>+    PRBool isConnected();
>+    PRBool isConnecting();
>+
>+private:
>+    nsMaemoNetworkManager();
>+    nsMaemoNetworkManagerPrivate* const d;
>+};

Use Leading Caps for method names:

OpenConnectionSync(), CloseConnection(), IsConnected(), IsConnecting()

>diff --git a/netwerk/system/maemo/Makefile.in b/netwerk/system/maemo/Makefile.in

>+REQUIRES	= xpcom \
>+		  string \
>+		  pref \
>+		  $(NULL)

I don't think you need to use REQUIRES anymore

>+ifdef MOZ_PLATFORM_HILDON
>+    CPPSRCS += nsMaemoNetworkLinkService.cpp
>+endif

Is this #ifdef needed here? This file should only be built if MOZ_PLATFORM_HILDON, right?

>+ifdef MOZ_PLATFORM_HILDON
>+LIBS += -lconic
>+OS_INCLUDES += $(GLIB_CFLAGS)
>+LOCAL_INCLUDES += -I/usr/include/dbus-1.0 -I/usr/lib/dbus-1.0/include -I/usr/include/conic -I$(srcdir)/../../base/src
>+endif

Same here

>diff --git a/netwerk/system/maemo/nsMaemoNetworkLinkService.cpp b/netwerk/system/maemo

>+NS_IMETHODIMP
>+nsMaemoNetworkLinkService::Observe(nsISupports *subject,
>+                                   const char *topic,
>+                                   const PRUnichar *data)
>+{
>+    if (!strcmp( topic, "xpcom-shutdown" ))
>+    {
>+        Shutdown();
>+    }

Use aSubject, aTopic and aData as arguments

>+    nsCOMPtr<nsIObserverService> observerService =
>+        do_GetService( "@mozilla.org/observer-service;1", &rv );
>+    NS_ENSURE_SUCCESS( rv, rv );
>+
>+    rv = observerService->AddObserver( this, "xpcom-shutdown", PR_FALSE );
>+    NS_ENSURE_SUCCESS( rv, rv );

No space padding needed in the parens

>diff --git a/toolkit/library/Makefile.in b/toolkit/library/Makefile.in

> ifdef MOZ_PLATFORM_HILDON
>-EXTRA_DSO_LDOPTS += $(LIBHILDONMIME_LIBS)
>+EXTRA_DSO_LDOPTS += $(LIBHILDONMIME_LIBS) -lconic
> endif 

Might be nice to get conic in a LIBCONIC_LIBS

>diff --git a/toolkit/toolkit-tiers.mk b/toolkit/toolkit-tiers.mk

> ifdef MOZ_ENABLE_DBUS
>+ifndef MOZ_PLATFORM_HILDON
> tier_toolkit_dirs    += toolkit/system/dbus
>+endif
> endif

Why is this needed?

-----
Overall this looks very good. We should get Doug Turner and Ted Mielczarek (for the build & makefile parts) to review as well.

Comment 6

[Jeremias Bosch (:jbos)]

Thanks for the Reviews, I will change the things you mentioned and add the patch later. 

To your question:

>>diff --git a/toolkit/toolkit-tiers.mk b/toolkit/toolkit-tiers.mk

>> ifdef MOZ_ENABLE_DBUS
>>+ifndef MOZ_PLATFORM_HILDON
>> tier_toolkit_dirs    += toolkit/system/dbus
>>+endif
>> endif

> Why is this needed?

The Problem is that in linux the network status is managed here (in /toolkit/dbus/nsNetworkManagerListener) and not in network/system as for every other platform. 

From the file /toolkit/dbus/nsDBusService.h
> [..]
> * Currently the only daemon we communicate with is NetworkManager. We listen
> * for NetworkManager state changes; we set nsIOService's offline status to
> * FALSE when NetworkManager reports NM_STATE_CONNECTED, and to TRUE otherwise.
> * We also solicit the current status from NetworkManager when this component
> * gets loaded.
> [..]

Since the patch adds a NetworkManager for Linux/Maemo/HILDON-Environment, its wrong to have a second network manager here which interfere also with nsIOService and causing uncontrolled network states (i.e. some errors: setting fennec uncontrolled by the MaemoNetworkLinkService to offline-mode, Fennec does not make use of autodial, Fennec does not make use of MaemoNetworkLinkService at all,...).

I hope its clear now.

---

Comment 7

[Jeremias Bosch (:jbos)]

Attachment: Patch to add use of connectivity to fennec for maemo

I fixed the issues you mentioned in your last comment. The second patch is for mobile.js to add the autodial preference!

Comment 8

[Jeremias Bosch (:jbos)]

Attachment: enables the autodial function for hildon environment in mobile.js

As mentioned in comment #5 i added the autodial preference to mobile.js and not force it in nsIOService.cpp anymore.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 387449 [details] [diff] [review]
Patch to add use of connectivity to fennec for maemo

>+++ b/netwerk/base/src/nsMaemoNetworkManager.cpp

Still some "( " and  " )" in here

>+++ b/netwerk/system/maemo/Makefile.in
>\ No newline at end of file

Very good. I'll set for DougT to review.

Comment 10

[Jeremias Bosch (:jbos)]

Attachment: Patch to add use of connectivity to fennec for maemo

Thanks for the + Mark. Changed the things from comment #9.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 387455 [details] [diff] [review]
Patch to add use of connectivity to fennec for maemo

Ted - could you look at the build parts?

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 387455 [details] [diff] [review]
Patch to add use of connectivity to fennec for maemo

nsAutodailMaemo -> nsAutodialMaemo

Can we use the tri license on these files, please?

Space between the #includes and the start of the implementation:

+#include "nsThreadUtils.h"
+nsAutodial::nsAutodial()


style nit.  i like 2 spaces.  I think that is most common in /netwerk/

no need to have comments that state the obvious:

+    // ctor

+    // dtor


Lets use a more descriptive member name:
+    nsMaemoNetworkManagerPrivate* const d;


+nsMaemoNetworkManager::nsMaemoNetworkManager(): d(new nsMaemoNetworkManagerPrivate())

Can we move much/all of this initialization functionality into an init() method so that we can handle error cases?

Also, lets test for |d| before using it.


lets fix the above, and I can take another look.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 387455 [details] [diff] [review]
Patch to add use of connectivity to fennec for maemo

diff --git a/netwerk/base/src/nsAutodailMaemo.cpp b/netwerk/base/src/nsAutodailMaemo.cpp

I think it's spelled "dial". :)

+ifdef MOZ_PLATFORM_HILDON
+	LIBS += $(LIBCONIC_LIBS)
+	OS_INCLUDES += $(GLIB_CFLAGS)
+	LOCAL_INCLUDES += $(LIBCONIC_CFLAGS)

Don't use actual tabs here, just two spaces is fine.

r=me on the build parts with those nits addressed.

Comment 14

[Jeremias Bosch (:jbos)]

Attachment: Patch to add use of connectivity to fennec for maemo

I changed the things you come up with.

Just left this things be:

+    // ctor

+    // dtor

Because its the same in nsAutodialWin/CE its consistent that way.

I do not think that this is necessary: "lets test for |d| before using it." because |d| is 

nsMaemoNetworkManagerPrivate* const d

Comment 15

[Jeremias Bosch (:jbos)]

ping

Comment 16

[Doug Turner (:dougt)]

the file name is still wrong.

why does "nsMaemoNetworkManagerPrivate* const pimpl" get you around a failed malloc/new?  if you OOM, you're going to crash, right?


Also, change "pimpl" to something else like "mImpl".  Frankly, i am not sure why you can't just roll this class into nsMaemoNetworkManager.

Comment 17

[Tobias Hunger]

PImpl is a well known technique, see:
http://en.wikipedia.org/wiki/Pimpl

In Qt this pointer is always called d (or d_ptr), but you did not like that, so we moved on to another well known name:-)

Why use that here? It improves binary compatibility (we have no IDL for that in this case) and hides all the private implementation details from the view of a developer using the interface defined by nsMaemoNetworkManager.

The pointer is const (not the data pointed to), so only one test is required when the pointer is initialized. It can never change afterwards. This is why testing for != 0 does not make sense when *using* the d/PImpl pointer.

There should be no need to move this initialization into a init method. New should throw an exception on OOM, so the issue is noticed. Doing the initialization outside the initializer list further stops us from using a const pointer to the non-const data, introducing all kinds of issues with changing the pointer accidentally.

Comment 18

[Doug Turner (:dougt)]

I am familiar with the technique, mostly was just unhappy about the variable name.  the var name is not used in mozilla (for better or worse):

http://mxr.mozilla.org/mozilla-central/search?string=pimpl

mImpl would be a better choice, imo.



+#include "nsAutodailMaemo.h"

update the file name



I will try the patch out tomorrow, and review the dbus bits and pieces.  Looks good so far!

Comment 19

[Jeremias Bosch (:jbos)]

Attachment: Patch to add use of connectivity to fennec for maemo

Sorry, i missed somehow the header file... don't know how that happened, but anyway Tobias mentioned everything already, i removed the init Function since it does not make any sense.

Comment 20

[Doug Turner (:dougt)]

Comment on attachment 388430 [details] [diff] [review]
Patch to add use of connectivity to fennec for maemo

got this when compiling your patch on a clean scratchbox.

c++ -o nsMaemoNetworkManager.o -c -fvisibility=hidden -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_COM_OBSOLETE -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux -DIMPL_NS_NET -I/usr/include/conic -I/usr/include/dbus-1.0 -I/usr/lib/dbus-1.0/include -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include   -I/home/dougt/builds/mobile/mozilla-central/netwerk/base/src -I. -I../../../dist/include -I../../../dist/include/nsprpub  -I/home/dougt/builds/mobile/mobilebase/xulrunner/dist/include/nspr -I/home/dougt/builds/mobile/mobilebase/xulrunner/dist/include/nss    -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include        -fPIC   -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -pedantic -fno-strict-aliasing -pthread -pipe  -DNDEBUG -DTRIMMED -Os -freorder-blocks -fno-reorder-functions -finline-limit=50   -DMOZILLA_CLIENT -include ../../../mozilla-config.h -Wp,-MD,.deps/nsMaemoNetworkManager.pp /home/dougt/builds/mobile/mozilla-central/netwerk/base/src/nsMaemoNetworkManager.cpp
In file included from ../../../dist/include/nsCOMArray.h:42,
                 from ../../../dist/include/nsCategoryCache.h:48,
                 from /home/dougt/builds/mobile/mozilla-central/netwerk/base/src/nsIOService.h:58,
                 from /home/dougt/builds/mobile/mozilla-central/netwerk/base/src/nsMaemoNetworkManager.cpp:38:
../../../dist/include/nsVoidArray.h: In member function 'void nsAutoVoidArray::ResetToAutoBuffer()':
../../../dist/include/nsVoidArray.h:193: warning: cast from 'char*' to 'nsVoidArray::Impl*' increases required alignment of target type
/home/dougt/builds/mobile/mozilla-central/netwerk/base/src/nsMaemoNetworkManager.cpp: In member function 'void nsMaemoNetworkManagerPrivate::ConnectionEventCallback(ConIcConnection*, ConIcConnectionEvent*)':
/home/dougt/builds/mobile/mozilla-central/netwerk/base/src/nsMaemoNetworkManager.cpp:84: error: 'CON_IC_STATUS_NETWORK_UP' was not declared in this scope
/home/dougt/builds/mobile/mozilla-central/netwerk/base/src/nsMaemoNetworkManager.cpp: At global scope:
/home/dougt/builds/mobile/mozilla-central/netwerk/base/src/nsMaemoNetworkManager.cpp:48: warning: 'void _g_statistics_event(ConIcConnection*, ConIcStatisticsEvent*, void*)' declared 'static' but never defined
make[7]: *** [nsMaemoNetworkManager.o] Error 1
make[7]: Leaving directory `/home/dougt/builds/mobile/mobilebase/xulrunner/netwerk/base/src'
make[6]: *** [libs] Error 2

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

I did a | fakeroot apt-get install libconic0-dev | into my scrtahcbox, just to
be sure I had the conic files. I still get the same error as Doug:

/nsMaemoNetworkManager.cpp:84:
error: 'CON_IC_STATUS_NETWORK_UP' was not declared in this scope

I am building using cs2007q3 in a CHINOOK-ARMEL sbox.

Comment 23

[Jeremias Bosch (:jbos)]

Im not able to build plain Mozilla Central at the moment, so i Need to sort this out Today (some sqlite3 Error). Than i can see whats going on with the Patch. Using Chinook for à diablo based Patch Could Be the issue, but i will See .

Comment 24

[Jeremias Bosch (:jbos)]

The Patch is definitely compiling here on an Diablo-Arm Target. It have to be your old CHINOOK-ARML sbox. I cant test it with the newest mozilla central see #504761

Comment 25

[Jeremias Bosch (:jbos)]

Update.

With the Patch from Bug 503570 or the most recent mozilla central it works again, i can verify that the patch is compiling and working as it supposed to be.

Comment 26

[Antonio Gomes (tonikitoo)]

(In reply to comment #25)
> Update.
> 
> With the Patch from Bug 503570 or the most recent mozilla central it works
> again, i can verify that the patch is compiling and working as it supposed to
> be.

jeremias, if you have not tried it yet, please try setting up a build rootstrap based on the following steps:

https://wiki.mozilla.org/Mobile/Build/cs2007q3

i believe most of the mozilla guys are still stuck on using 4.0.1 SDK (chinook) , and not 4.1.x (not diablo SDK) ... and then those libconic problems can be reproducible for you too (?)

Comment 27

[Jeremias Bosch (:jbos)]

> jeremias, if you have not tried it yet, please try setting up a build rootstrap
> based on the following steps:
> https://wiki.mozilla.org/Mobile/Build/cs2007q3
> 

> i believe most of the mozilla guys are still stuck on using 4.0.1 SDK (chinook)
> , and not 4.1.x (not diablo SDK) ... and then those libconic problems can be
> reproducible for you too (?)

This patch is for Diablo, not for a previous SDK Version. So it does not work with Chinook. Unfortunately there is no way to distinguish Chinook from Diablo 
in fennec/mozilla so what we do about it. Support for Chinook won't be made by this patch, the solution would be to not compile it in a Chinook Version of fennec - when there is a way to distinguish it... 

I think they should really update then this Description and there environment.

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

vote for not blocking1.0

Comment 29

[Stuart Parmenter]

why can we not just dlopen() some functions here and get this landed?

Comment 30

[Jeremias Bosch (:jbos)]

Attachment: Patch to add use of connectivity to fennec for maemo

Hello,
For the case that you really want to get that landed in 1.0. I recently just got some old documentation in hand, regarding to that documentation the only change in libconic which should bothering the patch is the NETWORK_UP state and that is not really used by the patch-implementation anyway yet. 
So I removed the state from the patch and it should now work now also in CHINOOK. 

When you get these patch landed you should also look into Bug 495674, its just a very small extension of this patch and of course it works fine.

-JB

Comment 31

[Jeremias Bosch (:jbos)]

Sorry i meant Bug 458211

-JB

Comment 32

[Mark Finkle (:mfinkle) (use needinfo?)]

I successfully built Fennec with the latest patch using the Chinook SDK. However, when attempting to test using the steps in comment 0, I saw some errors in the console:

fennec[1423]: GLIB CRITICAL ** ConIc - con_ic_connection_disconnect_by_id: assertion 'connection != NULL && id != NULL' failed
fennec[1423]: GLIB DEBUG ConIc - con_ic_connection_send_event(0x40263b20, (null), (null), 1)
fennec[1423]: GLIB DEBUG ConIc - con_ic_connection_send_event(0x40263b20, (null), (null), 1)


The internet connection was not started.

Comment 33

[Doug Turner (:dougt)]

Comment on attachment 398593 [details] [diff] [review]
Patch to add use of connectivity to fennec for maemo

lots of problems. r-

Comment 34

[Doug Turner (:dougt)]

Attachment: conn patch for maemo n900

Comment 35

[Doug Turner (:dougt)]

mobile.js needs to add both:

pref("network.autodial-helper.enabled", true);                                                                                              
pref("network.manage-offline-status", true);

Comment 36

[Doug Turner (:dougt)]

Attachment: conn patch for maemo n900 (v2)

Comment 37

[Doug Turner (:dougt)]

Attachment: conn patch for maemo n900 (v3)

Comment 38

[Doug Turner (:dougt)]

Comment on attachment 406770 [details] [diff] [review]
conn patch for maemo n900 (v3)

the call to CloseConnection() from Shutdown must be removed.  We shouldn't attempt to close the internet connection simply because mozilla exits.  The OS will cleanup our connection per the docs.

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 406770 [details] [diff] [review]
conn patch for maemo n900 (v3)

+++ b/configure.in	Thu Oct 15 11:41:41 2009 -0700
+   PKG_CHECK_MODULES(LIBCONIC,conic)

add a space after the comma

+++ b/netwerk/base/src/Makefile.in	Thu Oct 15 11:41:41 2009 -0700
+  OS_INCLUDES += $(GLIB_CFLAGS)
+  LOCAL_INCLUDES += $(LIBCONIC_CFLAGS)

why not put both CFLAGS into OS_INCLUDES?

+++ b/netwerk/build/nsNetModule.cpp	Thu Oct 15 11:41:41 2009 -0700
+#include "nsMaemoNetworkLinkService.h"
+NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsMaemoNetworkLinkService, Init)

The define is called PLATFORM_HILDON... shouldn't the classes/directories also be called Hildon instead of Maemo then?

+++ b/toolkit/toolkit-tiers.mk	Thu Oct 15 11:41:41 2009 -0700
 ifdef MOZ_ENABLE_DBUS
+ifndef MOZ_PLATFORM_HILDON
 tier_toolkit_dirs    += toolkit/system/dbus

why?

+++ b/netwerk/base/src/nsAutodialMaemo.cpp	Thu Oct 15 15:43:45 2009 -0700

same Maemo/Hildon comment

+++ b/netwerk/base/src/nsAutodialMaemo.h	Thu Oct 15 11:56:06 2009 -0700
+#include "nsCOMPtr.h"

no need for that

+#include "nscore.h"

I don't think you need this either

+public:
+
+    nsAutodial();
+    virtual ~nsAutodial();

remove the empty line. also no need to make the constructor virtual.

+++ b/netwerk/base/src/nsMaemoNetworkManager.cpp	Fri Oct 16 13:43:19 2009 -0700

Same Maemo/Hildon comment, also - shouldn't this file be in netwerk/system?

+ *  Jeremias Bosch <[e-mail]>

You're using a rather inconsistent indentation for his name in the various files :)

+static void connection_event_callback(ConIcConnection *aConnection,
+                                ConIcConnectionEvent *aEvent,
+                                gpointer aUser_data);

incorrect indentation on the last two lines

But it doesn't look like you need the forward declaration anyway

+  nsCOMPtr<nsIIOService> ioService = do_GetService("@mozilla.org/network/io-service;1");

line too long, please break at 80 characters

instead of forcing the io service to go offline, shouldn't you broadcast a NS_NETWORK_LINK_TOPIC notification?

+  if (!gConnectionMutex)
+    return;

this can't happen, right?

why are you using a glib mutex instead of mozilla::Mutex? The latter also provides a MutexAutoLock class which ensures that you can't forget an unlock call.

It would be kinda nice if you could document which thread the various functions are called on. If I got this right:

connection_event_callback - main thread only (via the glib event loop)
nsMaemoNetworkManager::OpenConnectionSync - socket transport thread
- nsMaemoNetworkManager::CloseConnection - main thread
+nsMaemoNetworkManager::IsConnected() - socket transport thread
+nsMaemoNetworkManager::IsConnecting() - not called
+nsMaemoNetworkManager::Startup() - main thread
+nsMaemoNetworkManager::Shutdown() - main thread

is that correct?

+nsMaemoNetworkManager::OpenConnectionSync()
+  if (!con_ic_connection_connect(gConnection, CON_IC_CONNECT_FLAG_NONE))
+    g_error("openConnectionSync: Error while connecting. %p \n", (void*) PR_GetCurrentThread());

line too long. shouldn't you also return false?

+  g_cond_wait(gConnectionCond, gConnectionMutex);

please follow the API docs:
It is important to use the g_cond_wait() and g_cond_timed_wait() functions only inside a loop, which checks for the condition to be true as it is not guaranteed that the waiting thread will find it fulfilled, even if the signaling thread left the condition in that state. This is because another thread can have altered the condition, before the waiting thread got the chance to be woken up, even if the condition itself is protected by a GMutex, like above. 

+nsMaemoNetworkManager::CloseConnection()

why don't you have to lock the mutex here?

+  g_type_init();

shouldn't you also call g_thread_init()?

+  g_signal_connect(G_OBJECT(gConnection),
+		   "connection-event",
+		   G_CALLBACK(connection_event_callback),
+		   nsnull);


incorrect indentation

+nsMaemoNetworkManager::Shutdown()
+{
+  if (!gConnection)
+    return;

Is this possible?

+++ b/netwerk/system/maemo/nsMaemoNetworkLinkService.cpp	Thu Oct 15 15:44:44 2009 -0700
+nsMaemoNetworkLinkService::GetLinkStatusKnown(PRBool *aIsKnown)

isn't this a lie? if internalstate == InternalState_Invalid, you don't know the state.

+  if (!strcmp( aTopic, "xpcom-shutdown" ))

no spaces after or before parentheses

Comment 40

[Doug Turner (:dougt)]

thanks for the review.  I am fixing the problems you found.  What follows are answers and/or questions to some of your concerns:


1) regarding Maemo verses Hildon.  Ideally there would have been a MAEMO #define since libconic is part of that.  However, mozilla has blurred the line between what is hildon and what is maemo.  for us, it is basically the same thing.

2) nscore.h is needed in the header because we use nsresult:

/home/dougt/builds/mobile/mozilla-1.9.2/netwerk/base/src/nsAutodialMaemo.h:51: error: 'nsresult' does not name a type
...

3) shouldn't you also call g_thread_init()?

no idea; not sure I needed to.  everything seems to work without doing this. Since I am going to be converting over to Mozilla:mutex's, maybe this is less of a concern?

4)

+nsMaemoNetworkManager::Shutdown()
+{
+  if (!gConnection)
+    return;

Is this possible?

sure -- if we fail to allocate a mutex, condvar, or if con_ic_connection_new fails.

Comment 41

[Doug Turner (:dougt)]

Attachment: conn patch for maemo n900 (v4)

Comment 42

[Wolfgang Rosenauer [:wolfiR]]

Attachment: conn patch for maemo n900 (alternative build config)

I've copied dougt's v4 patch and changed the build configuration so that libconic detection and usage is independent of MOZ_PLATFORM_HILDON. libconic is autodetected and can be disabled by --disable-libconic.

Comment 43

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #40)
> 3) shouldn't you also call g_thread_init()?
> 
> no idea; not sure I needed to.  everything seems to work without doing this.
> Since I am going to be converting over to Mozilla:mutex's, maybe this is less
> of a concern?

The documentation for g_mutex_lock says that it does nothing if g_thread_init hasn't been called; so if you were still using GMutex, it would be better to make sure that threads are inited :)

Comment 44

[Doug Turner (:dougt)]

the latest patches do not use g_mutex_lock.  instead, i am using mozilla::monitor as you suggested.

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 407484 [details] [diff] [review]
conn patch for maemo n900 (alternative build config)

What was your opinion on my question whether nsMaemoNetworkManager.cpp should be in netwerk/system instead?


+++ b/netwerk/base/src/nsMaemoNetworkManager.cpp
+  observerService->NotifyObservers(nsnull /* link service.  no one uses it. */,

So, that's really not a good reason to pass null here. But the documentation (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsINetworkLinkService.idl#69) doesn't specify what the subject of the notification is, so I'll let this pass...

just keep in mind that there's a chance that this will introduce an incompatibility that might break extensions.

+  if (!con_ic_connection_connect(gConnection,
+				 CON_IC_CONNECT_FLAG_NONE))

incorrect indentation on the second line

+    g_error("openConnectionSync: Error while connecting. %p \n",
+	    (void*) PR_GetCurrentThread());

same for the second line here

repeating my question from above:
+nsMaemoNetworkManager::CloseConnection()

why don't you have to lock the monitor here?


+  gMonitor = new Monitor("MaemoAutodialer");
+  NS_ASSERTION(gMonitor, "Out of memory!");

personally I'd make this function return a bool indicating whether it succeeded

+++ b/netwerk/base/src/nsNativeConnectionHelper.cpp
+  // On mobile platforms, instead of relying on the link service, we
+  // should ask the dialer directly.  This allows the dialer to update
+  // link status more forcefully rather than passively watching link
+  // status changes.

so.. the link status change gets broadcasted slowly on mobile? or why is this needed?

+#if ! defined(MOZ_ENABLE_LIBCONIC) && ! defined(WINCE_WINDOWS_MOBILE)

no spaces after !


sorry, didn't notice this before, but there's no point in specifying LIBS in netwerk/base/src/Makefile.in or netwerk/system/maemo/Makefile.in. As those directories only build static libraries, they never use $(LIBS). netwerk/build/Makefile.in probably needs it, though, at least if you ever want to build non-libxul builds (like, debug builds)

Comment 46

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 407329 [details] [diff] [review]
conn patch for maemo n900 (v4)

(I prefer the other patch)

Comment 47

[Doug Turner (:dougt)]

Attachment: conn patch for maemo n900 (v5)

Comment 48

[Doug Turner (:dougt)]

this patch mostly works.  I say mostly because sometimes I see that pressing on a link, does bring up the connection dialog, creates a new connection, and sets tryAgain to true in the nsSocketTransport2, BUT it doesn't complete the request.  The browser just spins.

christian, any ideas?

Comment 49

[Doug Turner (:dougt)]

Attachment: conn patch for maemo n900 (v5 - trunk)

Comment 50

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 407654 [details] [diff] [review]
conn patch for maemo n900 (v5 - trunk)

dougt, this patch is not in the right place I *think*

Comment 51

[Doug Turner (:dougt)]

Comment on attachment 407654 [details] [diff] [review]
conn patch for maemo n900 (v5 - trunk)

grrr.

Comment 52

[Doug Turner (:dougt)]

Attachment: conn patch for maemo n900 (v5 - trunk)

Comment 53

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 407768 [details] [diff] [review]
conn patch for maemo n900 (v5 - trunk)

+++ b/netwerk/base/src/Makefile.in	Wed Oct 21 11:34:33 2009 -0700
+ifdef MOZ_ENABLE_LIBCONIC
+  OS_INCLUDES += $(GLIB_CFLAGS) $(LIBCONIC_CFLAGS)
+endif

don't think you need this here anymore


I'm confused by the diff:
diff -r df08955601c5 netwerk/system/maemo/nsMaemoNetworkManager.cpp
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/netwerk/base/src/nsMaemoNetworkManager.cpp	Wed Oct 21 13:17:10 2009 -0700

I assume it's really in system/maemo and just displayed weirdly here.

+  // as a service to our customers, we also manage offline support here.
+  // this is due to the fact that managed offline support and autodialers
+  // do not work well together.
+  nsCOMPtr<nsIIOService> ioService = do_GetService("@mozilla.org/network/io-service;1");
+  if (!ioService)
+    return;
+
+  //  ioService->SetOffline(gInternalState != InternalState_Connected);

I'm still skeptical about why this is needed, but one way or another you shouldn't have that line commented out.

why does http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsIOService.cpp#853 not do the right thing?

+  //NotifyNetworkLinkObservers();

why did you comment out this line?

Comment 54

[Jeremias Bosch (:jbos)]

Attachment: This patch enables autodial in fennec (v2)

I updated the fennec patch like Doug mentioned it in #35.

Comment 55

[Jeremias Bosch (:jbos)]

[QUOTE]
diff -r df08955601c5 netwerk/system/maemo/nsMaemoNetworkManager.cpp
--- /dev/null    Thu Jan 01 00:00:00 1970 +0000
+++ b/netwerk/base/src/nsMaemoNetworkManager.cpp    Wed Oct 21 13:17:10 2009
-0700
[/QUOTE]

needs to be:

diff -r df08955601c5 netwerk/system/maemo/nsMaemoNetworkManager.cpp
--- /dev/null    Thu Jan 01 00:00:00 1970 +0000
+++ b/netwerk/system/maemo/nsMaemoNetworkManager.cpp    Wed Oct 21 13:17:10 2009
-0700

otherwise it wont compile.

----
[QUOTE]
+  // as a service to our customers, we also manage offline support here.
+  // this is due to the fact that managed offline support and autodialers
+  // do not work well together.
+  nsCOMPtr<nsIIOService> ioService =
do_GetService("@mozilla.org/network/io-service;1");
+  if (!ioService)
+    return;
+
+  //  ioService->SetOffline(gInternalState != InternalState_Connected);

I'm still skeptical about why this is needed, but one way or another you
shouldn't have that line commented out.
[/QUOTE]

The point here is to set the browser into the offline mode every time when the connection gets lost. There are possibilities where no new connection can be established.  

[QUOTE]
why does
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsIOService.cpp#853
not do the right thing?
[/QUOTE]

Because 'disconnecting' does not cause a NS_NETWORK_LINK_TOPIC. 

One example here: The user does not have a connection established when he starts fennec. In this case Fennec will request a connection through libconic. In some cases now (i.e. no known network available) the user will see a dialog. But in this dialog he have the possibility to cancel it which means there wasn't a connection established and there wont be a NS_NETWORK_LINK_TOPIC. The only information we will get is a disconnected event from libconic, which change our internal state to DISCONNECTED.

So the call is now. "When we are not connected, we are offline."

I hope that clarify that thingy a little.

Comment 56

[Jeremias Bosch (:jbos)]

The patch should not have a review+. 

When you move nsMaemoNetworkManager.cpp to system/maemo you have also to fix the includes in nsAutodialMaemo.cpp to ../system/maemo/nsMaemoNetworkManager.cpp

I didn't want to do that (../system/maemo), thats why i placed nsMaemoNetworkManager.* in base.

Comment 57

[Jeremias Bosch (:jbos)]

Ok actually what I said in #55 was not completely true regarding to the newest patch. I missed the part that you actually use NS_NETWORK_LINK_*, but just removed the call of the complete function. 

So as Christian said, i also think we do not need this anymore this should be done automatically:

+  // as a service to our customers, we also manage offline support here.
+  // this is due to the fact that managed offline support and autodialers
+  // do not work well together.
+  nsCOMPtr<nsIIOService> ioService =
do_GetService("@mozilla.org/network/io-service;1");
+  if (!ioService)
+    return;
+
+  //  ioService->SetOffline(gInternalState != InternalState_Connected);

Comment 58

[Jeremias Bosch (:jbos)]

Attachment: conn patch for maemo n900 (v6 - trunk)

(changes) 
- Enables NotifyNetworkLinkObservers again. 
- Fix includes
- Change path of nsMaemoNetworkManager to netwerk/system/maemo

Comment 59

[Doug Turner (:dougt)]

@jeremias - I applied your patch and i can not establish a connection.  I think this is because calling NotifyNetworkLinkObservers() will continue to put us into an offline state.

Comment 60

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 407991 [details] [diff] [review]
 conn patch for maemo n900 (v6 - trunk)

you shouldn't include files using relative paths, you should add the file to an EXPORTS= directive in the makefile instead.

Comment 61

[Doug Turner (:dougt)]

christian, please ignore the patches from jeremias; they are not working for me.  I will put something up that works and doesn't have that mistake of using relative patch #includes.  Also, we don't want to export that file, we can use LOCAL_INCLUDES like other makefiles do.  (no one outside of necko needs this autodialer file)

Comment 62

[Jeremias Bosch (:jbos)]

@Doug, ok I'm back on Diablo where the patch was original created for. (It should definitely be compatible.) 
Well, I need to check what has changed in detail but unfortunately it seems totally broken function-wise. It never switch into the offline state, sometimes it keeps loading for ever, it mostly (80%) does not request a new connection when the connection was lost (esp. when there was no connection at startup) or fennec keeps asking for a new connection and the user can't cancel it.

Comment 63

[Jeremias Bosch (:jbos)]

Attachment: conn patch for maemo n900 (v6 - trunk)

This patch is based on a pre-version doug send to me. I fixed it finally up and tested it successfully on a Diablo Device.

The cause for the most problems i saw before was, that in linux is in /toolkit/system/dbus a networkmanager. This one "overwrite" our libconic networkmanger in compreg.dat since maemo is a linux.  
For more information check Bug 312793. And before you may ask, we do not want to use plain dbus messages here, since libconic might changed the dbus messages internally in different versions within the past and upcoming future. And off course they provide us with a nice and stable interface. 

Doug and I come also to the point that setting the browser to offline as soon as the device loose its connection and setting it back online as soon as the connection returns is the most direct and best way to handle the offline mode.

You also want to use the patches within the Bugs 524937 and 523296, they will provide you the ability to return back to online mode within the offline mode "error page", by clicking try again.

Comment 64

[Christian :Biesinger (don't email me, ping me on IRC)]

OK, so why should Maemo be the only platform that doesn't use NS_NETWORK_LINK_STATUS_TOPIC for the online/offline mode?

Comment 65

[Doug Turner (:dougt)]

NS_NETWORK_LINK_STATUS_TOPIC stuff is just for managing offline status.  The patch explictly sets it offline status and we don't set the pref that tells necko to monitor these notifications.

Comment 66

[Doug Turner (:dougt)]

the last patch continues to not work on the n900.  further investigation is required.

Comment 67

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #65)
> NS_NETWORK_LINK_STATUS_TOPIC stuff is just for managing offline status.  The
> patch explictly sets it offline status and we don't set the pref that tells
> necko to monitor these notifications.

Well yeah, that was obvious from the patch. But why do it that way?

Comment 68

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #66)
> the last patch continues to not work on the n900.  further investigation is
> required.

It also doesn't seem to work on the n810

Comment 69

[Jeremias Bosch (:jbos)]

Hmm, at least for the n810 i'm sure it does work! Did you apply both patches? The One for xulrunner and the One for fennec? Do you Build them in à chinook or à diablo Environment?

You might also need to reset compreg.dat in your profile.

Comment 70

[Brad Lassey [:blassey] (use needinfo?)]

no, I missed the fennec patch.  Thanks for pointing that out.

Comment 71

[Jeremias Bosch (:jbos)]

@brad ok, so it works now, you should consider to take the offline page patches to get the best results.

@christian
The issues we saw before:
 - keeps loading and nerver open a page
 - no offline mode page displayed
 - continues asking for a connection
are caused by the NS_NETWORK_LINK_STATUS_TOPIC stuff.

In the code of WinCE you also wont find the NS_NETWORK_LINK_STATUS_TOPIC stuff, they actually not useing the offline-mode at all.

Comment 72

[Jeremias Bosch (:jbos)]

I tried the patch on a N900 and actually it does work fine! The only thing you will face is a grey page because of this Bug 520000 . The workaround is just to press reload in fennec.

Comment 73

[Juha Seppänen]

Any chance to get this landed? Anything pending apart from 520000?

Comment 74

[Oleg Romashin (:romaxa)]

Would it be possible also re-read Maemo proxy settings from GConf when connection is changed?

Comment 75

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #71)
> The issues we saw before:
>  - keeps loading and nerver open a page
>  - no offline mode page displayed
>  - continues asking for a connection
> are caused by the NS_NETWORK_LINK_STATUS_TOPIC stuff.

So how does this notification cause those issues? Basically it seems to me that using the topic is the right way to go about this, it's what all the desktop platforms use and it's not at all clear to me why mobile should be different.

Comment 76

[Wolfgang Rosenauer [:wolfiR]]

Ok, a short update:
This patch doesn't have a nice user experience because it relies on other things to be changed (for example bug 520000 which makes the patch looking like it wouldn't work and bug 523296 which would be needed to be able to leave offline mode at all again from the browser side).

Technically it apparently works though.

To 'fix' the user experience the proposal would be to get rid of the error page by triggering libconic explicitely to "dial out" if we would hit the error. So libconic and its UI would basically offer the "error handling" according to its configuration. If the user (or configuration) decides to stay online we want to keep the last displayed page also instead of showing an error which can only be left by clicking BACK.

Comment 77

[Mark Finkle (:mfinkle) (use needinfo?)]

Christian - After talking with all parties, we are ready for you to review attachment 408833 [details] [diff] [review]. It's not the ideal behavior, but it is good enough to Fennec 1.0 and we will be filing followup bugs to improve the behavior for future releases.

The patch does require additional front-end work for Fennec and we will be working on that in bug 523296.

Comment 78

[Mark Finkle (:mfinkle) (use needinfo?)]

Just to clarify. We will be filing a followup bug(s) to improve the connection behavior. We should not ever show the offline error page, unless the autodial fails to find a connection.

Comment 79

[Mark Finkle (:mfinkle) (use needinfo?)]

Also, we should not be touching IOService::SetOffline directly. We should be using the notifications only.

Comment 80

[timeless]

Comment on attachment 408833 [details] [diff] [review]
conn patch for maemo n900 (v6 - trunk)   

>diff -r c600af9cdd05 netwerk/base/src/Makefile.in
>@@ -99,6 +99,11 @@ ifeq ($(MOZ_WIDGET_TOOLKIT),os2)
>+ifdef MOZ_ENABLE_LIBCONIC
>+    CPPSRCS += nsAutodialMaemo.cpp
>+    CPPSRCS += nsNativeConnectionHelper.cpp
>+    LOCAL_INCLUDES += -I$(srcdir)/../../system/maemo
>+endif

>+ifdef MOZ_ENABLE_LIBCONIC
>+  OS_INCLUDES += $(GLIB_CFLAGS) $(LIBCONIC_CFLAGS)
>+endif

indentation seems quasi random

>+            // On Windows and Maemo (libconic) we should first check from the OS

from => with
>+            // if autodial is enabled.  If it is enabled then we are allowed

if => to see if

>@@ -1156,6 +1159,12 @@ static const nsModuleComponentInfo gNetM
>       NS_NETWORK_LINK_SERVICE_CONTRACTID,
>       nsNetworkLinkServiceConstructor
>     },
>+#elif defined(MOZ_ENABLE_LIBCONIC)
>+    { NS_NETWORK_LINK_SERVICE_CLASSNAME,
>+      NS_NETWORK_LINK_SERVICE_CID,
>+      NS_NETWORK_LINK_SERVICE_CONTRACTID,
>+      nsMaemoNetworkLinkServiceConstructor

This is wrong. a CID is supposed to be tied to a single implementation.

>+    // Get the autodial info from the OS and init this object with it. Call it any
>+    // time to refresh the object's settings from the OS.

That this isn't a public (.idl) method is odd given the "call it any time" statement.
>+    nsresult Init();

>+nsMaemoNetworkLinkService::Init(void)
>+{
>+  nsresult rv;
>+  

trailing whitespace

>+static void NotifyNetworkLinkObservers()
>+{
>+  nsCOMPtr<nsIIOService> ioService = do_GetService("@mozilla.org/network/io-service;1");
>+  if (!ioService)
>+    return;
>+  
>+    ioService->SetOffline(gInternalState != InternalState_Connected);

overidented

>+    if (CON_IC_STATUS_CONNECTED == status)
>+      gInternalState = InternalState_Connected;
>+    else //when we are not connected, we are always disconnected
>+      gInternalState = InternalState_Disconnected;

use ?: notation?


>+  gMonitor = new Monitor("MaemoAutodialer");
>+  NS_ASSERTION(gMonitor, "Out of memory!");

bogus assert, lose it.

>+  if (!gMonitor)
>+    return PR_FALSE;

>+      // notify anyone waiting    

trailing whitespace

Comment 81

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 408833 [details] [diff] [review]
conn patch for maemo n900 (v6 - trunk)   

Please also fix timeless's comments.

+++ b/netwerk/base/src/nsAutodialMaemo.h	Tue Oct 27 09:20:18 2009 -0700
+    // Get the autodial info from the OS and init this object with it. Call it any
+    // time to refresh the object's settings from the OS.
+    nsresult Init();

That comment seems wrong, since the method doesn't do anything.

+++ b/netwerk/system/maemo/Makefile.in	Tue Oct 27 09:21:12 2009 -0700
+CPPSRCS += nsMaemoNetworkLinkService.cpp nsMaemoNetworkManager.cpp

I think normal formatting would be:
CPPSRCS += \
           nsMaemoNetworkLinkService.cpp \
           nsMaemoNetworkManager.cpp \
           $(NULL)

+OS_INCLUDES += $(GLIB_CFLAGS)
+LOCAL_INCLUDES += $(LIBCONIC_CFLAGS) -I$(srcdir)/../../base/src

As asked in comment 39, why not put both _CFLAGS in OS_INCLUDES?

+++ b/netwerk/system/maemo/nsMaemoNetworkManager.cpp	Tue Oct 27 11:58:38 2009 -0700
+      // notify anyone waiting    
+      MonitorAutoEnter mon(*gMonitor);
+      gInternalState = InternalState_Invalid;    
+      mon.Notify();
+    }
+
+    delete gMonitor;

But by the time you delete gMonitor, there could still be another thread waiting on the monitor. NSPR doesn't allow destroying monitors that are still potentially in use.

Alternatively, if you are sure that there are no threads waiting on the monitor here, there's no point in calling Notify.

Comment 82

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #80)

> >@@ -1156,6 +1159,12 @@ static const nsModuleComponentInfo gNetM
> >       NS_NETWORK_LINK_SERVICE_CONTRACTID,
> >       nsNetworkLinkServiceConstructor
> >     },
> >+#elif defined(MOZ_ENABLE_LIBCONIC)
> >+    { NS_NETWORK_LINK_SERVICE_CLASSNAME,
> >+      NS_NETWORK_LINK_SERVICE_CID,
> >+      NS_NETWORK_LINK_SERVICE_CONTRACTID,
> >+      nsMaemoNetworkLinkServiceConstructor
> 
> This is wrong. a CID is supposed to be tied to a single implementation.
> 

I made all changes except this one. Windows, Linux and now Maemo all use the same NS_NETWORK_LINK_SERVICE_XXX for their respective implementations. Since all are mutually exclusive, it works for now. I can file a bug to fix this up in general.

Comment 83

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #81)
> (From update of attachment 408833 [details] [diff] [review])
> Please also fix timeless's comments.

Done as noted above

> +    // Get the autodial info from the OS and init this object with it. Call it
> any
> +    // time to refresh the object's settings from the OS.
> +    nsresult Init();
> 
> That comment seems wrong, since the method doesn't do anything.

Removed it

> +CPPSRCS += nsMaemoNetworkLinkService.cpp nsMaemoNetworkManager.cpp
> 
> I think normal formatting would be:
> CPPSRCS += \
>            nsMaemoNetworkLinkService.cpp \
>            nsMaemoNetworkManager.cpp \
>            $(NULL)

Changed

> +OS_INCLUDES += $(GLIB_CFLAGS)
> +LOCAL_INCLUDES += $(LIBCONIC_CFLAGS) -I$(srcdir)/../../base/src
> 
> As asked in comment 39, why not put both _CFLAGS in OS_INCLUDES?

No idea. I put LIBCONIC_CFLAGS in OS_INCLUDES

> +      // notify anyone waiting    
> +      MonitorAutoEnter mon(*gMonitor);
> +      gInternalState = InternalState_Invalid;    
> +      mon.Notify();
> +    }
> +
> +    delete gMonitor;
> 
> But by the time you delete gMonitor, there could still be another thread
> waiting on the monitor. NSPR doesn't allow destroying monitors that are still
> potentially in use.
> 
> Alternatively, if you are sure that there are no threads waiting on the monitor
> here, there's no point in calling Notify.

After talking on IRC about this, we decided to leak the monitor for now. I added a comment in the code making it explicit about why we did it.

Comment 84

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch for landing

Patch with comments addressed

Comment 85

[Mark Finkle (:mfinkle) (use needinfo?)]

Patch didn't break anything on try server and my build was functional in a local Fennec Maemo build.

Comment 86

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/784cd37c4665

pushed to m-192:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/eff5f946faf4