Do not open a writable stream to dest local folder in mailDatabase

VERIFIED FIXED

Status

MailNews Core
Database
VERIFIED FIXED
16 years ago
10 years ago

People

(Reporter: Navin Gupta, Assigned: Navin Gupta)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2 rtm])

Attachments

(1 attachment)

(Assignee)

Description

16 years ago
This happens in StartBatch() which is called from EnableNotifications. 
We are doing this to disable count notifications. There is no need to open a 
writable stream when we are not doing anything with it. 

This could be the cause of corruption in moving msgs to local folders because
we again open a writable stream in local folder to copy msgs to dst folder.

Comment 1

16 years ago
I agree that we shouldn't open the stream, but if we're not using it, then I'm
having a hard time to imagine how it could cause a corruption. My believe is
that corruptions are caused by two pieces of code writing to two different streams.
(Assignee)

Comment 2

16 years ago
I don't know how it could cause corruption but it is pretty bad. 
I am thinking of fixing it by passing a boolean to StartBatch(createStream)
EndBatch(createStream). So I will have to add boolean to
EnableNotifications(..., , createStream) also. 

Comment 3

16 years ago
would it be possible for them to go through the stream sharing code? The idea
behind sharing the stream is that everyone who tries to create a write stream
should get it from the same bottlenecked place....
(Assignee)

Comment 4

16 years ago
ya, good idea, we could do that also but we will have to change CopyState
fileStream from nsOutputFileStream to nsIOFileStream. this may help us to 
get rid of the StartBatch() and EndBatch() completely, though we could keep it
for future use. 
(Assignee)

Comment 5

16 years ago
wait a minute, we want to share the stream if there is one for the srcFolder not
for the dstFolder. For srcFolder we are not creating any stream in localFolder
code so sharing doesn't come into play. 

Comment 6

16 years ago
How come sharing doesn't come into play? You were saying we were opening two
write streams on the dest folder, causing corruption, or is that not true?
(Assignee)

Comment 7

16 years ago
Right now we are opening two streams to dstFolder, one in localFolderCopyState
code and another one in mailDatabase::StartBatch() but mailDatabase one is not
used for anything. It is opened in StartBatch() and closed in EndBatch(). 

All we want is not create a stream to dstFolder in mailDatabase code. 
(Assignee)

Comment 8

16 years ago
I think using a boolean would be better here, basically comment #2.
(Assignee)

Comment 9

16 years ago
Created attachment 80689 [details] [diff] [review]
proposed fix

Added a boolean dbBatching to not do batching for local dest folder when
moving/copying msgs. I also removed ifDef #MAC so I will need to test it on
mac, works on win2k. David, can you review ? thx.
(Assignee)

Comment 10

16 years ago
checked on mac move msgs from imap, local -> local works fine. 
(Assignee)

Comment 11

16 years ago
david, can you review this ? thx. 

Comment 12

16 years ago
Comment on attachment 80689 [details] [diff] [review]
proposed fix

r=bienvenu - the one thing I really don't like about this is that now we've
made it just a bit more complicated for client code - now client code has to
make sure that when it code enables and disables  notifications that it also
passes the correct dbBatching parameter (because they have to match or you'll
end up with a db permanently batched. But I don't see much of a way around it
w/o having the db expose whether it's batching or not.
Attachment #80689 - Flags: review+

Comment 13

16 years ago
Comment on attachment 80689 [details] [diff] [review]
proposed fix

sr=mscott
Attachment #80689 - Flags: superreview+
(Assignee)

Comment 14

16 years ago
fixed
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 15

16 years ago
marking adt1.0.0 and nsbeta1+ because this is required for bug 139576.  If we
don't take that then we don't need this.
Blocks: 139576
Keywords: adt1.0.0, nsbeta1+
Whiteboard: [adt2 rtm]

Updated

16 years ago
QA Contact: gayatri → sheelar

Comment 16

16 years ago
marking verified - this was checked into the trunk.
Status: RESOLVED → VERIFIED

Comment 17

16 years ago
adt1.0.1+ (on ADT's behalf) for checkin to the 1.0 branch, pending Driver's
approval.
Keywords: adt1.0.0 → adt1.0.1+, mozilla1.0.1

Comment 18

16 years ago
minusing for 1.0.1 drivers. if this is causing corruption, we want it. otherwise
it seems like optimization that isn't part of the 1.0.1 goal. let us know if
we're wrong.
Keywords: mozilla1.0.1 → mozilla1.0.1-

Comment 19

16 years ago
Navin said this change IS required if we check in the fix for bug 139576. And 
bug 139576 is checked into the branch. But I believe I checked this fix into the
branch when I checked in the fix for 139576 into the branch. 
(Assignee)

Comment 20

16 years ago
David, thx for checking this into the branch. I made sure when I came back from
vacation. 
Keywords: mozilla1.0.1- → fixed1.0.1
Taking this bug as QA contact, because I believe it's okay to verify this (from
a QA standpoint) using LXR, as development has looked at the code itself.
OS: Windows NT → All
QA Contact: sheelar → stephend
Hardware: PC → All

Comment 22

16 years ago
Thanks Stephend;)
Navin checked http://bugzilla.mozilla.org/attachment.cgi?id=80689&action=view
into the branch on April 30th.
Keywords: fixed1.0.1 → verified1.0.1
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.