Closed Bug 135275 Opened 22 years ago Closed 22 years ago

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

Categories

(MailNews Core :: Database, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: naving, Assigned: naving)

References

Details

(Whiteboard: [adt2 rtm])

Attachments

(1 file)

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.
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.
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. 
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....
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. 
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. 
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?
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. 
I think using a boolean would be better here, basically comment #2.
Attached patch proposed fixSplinter Review
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.
checked on mac move msgs from imap, local -> local works fine. 
david, can you review this ? thx. 
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 on attachment 80689 [details] [diff] [review]
proposed fix

sr=mscott
Attachment #80689 - Flags: superreview+
fixed
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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]
QA Contact: gayatri → sheelar
marking verified - this was checked into the trunk.
Status: RESOLVED → VERIFIED
adt1.0.1+ (on ADT's behalf) for checkin to the 1.0 branch, pending Driver's
approval.
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.
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. 
David, thx for checking this into the branch. I made sure when I came back from
vacation. 
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
Thanks Stephend;)
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: