(not reading, please use seth@sspitzer.org instead) Reporter
	Description • 22 years ago

if you use turbo, and you launch mail, and you use biff, the biff timer 
continues to fire after we've exited.

see http://bugscape.mcom.com/show_bug.cgi?id=15485 for how we found this.

I'm not sure if it is worth it to fix this at this point.  stopping the timer 
isn't the big issue (since it fires at most once a minute, and does little 
work).  the big issue is that the biff service stays loaded, and the account 
manager is loaded, which we probably want for single profile turbo.

we should think hard before fixing this.  see 
http://bugscape.mcom.com/show_bug.cgi?id=15485, read all the comments.

	(not reading, please use seth@sspitzer.org instead) Reporter
	Comment 1 • 22 years ago

the most interesting thing would be to see if there are other, faster firing 
timers going off after exit.  (like bookmarks timers, etc)

if we had a bunch going off when we were "exited", and they were doing a lot of 
work, that might be bad.

	Syd Logan
	Comment 2 • 22 years ago

I was thinking we might survey how many classes create timers and *don't* handle
the session-logout notification.

	Peter Trudelle
	Comment 3 • 22 years ago

cc law & morse.  we should certainly check this.

	Navin Gupta
	Comment 4 • 22 years ago

The fix is to kill the timer on session-logout (suggested in bugscape 15485)
notification and to recreate the timer if it is not already created when we
start-up again (happens only on turbo exit). We don't force creation of timer,
a new timer will be created only if we have biff entries.

	Navin Gupta
	Comment 5 • 22 years ago

Cavin, can I get r=?, thx

	Cavin Song
	Comment 6 • 22 years ago

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

r=cavin.

	Navin Gupta
	Comment 7 • 22 years ago

David, can you sr ? thx. 

	timeless
	Comment 8 • 22 years ago

i'm told he's on vacation, might i suggest you ask someone else?

	David :Bienvenu
	Comment 9 • 22 years ago

clearing the timer makes sense, but why do we need to recreate it? Shouldn't
that happen normally when the profile is loaded? If it's not, that makes me
suspect we're not cleaning up enough when we exit from turbo.

	Navin Gupta
	Comment 10 • 22 years ago

biffManager is a service (stays in memory) and we never unload any accounts on
turbo exit. Also we are starting back up the same profile, so there is no
profile-change. All the servers are the same (there is no unloading/loading of
server as expected), biff entries are the same. 

	Stephen P. Morse
	Comment 11 • 22 years ago

Opened bug 144559 to address the issue in comment 2 of this report.

	David :Bienvenu
	Comment 12 • 22 years ago

I wonder if we should unload accounts when we exit in turbo mode...it might fix
a lot of other problems - is there a notification for exiting without switching
profiles that we could use to unload accounts?

	Navin Gupta
	Comment 13 • 22 years ago

Why would we want to do that. We would have to first unload and then load the
same things. It will be an expensive no-op. 

	David :Bienvenu
	Comment 14 • 22 years ago

Because it makes the code much more complicated, and leaves us in this twilight
state, where we're half shutdown, and half not shutdown. So, for example, we
shut down all active connections to the servers, shut down our biff timer, etc,
but leave accounts loaded. So now we need to have more code to handle to do the
half of startup that we've shutdown when we half shutdown...in general, it just
adds to code complexity. Loading accounts is a  small part of our startup time,
so I'm not sure I'd categorize it as an expensive no-op. For example, this patch
might just be one line (adding an unloadAccounts call in session-logout).
Unloading Accounts will shutdown the biff, and it will get started up normally
when we reload the profile. If we think it's worth the added code complexity,
then we can go with this patch.

	David :Bienvenu
	Comment 15 • 22 years ago

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

sr=bienvenu, if we think the code complexity vs. startup time costs are worth
it.

	Navin Gupta
	Comment 16 • 22 years ago

