Closed Bug 611843 Opened 9 years ago Closed 8 years ago

Don't fsync extensions.ini

Categories

(Toolkit :: Add-ons Manager, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: taras.mozilla, Assigned: marco)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

No description provided.
I'd like to do this, could someone point me to the code that does the fsync on that file?
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.
Attached patch Patch (obsolete) — Splinter Review
Thank you, Dave. I've found the code at issue :)
I don't know if it's enough, however.
Attachment #576318 - Flags: review?(dtownsend)
So do we know for sure that the regular file output stream doesn't fsync?
Attached patch Patch v2 (obsolete) — Splinter Review
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)
(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.
IIRC, I simply ran it with a debugger and it fsynced.
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 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)
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 ;)
Attached file strace log
Without the patch, it fsyncs. With the patch, it doesn't fsync.
Attachment #576339 - Flags: review?(dtownsend)
Status: NEW → ASSIGNED
Assignee: nobody → mar.castelluccio
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 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-
Attached patch Patch v3 (obsolete) — Splinter Review
This is the try run: https://tbpl.mozilla.org/?tree=Try&rev=5b8e839e98a2
Attachment #576339 - Attachment is obsolete: true
Attachment #613538 - Flags: review?
Attachment #613538 - Flags: review? → review?(dtownsend+bugmail)
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+
Attached patch Patch v4Splinter Review
Addressed the nit
Attachment #613538 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/99f3fe5ff9c4
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/99f3fe5ff9c4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.