Closed
Bug 611843
Opened 14 years ago
Closed 12 years ago
Don't fsync extensions.ini
Categories
(Toolkit :: Add-ons Manager, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: taras.mozilla, Assigned: marco)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 3 obsolete files)
1.44 KB,
text/plain
|
Details | |
1.35 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•13 years ago
|
||
I'd like to do this, could someone point me to the code that does the fsync on that file?
Comment 2•13 years ago
|
||
I would guess in nsSafeFileOutputStream somewhere. We might also be able to just drop the use of nnsSafeFileOutputStream but I'm a bit wary of extensions.ini falling out of sync with extensions.sqlite.
Assignee | ||
Comment 3•13 years ago
|
||
Thank you, Dave. I've found the code at issue :) I don't know if it's enough, however.
Attachment #576318 -
Flags: review?(dtownsend)
Comment 4•13 years ago
|
||
So do we know for sure that the regular file output stream doesn't fsync?
Assignee | ||
Comment 5•13 years ago
|
||
In nsFileStreams.cpp I see that nsSafeFileOutputStream calls Flush() (that calls PR_Sync) when it's closed, unlike nsFileOutputStream. Infact there isn't any function to close the normal (not safe) stream.
Attachment #576318 -
Attachment is obsolete: true
Attachment #576339 -
Flags: review?(dtownsend)
Attachment #576318 -
Flags: review?(dtownsend)
Comment 6•13 years ago
|
||
(In reply to Marco Castelluccio from comment #5) > Created attachment 576339 [details] [diff] [review] [diff] [details] [review] > Patch v2 > > In nsFileStreams.cpp I see that nsSafeFileOutputStream calls Flush() (that > calls PR_Sync) when it's closed, unlike nsFileOutputStream. > Infact there isn't any function to close the normal (not safe) stream. Regular file output streams have a close function which calls PR_Close, I don't know if this fsyncs or not.
Comment 7•13 years ago
|
||
IIRC, I simply ran it with a debugger and it fsynced.
Assignee | ||
Comment 8•13 years ago
|
||
I think PR_Close shouldn't sync, it should only close the file descriptor. David, which file fsynced? "extensions.ini" or "extensions.ini.tmp"? If it's "extensions.ini", maybe somewhere there's another fsync I didn't see.
Comment 9•13 years ago
|
||
Comment on attachment 576339 [details] [diff] [review] Patch v2 Review of attachment 576339 [details] [diff] [review]: ----------------------------------------------------------------- There seems to be some confusion here over whether this helps or not. Can you figure that out then re-request review?
Attachment #576339 -
Flags: review?(dtownsend)
Assignee | ||
Comment 10•13 years ago
|
||
Dave, look at the patch for bug 611840 (that is also about fsync). The author used PR_Close to close the file descriptor. However I'll see with a debugger if it fsyncs, just to be sure ;)
Assignee | ||
Comment 11•12 years ago
|
||
Without the patch, it fsyncs. With the patch, it doesn't fsync.
Assignee | ||
Updated•12 years ago
|
Attachment #576339 -
Flags: review?(dtownsend)
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → mar.castelluccio
Comment 12•12 years ago
|
||
Have you run this through try server at all? I'm a little surprised that we'd be able to move the file before closing it on windows. I think I'd like to see an explicit close before the move since that ensures the data is all written.
Comment 13•12 years ago
|
||
Comment on attachment 576339 [details] [diff] [review] Patch v2 Yeah as I suspected this fails on windows. You need to close the file before moving it.
Attachment #576339 -
Flags: review?(dtownsend) → review-
Assignee | ||
Comment 14•12 years ago
|
||
This is the try run: https://tbpl.mozilla.org/?tree=Try&rev=5b8e839e98a2
Attachment #576339 -
Attachment is obsolete: true
Attachment #613538 -
Flags: review?
Assignee | ||
Updated•12 years ago
|
Attachment #613538 -
Flags: review? → review?(dtownsend+bugmail)
Comment 15•12 years ago
|
||
Comment on attachment 613538 [details] [diff] [review] Patch v3 Review of attachment 613538 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/XPIProvider.jsm @@ +5641,5 @@ > > if (fullCount > 0) { > LOG("Writing add-ons list"); > + > + let addonsListTmp = FileUtils.getFile(KEY_PROFILEDIR, [FILE_XPI_ADDONS_LIST+".tmp"], Nit: spaces around operators
Attachment #613538 -
Flags: review?(dtownsend+bugmail) → review+
Assignee | ||
Comment 16•12 years ago
|
||
Addressed the nit
Attachment #613538 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 18•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/99f3fe5ff9c4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•