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.
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.
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+
Status: NEW → RESOLVED
Last Resolved: 17 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.
Keywords: adt1.0.0, nsbeta1+
Whiteboard: [adt2 rtm]
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.
Keywords: adt1.0.0 → adt1.0.1+, mozilla1.0.1
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-
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.
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
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
You need to log in before you can comment on or make changes to this bug.