Bug 54774 – [fileapi] Implement LocalFileSystem.resolveFileSystemURI

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adam Klein]

[fileapi] Implement LocalFileSystem.resolveFileSystemURI

[Adam Klein]

Created attachment 83014 [details]
Patch

[Adam Klein]

Note that patch still needs a bit of code to compare the origin URL to the caller's security origin.

[Adam Klein]

Created attachment 83025 [details]
Patch

[Eric U.]

Comment on attachment 83025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83025&action=review

> LayoutTests/fast/filesystem/resources/resolve-uri.js:14
> +    testFailed("Error occured:" + error.code);

Typo: occurred

> LayoutTests/fast/filesystem/resources/resolve-uri.js:38
> +var jsTestIsAsync = true;

I think it's worth testing more than one URL.
For example, try one with a directory, try both temporary and persistent, try the root directory, try removing the entry before resolving the URL.
Also, generate some by hand and test a few kinds of malformed URLs, and try directories with and without the trailing slash.

> Source/WebCore/fileapi/DOMFileSystemBase.cpp:53
> +const char DOMFileSystemBase::kTemporaryPathString[] = "temporary";

Now that we've got these constants defined, we should really use them in toURI as well.

> Source/WebCore/fileapi/DOMFileSystemBase.cpp:62
> +    if (path.isEmpty() || !path[0] == '/')

Do you mean:

	path[0] != '/'

?

> Source/WebCore/fileapi/DOMFileSystemBase.cpp:75
> +    if (path.isEmpty() || !path[0] == '/')

Same here.

> Source/WebCore/page/DOMWindow.cpp:766
> +    if (!AsyncFileSystem::isAvailable() || !securityOrigin->canAccessFileSystem() || !securityOrigin->canRequest(parsedURL)) {

I'm not sure this is strict enough.  Should the fact that the security origin can load from this URL be enough to grant it [writeable] access to the Entry?
Currently we're talking about not letting any cross-origin requests be made at all, but if we should relax that, we wouldn't necessarily want to allow this as well.

[Adam Barth]

Comment on attachment 83025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83025&action=review

This could use a bunch more tests for the URL cracking code.  You're not testing any of the error cases, as far as I can tell.

> Source/WebCore/fileapi/DOMFileSystemBase.cpp:68
> +        path = path.substring(sizeof(kTemporaryPathString) - 1);

pls use a constant for this value.

>> Source/WebCore/fileapi/DOMFileSystemBase.cpp:75
>> +    if (path.isEmpty() || !path[0] == '/')
> 
> Same here.

Please test the cases where the URL contains \ instead of / to make sure we're canonicalizing things properly.

> Source/WebCore/fileapi/DOMFileSystemBase.h:64
> +    static const char kPersistentPathString[];
> +    static const char kTemporaryPathString[];

Another way to do this is to have static functions that return const String& and use DEFINE_STATIC_LOCAL to define the strings.  That way, you get the length for free and it's easier to use various String methods.

> Source/WebCore/page/DOMWindow.cpp:765
> +    KURL parsedURL(ParsedURLString, uri);

This is unlikely to be correct.  ParsedURLString means that |uri| has already been canonicalized by KURL, but |uri| here can be an arbitrary string.  What is the expected behavior when |uri| is a string like "foo" ?

[Kinuko Yasuda]

Comment on attachment 83025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83025&action=review

>> Source/WebCore/fileapi/DOMFileSystemBase.h:64
>> +    static const char kTemporaryPathString[];
> 
> Another way to do this is to have static functions that return const String& and use DEFINE_STATIC_LOCAL to define the strings.  That way, you get the length for free and it's easier to use various String methods.

If you do this please make sure it's called only on the document thread (maybe with an assert).  (As most of the DOMFileSystemBase's methods are called on both main thread and worker thread.)

> Source/WebCore/page/DOMWindow.cpp:778
> +    LocalFileSystem::localFileSystem().readFileSystem(document, type, 0, ResolveURICallbacks::create(successCallback, errorCallback, document, filePath));

(Not a comment for this change) actually probably we should drop the 'size' parameter from readFileSystem as it's supposed to return a read-only file system.

