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)
MailNews Core
Database
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: naving, Assigned: naving)
References
Details
(Whiteboard: [adt2 rtm])
Attachments
(1 file)
12.96 KB,
patch
|
Bienvenu
:
review+
mscott
:
superreview+
|
Details | Diff | Splinter Review |
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•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 years ago
|
||
I think using a boolean would be better here, basically comment #2.
Assignee | ||
Comment 9•22 years ago
|
||
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•22 years ago
|
||
checked on mac move msgs from imap, local -> local works fine.
Assignee | ||
Comment 11•22 years ago
|
||
david, can you review this ? thx.
Comment 12•22 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•22 years ago
|
||
Comment on attachment 80689 [details] [diff] [review] proposed fix sr=mscott
Attachment #80689 -
Flags: superreview+
Assignee | ||
Comment 14•22 years ago
|
||
fixed
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 15•22 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.
Updated•22 years ago
|
QA Contact: gayatri → sheelar
Comment 16•22 years ago
|
||
marking verified - this was checked into the trunk.
Status: RESOLVED → VERIFIED
Comment 17•22 years ago
|
||
adt1.0.1+ (on ADT's behalf) for checkin to the 1.0 branch, pending Driver's approval.
Comment 18•22 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•22 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•22 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•22 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
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•