Bug 116096 – DOMFileSystemBase: merge BlackBerry implementation

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Alberto Garcia]

As a follow-up to bug 114950, I think we can remove
DOMFileSystemBlackBerry.cpp completely if we do the necessary changes
to DOMFileSystemBase.cpp

Here is the situation:

- supportsToURL() and isValidType() can be directly removed from the
  BlackBerry file since the cross-platform implementation is
  identical.

- crackFileSystemURL() adds code to deal with the cases where
  url.innerURL() is NULL. That's the case for non-GoogleURL, so I
  think we can just move this part to the cross-platform code.

  Actually, since there are no GoogleURLs anymore we can also remove
  KURL::innerURL() and all code that expects it to be non-NULL (I just
  checked, there's only a couple of cases).

- createFileSystemURL() has a slightly different implementation.

  I belive the cross-platform code is wrong, though:

  KURL url = m_filesystemRootURL;
  // Remove the extra leading slash.
  url.setPath(url.path() + encodeWithURLEscapeSequences(fullPath.substring(1)));

  The problem here is that KURL::setPath() adds a leading slash if it's
  not present, which converts this root URL

    filesystem:file:///temporary/

  into

    filesystem:/file:///temporary/

  Additionally, the cross-platform code strips the leading slash from
  fullPath. This doesn't work in BlackBerry because
  m_filesystemRootURL doesn't contain a trailing slash, but I think we
  can fix that easily.

[Alberto Garcia]

Created attachment 201699 [details]
Patch

Here goes my first proposal.

[Darin Adler]

Comment on attachment 201699 [details]
Patch

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

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:87
> +        KURL originURL(ParsedURLString, url.path());

Is it really OK to parse just the path as a URL and claim it is a “parsed URL string”? What are you trying to accomplish here? How will originURL.path() be different from url.path()?

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:90
> +        if (path.isEmpty() || path[0] != '/')
> +            return false;

No need for the path.isEmpty() check here. path[0] will return 0 if the path is empty.

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:103
> +        if (path.isEmpty() || path[0] != '/')
> +            return false;

Ditto.

[Alberto Garcia]

(In reply to comment #2)
> > +        KURL originURL(ParsedURLString, url.path());

> Is it really OK to parse just the path as a URL and claim it is a
> “parsed URL string”? What are you trying to accomplish here? How
> will originURL.path() be different from url.path()?

The URL is something like this:

filesystem:file:///temporary/foo

In this case originURL would be file:///temporary/foo
and originURL.path() would be /temporary/foo

> > Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:90
> > +        if (path.isEmpty() || path[0] != '/')
> > +            return false;

> No need for the path.isEmpty() check here. path[0] will return 0 if
> the path is empty.

You're right, I'll correct this.

[Leo Yang]

Comment on attachment 201699 [details]
Patch

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

>> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:87
>> +        KURL originURL(ParsedURLString, url.path());
> 
> Is it really OK to parse just the path as a URL and claim it is a “parsed URL string”? What are you trying to accomplish here? How will originURL.path() be different from url.path()?

The filesystem url is something like "filesystem:http://example.com/temporary/myfile.png", which is basically a wrapper. So url.path() is "http://example.com/temporary/myfile.png" and originalURL.path() is "/temporary/myfile.png"

[Benjamin Poulain]

Comment on attachment 201699 [details]
Patch

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

It is awesome you are merging the two code. The KURL thingy is pretty bad though.

>>> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:87
>>> +        KURL originURL(ParsedURLString, url.path());
>> 
>> Is it really OK to parse just the path as a URL and claim it is a “parsed URL string”? What are you trying to accomplish here? How will originURL.path() be different from url.path()?
> 
> The filesystem url is something like "filesystem:http://example.com/temporary/myfile.png", which is basically a wrapper. So url.path() is "http://example.com/temporary/myfile.png" and originalURL.path() is "/temporary/myfile.png"

Darin is absolutely right, this is completely wrong.

ParsedURLString should _ONLY_ be used if the input string was already parsed and canonicalized by KURL. Code like this is a great way to get security problems.

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:100
> +        path = path.substring(1);
> +
> +        if (path.startsWith(temporaryPathPrefix)) {
> +            type = FileSystemTypeTemporary;
> +            path = path.substring(temporaryPathPrefixLength);
> +        } else if (path.startsWith(persistentPathPrefix)) {
> +            type = FileSystemTypePersistent;
> +            path = path.substring(persistentPathPrefixLength);
> +        } else
> +            return false;

This is very inefficient code, I don't know if you care about performance here?

[Alberto Garcia]

(In reply to comment #5)

Hey, sorry for the late reply.

> > The filesystem url is something like "filesystem:http://example.com/temporary/myfile.png", which is basically a wrapper. So url.path() is "http://example.com/temporary/myfile.png" and originalURL.path() is "/temporary/myfile.png"
> ParsedURLString should _ONLY_ be used if the input string was already parsed and canonicalized by KURL. Code like this is a great way to get security problems.

Sure I can change that, I wonder if we should double check SecurityOrigin::extractInnerURL() as well which seems to be a similar case?

> > Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:100
> > +        path = path.substring(1);
> > +
> > +        if (path.startsWith(temporaryPathPrefix)) {
> > +            type = FileSystemTypeTemporary;
> > +            path = path.substring(temporaryPathPrefixLength);
> > +        } else if (path.startsWith(persistentPathPrefix)) {
> > +            type = FileSystemTypePersistent;
> > +            path = path.substring(persistentPathPrefixLength);
> > +        } else
> > +            return false;
>
> This is very inefficient code, I don't know if you care about performance here?

That I can also rework a bit.

[Alberto Garcia]

Created attachment 202793 [details]
Patch

New patch, hope this one is a bit better.

[Darin Adler]

Comment on attachment 202793 [details]
Patch

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

This is OK, but not ideal. A few comments below.

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:90
> +        if (path.isEmpty() || path[0] != '/')
> +            return false;

The path.isEmpty() check here is redundant. Please remove it. I think I made this same comment on an earlier version of this patch.

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:92
> +        if (path.find(temporaryPathPrefix, 1) == 1) {

find(x, 1) == 1 is an inefficient way to check if a prefix is present. That’s because it will keep searching for the prefix in the whole string to return an exact value of where we found it then afterward just check if it’s 1. It’s just not a good way to write the code. The good way to write it would be with hasPrefix, which we could easily do if temporaryPathPrefix had a "/" in it.

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:95
> +        } else if (path.find(persistentPathPrefix, 1) == 1) {

Same comment as above.

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:102
> +        if (path.isEmpty() || path[0] != '/')
> +            return false;

The path.isEmpty() check here is redundant. Please remove it. I think I made this same comment on an earlier version of this patch.

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:104
> +        filePath.swap(path);

A little strange to do a swap here instead of an assignment. I suppose it’s more efficient.

[Alberto Garcia]

Created attachment 203068 [details]
Patch

(In reply to comment #8)
> The path.isEmpty() check here is redundant. Please remove it. I
> think I made this same comment on an earlier version of this patch.

You're right, sorry!

I've just uploaded a new patch addressing all the other issues that
you mentioned.

temporaryPathPrefix and friends are public constants exported from the
header file, so we cannot really change them.

For this new patch I used String::substringSharingImpl(), which is
more efficient and allows us to use startsWith() to check the prefix.

[Alberto Garcia]

Created attachment 203069 [details]
Patch

(wrong file, sorry)

[Build Bot]

Comment on attachment 203069 [details]
Patch

Attachment 203069 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/747147

New failing tests:
fast/dom/location-new-window-no-crash.html

[Build Bot]

Created attachment 203189 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Alberto Garcia]

Ping

[Rob Buis]

Comment on attachment 203069 [details]
Patch

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

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:108
> +    }

So how exactly will this change behavior for non BB ports? And which ports would that be?

> Source/WebCore/platform/blackberry/LocalFileSystemBlackBerry.cpp:88
> +    builder.append('/');

Is this change related to what the patch intends?

[Alberto Garcia]

(In reply to comment #14)

[...]

I think your questions are already answered in the bug description :)

I don't know if Benjamin or Darin haver further comments about this?

[Alberto Garcia]

I guess this is a WONTFIX now (bug 122137)