Unloading accounts will not shutdown the biff timer. you will have to explicitly
call shutdown on biffManager to kill the timer. Are you hinting at calling
nsMsgAccountManager::Shutdown on turbo exit. 

	David :Bienvenu
	Comment 17 • 22 years ago

My thinking was this: unloading the accounts will unload all the servers, which
will remove all the servers from the biff entry list, so that when the timer
fires next, it won't have any work to do, and will thus be harmless (this also
should be the same case as when we delete the only server with biff set up, and
thus should be handled). It should also be the case that after the timer fires,
we won't set up another timer. Then, when we restart, we will add back the
accounts, which will cause biff entries to get added for each account, which
will cause us to set up the appropriate timer.

	Navin Gupta
	Comment 18 • 22 years ago

But the timer would still keep firing, without doing any work, why would we want
to do that. That is how we have right now because we utlimately don't do
server->PerformBiff()  here in this code

      if(!serverBusy && (!serverRequiresPassword || (password.Length() > 0)))
        current->server->PerformBiff();
      mBiffArray->RemoveElementAt(i);

	Sheela Ravindran
	Comment 19 • 22 years ago

change qa contact 

	David :Bienvenu
	Comment 20 • 22 years ago

it would only fire once - it's not a recurring timer, as near as I can tell. The
difference between what I'm describing and what we have now is that in my
scenario, the timer would only fire once, and never again, because there would
be no servers listed to biff.

	Navin Gupta
	Comment 21 • 22 years ago

ah, ok. I'll measure the time taken to load and unload accts and report back. 

	Navin Gupta
	Comment 22 • 22 years ago

No of accounts in the profile: 1 imap, 1 pop, 2 news account(I don't have biff
for news account)

          LoadAccounts()   UnloadAccounts()  Startuptime (after turbo exit)
             printfs          printfs          stopwatch (naked eye)
Time(ms)     ~18-22           ~12-15         ~1500 - 2000 

So performance loss = ~2%

Startup: mozilla -mail -P profile

	(not reading, please use seth@sspitzer.org instead) Reporter
	Comment 23 • 22 years ago

it sounds like discussion is between:

do we unload all accounts (so that we take less memory, avoid certain potential
bugs) vs. not unloading accounts (so that starting mail back up is faster, our
code is simpler).

I think we should investigate what it would take to unload all accounts at
session logout, and then load them back.  One concern I have is that a leak
might cause us not to unload properly, and then loading back up after an
incomplete unload would cause problems.

By doing less on session-logout / restart, things might be kept simpler and we
might be avoiding other problems.  It makes session logout / restart more like
closing / reopening.

I think if they user actually wants to exit, we should let them.  it gives us a
chance to free memory, for the system to recover resources.  They're doing us a
favor!  My opinion, exit when in turbo should exit the app completely and then
restart it in a turbo mode.  I think this is covered by another bug that bill
law owns.  That might be nontrivial to implement, but it seems like a lot of
problems would go away, at the expense of profile switching (or exit /
restarting in turbo mode) would be slower.

Back to this patch:

1)

+  if (!strcmp(aTopic, "session-logout") && mBiffTimer)
+  {
+    mBiffTimer->Cancel();
+    mBiffTimer = nsnull;
+  }
+
+  if(!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID))

this should be:

+ if ()
+ {
+ }
+ else if ()
+ {
+ }

2)

-    //Don't remove from Observer service in Shutdown because Shutdown also gets
called
-    //from xpcom shutdown observer.  And we don't want to remove from the
service in that case.
-    nsCOMPtr<nsIObserverService> observerService = 
-      do_GetService("@mozilla.org/observer-service;1", &rv);
-    if (NS_SUCCEEDED(rv))
-    {    
-      observerService->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID);
-    }

did you remove this code because nsMsgBiffManager implements nsWeakInterface and
there for we don't have to remove ourselves as an observer?

