104020 - Need to send out notification when SetOffline has completed

Status: VERIFIED FIXED

(Core :: Networking, defect, P2)

Description

[Conrad Carlen (not reading bugmail)]

This came from bug 101329.

When shutting down the current profile, we need to know when all network
activity  has indeed been stopped. Currently, necko notifies observers with the
"network:offline-status-changed" topic as it begins(?) to go offline. What's
needed is another notification, say "network:offline-complete" when necko knows
that all transactions have really stopped.

If we need two stages of notification, maybe the topic for Observe() should be
"network:offline-status" in both cases and either "beginning" or "completed" as
the someData param.

Comment 1

[Darin Fisher]

-> 0.9.6

Comment 2

[Darin Fisher]

Attachment: v1.0 patch

Comment 3

[Darin Fisher]

the patch for this was simpler then i thought.  shutting down the DNS and socket
transport services happens synchronously (and without blocking for any real
length of time).  this means that we can solve this bug by simply delaying
offline notification until after the services are shutdown.

Comment 4

[Darin Fisher]

cc'ing gagan and dougt for reviews.

Comment 5

[Conrad Carlen (not reading bugmail)]

  That makes things simpler. If SetOffline() is truly synchronous, the profile
mgr does not have to register as a listener to be notified when the shutdown
happens - it can just call Offline(PR_TRUE) as the first step of a profile switch.
  Better than that, IOService could register as a profile change observer and do
this itself. The advantage is that then we don't need a do_GetService() call in
the profile mgr which may create an instance of IOService before really nescesary.

If this sounds like the way to go, I'll make a patch for this and bug 104021.

Comment 6

[Conrad Carlen (not reading bugmail)]

Attachment: previous patch + goes offline automatically

Comment 7

[Conrad Carlen (not reading bugmail)]

For 2nd patch, see attachment 56684 [details] [diff] [review] from bug 104021.

Comment 8

[Darin Fisher]

conrad: nsSocketTransportService::Shutdown calls Join on the worker thread, so
it cannot return until the worker thread terminates, and without a worker thread
there can't be any more "async" socket activity.  so, i'm fairly certain that
this is the right thing.  i'll try out your patch tomorrow.

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 56661 [details] [diff] [review]
v1.0 patch

We may have to add a new notification if clients want  to know about going offline prior to sts/dns shutdown.

Comment 10

[Darin Fisher]

dougt: but the observer notification would be asynchronous.  thus, by the time
the observers received notification, we'd likely already be offline.  so, i
don't see the point of sending out a "now we're going offline" notification.

Comment 11

[Darin Fisher]

Comment on attachment 56682 [details] [diff] [review]
previous patch + goes offline automatically

>Index: nsIOService.cpp

> nsIOService::nsIOService()
>-    : mOffline(PR_FALSE)
>+    : mOffline(PR_FALSE),
>+      mProfileChangeWentOffline(PR_FALSE)

how about mOfflineForProfileChange instead?


>-
>+    
>+    // Register for profile change notifications
>+    nsCOMPtr<nsIObserverService> observerService =
>+        do_GetService("@mozilla.org/observer-service;1", &rv);

why capture rv if it's not going to be used?


otherwise, sr=darin

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 56682 [details] [diff] [review]
previous patch + goes offline automatically

make to two bools:

PRBool mOffline;
PRBool mProfileChangeWentOffline;


PRBoolPacked.  then r=me

Comment 13

[Darin Fisher]

-> ccarlen for checkin

Comment 14

[Conrad Carlen (not reading bugmail)]

Thanks. I'll make the suggested changes. While we're at it, can you guys review
the companion bug 104021. This won't have any effect without that one.

Comment 15

[Conrad Carlen (not reading bugmail)]

Checked in - with reviewers suggestions.

Comment 16

[David :Bienvenu]

This breaks mail/news - I aleady fixed this once - we need to get notified
before we go offline so we can a bit of processing to clean up our connections.
Now, I crash going offline if I have more than one imap folder open.