[Adam Klein]

Comment on attachment 83025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83025&action=review

>> LayoutTests/fast/filesystem/resources/resolve-uri.js:14
>> +    testFailed("Error occured:" + error.code);
> 
> Typo: occurred

Fixed.

>> LayoutTests/fast/filesystem/resources/resolve-uri.js:38
>> +var jsTestIsAsync = true;
> 
> I think it's worth testing more than one URL.
> For example, try one with a directory, try both temporary and persistent, try the root directory, try removing the entry before resolving the URL.
> Also, generate some by hand and test a few kinds of malformed URLs, and try directories with and without the trailing slash.

Sorry the initial upload didn't include more tests; will add lots more. I mainly need to come up with a bit more framework to make it easy to add new testcases.

>> Source/WebCore/fileapi/DOMFileSystemBase.cpp:53
>> +const char DOMFileSystemBase::kTemporaryPathString[] = "temporary";
> 
> Now that we've got these constants defined, we should really use them in toURI as well.

Only not using them there because that usage is moving, in another patch, from one file to the other.  Depending on which lands first, I'll consolidate, and there won't be any more references to the bare string.

>> Source/WebCore/fileapi/DOMFileSystemBase.cpp:62
>> +    if (path.isEmpty() || !path[0] == '/')
> 
> Do you mean:
> 
>     path[0] != '/'
> 
> ?

Wow, I did, that's really bad. I think I'm too used to unittests catching my mistakes.  Fixed.

>> Source/WebCore/fileapi/DOMFileSystemBase.cpp:68
>> +        path = path.substring(sizeof(kTemporaryPathString) - 1);
> 
> pls use a constant for this value.

Will do.

>>> Source/WebCore/fileapi/DOMFileSystemBase.cpp:75
>>> +    if (path.isEmpty() || !path[0] == '/')
>> 
>> Same here.
> 
> Please test the cases where the URL contains \ instead of / to make sure we're canonicalizing things properly.

Fixed the logical error. And will add this to the list of tests I need to create. The '\' '/' thing worked correctly with GURL, but you're right that I haven't checked into KURL's behavior.

>>> Source/WebCore/fileapi/DOMFileSystemBase.h:64

>> 
>> Another way to do this is to have static functions that return const String& and use DEFINE_STATIC_LOCAL to define the strings.  That way, you get the length for free and it's easier to use various String methods.
> 
> If you do this please make sure it's called only on the document thread (maybe with an assert).  (As most of the DOMFileSystemBase's methods are called on both main thread and worker thread.)

This will also be used, eventually, from worker threads, so if DEFINE_STATIC_LOCAL isn't threadsafe it seems I shouldn't use it at all.

>> Source/WebCore/page/DOMWindow.cpp:765
>> +    KURL parsedURL(ParsedURLString, uri);
> 
> This is unlikely to be correct.  ParsedURLString means that |uri| has already been canonicalized by KURL, but |uri| here can be an arbitrary string.  What is the expected behavior when |uri| is a string like "foo" ?

I guess I was reading KURL's interface poorly: I was interpreting "ParsedURLString" as "ParseURLString", an instruction of what to do to the argument rather than a description of that argument. I'd expected that I'd end up with an invalid URL if the string was "foo".  If it was, e.g., "http://www.google.com/index.html", DOMFileSystemBase::crackFileSystemURL() will complain about the wrong protocol.

My next patch upload will add lots more tests with random strings.  But do you have a better suggestion for how to tell whether this looks like a valid URL?  Should I see if it begins with "filesystem:" before doing anything else?

