Internet connection should be initiated if needed

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Networking
--
enhancement
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Claudius Henrichs, Assigned: mfinkle)

Tracking

Trunk
mozilla1.9.3a1
Other
Linux
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
wanted-fennec1.0 ?

Firefox Tracking Flags

(status1.9.2 beta4-fixed, fennec1.0b3+)

Details

Attachments

(4 attachments, 15 obsolete attachments)

26.70 KB, patch
Biesinger
: review+
Details | Diff | Splinter Review
527 bytes, patch
Details | Diff | Splinter Review
26.53 KB, patch
Biesinger
: review+
Details | Diff | Splinter Review
30.34 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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
Code from maemo microb that does this?

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

Updated

9 years ago
tracking-fennec: --- → ?
tracking-fennec: ? → 1.0b3+
Created attachment 387156 [details] [diff] [review]
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.
Attachment #387156 - Flags: review?(mark.finkle)

Updated

9 years ago
Attachment #387156 - Flags: review?(mark.finkle) → review+

Updated

9 years ago
Attachment #387156 - Flags: review+ → review?(mark.finkle)

Comment 3

9 years ago
Patch works for me, but I am not able to set review+ here and have it stick:-)
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

9 years ago
Attachment #387156 - Flags: review?(mark.finkle) → review-
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.
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.

---
Created attachment 387449 [details] [diff] [review]
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!
Attachment #387156 - Attachment is obsolete: true
Attachment #387449 - Flags: review?
Created attachment 387451 [details] [diff] [review]
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.
Attachment #387451 - Flags: review?
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.
(Assignee)

Updated

9 years ago
Attachment #387449 - Flags: review? → review?(doug.turner)
(Assignee)

Updated

9 years ago
Attachment #387451 - Flags: review? → review+
Created attachment 387455 [details] [diff] [review]
Patch to add use of connectivity to fennec for maemo

Thanks for the + Mark. Changed the things from comment #9.
Attachment #387449 - Attachment is obsolete: true
Attachment #387455 - Flags: review?(doug.turner)
Attachment #387449 - Flags: review?(doug.turner)
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?
Attachment #387455 - Flags: review?(ted.mielczarek)
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.
Attachment #387455 - Flags: review?(doug.turner) → review-
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.
Attachment #387455 - Flags: review?(ted.mielczarek) → review+
Created attachment 387621 [details] [diff] [review]
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
Attachment #387455 - Attachment is obsolete: true
Attachment #387621 - Flags: review?(doug.turner)

Updated

9 years ago
Blocks: 458211

Updated

9 years ago
Blocks: 401821
ping
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

9 years ago
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.
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!
Created attachment 388430 [details] [diff] [review]
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.
Attachment #387621 - Attachment is obsolete: true
Attachment #388430 - Flags: review?(doug.turner)
Attachment #387621 - Flags: review?(doug.turner)
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
Attachment #388430 - Flags: review?(doug.turner) → review-
Duplicate of this bug: 501891

Updated

9 years ago
Assignee: nobody → jeremias.bosch
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.
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 .
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
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.
(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 (?)
> 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.
vote for not blocking1.0

Comment 29

8 years ago
why can we not just dlopen() some functions here and get this landed?
Created attachment 398593 [details] [diff] [review]
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
Attachment #388430 - Attachment is obsolete: true
Attachment #398593 - Flags: review?(doug.turner)
Sorry i meant Bug 458211

-JB

Updated

8 years ago
Flags: wanted-fennec1.0?
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 on attachment 398593 [details] [diff] [review]
Patch to add use of connectivity to fennec for maemo

lots of problems. r-
Attachment #398593 - Flags: review?(doug.turner) → review-
Created attachment 406556 [details] [diff] [review]
conn patch for maemo n900
Assignee: jeremias.bosch → doug.turner
Attachment #387451 - Attachment is obsolete: true
Attachment #398593 - Attachment is obsolete: true
Attachment #406556 - Flags: review?
mobile.js needs to add both:

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

Updated

8 years ago
Attachment #406556 - Flags: review? → review?(cbiesinger)
Created attachment 406753 [details] [diff] [review]
conn patch for maemo n900 (v2)
Attachment #406556 - Attachment is obsolete: true
Attachment #406753 - Flags: review?
Attachment #406556 - Flags: review?(cbiesinger)
Created attachment 406770 [details] [diff] [review]
conn patch for maemo n900 (v3)
Attachment #406753 - Attachment is obsolete: true
Attachment #406753 - Flags: review?

Updated

8 years ago
Attachment #406770 - Flags: review?(cbiesinger)
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 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 <jeremias.bosch@gmail.com>

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
Attachment #406770 - Flags: review?(cbiesinger) → review-
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.
Created attachment 407329 [details] [diff] [review]
conn patch for maemo n900 (v4)
Attachment #406770 - Attachment is obsolete: true
Attachment #407329 - Flags: review?

Updated

8 years ago
Attachment #407329 - Flags: review? → review?(cbiesinger)
Created attachment 407484 [details] [diff] [review]
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.
(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 :)
the latest patches do not use g_mutex_lock.  instead, i am using mozilla::monitor as you suggested.
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)
Attachment #407329 - Flags: review?(cbiesinger)
Comment on attachment 407329 [details] [diff] [review]
conn patch for maemo n900 (v4)

