76914 - Sorting in mail by Sender or Subject causes browser to core dump.

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Barrett Lindstrom]

This has been reproduced in both mozilla and netscape daily trunk builds 
#2001041921.

If you click on either the Sender or Subject tab to sort the messages in mail, 
the browser will coredump.

STEPS TO REPRODUCE:
1.  Start N6 (or mozilla) mail.
2.  After you log in, click on either the sender tab or the subject tab.

EXPECTED RESULTS:  The mail messages should sort by the tab that was clicked.

ACTUAL RESULTS:  The browser core dumps.

I'm setting the componant to Mail window front end, because I have no idea what 
to set it to, and Bugzilla won't let me leave it not set.

Comment 1

[Barrett Lindstrom]

Updated qa contact and adding blocker....

Comment 2

[hirata masakazu]

Please see if bug 76919 is a dupe of this.

Comment 3

[Barrett Lindstrom]

hmmmmm.  Seems similar, but not quite the same.  On HPUX, you only have to click 
the subject or sender tab once, and the browser will core dump every time 
without fail.  Also, if your mail has been set to sort by either subject or 
sender previously (using an older N6 to change the sort and then exiting), as 
soon as it trys to get the messages from the server, the browser will core dump. 
 Since this doesn't happen on linux, I can't see it as being a non-platform 
specific bug.  It would be strange to have a bug that affected only Mac and 
Hpux, but I suppose stranger things have happend.  I will add these comments to 
bug 76919.  shannond, any thoughts on this?

Comment 4

[Shannon Diener]

Attachment: stack trace of crash

Comment 5

[Shannon Diener]

I suppose that its possible that this could be related to bug 76919, but lets
keep them separate for now.  I'll keep an eye on the other bug

Comment 6

[Shannon Diener]

This problem was introduced somewhere between 2/26/2001 and 3/30/2001 ... still
narrowing down ... 

Comment 7

[Shannon Diener]

narrowed down to between 3/15 and 3/22

Comment 8

[Shannon Diener]

Looks like this problem started after the mailnews performance branch landing

Comment 9

[Shannon Diener]

Attachment: updated stack trace

Comment 10

[Scott MacGregor]

to answer Shannon's question, no we aren't seeing this crash on other platforms.
seems to be HPUX only. You posted a stack trace, but what's going on in the
stack trace? i.e. where in the sort method are you crashing? Is something null
or is a piece of memory corrupted?

Comment 11

[Shannon Diener]

The crash occurs in this same spot in the code while in the process of the sort.
 Depending on the contents of my inbox the crash could occur at any random
message - sometimes it crashes on the first message (numSoFar = 0)... sometimes
it crashes on the fifth ... etc.  But I havent' been able to find a pattern for
why it crashes where it does and on which message this happens.  I haven't found
anything to be null and am still trying to determine if something is corrupted.

Comment 12

[Shanjian Li]

I will take a look at this bug.

Comment 13

[Shanjian Li]

Attachment: proposed fix

Comment 14

[Shanjian Li]

The problem is caused by memory alignment. On HPUX, int32 need to be accessed in 4-byte
boundary. Otherwise it will crash the system. We need to bear this in mind. It is a 
virtue of our HPUX.

shannon, please take care of the rest stuff (r=, sr=, checkin).

Comment 15

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

I'll do the review / super-review.

Comment 16

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

I have problems with this fix.

1) first, you've only fixed one of the callers GetFieldTypeAndLenForSort().

there are multiple callers.  instead of adjusting the count afterwards, let's 
make the default values be the right size.

(kMaxSubjectKey, kMaxLocationKey, kMaxAuthorKey, kMaxRecipientKey)

if you do change the size, include a comment about why the size has to be a 
multiple of whatever.  (so when it gets changed next time, we won't break HPUX)

2)

+ //memory alignment before we adjust bTemp. Otherwise it will crash on some 
system
+ bytesToCopy += (4-bytesToCopy%4);

shouldn't you be updating bytesToCopy before doing the memcpy?

before we check any patch it, we'll need to test it on linux, mac and win32.

these remind me of a problem I had with sizeof(EntryInfo) not returning what I 
thought it would.  see 
http://mail-index.netbsd.org/current-users/1995/07/12/0018.html
for an example of what I'm talking about.

