105497 - Opening folders with 64K+ mails hangs/crash the messenger

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Juan Pablo Alcaraz]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.1) Gecko/20010607
BuildID:    0.9.5

We have in a Exchange 5.5 sp2 server a folder with more than 65000 mails. We
connect to it with IMAP. Well, when we retrieve the headers the mailer hangs.

Reproducible: Always
Steps to Reproduce:
1. Setup a mail count (the server must be an exchange server 5.5 sp2). Connect
to it using IMAP.

2. Load Mozilla, open messenger and open the folder with 65000+ messages

3. Wait that messenger download the headers.



Actual Results:  The mailer hangs!

Expected Results:  The mailer must to download the headers quickly... and it
must not hang :-)

The workstation is a Windows Workstations 4 SP 6


I try to put in CC to [e-mail]. He reproduce the error :-), but
bugzilla doesn't allow me

Comment 1

[Juan Pablo Alcaraz]

I put to [e-mail]. He can reproduce the bug.

Comment 2

[jmorzins]

The reporter hasn't specified what they mean by "hang".  If they mean "we wait
patiently for mozilla to finish downloading the headers, and it never does",
then this bug may be a duplicate of bug 103401.  (It would be interesting to
know if they can continue to access smaller folders, even after the attempt to
read the large folder hung.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Juan Pablo Alcazar, could you please answer [e-mail]'s question? What do 
you mean by "hang"?   Do you mean "the headers are not downloaded"?  Or "the 
entire Mozilla UI freezes and nothing can be done to any Mozilla window"?

Comment 4

[Juan Pablo Alcaraz]

Hi.
The browser freezes completly. We can not access to any open browser windows.
This error does not happen in Netscape Communicator 4.76

Comment 5

[Boris Zbarsky [:bzbarsky]]

confirming for investigation.

Comment 6

[leon.zhang]

I can not reproduce hanging phenomenon using trunk 20020909.
But i meet a ASSERTION when downlaod 50000th message(total 65000):

###!!! ASSERTION: morkBool_kFalse: '0', file
g:/mozilla-trunk/20020909/mozilla/db/mork/src/morkConfig.cpp, line 57
Break: at file g:/mozilla-trunk/20020909/mozilla/db/mork/src/morkConfig.cpp, line 57

stack status:

mork_assertion_signal(const char * 0x05528dc0) line 57 + 31 bytes
morkEnv::NewWarning(const char * 0x05528c08) line 417 + 19 bytes
morkNode::RefsOverflowWarning(morkEnv * 0x043776f0) line 364
morkNode::AddWeakRef(morkEnv * 0x043776f0) line 613
morkNode::SlotWeakNode(morkNode * 0x04378a00, morkEnv * 0x043776f0, morkNode * *
0x0748add4) line 505 + 18 bytes
morkStore::SlotWeakStore(morkStore * 0x04378a00, morkEnv * 0x043776f0, morkStore
* * 0x0748add4) line 770 + 20 bytes
morkRowObject::morkRowObject(morkEnv * 0x043776f0, const morkUsage & {...},
nsIMdbHeap * 0x04377300, morkRow * 0x07510b08, morkStore * 0x04378a00) line 111
+ 20 bytes
morkRow::AcquireRowObject(morkEnv * 0x043776f0, morkStore * 0x04378a00) line 274
+ 57 bytes
morkRow::AcquireRowHandle(morkEnv * 0x043776f0, morkStore * 0x04378a00) line 285
+ 16 bytes
morkStore::NewRowWithOid(morkStore * const 0x04378a2c, nsIMdbEnv * 0x0437771c,
const mdbOid * 0x0012fa1c, nsIMdbRow * * 0x0012fa30) line 2105 + 19 bytes
nsMsgDatabase::CreateNewThread(unsigned int 0x0000e1c4, const char * 0x0748ac78,
nsMsgThread * * 0x0012fa54) line 3302 + 38 bytes
nsMsgDatabase::AddNewThread(nsMsgHdr * 0x0748ab80) line 3652 + 32 bytes
nsMsgDatabase::ThreadNewHdr(nsMsgHdr * 0x0748ab80, int & 0x0012fb90) line 3474 +
24 bytes
nsMsgDatabase::AddNewHdrToDB(nsMsgDatabase * const 0x04377480, nsIMsgDBHdr *
0x0748ab80, int 0x00000001) line 2714 + 22 bytes
nsImapMailFolder::NormalEndHeaderParseStream(nsIImapProtocol * 0x06e5aad8) line 2661
nsImapMailFolder::ParseMsgHdrs(nsImapMailFolder * const 0x042b0e78,
nsIImapProtocol * 0x06e5aad8, nsIImapHeaderXferInfo * 0x06e5ad60) line 2519 + 18
bytes
XPTC_InvokeByIndex(nsISupports * 0x042b0e78, unsigned int 0x00000010, unsigned
int 0x00000002, nsXPTCVariant * 0x073ec9a0) line 106
EventHandler(PLEvent * 0x074067e8) line 567 + 41 bytes
PL_HandleEvent(PLEvent * 0x074067e8) line 643 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00f9af58) line 573 + 9 bytes