Comment 17

[David :Bienvenu]

The crash is because the sockettransport is holding onto a deleted lock. The
crash is easy to reproduce - just open two imap folders and then go offline. I
believe this change also is responsible for
http://bugzilla.mozilla.org/show_bug.cgi?id=119592

So I think we need some more general solution to this problem.

Comment 18

[David :Bienvenu]

Here's the crash stack trace:
PR_Lock(PRLock * 0xfeeefeee) line 235 + 3 bytes
nsAutoLock::nsAutoLock(PRLock * 0xfeeefeee) line 150 + 13 bytes
nsSocketBS::GetTransport(nsSocketTransport * * 0x0012c0d4) line 2142
nsSocketBS::ReleaseSocket(PRFileDesc * 0x048ad590) line 2125
nsSocketBOS::Write(nsSocketBOS * const 0x04674b04, const char * 0x048af490,
unsigned int 0x00000009, unsigned int * 0x0012c158) line 2344
nsImapProtocol::TellThreadToDie(nsImapProtocol * const 0x030181b8, int
0x00000001) line 883 + 50 bytes
nsImapIncomingServer::CloseCachedConnections(nsImapIncomingServer * const
0x04056c38) line 986
nsMsgAccountManager::closeCachedConnections(nsHashKey * 0x04057570, void *
0x04056c38, void * 0x00000000) line 1103
_hashEnumerate(PLHashEntry * 0x040575c8, int 0x00000005, void * 0x0012c214) line
198 + 26 bytes
PL_HashTableEnumerateEntries(PLHashTable * 0x02f354e8, int (PLHashEntry *, int,
void *)* 0x10029620 _hashEnumerate(PLHashEntry *, int, void *), void *
0x0012c214) line 429 + 15 bytes
nsHashtable::Enumerate(int (nsHashKey *, void *, void *)* 0x037b8c00
nsMsgAccountManager::closeCachedConnections(nsHashKey *, void *, void *), void *
0x00000000) line 361 + 21 bytes
nsMsgAccountManager::CloseCachedConnections(nsMsgAccountManager * const
0x02f35488) line 1465
nsMsgAccountManager::Observe(nsMsgAccountManager * const 0x02f3548c, nsISupports
* 0x01849a58, const char * 0x01bc2e2c, const unsigned short * 0x01bc2e1c) line 254
nsObserverService::NotifyObservers(nsObserverService * const 0x0144b190,
nsISupports * 0x01849a58, const char * 0x01bc2e2c, const unsigned short *
0x01bc2e1c) line 213
nsIOService::SetOffline(nsIOService * const 0x01849a58, int 0x00000001) line 844
+ 63 bytes
nsMsgOfflineManager::SetOnlineState(int 0x00000000) line 367 + 33 bytes
nsMsgOfflineManager::SynchronizeForOffline(nsMsgOfflineManager * const
0x04884f18, int 0x00000000, int 0x00000000, int 0x00000000, int 0x00000001,
nsIMsgWindow * 0x0404b788) line 354 + 10 bytes
XPTC_InvokeByIndex(nsISupports * 0x04884f18, unsigned int 0x00000008, unsigned
int 0x00000005, nsXPTCVariant * 0x0012c63c) line 106

Comment 19

[Conrad Carlen (not reading bugmail)]

So, the problem is the one brought up in comment 9 and thought not to be a
problem in comment 10. Since it is a problem, would mailnews be happy if a
notification was sent out before going offline in addition to after?

Comment 20

[David :Bienvenu]

Conrad, yes, that was the way it worked, and we were fine with that :-)

Comment 21

[Conrad Carlen (not reading bugmail)]

We need both the "before" and the "after" notification. mailnews can observe the
"before." PSM will observe the "after."

Comment 22

[David :Bienvenu]

Attachment: proposed fix

I added a new notification about-to-go-offline. I'll attach a patch to the
mailnews code that listens for this notification.

Comment 23

