GNOME Bugzilla – Bug 691794
Bookmarks lost after crash
Last modified: 2013-11-29 10:02:06 UTC
The bookmarks saving in epiphany uses ephy_file_switch_temp_file(), which is racy and doesn't save the file atomically. Instead, the bookmarks code should be using g_file_set_contents() which will create a temporary file, and move it over the original. If either of those fail, the original file will remain untouched.
So you say that the promise of ephy_file_switch_temp_file() of saving atomically is just bs? That's bad.
Created attachment 234178 [details] [review] ephy-node-db: use g_file_set_contents() when saving to disk Since g_file_set_contents() is atomic and already takes care of saving first to a temporary file, we don't need to use ephy_file_switch_temp_file().
Created attachment 234179 [details] [review] ephy-bookmarks-export: use g_file_set_contents() when saving to RDF
Next would be nuke that method as it's no longer used.
Review of attachment 234178 [details] [review]: ::: lib/ephy-node-db.c @@ +512,3 @@ ret = -1; } + xmlBufferFree (buffer); This should go into the failed: section.
(In reply to comment #1) > So you say that the promise of ephy_file_switch_temp_file() of saving > atomically is just bs? That's bad. It's racy because it checks for the file's existence by hand, instead of relying on the retval from the system calls.
Review of attachment 234178 [details] [review]: Looks good, agreed with your comment there.
Review of attachment 234179 [details] [review]: ::: src/bookmarks/ephy-bookmarks-export.c @@ +434,2 @@ { + g_warning ("Error saving bookmarks as RDF: %s\n", error->message); There's kinda a log already below no? I guess we should have one or the other, but not both.
Attachment 234178 [details] pushed as fdf27e6 - ephy-node-db: use g_file_set_contents() when saving to disk Attachment 234179 [details] pushed as 213b032 - ephy-bookmarks-export: use g_file_set_contents() when saving to RDF
I also removed already the racy method.
*** Bug 662549 has been marked as a duplicate of this bug. ***