to summarize, I'm not happy with all the crazy pointer arithmetic that goes on 
nsMsgDBView.cpp.  it's just an implementation of an arena.  I'd like to clean 
it up to use the arenas we've already got in mozilla, but until then, I'll 
gladly take a fix to make this work on HPUX.

please come up with a cleaner fix.  if you are not able to test it on win32, 
linux and mac, let me know what platforms you can test on and I'll cover the 
rest.



       

Comment 17

[Shanjian Li]

Yes, modifying the max len constant is better. (I did not know they
are from constants.) Multiple callers? Please check it to see what
Seth mean and fix it. Modify size before memory copy? 
I do not think so! You don't want to access memory outside its desired range. 
(Probably I should not reuse variable "bytesToCopy".) 
Put something like:

  PRInt32 bytesToPad = 4 - bytesToCopy & 0x00000003;
  if (bytesToPad == 4) bytesToByte = 0;
Then increase the amounf of (bytesToCopy+bytesToPad).

Comment 18

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

I'm not sure I understand all your comments, but I'll just wait for the next patch.

one more comment:  in the next version of the patch, instead of doing "4" do
sizeof(PRInt32).

Comment 19

[Shannon Diener]

Attachment: patch from Seth's and Shanjian's comments

Comment 20

[Shannon Diener]

I've attached a patch created from Seth's and Shanjian's comments.  This works
on HP-UX and doesn't bust sorting on Linux.  

Comment 21

[Shannon Diener]

Attachment: better if-statement format

Comment 22

[Shanjian Li]

I do not understand why you want to add additional 4 to those constant. 
That's not necessary. Everything else looks fine to me.

Comment 23

[Shannon Diener]

The first part of my patch does the same thing as the first part of Shanjian's 
patch only to the constants to which maxLen is assigned as opposed to performing 
the alignment on maxLen after it is returned.  However, I see that this is 
unnecessary because those values (kMaxSubjectKey, kMaxLocationKey, 
kMaxAuthorKey, kMaxRecipientKey) do not require alignment (are already divisible 
by 4).  Therefore, only the second part of my patch is necessary.  Is this 
correct?

Comment 24

[Shannon Diener]

Reassigning to Shanjian based on his experience with this problem on HP-UX

Comment 25

[Shanjian Li]

reassign to shannon.

Comment 26

[Shanjian Li]

shannon, please update your patch. Ater that, r=shanjian.

Comment 27

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

is the final patch simply going to be changing the constants and adding a
comment explaining why those values are necessary for HPUX?

Comment 28

[Shanjian Li]

Sorry, I am not the owner of this module, I shouldn't give r=. I withdraw. 
Seth, could you give both r/sr? We really want to fix this ASAP.

Comment 29

[Shannon Diener]

Attachment: adjusted patch - based on shanjian's comments

Comment 30

[Shannon Diener]

Adjusted patch as requested by Shanjian ... Is this still correct Shanjian? 
Seth, is this okay with you?  This fixes the problem on HPUX and doesn't seem to
have any ill affects on Linux

Comment 31

[Shanjian Li]

Attachment: update shannon's patch

Comment 32

[Shanjian Li]

We need to put some comment to place where those constants are defined. 
That's to make sure people do not break it when they change the value. 

Comment 33

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

the patch keeps changing.  I thought the fix was to change the constants and put
comments explaining why they had to be a certain multiple of X so that HPUX
wouldn't crash.

I'm the proper owner of this, I'll drive it in.  but the patch needs to settle
down first.

Comment 34

[Shanjian Li]

Those constants are already in 4-byte boundary. But we do need to put some comment 
there to make sure future change will not break this. That's why I updated shannon's
latest patch. Other than that, they are the same. 

Comment 35

[scottputterman]

moving to 0.9.2

Comment 36

[Shanjian Li]

Please try to resolve this in 0.9.1. This has great impact on HPUX. 
The last patch will be the final, unless you have other suggestions.

Comment 37

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

please test the final patch on mac, win and linux.

it's on my list of fixes to review today.

Comment 38

[Shannon Diener]

Attachment: patch changes reflecting Seth's comments

Comment 39

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

r/sr=sspitzer

assuming testing goes well, check it in.

Comment 40

[Shannon Diener]

checked in 

Comment 41

[Barrett Lindstrom]

Verified fixed on build # 2001052021 on hpux 11i.