[David :Bienvenu]

Attachment: mailnews part of proposed fix

change mailnews code to listen for new notification.

Comment 24

[David :Bienvenu]

can I get an r and sr from Conrad and Darin, thx!

Comment 25

[Conrad Carlen (not reading bugmail)]

Doh! I was just about to post a similar patch! (CVS chugging away) Thanks for
making a patch but, if you were going to do that, you should have taken the bug
and I wouldn't have wasted time making the same patch.

Comment 26

[David :Bienvenu]

sorry, it just took a minute and I did it on an impulse :-)

Comment 27

[(not reading, please use [e-mail] instead)]

Comment on attachment 73054 [details] [diff] [review]
mailnews part of proposed fix

sr=sspitzer

Comment 28

[(not reading, please use [e-mail] instead)]

Comment on attachment 73053 [details] [diff] [review]
proposed fix

r=sspitzer

Comment 29

[(not reading, please use [e-mail] instead)]

Comment on attachment 73053 [details] [diff] [review]
proposed fix

make that sr=sspitzer, I'll leave the r= to ccarlen.

Comment 30

[Conrad Carlen (not reading bugmail)]

Comment on attachment 73053 [details] [diff] [review]
proposed fix



>+        // this allows users to attempt a little cleanup before dns and socket transport are shut down.
>+            (void)observerService->NotifyObservers(NS_STATIC_CAST(nsIIOService *, this),
>+                                                   "network:offline-about-to-go-offline",
>+                                                   NS_LITERAL_STRING("offline").get());

One little thing: now there's 2 occurances of NS_LITERAL_STRING("offline") in
this block. Use 1 NS_NAMED_LITERAL_STRING(offlineString, "offline") That macro
actually generates code on some platforms.
With that, r=ccarlen

>         // be sure to try and shutdown both (even if the first fails)
>         if (mDNSService)
>             rv1 = mDNSService->Shutdown();  // shutdown dns service first, because it has callbacks for socket transport

Comment 31

[David :Bienvenu]

Attachment: patch to nsIOService using NS_NAMED_LITERAL_STRING

Cool, I didn't know about that - yet another thing I can torture people with in
sr's! :-)

Comment 32

[Conrad Carlen (not reading bugmail)]

Comment on attachment 73069 [details] [diff] [review]
patch to nsIOService using NS_NAMED_LITERAL_STRING

r=ccarlen

Comment 33

[Conrad Carlen (not reading bugmail)]

Comment on attachment 73054 [details] [diff] [review]
mailnews part of proposed fix

r=ccarlen

Comment 34

[(not reading, please use [e-mail] instead)]

Comment on attachment 73069 [details] [diff] [review]
patch to nsIOService using NS_NAMED_LITERAL_STRING

sr=sspitzer

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 73054 [details] [diff] [review]
mailnews part of proposed fix

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 36

[Asa Dotzler [:asa]]

Comment on attachment 73069 [details] [diff] [review]
patch to nsIOService using NS_NAMED_LITERAL_STRING

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 37

[Conrad Carlen (not reading bugmail)]

-> bienvenu

Comment 38

[David :Bienvenu]

fix checked in - I'm changing qa contact to stephend since he's seen the crash
described here in comment 18
http://bugzilla.mozilla.org/show_bug.cgi?id=104020#c18 and can tell if it's fixed. 

Comment 39

[[e-mail] (gone - use [e-mail] instead)]

I tried the testcase in http://bugzilla.mozilla.org/show_bug.cgi?id=104020#c17
without any problem.

Builds:

Mac OS X 10.1.3 - 2002-03-14-08
RedHat 7.2 - 2002-03-14-08
Windows 2000 - 2002-03-14-03

Also, I queried talkback and got:

http://climate/reports/searchstacksignature.cfm?stacksig=PR_Lock

 2002021308   MozillaTrunk   Linux 2.4.9-21

Verified FIXED (talkback's output isn't particularly useful for this bug, but I
included it nonetheless).