Bug 27954 – [Chromium] Make it possible for FileSystem routines to work when outside the sandbox.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

We can no longer assume that WebKit is always run within a sandbox.  Code running in the browser process should not use the VFS.  File system operations should be implementable.

[Jeremy Orlow]

Created attachment 34005 [details]
Patch v1

[Jeremy Orlow]

Created attachment 34029 [details]
Patch v1

[Dumitru Daniliuc]

LGTM.

Minor: add NOTREACHED()s to SQLiteFileSystemChromium{Mac,Linux}.cpp

[Jeremy Orlow]

Created attachment 34030 [details]
Patch v1

[Darin Fisher (:fishd, Google)]

Comment on attachment 34030 [details]
Patch v1

> Index: WebCore/platform/chromium/ChromiumBridge.h
...
>          // File ---------------------------------------------------------------
> -        static bool getFileSize(const String& path, long long& result);
> +        static bool sandboxEnabled();

I think sandboxEnabled is not specific to file system stuff.  How about putting
this in a separate section?


> Index: WebCore/platform/sql/chromium/SQLiteFileSystemChromiumLinux.cpp
...
>      // stub for registering Chromium's SQLite VFS for Linux
> +    ASSERT_NOT_REACHED();

Isn't notImplemented() more correct?


> Index: WebCore/platform/sql/chromium/SQLiteFileSystemChromiumMac.cpp
...
>      // stub for registering Chromium's SQLite VFS for Mac
> +    ASSERT_NOT_REACHED();

ditto


> Index: WebCore/platform/sql/chromium/SQLiteFileSystemChromiumWin.cpp
...
> +    // FIXME: Make sure there aren't any unintended consequences when VFS code is called in the browser process.
> +    if (!ChromiumBridge::sandboxEnabled()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

notImplemented?


These issues aren't major, so R=me

[Jeremy Orlow]

(In reply to comment #5)
> (From update of attachment 34030 [details])
> > Index: WebCore/platform/chromium/ChromiumBridge.h
> ...
> >          // File ---------------------------------------------------------------
> > -        static bool getFileSize(const String& path, long long& result);
> > +        static bool sandboxEnabled();
> 
> I think sandboxEnabled is not specific to file system stuff.  How about putting
> this in a separate section?

I'll make this change.

> > Index: WebCore/platform/sql/chromium/SQLiteFileSystemChromiumLinux.cpp
> ...
> >      // stub for registering Chromium's SQLite VFS for Linux
> > +    ASSERT_NOT_REACHED();
> 
> Isn't notImplemented() more correct?

Not really.  notImplemented is not very noisy.  In this case, if we're hitting that code, something is very wrong.

Either way, the code should be going away as Dumi completes the implementation.

[Jeremy Orlow]

Sending        WebCore/ChangeLog
Sending        WebCore/platform/chromium/ChromiumBridge.h
Sending        WebCore/platform/chromium/FileSystemChromium.cpp
Sending        WebCore/platform/sql/chromium/SQLiteFileSystemChromium.cpp
Sending        WebCore/platform/sql/chromium/SQLiteFileSystemChromiumLinux.cpp
Sending        WebCore/platform/sql/chromium/SQLiteFileSystemChromiumMac.cpp
Sending        WebCore/platform/sql/chromium/SQLiteFileSystemChromiumWin.cpp
Transmitting file data .......
Committed revision 46741.
http://trac.webkit.org/changeset/46741