comments?

	(not reading, please use seth@sspitzer.org instead) Reporter
	Comment 24 • 22 years ago

another concern:  what if we find issues where to fix them, we *have* to unload
/ reload at session logout / restart.  what will that mean for this code?  (it
sounds like it would be unnecessary)

the current state, we just fire the biff timer at most once a minute, and do
nothing.  And biff resumes working when you start back up.

I don't think any fix would be considered for the mozilla 1.0 branch.  At least,
I don't see it making sense risk v. reward.  

So this would be trunk only, so let's research the other option, which is to
unload / reload.

	Navin Gupta
	Comment 25 • 22 years ago

your comment #20 is wrong. After we have done turbo exit ->unloaded the accounts
the timer will fire and call nsMsgBiffManager::PerformBiff(). biffArray count 
is 0 so we would just call SetupNextBiff(). In SetupNextBiff() again we won't do
anything because biffArray count is 0. So the timer will keep firing. So your
suggestion will not fix this bug. 

	David :Bienvenu
	Comment 26 • 22 years ago

Navin, I'm not sure you read my comment about this not being a recurring timer.
As near as I can tell, this is a timer that fires once, and never fires again.

	Navin Gupta
	Comment 27 • 22 years ago

sorry, I'll take that back, the timer won't fire but it will be in memory.
I'm seeing other problem where just after you have called UnloadAccounts(), 
somebody calls loadAccounts() during turbo shutdown. I'll attach the stack
trace.  

	Navin Gupta
	Comment 28 • 22 years ago

stack trace..., this is after you call UnloadAccounts in "session-logout" of
nsMsgAccountManager. I mean after you have unloaded the accounts on turbo exit.

