Bug 192562 - [GLib] FileSystem::moveFile() should fall back to copying
Summary: [GLib] FileSystem::moveFile() should fall back to copying
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords: InRadar
Depends on:
Blocks: 154553
  Show dependency treegraph
 
Reported: 2018-12-10 11:32 PST by Adrian Perez
Modified: 2018-12-10 12:47 PST (History)
8 users (show)

See Also:


Attachments
Patch (1.71 KB, patch)
2018-12-10 11:56 PST, Adrian Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrian Perez 2018-12-10 11:32:58 PST
Note that:

 - Cocoa (in “Source/WebCore/platform/cocoa/FileSystemCocoa.mm”) uses
   [NSFileManager movItemAtURL:toURL:], which falls back to copying, as per
   https://developer.apple.com/documentation/foundation/nsfilemanager/1414750-moveitematurl?language=objc 

 - Windows (in “Source/WebCore/platform/win/FileSystemWin.cpp”) uses
   MoveFileEx(), passing flags MOVEFILE_COPY_ALLOWED|MOVEFILE_REPLACE_EXISTING

But in the other hand:

 - “Source/WebCore/platform/posix/FileSystemPOSIX.cpp” uses a plain
   rename(2), which does NOT fall-back to copying.

 - “Source/WebCore/platform/glib/FileSystemGlib.cpp” uses g_rename(),
   which ends up calling rename(2) -- why this does not simply use
   the POSIX implmentation beats me ¯\_(ツ)_/¯

While working on the content extensions code, I have noticed that compiling
a content extension involves creating a temporary file, and then moving it
to its final location. For the GLib-based ports (GTK+, WPE) on Unix, the
temporary file will be created under “/tmp”, and then FileSystem::moveFile()
is used to place it in its final location, typically under the user's $HOME
directory.

There is a catch, though: frequently “/tmp” is on a different mount point
than the final location! This is *very* often true in modern GNU/Linux
distributions that have “/tmp” on tmpfs. Or, even if “/tmp” is the same
file system as “/”, not uncommonly “/home” can be a separate file system
itself.

To me it is quite clear that FileSyste::moveFile() has to fall-back to
copying, otherwise anything that makes use if this function will be far
too easy to randomly break in unexptected ways.
Comment 1 Adrian Perez 2018-12-10 11:42:11 PST
For FileSystemGLib I am planning to rewrite the function to use
g_file_move(), which does implements a copy+delete fallback.
Comment 2 Adrian Perez 2018-12-10 11:56:08 PST
Created attachment 356977 [details]
Patch
Comment 3 WebKit Commit Bot 2018-12-10 12:46:36 PST
Comment on attachment 356977 [details]
Patch

Clearing flags on attachment: 356977

Committed r239043: <https://trac.webkit.org/changeset/239043>
Comment 4 WebKit Commit Bot 2018-12-10 12:46:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Radar WebKit Bug Importer 2018-12-10 12:47:27 PST
<rdar://problem/46604998>