Bug 27954

Summary: [Chromium] Make it possible for FileSystem routines to work when outside the sandbox.
Product: WebKit Reporter: Jeremy Orlow <jorlow>
Component: New BugsAssignee: Jeremy Orlow <jorlow>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dumi, fishd
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch v1
none
Patch v1
none
Patch v1 fishd: review+

Description Jeremy Orlow 2009-08-03 14:44:26 PDT
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.
Comment 1 Jeremy Orlow 2009-08-03 14:50:24 PDT
Created attachment 34005 [details]
Patch v1
Comment 2 Jeremy Orlow 2009-08-03 17:23:09 PDT
Created attachment 34029 [details]
Patch v1
Comment 3 Dumitru Daniliuc 2009-08-03 17:34:08 PDT
LGTM.

Minor: add NOTREACHED()s to SQLiteFileSystemChromium{Mac,Linux}.cpp
Comment 4 Jeremy Orlow 2009-08-03 17:36:31 PDT
Created attachment 34030 [details]
Patch v1
Comment 5 Darin Fisher (:fishd, Google) 2009-08-03 17:44:58 PDT
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
Comment 6 Jeremy Orlow 2009-08-03 17:51:54 PDT
(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.
Comment 7 Jeremy Orlow 2009-08-03 18:09:08 PDT
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