(I prefer the other patch)
Created attachment 407597 [details] [diff] [review]
conn patch for maemo n900 (v5)
Attachment #407329 - Attachment is obsolete: true
Attachment #407484 - Attachment is obsolete: true
Attachment #407597 - Flags: review?
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?
Created attachment 407654 [details] [diff] [review]
conn patch for maemo n900 (v5 - trunk)
Attachment #407597 - Attachment is obsolete: true
Attachment #407597 - Flags: review?
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*

Updated

8 years ago
Attachment #407654 - Attachment is obsolete: true
Comment on attachment 407654 [details] [diff] [review]
conn patch for maemo n900 (v5 - trunk)

grrr.
Created attachment 407768 [details] [diff] [review]
conn patch for maemo n900 (v5 - trunk)
Attachment #407768 - Flags: review?(cbiesinger)
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?
Attachment #407768 - Flags: review?(cbiesinger) → review+
Created attachment 407957 [details] [diff] [review]
This patch enables autodial in fennec (v2)

I updated the fennec patch like Doug mentioned it in #35.
Attachment #407957 - Flags: review?(doug.turner)
[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.
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.
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);
Created attachment 407991 [details] [diff] [review]
 conn patch for maemo n900 (v6 - trunk)

(changes) 
- Enables NotifyNetworkLinkObservers again. 
- Fix includes
- Change path of nsMaemoNetworkManager to netwerk/system/maemo
@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 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.
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)
@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.
Created attachment 408833 [details] [diff] [review]
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.
Attachment #407991 - Attachment is obsolete: true
Attachment #408833 - Flags: review?(cbiesinger)
OK, so why should Maemo be the only platform that doesn't use NS_NETWORK_LINK_STATUS_TOPIC for the online/offline mode?
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.
the last patch continues to not work on the n900.  further investigation is required.
(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?
(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
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.
no, I missed the fennec patch.  Thanks for pointing that out.
@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.
Depends on: 520000
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

8 years ago
Any chance to get this landed? Anything pending apart from 520000?
Would it be possible also re-read Maemo proxy settings from GConf when connection is changed?
(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.
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.
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.
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.
Also, we should not be touching IOService::SetOffline directly. We should be using the notifications only.

Comment 80

8 years ago
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 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.
Attachment #408833 - Flags: review?(cbiesinger) → review+
(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.
(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.
Created attachment 413027 [details] [diff] [review]
patch for landing

Patch with comments addressed
Assignee: mozbugz → mark.finkle
Patch didn't break anything on try server and my build was functional in a local Fennec Maemo build.
(Assignee)

Updated

8 years ago
Attachment #413027 - Flags: approval1.9.2?
(Assignee)

Updated

8 years ago
Component: Linux/Maemo → Networking
Product: Fennec → Core
QA Contact: maemo-linux → networking
Flags: blocking1.9.2+
(Assignee)

Updated

8 years ago
Attachment #413027 - Flags: approval1.9.2?
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
Status: NEW → RESOLVED
Last Resolved: 8 years ago
status1.9.2: --- → final-fixed
Resolution: --- → FIXED

Updated

8 years ago
Attachment #407957 - Flags: review?(mozbugz)
Depends on: 532072
Depends on: 532078
Depends on: 536031
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.