Comment 7

[leon.zhang]

After delete some messages, downlaoding proocess can be completed.

But cannnot open the inbox folder again, and the ASSERTION above block the open
action.

Comment 8

[leon.zhang]

After traceded the souce of the bug, I found that two factors caused
phenomenon(ASSERTION-->crash).

1) #define morkNode_kMaxRefCount 0x0FFFF
2) typedef mork_u2 mork_uses;    // 2-byte strong uses count
  typedef mork_u2 mork_refs;    // 2-byte actual reference count

this data is 2 bytes, so when message count over 64K, then lead to overflow of
counter( refs and uses in function:  morkNode::AddStrongRef(morkEnv* ev)).

morkNode::AddStrongRef(morkEnv* ev)
{
  mork_uses outUses = 0;
  if ( this )
  {
    if ( this->IsNode() )
    {
      mork_uses uses = mNode_Uses;
      mork_refs refs = mNode_Refs;
      if ( refs < uses ) // need to fix broken refs/uses relation?
      { 
        this->RefsUnderUsesWarning(ev);
        mNode_Refs = mNode_Uses = refs = uses;
      }
      if ( refs < morkNode_kMaxRefCount ) // not too great?
      {
        mNode_Refs = ++refs;
        mNode_Uses = ++uses;
      }
      else
        this->RefsOverflowWarning(ev);

      outUses = uses;
    }
    else
      this->NonNodeError(ev);
  }
  else
    ev->NilPointerError();
  return outUses;
}

Comment 9

[leon.zhang]

Attachment: modify the counter to 4 bytes

according to previous comments

Comment 10

[Antonio.Xu]

Hello,David
Can you take a look at the patch of this bug?

Comment 11

[leon.zhang]

the title of the attachhment should be "32 bits" or "4 bytes"

Comment 12

[Henry Jia]

Comment on attachment 98677 [details] [diff] [review]
modify the counter to 4 bytes

Seems you find the correct reason. Please also give out the test enviroment and
test result.

Comment 13

[Henry Jia]

Change the summary according to the comments.

Comment 14

[leon.zhang]

I have sent 86000 message to an emal box,and reveived all of messages using
mozilla which applied current patch withhout crash.

test env: 
    win2000 + trunk20020909 + current patch

Comment 15

[David :Bienvenu]

yeah, this is the obvious, brute force solution, but unfortunately, it adds
quite a bit to our general memory foot print because every mork object will get
bigger. I think a better solution is to not ref-cnt the store object at all, or
at least, not from the row objects.

Comment 16

[leon.zhang]

In fact,current solution will introduce addtional 4 bytes for every mork object.
Is is a big problem?  Will current solution import other side-effects except
comsuming additional memory?

Comment 17

[David :Bienvenu]

