196950 - flushing to disk (at least twice) when I mark a local message as read / unread.

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

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

flushing to disk (at least twice) when I mark a message as read / unread.

here's the stack.  UpdateFolderFlag() is doing the flushing.

nsMailDatabase::UpdateFolderFlag(nsIMsgDBHdr * 0x047f41f0, int 0, int 1,
nsIOFileStream * * 0x0012cd5c) line 429
nsMailDatabase::SetHdrFlag(nsIMsgDBHdr * 0x047f41f0, int 0, int 1) line 324
nsMsgDatabase::SetHdrReadFlag(nsIMsgDBHdr * 0x047f41f0, int 0) line 1738
nsMsgDatabase::MarkHdrReadInDB(nsIMsgDBHdr * 0x047f41f0, int 0,
nsIDBChangeListener * 0x03b0cc0c) line 1764
nsMsgDatabase::MarkHdrRead(nsMsgDatabase * const 0x03b8f4f0, nsIMsgDBHdr *
0x047f41f0, int 0, nsIDBChangeListener * 0x03b0cc0c) line 2129 + 26 bytes
nsMsgDatabase::MarkRead(nsMsgDatabase * const 0x03b8f4f0, unsigned int 10318298,
int 0, nsIDBChangeListener * 0x03b0cc0c) line 1785 + 29 bytes
nsMsgDBView::SetReadByIndex(unsigned int 176, int 0) line 2421 + 75 bytes
nsMsgDBView::ToggleReadByIndex(unsigned int 176) line 2394
nsMsgDBView::ApplyCommandToIndices(int 2, unsigned int * 0x0012cf6c, int 1) line
2179 + 21 bytes
nsMsgDBView::CycleCell(nsMsgDBView * const 0x03b0cc10, int 176, const unsigned
short * 0x047a7390) line 1583
XPTC_InvokeByIndex(nsISupports * 0x03b0cc10, unsigned int 28, unsigned int 2,
nsXPTCVariant * 0x0012d114) line 102
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_METHOD) line 2023 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x00fb7aa0, JSObject * 0x02df79f0, unsigned int 2,
long * 0x04507850, long * 0x0012d3f0) line 1292 + 14 bytes
js_Invoke(JSContext * 0x00fb7aa0, unsigned int 2, unsigned int 0) line 843 + 23
bytes
js_Interpret(JSContext * 0x00fb7aa0, long * 0x0012e2b0) line 2828 + 15 bytes
js_Invoke(JSContext * 0x00fb7aa0, unsigned int 1, unsigned int 2) line 860 + 13
bytes
js_InternalInvoke(JSContext * 0x00fb7aa0, JSObject * 0x01703880, long 23497856,
unsigned int 0, unsigned int 1, long * 0x0012e50c, long * 0x0012e3dc) line 935 +
20 bytes
JS_CallFunctionValue(JSContext * 0x00fb7aa0, JSObject * 0x01703880, long
23497856, unsigned int 1, long * 0x0012e50c, long * 0x0012e3dc) line 3433 + 31 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x00fd9528, void * 0x01703880,
void * 0x01668c80, unsigned int 1, void * 0x0012e50c, int * 0x0012e510, int 0)
line 1040 + 33 bytes
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x046f40e0, nsIDOMEvent
* 0x039d8518) line 181 + 77 bytes
nsXBLPrototypeHandler::ExecuteHandler(nsIDOMEventReceiver * 0x037e37d0,
nsIDOMEvent * 0x039d8518) line 449
nsXBLEventHandler::DoMouse(nsIAtom * 0x02ee5bd8, nsIDOMEvent * 0x039d8518) line 149
nsXBLMouseHandler::MouseDown(nsXBLMouseHandler * const 0x037e3828, nsIDOMEvent *
0x039d8518) line 106
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x03072280,
nsIPresContext * 0x02f5dce8, nsEvent * 0x0012f7a4, nsIDOMEvent * * 0x0012f3a4,
nsIDOMEventTarget * 0x04835918, unsigned int 7, nsEventStatus * 0x0012f59c) line
1307 + 41 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x03072190, nsIPresContext *
0x02f5dce8, nsEvent * 0x0012f7a4, nsIDOMEvent * * 0x0012f3a4, unsigned int 7,
nsEventStatus * 0x0012f59c) line 3316
PresShell::HandleEventInternal(nsEvent * 0x0012f7a4, nsIView * 0x035def58,
unsigned int 1, nsEventStatus * 0x0012f59c) line 6262 + 47 bytes
PresShell::HandleEvent(PresShell * const 0x02f5d0a4, nsIView * 0x035def58,
nsGUIEvent * 0x0012f7a4, nsEventStatus * 0x0012f59c, int 0, int & 1) line 6186 +
25 bytes
nsViewManager::HandleEvent(nsView * 0x035def58, nsGUIEvent * 0x0012f7a4, int 0)
line 2208
nsView::HandleEvent(nsViewManager * 0x02f5c898, nsGUIEvent * 0x0012f7a4, int 0)
line 309
nsViewManager::DispatchEvent(nsViewManager * const 0x02f5c898, nsGUIEvent *
0x0012f7a4, nsEventStatus * 0x0012f6a0) line 1938 + 23 bytes
HandleEvent(nsGUIEvent * 0x0012f7a4) line 83
nsWindow::DispatchEvent(nsWindow * const 0x035df004, nsGUIEvent * 0x0012f7a4,
nsEventStatus & nsEventStatus_eIgnore) line 1147 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f7a4) line 1168
nsWindow::DispatchMouseEvent(unsigned int 302, unsigned int 1, nsPoint *
0x00000000) line 5429 + 21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 302, unsigned int 1, nsPoint *
0x00000000) line 5686
nsWindow::ProcessMessage(unsigned int 513, unsigned int 1, long 14680593, long *
0x0012fc34) line 4135 + 28 bytes
nsWindow::WindowProc(HWND__ * 0x000a0402, unsigned int 513, unsigned int 1, long
14680593) line 1434 + 27 bytes
USER32! 77e3a290()
USER32! 77e145b1()
USER32! 77e1a752()
nsAppShellService::Run(nsAppShellService * const 0x00fb55e0) line 480
main1(int 2, char * * 0x00271cd0, nsISupports * 0x00f11330) line 1269 + 32 bytes
main(int 2, char * * 0x00271cd0) line 1640 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e9ca90()