nsMsgAccountManager::LoadAccounts(nsMsgAccountManager * const 0x0304adc8) line 
1300
nsMsgAccountManager::GetAllServers(nsMsgAccountManager * const 0x0304adc8, 
nsISupportsArray * * 0x0012d780) line 1243 + 12 bytes
nsMsgAccountManagerDataSource::createRootResources(nsIRDFResource * 0x01dee0b0, 
nsISupportsArray * 0x030f35f0) line 597 + 47 bytes
nsMsgAccountManagerDataSource::GetTargets(nsMsgAccountManagerDataSource * const 
0x03cb7ca0, nsIRDFResource * 0x0304c120, nsIRDFResource * 0x01dee0b0, int 1, 
nsISimpleEnumerator * * 0x031db3e8) line 574 + 21 bytes
CompositeAssertionEnumeratorImpl::GetEnumerator(nsIRDFDataSource * 0x03cb7ca0, 
nsISimpleEnumerator * * 0x031db3e8) line 573 + 37 bytes
CompositeEnumeratorImpl::HasMoreElements(CompositeEnumeratorImpl * const 
0x031db3d8, int * 0x0012db88) line 239 + 22 bytes
nsRDFConMemberTestNode::FilterInstantiations(InstantiationSet & {...}, void * 
0x0012e018) line 389 + 42 bytes
TestNode::Propagate(const InstantiationSet & {...}, void * 0x0012e018) line 1044 
+ 19 bytes
TestNode::Propagate(const InstantiationSet & {...}, void * 0x0012e018) line 1054
RootNode::Propagate(const InstantiationSet & {...}, void * 0x0012e018) line 760 
+ 30 bytes
nsXULTreeBuilder::OpenSubtreeOf(nsTreeRows::Subtree * 0x00000000, nsIRDFResource 
* 0x0012e094, int * 0x001a2667) line 1615
nsXULTreeBuilder::OpenContainer(int 3080192, nsIRDFResource * 0x02ff0c88) line 
1569
nsXULTreeBuilder::Rebuild(nsXULTreeBuilder * const 0x002f0000) line 1075
nsXULTreeBuilder::SetTree(nsXULTreeBuilder * const 0x002f0000, nsITreeBoxObject 
* 0xc0000000) line 834
nsTreeBoxObject::SetDocument(nsTreeBoxObject * const 0x00000040, nsIDocument * 
0x00000010) line 87
nsXULDocument::SetBoxObjectFor(nsXULDocument * const 0x00000020, nsIDOMElement * 
0x00130560, nsIBoxObject * 0x00130560) line 6887
nsXULElement::SetDocument(nsXULElement * const 0x03c47628, nsIDocument * 
0x0534da00, int 3080192, int 15) line 2066
nsXULElement::SetDocument(nsXULElement * const 0x40000003, nsIDocument * 
0x00000000, int 1244420, int 3080192) line 2134
nsXULElement::SetDocument(nsXULElement * const 0x000000fe, nsIDocument * 
0xc0000000, int 2012951904, int 3081736) line 2134
nsXULElement::SetDocument(nsXULElement * const 0x0012fdf0, nsIDocument * 
0x002f0000, int 3080896, int 2012842704) line 2134
nsXULDocument::SetScriptGlobalObject(nsXULDocument * const 0x00000000, 
nsIScriptGlobalObject * 0x77fb80db) line 1635
DocumentViewerImpl::Close(DocumentViewerImpl * const 0x00000000) line 1621
nsDocShell::Destroy(nsDocShell * const 0x77fb80db) line 2713
nsWebShell::Destroy(nsWebShell * const 0x0012e7bc) line 1264
nsXULWindow::Destroy(nsXULWindow * const 0x00000000) line 396
nsWebShellWindow::Destroy(nsWebShellWindow * const 0x77fca4cb) line 1739 + 9 
bytes
nsChromeTreeOwner::Destroy(nsChromeTreeOwner * const 0x03c294b8) line 230
GlobalWindowImpl::ReallyCloseWindow(GlobalWindowImpl * const 0x002f0000) line 
3030
GlobalWindowImpl::CloseWindow(nsISupports * 0x77fa6c29) line 4167
nsJSContext::ScriptEvaluated(nsJSContext * const 0x77f902ed, int 304) line 1477 
+ 18 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x0012e8d8, void * 0x0012e8d4, 
void * 0x0012e890, unsigned int 268461183, void * 0x0012e8d4, int * 0x0012e89c, 
int 268462239) line 1050
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x00000005, nsIDOMEvent 
* 0x0012ea54) line 182 + 77 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x0012ec34, 
nsIDOMEvent * 0x0012ec34, nsIDOMEventTarget * 0x0012ec30, unsigned int 1240020, 
unsigned int 269143112) line 1219 + 20 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x002f0000, 
nsIPresContext * 0x77fcb30b, nsEvent * 0x0398dbb0, nsIDOMEvent * * 0x00001250, 
nsIDOMEventTarget * 0xfeeefeee, unsigned int 52902984, nsEventStatus * 
0x77fcc85a) line 2209 + 36 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x00000001, nsIPresContext * 
0x005bfd6c, nsEvent * 0x00010001, nsIDOMEvent * * 0x00000000, unsigned int 
2012971227, nsEventStatus * 0x02f58440) line 3447
PresShell::HandleDOMEventWithTarget(PresShell * const 0x77e94c28, nsIContent * 
0xffffffff, nsEvent * 0x0012f424, nsEventStatus * 0x0012f2fc) line 6033 + 36 
bytes
nsMenuFrame::Execute() line 1679
nsMenuFrame::HandleEvent(nsMenuFrame * const 0xffffffff, nsIPresContext * 
0x0012f59c, nsGUIEvent * 0x0019dfc4, nsEventStatus * 0x002f0000) line 486
PresShell::HandleEventInternal(nsEvent * 0x01f26148, nsIView * 0x77f902bd, 
unsigned int 2012807917, nsEventStatus * 0x00000060) line 6001 + 38 bytes
PresShell::HandleEvent(PresShell * const 0x0012f62c, nsIView * 0x03604f0f, 
nsGUIEvent * 0x02fb1db0, nsEventStatus * 0x0012f6e0, int 1242900, int & 
-13597301) line 5909 + 25 bytes
nsViewManager::HandleEvent(nsView * 0x03603522, nsGUIEvent * 0x00000104, int 
32661824) line 2076
nsView::HandleEvent(nsViewManager * 0x03610703, nsGUIEvent * 0x00000f3c, int 
1032357892) line 306
nsViewManager::DispatchEvent(nsViewManager * const 0x02ff06c0, nsGUIEvent * 
0x0012f8cc, nsEventStatus * 0x0012f7c8) line 1881 + 23 bytes
HandleEvent(nsGUIEvent * 0x0012f801) line 83
nsWindow::DispatchEvent(nsWindow * const 0x00000001, nsGUIEvent * 0x020364c4, 
nsEventStatus &) line 889 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x03ebf4fc) line 910
nsWindow::DispatchMouseEvent(unsigned int 3080192, unsigned int 32661824, 
nsPoint * 0x00000000) line 4754 + 21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 2012773192, unsigned int 32661824, 
nsPoint * 0x002f0000) line 5011
nsWindow::ProcessMessage(unsigned int 805462114, unsigned int 5, long 0, long * 
0x00302960) line 3624 + 28 bytes
nsWindow::WindowProc(HWND__ * 0x0012fd1c, unsigned int 268966609, unsigned int 
3109320, long 0) line 1154 + 27 bytes
USER32! 77e13eb0()
USER32! 77e1401a()
USER32! 77e192da()
nsAppShellService::Run(nsAppShellService * const 0x01800fd8) line 451
main1(int 4, char * * 0x003047d0, nsISupports * 0x00000000) line 1456 + 32 bytes
main(int 4, char * * 0x003047d0) line 1805 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87903()

	Navin Gupta
	Comment 29 • 22 years ago