as I said, it will grow each mork object by 4 bytes. For example, a 1000 msg
folder will take around an extra 8K bytes or so. I'd really rather not do that,
if possible, since we'd like to reduce our memory footprint, not increase it.

Comment 18

[leon.zhang]

I also did not want to modify the data structure,but if not do like current
solution, the change to source code will be very big.

Codes related to mNode_Refs should be changed :(

Comment 19

[leon.zhang]

How to balance?

Comment 20

[leon.zhang]

Attachment: proposed patch

a proposed patch.
according to suggestions of bienvenu.

Comment 21

[leon.zhang]

Not execute the factual Add??Ref or Cut??Ref to morknode,only return 1 to parent
function.

Comment 22

[leon.zhang]

no crash.

Comment 23

[leon.zhang]

another thought based the patch attachment (id=99034):
    we can not change the values of mNode_Uses and mNode_Refs,not only "return
1" to parent function.

another patch according to this thought:

Index: morkNode.cpp
===================================================================
RCS file: /cvsroot/mozilla/db/mork/src/morkNode.cpp,v
retrieving revision 1.13
diff -u -r1.13 morkNode.cpp
--- morkNode.cpp	16 Feb 2002 22:46:30 -0000	1.13
+++ morkNode.cpp	13 Sep 2002 08:12:41 -0000
@@ -515,22 +515,7 @@
   {
     if ( this->IsNode() )
     {
-      mork_uses uses = mNode_Uses;
-      mork_refs refs = mNode_Refs;
-      if ( refs < uses ) // need to fix broken refs/uses relation?
-      { 
-        this->RefsUnderUsesWarning(ev);
-        mNode_Refs = mNode_Uses = refs = uses;
-      }
-      if ( refs < morkNode_kMaxRefCount ) // not too great?
-      {
-        mNode_Refs = ++refs;
-        mNode_Uses = ++uses;
-      }
-      else
-        this->RefsOverflowWarning(ev);
-
-      outUses = uses;
+      outUses = mNode_Uses;
     }
     else
       this->NonNodeError(ev);
@@ -549,9 +534,7 @@
     if ( this->IsNode() )
     {
       mork_uses uses = mNode_Uses;
-      if ( uses ) // not yet zero?
-        mNode_Uses = --uses;
-      else
+      if ( !uses ) // not yet zero?
         this->UsesUnderflowWarning(ev);
 
       didCut = morkBool_kTrue;
@@ -562,7 +545,6 @@
           if ( !mNode_Refs ) // no outstanding reference?
           {
             this->RefsUnderflowWarning(ev);
-            ++mNode_Refs; // prevent potential crash during close
           }
           this->CloseMorkNode(ev); // polymorphic self close
           // (Note CutNode() is not polymorphic -- so don't call that.)
@@ -604,13 +586,7 @@
   {
     if ( this->IsNode() )
     {
-      mork_refs refs = mNode_Refs;
-      if ( refs < morkNode_kMaxRefCount ) // not too great?
-        mNode_Refs = ++refs;
-      else
-        this->RefsOverflowWarning(ev);
-        
-      outRefs = refs;
+      outRefs = mNode_Refs;
     }
     else
       this->NonNodeError(ev);
@@ -630,9 +606,7 @@
     {
       mork_uses uses = mNode_Uses;
       mork_refs refs = mNode_Refs;
-      if ( refs ) // not yet zero?
-        mNode_Refs = --refs;
-      else
+      if ( !refs ) // not yet zero?
         this->RefsUnderflowWarning(ev);
 
       if ( refs < uses ) // need to fix broken refs/uses relation?


result: no crash

Comment 24

[leon.zhang]

count of current message in INbox folder: 94560

Comment 25

[David :Bienvenu]

I was not suggesting turning off all ref-counting in mork, which is more or less
what your patch does, right? I was suggesting making it so the mork row objects
don't addref the store, since the store is more or less guaranteed to own the
row's, and live longer.

Comment 26

[Juan Pablo Alcaraz]

If you have 10000+ mails you have a machine with enough power to try them so
40kb+ of extra memory is not problem.

At least in the current version. Later you can reduce memory usage if you like :-)

Comment 27

[David :Bienvenu]

Attachment: proposed fix

this is what I was proposing. I haven't tried running it yet, but it prevents
the row object from ref-cnting the store.

Comment 28

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

Isn't this the same bug essentially as
http://bugzilla.mozilla.org/show_bug.cgi?id=123677#c5

Comment 29

[David :Bienvenu]

taking. I've tried this fix on a newsgroup with 59,000 messages and it worked (I
think even 59,000 would blow the ref cnt). I'll try to get a bigger folder to
make sure but if someone with access to the 64K folder could try my patch (w/o
the other patch), I'd appreciate it.

Comment 30

[David :Bienvenu]

Comment on attachment 99034 [details] [diff] [review]
proposed patch

this patch is a no-go, sorry.

Comment 31

[leon.zhang]

about comments #25.
   my thought is that we can avoid increasing or decreasing the counter of
morkNode. Because functions of morknode is the basic function which will be
called by other parent object,....., so my modification is focused in thes
functions.  My basic start point is modifying the source codes as few as
possible. :)


About comments #26:
   according to comments #17, the comments #26 should be:
     "If you have 10000+ mail folders you have a machine with enough power to try 
                         ~~~~~~~~~~~~ not mail 
them so 40kb+ of extra memory is not problem. ......."
    But for every client users, It is very difficult to manage so many folders.

Comment 32

[leon.zhang]

Attachment: a new proposed patch

based on bienvenu's patch and comments

test env: 94561
test result: no crash

Comment 33

[leon.zhang]

test env: 94561 messages in INbox folder

Comment 34

[David :Bienvenu]

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

no, this is not obsolete. Did you try it?

Comment 35

[David :Bienvenu]

Comment on attachment 99197 [details] [diff] [review]
a new proposed patch

this disables more ref-cnting than needs to be disabled.  I would have thought
my patch was sufficient. Did it not work for you?

Comment 36

[leon.zhang]

hi,bienvenu
I have test your patch(id=99057), still meet crash.
So I modify more crash point based on your patch.
Current patch(id=99197) works when access 94561 meesages.

Comment 37

[David :Bienvenu]

I'll have to look at this towards the end of next week, Thursday or Friday. So
please be patient.

Comment 38

[David :Bienvenu]

Attachment: propsed fix

does this fix work for you? your patch is almost correct, but it doesn't fix
the comments in morkRowObject.h and morkTable.h, and it incorrectly removes the
this->CutWeakRef(ev->AsMdbEnv()) in morkRowObject.cpp - that handles the weak
ref between the morkRow and the morkRowObject, and doesn't have anything to do
with the ref-counting we want to remove (that between the morkRowObject and the
morkRowSpace and morkStore).

Comment 39

[leon.zhang]

hi David,
the patch wroks well, no crash now

Comment 40

[leon.zhang]

s/wroks/works/

test enviornment: a mailbox containing 94560+ messages
.

Comment 41

[David :Bienvenu]

thanks a lot, Leon. Cavin, can I get an r= and Alec, can I get an sr=? The fix
is to disable certain weak-ref counting in cases where the ownership model is
clear, and that ref-counting causes an overflow and subsequent underflow.

Comment 42

[Cavin Song]

Comment on attachment 100185 [details] [diff] [review]
propsed fix

r=cavin. good one.

Comment 43

[Alec Flett]

Comment on attachment 100185 [details] [diff] [review]
propsed fix

mmm...mork.

sr=alecf

Comment 44

[David :Bienvenu]

fix checked in, thx for your help, Leon.

Comment 45

[leon.zhang]

my pleasure :)

Comment 46

[David :Bienvenu]

*** Bug 123677 has been marked as a duplicate of this bug. ***

Comment 47

[Juan Pablo Alcaraz]

I's work for me!
Thanks all!