971686 - Backups can't be properly stored if the /tmp folder is on a different filesystem

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

writeAtomic can't ensure to be atomic when moving across filesystems, so it fails moving the file and we can't store the backups.
At this point I guess we should go back to the original solution that stored the .tmp file in the dest folder, and cleanup dangling .tmp files in maintenance.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1

Comment 2

[Marco Bonardo [:mak]]

I will merge this into the patch in bug 824433 for uplift.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8374824 [details] [diff] [review]
patch v1

David, would you mind reviewing this? I need to uplift the whole thing soonish.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8374824 [details] [diff] [review]
patch v1

Review of attachment 8374824 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

::: toolkit/components/places/PlacesBackups.jsm
@@ +132,5 @@
>        let backupFolderPath = yield this.getBackupFolder();
>        let iterator = new OS.File.DirectoryIterator(backupFolderPath);
>        yield iterator.forEach(function(aEntry) {
> +        // Since this is a lazy getter, we can safely remove any .tmp file
> +        // without risking to remove an active backup.

Also, since I/O calls are serialized, we are sure that OS.File.remove cannot interrupt OS.File.writeAtomic.

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.1

slightly better comment

Comment 6

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f504ccd3c3

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c8f504ccd3c3