logged bug 145309 for rebuilding tree on exit.

	stephend@netscape.com (gone - use stephen.donner@gmail.com instead)
	Comment 30 • 22 years ago

I was pointed to http://bugscape.mcom.com/show_bug.cgi?id=15485#c29 for steps on
how to reproduce this.

With branch build 2002-06-03 on Windows 2000, with 2 scenarios:

single profile vs. multiple profiles

and the following prefs set in all.js:

pref("browser.turbo.enabled", true);
pref("browser.turbo.singleProfileOnly", false);

I don't see the prompt that Lisa sees in step 7 of
http://bugscape.mcom.com/show_bug.cgi?id=15485#c29

I wasn't in on the original bug though, so I may be missing something crucial.

	(not reading, please use seth@sspitzer.org instead) Reporter
	Comment 31 • 21 years ago

mass re-assign.

	Wayne Mery (:wsmwk)
	Comment 32 • 19 years ago

does (In reply to comment #24)
> another concern:  what if we find issues where to fix them, we *have* to unload
> / reload at session logout / restart.  what will that mean for this code?  (it
> sounds like it would be unnecessary)
> 
> the current state, we just fire the biff timer at most once a minute, and do
> nothing.  And biff resumes working when you start back up.
> 
> I don't see it making sense risk v. reward.  
> 
> ... let's research the other option, which is to unload / reload.

What is present status of persuing unload / reload?
And should patch be marked obsolete?

	Wayne Mery (:wsmwk)
	Comment 33 • 18 years ago

should this block (or be blocked by) bug 144559 ?

	David :Bienvenu
	Comment 34 • 18 years ago

I don't believe turbo mode exists in TB (not sure about SM but I doubt it)

	Wayne Mery (:wsmwk)
	Comment 35 • 18 years ago

isn't turbo same as quicklaunch (which, yes, TB doesn't have)?
is the bug no longer valid?

	Wayne Mery (:wsmwk)
	Comment 36 • 15 years ago

turbo is no mor eon trunk