Comment 1

[Brendan Eich [:brendan]]

This shoulda been caught earlier, but I guess we don't have enough mail qa in
the community?  Or were we not listening to them?

Sorry I didn't try a 1.3 build sooner (I actually noticed this when I switched
from an old build [embarrassingly old] to my 1.4a trunk optimized build).

/be

Comment 2

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

the code that calls flush() in nsMailDatabase::UpdateFolderFlag() has been there
for a while.

here's the current fileStream usage pattern:

fileStream->write() 
(will call FileImpl::InternalFlush(), but won't call PR_Sync)

fileStream->flush()
(will call FileImpl::InternalFlush(), but *will* call PR_Sync)

fileStream->tell()
fileStream->seek()
(will call FileImpl::InternalFlush(), but won't call PR_Sync)

fileStream->readline()
fileStream->seek()
(will call FileImpl::InternalFlush(), but won't call PR_Sync)

fileStream->write()
(will call FileImpl::InternalFlush(), but won't call PR_Sync)

fileStream->flush()
(will call FileImpl::InternalFlush(), but *will* call PR_Sync)

fileStream->close()

I'm not sure we need to call flush() after each calls to write(), since
::InternalFlush() does get called.  but that optimization would just reduce one
call to PR_Sync()

But we still need to flush out the changes to disk, though.

Comment 3

[Brendan Eich [:brendan]]

I don't know what regressed, but something is worse.

Independent of that, there is *no* reason to sync after write.  Sync once per
second if you must, but do it via a timer.

/be

Comment 4

[David :Bienvenu]

we need to sync to keep the time stamp in the .msf file in sync with the actual
time stamp on disk. This is especially noticeable when users have their mail
folders on mounted drives. We should, however, only be syncing once per user
command (e.g., if the user marks 10 messages read, we only need to sync once).

Comment 5

[David :Bienvenu]

this is fixed - we only do it once now, after a mark read operation is done,
even for multiple messages. We do need to do it once - see bug 142196 for
testimonials, and be aware that 4.x did this as well.

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

How does PR_Sync() keep those more in date?  Are you worried about mid-operation
system crash updating the folder timestamp (synchronous metadata) but not the
contents of the .msf?  PR_Sync() won't prevent that from happening -- you can
still crash before issuing it, or the system can crash before or during the call.

Comment 7

[David :Bienvenu]

It's a mystery to me how PR_Sync keeps these time stamps more in sync. It only
seems to matter when the files are on a network drive. My guess is that it
somehow makes the network file system do the right thing and really truly flush
the data so the timestamp and file size are correct. As I've said before, this
is what 4.x did, and we had no reports of problems like this with 4.x, and
adding this Sync call to Mozilla fixed this problem for a number of people.