>> Source/WebCore/page/DOMWindow.cpp:766
>> +    if (!AsyncFileSystem::isAvailable() || !securityOrigin->canAccessFileSystem() || !securityOrigin->canRequest(parsedURL)) {
> 
> I'm not sure this is strict enough.  Should the fact that the security origin can load from this URL be enough to grant it [writeable] access to the Entry?
> Currently we're talking about not letting any cross-origin requests be made at all, but if we should relax that, we wouldn't necessarily want to allow this as well.

For now this check is exactly as strict as you suggest: canRequest requires same-origin, so this effectively makes the file only available on the same origin.  I used a call to securityOrigin rather than re-implementing that logic here.  But do you think I should instead duplicate the logic, in case we decide to loosen the canRequest restrictions?  It seems unlikely to me that we'd really loosen canRequest (which is for XHRs), though we might loosen canDisplay (for, e.g., <img> tags).

>> Source/WebCore/page/DOMWindow.cpp:778
>> +    LocalFileSystem::localFileSystem().readFileSystem(document, type, 0, ResolveURICallbacks::create(successCallback, errorCallback, document, filePath));
> 
> (Not a comment for this change) actually probably we should drop the 'size' parameter from readFileSystem as it's supposed to return a read-only file system.

Are there any other callers of readFileSystem? I didn't see any.  I could just do that in this change...

[Adam Klein]

Created attachment 83397 [details]
Patch

[Adam Klein]

Created attachment 83404 [details]
Renamed to resolveLocalFileSystemURI

[Kinuko Yasuda]

Comment on attachment 83404 [details]
Renamed to resolveLocalFileSystemURI

The FS part is looking good.

View in context: https://bugs.webkit.org/attachment.cgi?id=83404&action=review

> Source/WebCore/fileapi/FileSystemCallbacks.cpp:248
> +    if (m_successCallback) {

I think we do not need m_successCallback check here (and line 225 above).  If the user only provides errorCallback (it doesn't make sense but technically possible) the current code doesn't work as expected.
(Or we can skip the entire operation if neither callbacks are given, as it's read-only operation.)


As for readFileSystem, no I don't think there's any other call site. We used to have some code for devtools but the code was reverted due to devtool code refactoring.
If you're going to upload another patch please feel free to drop the size parameter.

[Eric U.]

Comment on attachment 83404 [details]
Renamed to resolveLocalFileSystemURI

View in context: https://bugs.webkit.org/attachment.cgi?id=83404&action=review

> Source/WebCore/fileapi/DOMFileSystemBase.cpp:57
> +bool DOMFileSystemBase::crackFileSystemURL(const KURL& url, AsyncFileSystem::Type& type, String& filePath)

We don't tend to use non-const reference parameters in the filesystem code.  It's not explicitly banned by the webkit style guide, but we prefer to use pointers for output parameters.

[Adam Klein]

Comment on attachment 83404 [details]
Renamed to resolveLocalFileSystemURI

View in context: https://bugs.webkit.org/attachment.cgi?id=83404&action=review

>> Source/WebCore/fileapi/DOMFileSystemBase.cpp:57
>> +bool DOMFileSystemBase::crackFileSystemURL(const KURL& url, AsyncFileSystem::Type& type, String& filePath)
> 
> We don't tend to use non-const reference parameters in the filesystem code.  It's not explicitly banned by the webkit style guide, but we prefer to use pointers for output parameters.

I was doing this to match what I perceived as WebKit style: there's no reference counting going on here, and these should never be null. So references seem appropriate.  Per a conversation just now on #webkit:

<aklein> what's the preferred style for "out" parameters in WebKit? it seems references are pretty common, are they preferred over pointers?
<abarth> aklein: yes

>> Source/WebCore/fileapi/FileSystemCallbacks.cpp:248
>> +    if (m_successCallback) {
> 
> I think we do not need m_successCallback check here (and line 225 above).  If the user only provides errorCallback (it doesn't make sense but technically possible) the current code doesn't work as expected.
> (Or we can skip the entire operation if neither callbacks are given, as it's read-only operation.)
> 
> 
> As for readFileSystem, no I don't think there's any other call site. We used to have some code for devtools but the code was reverted due to devtool code refactoring.
> If you're going to upload another patch please feel free to drop the size parameter.

Removed checks for m_successCallback (and calls to clear()).

And removed "size" argument to readFileSystem(), hiding the special-case of 0 inside LocalFileSystemChromium

[Adam Klein]

Created attachment 83740 [details]
Patch

[Adam Barth]

Comment on attachment 83740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83740&action=review

Some minor nits.  The one thing that seems slightly incorrect is passing a random string to KURL as if it were a canonicalized URLString.  We're working to improve the type system to make that impossible.  If you were writing this patch in a few months, the type system would prevent you from writing that line of code.  Please consider changing it before landing.

Thanks!

> LayoutTests/ChangeLog:34
> +        (expectedAndRunNext):
> +        (errorCallback):
> +        (createTestFile):
> +        ():
> +        (assertIsDirectory):
> +        (assertIsFile):
> +        (runBasicTest):
> +        (runHandmadeURI):
> +        (runBogusURI):
> +        (runWrongProtocol):
> +        (runNotEnoughSlashes):
> +        (runNotEnoughSlashes2):
> +        (runUseBackSlashes.):
> +        (runUseBackSlashes):
> +        (runDirectory.):
> +        (runDirectory):
> +        (runWithTrailingSlash.):
> +        (runWithTrailingSlash):
> +        (runNextTest):
> +        (cleanupAndRunNext):
> +        (fileSystemCallback):
> +        (var):

I would just remove this boilerplate from the changelog.  It's not really helping anyone.

> Source/WebCore/fileapi/DOMFileSystemBase.cpp:68
> +    path = path.substring(1);
> +
> +    if (path.startsWith(kTemporaryPathPrefix)) {

Can you give an offset to startsWith to avoid the extra malloc from creating the substring?

> Source/WebCore/fileapi/FileSystemCallbacks.cpp:195
> +// ResolveURICallbacks --------------------------------------------------------

We don't usually include these sorts of comments.

> Source/WebCore/fileapi/FileSystemCallbacks.cpp:223
> +bool ErrorCallbackWrapper::handleEvent(FileError*)

The error doesn't matter?

> Source/WebCore/fileapi/FileSystemCallbacks.cpp:229
> +} // namespace (anonymous)

Usually we would omit the (anonymous) part of this comment.

> Source/WebCore/fileapi/FileSystemCallbacks.cpp:251
>  // MetadataCallbacks ----------------------------------------------------------

I see that this file has a bunch of these comment already.

> Source/WebCore/page/DOMWindow.cpp:765
> +    KURL parsedURL(ParsedURLString, uri);

This still seems strange to me.  Everywhere else we call competeURL.  If this is what the spec says, it's probably wrong.  :(

[Adam Klein]

Comment on attachment 83740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83740&action=review

>> LayoutTests/ChangeLog:34
>> +        (var):
> 
> I would just remove this boilerplate from the changelog.  It's not really helping anyone.

Removed.

>> Source/WebCore/fileapi/DOMFileSystemBase.cpp:68
>> +    if (path.startsWith(kTemporaryPathPrefix)) {
> 
> Can you give an offset to startsWith to avoid the extra malloc from creating the substring?

I don't see anything in WTF::String matching std::string::compare(), which is what I'd need. I could play some tricks with characters(), memcmp(), and min(), if you think it's worth the loss of clarity.

>> Source/WebCore/fileapi/FileSystemCallbacks.cpp:195
>> +// ResolveURICallbacks --------------------------------------------------------
> 
> We don't usually include these sorts of comments.

As you see below, these are just matching the rest of the file.  Let me know if you think they should just be removed.

>> Source/WebCore/fileapi/FileSystemCallbacks.cpp:223
>> +bool ErrorCallbackWrapper::handleEvent(FileError*)
> 
> The error doesn't matter?

Good point, I've changed this to only look retry with getFile if the error is TYPE_MISMATCH_ERR, which the spec says _must_ be returned in the case we're worried about here.

But it turns out Chromium does this wrong and returns INVALID_STATE_ERR.  I've added a FIXME here and will clean this up in a future change.

>> Source/WebCore/fileapi/FileSystemCallbacks.cpp:229
>> +} // namespace (anonymous)
> 
> Usually we would omit the (anonymous) part of this comment.

Removed.

>> Source/WebCore/page/DOMWindow.cpp:765
>> +    KURL parsedURL(ParsedURLString, uri);
> 
> This still seems strange to me.  Everywhere else we call competeURL.  If this is what the spec says, it's probably wrong.  :(

Eric, can you comment here?  Are these URIs allowed to be relative?

[Adam Klein]

Created attachment 83844 [details]
Patch

[Michael Nordman]

Comment on attachment 83740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83740&action=review

>>> Source/WebCore/page/DOMWindow.cpp:765
>>> +    KURL parsedURL(ParsedURLString, uri);
>> 
>> This still seems strange to me.  Everywhere else we call competeURL.  If this is what the spec says, it's probably wrong.  :(

Looks like there are two issues...

1) I think this string is raw user input, if so we definitely shouldn't assume that it's a valid url. The comments on the KURL ctor being used imply that the string is known to be a valid url. I think a better ctor to parse an absolute url (of questionable validity) would be...
   KURL(const KURL& base, const String& relative)
... passing in an empty KURL() as the 'base' parameter.

2) If relative urls are supposed to be valid inputs to this scriptable method, we should be using doc->completeURL(str). I think the envisioned use case was an HTTP loaded page resolving filesystem urls... but good question... what if the page location is filesystem:something?

[Eric U.]

Comment on attachment 83740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83740&action=review

>>>> Source/WebCore/page/DOMWindow.cpp:765
>>>> +    KURL parsedURL(ParsedURLString, uri);
>>> 
>>> This still seems strange to me.  Everywhere else we call competeURL.  If this is what the spec says, it's probably wrong.  :(
>> 
>> Eric, can you comment here?  Are these URIs allowed to be relative?
> 
> Looks like there are two issues...
> 
> 1) I think this string is raw user input, if so we definitely shouldn't assume that it's a valid url. The comments on the KURL ctor being used imply that the string is known to be a valid url. I think a better ctor to parse an absolute url (of questionable validity) would be...
>    KURL(const KURL& base, const String& relative)
> ... passing in an empty KURL() as the 'base' parameter.
> 
> 2) If relative urls are supposed to be valid inputs to this scriptable method, we should be using doc->completeURL(str). I think the envisioned use case was an HTTP loaded page resolving filesystem urls... but good question... what if the page location is filesystem:something?

If the page location is already in the filesystem, then yes, relative urls should work.
You should be able to build a full static website in the filesystem, then copy it up to your server, and it should work identically in both places.

[Adam Klein]

Comment on attachment 83844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83844&action=review

> Source/WebCore/fileapi/DOMFileSystemBase.cpp:62
> +    KURL originURL(ParsedURLString, url.path());

Note that though I've switched to completeURL() below, I haven't here.  Is there any other URL-parsing code I should be calling? I can't really guarantee anything about the contents of url.path() (certainly not that it's the output of KURL::string()).  But passing in a Document and calling completeURL() here is obviously the wrong thing to do.

> Source/WebCore/page/DOMWindow.cpp:765
> +    KURL parsedURL(ParsedURLString, uri);

I've changed this to a call to document->completeURL() in my workdir.

[Adam Barth]

Comment on attachment 83844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83844&action=review

>> Source/WebCore/fileapi/DOMFileSystemBase.cpp:62
>> +    KURL originURL(ParsedURLString, url.path());
> 
> Note that though I've switched to completeURL() below, I haven't here.  Is there any other URL-parsing code I should be calling? I can't really guarantee anything about the contents of url.path() (certainly not that it's the output of KURL::string()).  But passing in a Document and calling completeURL() here is obviously the wrong thing to do.

Yeah, this case is harder.  You can try resolving it against a base URL of "about:blank".  This is probably also ok for now.  I suspect we'll need to revisit this line of code when we start deploying the URLString type more widely.

[Adam Klein]

Created attachment 83889 [details]
Patch for cq

[Adam Klein]

Created attachment 84078 [details]
Patch for cq; merged r79778

[Adam Barth]

Comment on attachment 84078 [details]
Patch for cq; merged r79778

Okiedokes

[WebKit Commit Bot]

Comment on attachment 84078 [details]
Patch for cq; merged r79778

Clearing flags on attachment: 84078

Committed r79917: <http://trac.webkit.org/changeset/79917>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/79917 might have broken GTK Linux 64-bit Debug