Bug 53529 – [fileapi] Add support for filesystem: URI handling

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adam Klein]

[fileapi] FileSystem type must be available to DOM code in order to implement Entry.toURI

[Adam Klein]

Created attachment 80817 [details]
Patch

[Adam Klein]

This is a pre-condition for implementing Entry.toURI() as specified in http://dev.w3.org/2009/dap/file-system/file-dir-sys.html.

[Eric U.]

Looks reasonable.

The info's a little redundant in the current Chromium implementation [it's in the root path as well], but we don't want to assume that, and I'd like to take out the root path anyway in favor of a more generic identifier later.

[Adam Klein]

Created attachment 80849 [details]
Patch

[Adam Klein]

CCing abarth to get his thoughts on the SecurityOrigin changes.

I've now included everything together in one patch: it didn't end up being too big, and I think it makes everything make more sense.

[Adam Barth]

Did you mean to mark the patch "review?"

[Adam Klein]

So marked. There are more tests to add, I think, but yes, review would probably be useful at this point.

[Adam Barth]

Comment on attachment 80849 [details]
Patch

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

> Source/WebCore/fileapi/FileSystemURL.cpp:66
> +KURL FileSystemURL::getOrigin(const KURL& url)
> +{
> +    ASSERT(url.protocolIs(kFileSystemProtocol));
> +
> +    KURL originUrl(ParsedURLString, url.path());
> +    originUrl.setPath("/");
> +    return originUrl;
> +}

The types on this function don't look right.  An "origin" is a SecurityOrigin, not a KURL...  Also, we've been centralizing all the knowledge about how to compute SecurityOrigins from URLs in the SecurityOrigin class.

> Source/WebCore/fileapi/FileSystemURL.h:53
> +class FileSystemURL {
> +public:
> +    static String createURLString(SecurityOrigin*, AsyncFileSystem::Type, const String& fullPath);
> +    static KURL getOrigin(const KURL&);
> +    static const char* fileSystemProtocol() { return kFileSystemProtocol; }
> +
> +private:
> +    static const char kFileSystemProtocol[];
> +    FileSystemURL() { }
> +};

Generally, we don't create classes that have all static members.  What does a FileSystemURL represent?  How is that different from a KURL?

> Source/WebCore/page/SecurityOrigin.cpp:68
> +// FIXME: Should this be part of SchemeRegistry?

Yes.

> Source/WebCore/page/SecurityOrigin.cpp:69
> +static bool schemeRequiresRequestCapabilityForDisplay(const String& scheme)

We probably don't want to use the term "Capability" here because Capability has a special meaning in security, and this doesn't quite match that meaning.

> Source/WebCore/page/SecurityOrigin.cpp:152
>          return adoptRef(new SecurityOrigin(BlobURL::getOrigin(url), sandboxFlags));

I see that you're copying the pattern here from BlobURL, but that's not a good pattern.  We should fix Blob URL not to use this style either.

[Adam Barth]

Comment on attachment 80849 [details]
Patch

The SecurityOrigin-related bits of this patch generally look good, but we should iterate on the style issues.

[Adam Klein]

Comment on attachment 80849 [details]
Patch

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

>> Source/WebCore/fileapi/FileSystemURL.h:53
>> +};
> 
> Generally, we don't create classes that have all static members.  What does a FileSystemURL represent?  How is that different from a KURL?

As you guessed, this is simply mirroring the code layout of BlobURL. createURLString() should be able to move entirely to Entry.cpp (it's not called elsewhere).  See my question in SecurityOrigin.cpp about what I should do with getOrigin().

>> Source/WebCore/page/SecurityOrigin.cpp:68
>> +// FIXME: Should this be part of SchemeRegistry?
> 
> Yes.

Will move once we've agreed on a name.

>> Source/WebCore/page/SecurityOrigin.cpp:69
>> +static bool schemeRequiresRequestCapabilityForDisplay(const String& scheme)
> 
> We probably don't want to use the term "Capability" here because Capability has a special meaning in security, and this doesn't quite match that meaning.

I disliked this name too; do you have an alternate suggestion? SchemeRegistry::shouldTreatURLSchemeDisplayAsRequestRestricted?  More words doesn't necessarily make it clearer to me.

>> Source/WebCore/page/SecurityOrigin.cpp:152
>>          return adoptRef(new SecurityOrigin(BlobURL::getOrigin(url), sandboxFlags));
> 
> I see that you're copying the pattern here from BlobURL, but that's not a good pattern.  We should fix Blob URL not to use this style either.

Do you have a pattern in mind?  Should the code that's currently in FileSystemURL::getOrigin() just move into the SecurityOrigin constructor?

[Adam Barth]

Comment on attachment 80849 [details]
Patch

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

>>> Source/WebCore/page/SecurityOrigin.cpp:69
>>> +static bool schemeRequiresRequestCapabilityForDisplay(const String& scheme)
>> 
>> We probably don't want to use the term "Capability" here because Capability has a special meaning in security, and this doesn't quite match that meaning.
> 
> I disliked this name too; do you have an alternate suggestion? SchemeRegistry::shouldTreatURLSchemeDisplayAsRequestRestricted?  More words doesn't necessarily make it clearer to me.

Yeah, all these functions are poorly named.  Maybe SchemeRegistry::canDisplayOnlyIfCanRequest ?

>>> Source/WebCore/page/SecurityOrigin.cpp:152
>>>          return adoptRef(new SecurityOrigin(BlobURL::getOrigin(url), sandboxFlags));
>> 
>> I see that you're copying the pattern here from BlobURL, but that's not a good pattern.  We should fix Blob URL not to use this style either.
> 
> Do you have a pattern in mind?  Should the code that's currently in FileSystemURL::getOrigin() just move into the SecurityOrigin constructor?

Yeah, or just into a free function in an anonymous namespace in SecurityOrigin.cpp.

[Adam Klein]

Created attachment 80867 [details]
Removed FileSystemURL, moved some code to SchemeRegistry

[Adam Barth]

Comment on attachment 80867 [details]
Removed FileSystemURL, moved some code to SchemeRegistry

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

> Source/WebCore/fileapi/Entry.cpp:95
> +    String uri = "filesystem:";
> +    uri += filesystem()->scriptExecutionContext()->securityOrigin()->toString();
> +    uri += "/";
> +    uri += (m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent");
> +    uri += m_fullPath;
> +    return uri;

Consider using StringBuilder instead of +=.  That should save a bunch of allocations.

> Source/WebCore/page/SecurityOrigin.cpp:85
> +        if (originURL.isValid()) {

If the originURL isn't valid, will we trigger the "m_isUnique = true" assignment on line 95?  If not, we should probably do that explicitly.

> Source/WebCore/page/SecurityOrigin.cpp:88
> +            m_port = originURL.port();

Do we need to do something with default ports, or will that happen automagically for us?

[Adam Klein]

Created attachment 80923 [details]
Use StringBuilder and set m_isUnique for invalid URLs

[Adam Klein]

Comment on attachment 80867 [details]
Removed FileSystemURL, moved some code to SchemeRegistry

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

>> Source/WebCore/fileapi/Entry.cpp:95
>> +    return uri;
> 
> Consider using StringBuilder instead of +=.  That should save a bunch of allocations.

Done. I was thinking std::string, now I know how to treat WTF::String.

>> Source/WebCore/page/SecurityOrigin.cpp:85
>> +        if (originURL.isValid()) {
> 
> If the originURL isn't valid, will we trigger the "m_isUnique = true" assignment on line 95?  If not, we should probably do that explicitly.

Added my own explicit setting in the else branch (the other one wasn't going to trigger).

>> Source/WebCore/page/SecurityOrigin.cpp:88
>> +            m_port = originURL.port();
> 
> Do we need to do something with default ports, or will that happen automagically for us?

That'll be handled by the last if statement in the constructor.  It worries me a little that the ordering is so important here, but it's still short enough to fit on one screen.

[Adam Barth]

Comment on attachment 80923 [details]
Use StringBuilder and set m_isUnique for invalid URLs

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

The SecurityOrigin-related parts of this patch look great!  I'll let someone else review the filesystem parts.

> Source/WebCore/fileapi/Entry.idl:46
> +        DOMString toURI();

Its too bad the web platform is so inconsistent w.r.t URL and URI.  Not really a problem with this patch, just rambling.

[Adam Klein]

Created attachment 80975 [details]
Adds test to exercise canDisplay

[Eric U.]

Comment on attachment 80975 [details]
Adds test to exercise canDisplay

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

> LayoutTests/fast/filesystem/filesystem-uri-origin.html:1
> +<a style="display:none" href="filesystem:http://www.webkit.org:8080/temporary/a/b/c/file.txt">foo</a>

Here you have webkit.org as the filesystem origin.  In directory-entry-to-uri-expected.txt you have file:// as the origin.  In an iframe test I see http://localhost:8000.  Please make sure these tests will work both on the webkit bots and when run locally.

> Source/WebCore/fileapi/Entry.cpp:95
> +    uriBuilder.append(m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent");

Do the strings "temporary" and "persistent" exist anywhere as constants?  It would be nice not to have them inline here.  At the very least they should be shared with the code that cracks the URI.

[Kinuko Yasuda]

Comment on attachment 80975 [details]
Adds test to exercise canDisplay

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

>> Source/WebCore/fileapi/Entry.cpp:95
>> +    uriBuilder.append(m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent");
> 
> Do the strings "temporary" and "persistent" exist anywhere as constants?  It would be nice not to have them inline here.  At the very least they should be shared with the code that cracks the URI.

Same for the protocol name "filesystem"?

The changes in FileSystem callback chains to include type parameter lgtm.

[Adam Klein]

Comment on attachment 80975 [details]
Adds test to exercise canDisplay

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

>> LayoutTests/fast/filesystem/filesystem-uri-origin.html:1
>> +<a style="display:none" href="filesystem:http://www.webkit.org:8080/temporary/a/b/c/file.txt">foo</a>
> 
> Here you have webkit.org as the filesystem origin.  In directory-entry-to-uri-expected.txt you have file:// as the origin.  In an iframe test I see http://localhost:8000.  Please make sure these tests will work both on the webkit bots and when run locally.

The file:/// URIs are there to avoid needing to be an http test. Would you rather I use http tests for those toURI() tests?

The iframe test uses localhost to distinguish it from 127.0.0.1 (where the hosted page is running).

The use of webkit.org:8080 here is purely ornamental, since this URI is never followed; it's simply being used to set up the dom so that the origin is available.  Changing this to localhost or similar wouldn't change anything...

>>> Source/WebCore/fileapi/Entry.cpp:95
>>> +    uriBuilder.append(m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent");
>> 
>> Do the strings "temporary" and "persistent" exist anywhere as constants?  It would be nice not to have them inline here.  At the very least they should be shared with the code that cracks the URI.
> 
> Same for the protocol name "filesystem"?
> 
> The changes in FileSystem callback chains to include type parameter lgtm.

Any suggestions about where these strings would live?  Right now, the URI cracking code is in Chromium.  Should it move into WebCore?

[Eric U.]

(In reply to comment #20)
> (From update of attachment 80975 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80975&action=review
> 
> >> LayoutTests/fast/filesystem/filesystem-uri-origin.html:1
> >> +<a style="display:none" href="filesystem:http://www.webkit.org:8080/temporary/a/b/c/file.txt">foo</a>
> > 
> > Here you have webkit.org as the filesystem origin.  In directory-entry-to-uri-expected.txt you have file:// as the origin.  In an iframe test I see http://localhost:8000.  Please make sure these tests will work both on the webkit bots and when run locally.
> 
> The file:/// URIs are there to avoid needing to be an http test. Would you rather I use http tests for those toURI() tests?

Nope; just wanted to make sure that it was all going to work.  That's fine.

> The iframe test uses localhost to distinguish it from 127.0.0.1 (where the hosted page is running).
> 
> The use of webkit.org:8080 here is purely ornamental, since this URI is never followed; it's simply being used to set up the dom so that the origin is available.  Changing this to localhost or similar wouldn't change anything...
> 
> >>> Source/WebCore/fileapi/Entry.cpp:95
> >>> +    uriBuilder.append(m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent");
> >> 
> >> Do the strings "temporary" and "persistent" exist anywhere as constants?  It would be nice not to have them inline here.  At the very least they should be shared with the code that cracks the URI.
> > 
> > Same for the protocol name "filesystem"?
> > 
> > The changes in FileSystem callback chains to include type parameter lgtm.
> 
> Any suggestions about where these strings would live?  Right now, the URI cracking code is in Chromium.  Should it move into WebCore?

Hmm...tricky, since it deals with security origin code.  Let's at least make sure that the strings don't exist in more than one place for chromium or more than one place for webkit.  Perhaps for WebCore it should be in AsyncFileSystem, along with Type?  Kinuko, what do you think?

[Adam Klein]

I actually had a followup question about tests: these clearly won't work without FILE_SYSTEM being enabled. Is there anything I need to do to mark them as ignored/disabled when run outside the chromium configuration?

[Adam Barth]

(In reply to comment #22)
> I actually had a followup question about tests: these clearly won't work without FILE_SYSTEM being enabled. Is there anything I need to do to mark them as ignored/disabled when run outside the chromium configuration?

Yeah, you can add them to the Skipped lists for ports that don't support this feature.  For example, LayoutTests/platform/mac/Skipped

[Michael Nordman]

I'm pretty certain the fast/filesystem directory is already in those skip lists so that individual tests need not be added.

[Adam Klein]

I've also added an http test, which will need to be skipped.  I've added it locally, will update the patch after the next round of comments is done.

[Adam Klein]

Any further comments?  Either about where to put the string constants, or anything else?

[Kinuko Yasuda]

Comment on attachment 80975 [details]
Adds test to exercise canDisplay

Again this generally looks good to me (as for changes in FileSystem API).  A few more cosmetic comments:

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

> Source/WebCore/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=53529

It might expect to see a few more lines that explain what changes are to be made by this patch.
(Also some of the individual files/changes listed below may deserve brief one-liner comments?)

>>>>> Source/WebCore/fileapi/Entry.cpp:95
>>>>> +    uriBuilder.append(m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent");
>>>> 
>>>> Do the strings "temporary" and "persistent" exist anywhere as constants?  It would be nice not to have them inline here.  At the very least they should be shared with the code that cracks the URI.
>>> 
>>> Same for the protocol name "filesystem"?
>>> 
>>> The changes in FileSystem callback chains to include type parameter lgtm.
>> 
>> Any suggestions about where these strings would live?  Right now, the URI cracking code is in Chromium.  Should it move into WebCore?
> 
> Hmm...tricky, since it deals with security origin code.  Let's at least make sure that the strings don't exist in more than one place for chromium or more than one place for webkit.  Perhaps for WebCore it should be in AsyncFileSystem, along with Type?  Kinuko, what do you think?

(Sorry I thought I had commented on this but seems like I failed to hit 'publish' button!)

The URI format will be on the spec right?  Then I think it's ok to construct the URI in WebCore and crack it in chromium (i.e. in embedder's code).  For WebCore side having constants in AsyncFileSystem sounds good to me.

[Adam Klein]

Comment on attachment 80975 [details]
Adds test to exercise canDisplay

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

>> Source/WebCore/ChangeLog:6
>> +        https://bugs.webkit.org/show_bug.cgi?id=53529
> 
> It might expect to see a few more lines that explain what changes are to be made by this patch.
> (Also some of the individual files/changes listed below may deserve brief one-liner comments?)

Updated all the ChangeLogs with more info, thanks for the suggestion (this is my first real (non-cleanup) WebKit patch)

>>>>>> Source/WebCore/fileapi/Entry.cpp:95
>>>>>> +    uriBuilder.append(m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent");
>>>>> 
>>>>> Do the strings "temporary" and "persistent" exist anywhere as constants?  It would be nice not to have them inline here.  At the very least they should be shared with the code that cracks the URI.
>>>> 
>>>> Same for the protocol name "filesystem"?
>>>> 
>>>> The changes in FileSystem callback chains to include type parameter lgtm.
>>> 
>>> Any suggestions about where these strings would live?  Right now, the URI cracking code is in Chromium.  Should it move into WebCore?
>> 
>> Hmm...tricky, since it deals with security origin code.  Let's at least make sure that the strings don't exist in more than one place for chromium or more than one place for webkit.  Perhaps for WebCore it should be in AsyncFileSystem, along with Type?  Kinuko, what do you think?
> 
> (Sorry I thought I had commented on this but seems like I failed to hit 'publish' button!)
> 
> The URI format will be on the spec right?  Then I think it's ok to construct the URI in WebCore and crack it in chromium (i.e. in embedder's code).  For WebCore side having constants in AsyncFileSystem sounds good to me.

Hmm, the trouble with putting these in AsyncFileSystem is that there are already two implementations of AsyncFileSystem, so there's not a single place on which to hang const char[] string constants.  WebCore/platform/AsyncFileSystem.cpp is not used by Chromium; we use WebKit/chromium/src/AsyncFileSystemChromium.cpp.  And in fact, AsyncFileSystem already has this duplicated string constant problem: AsyncFileSystem.cpp uses the strings "Temporary" and "Persistent" (part of the filesystem name), while in Chromium, those live in webkit/fileapi/file_system_path_manager.cc.

In short, I'm not sure what's to be gained from moving these constants out of Entry.cpp and into two other CPP files, given that this is the only reference in all of WebKit. Is there another place I'm missing where they would be used?  That might make it clearer where they should be (if they do need centralization).

[Adam Klein]

Created attachment 81663 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 81663 [details]
Patch

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

> Source/WebCore/fileapi/Entry.cpp:97
> +    return uriBuilder.toString();

Somehow it didn't come to me up until now, but shouldn't we put this in EntryBase (base class of Entry and EntrySync) rather than in Entry?
The spec seems to have toURI method also in EntrySync.
http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#widl-EntrySync-toURI

And as for string constants, you're right about AsyncFileSystem (its .cpp is not referred in chromium build).  In addition to the fact you pointed out, the URI stuff is not platform dependent stuff but spec based fact, so we should put them in WebCore/fileapi as a common implementation.
If we're going to put this code in EntryBase, I'd think about putting the constants in DOMFileSystemBase.{h,cpp}.
Or if we're sure we won't use the constants from anywhere else in webcore, it'd be also ok to leave them as literals.
So my take is to put them in DOMFileSystemBase or just leave them as is.
Thanks for taking your extra time on this issue!

[Adam Klein]

(In reply to comment #30)
> (From update of attachment 81663 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81663&action=review
> 
> > Source/WebCore/fileapi/Entry.cpp:97
> > +    return uriBuilder.toString();
> 
> Somehow it didn't come to me up until now, but shouldn't we put this in EntryBase (base class of Entry and EntrySync) rather than in Entry?
> The spec seems to have toURI method also in EntrySync.
> http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#widl-EntrySync-toURI

Hooking this up to EntrySync will require a bit more work, since neither EntryBase nor EntrySync currently have access to the scriptExecutionContext->securityOrigin(). Getting EntryBase access to that would make this patch significantly bigger, I fear.  Might it make sense to do that in a followup?

> And as for string constants, you're right about AsyncFileSystem (its .cpp is not referred in chromium build).  In addition to the fact you pointed out, the URI stuff is not platform dependent stuff but spec based fact, so we should put them in WebCore/fileapi as a common implementation.
> If we're going to put this code in EntryBase, I'd think about putting the constants in DOMFileSystemBase.{h,cpp}.
> Or if we're sure we won't use the constants from anywhere else in webcore, it'd be also ok to leave them as literals.
> So my take is to put them in DOMFileSystemBase or just leave them as is.
> Thanks for taking your extra time on this issue!

[Kinuko Yasuda]

Comment on attachment 81663 [details]
Patch

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

>>> Source/WebCore/fileapi/Entry.cpp:97
>>> +    return uriBuilder.toString();
>> 
>> Somehow it didn't come to me up until now, but shouldn't we put this in EntryBase (base class of Entry and EntrySync) rather than in Entry?
>> The spec seems to have toURI method also in EntrySync.
>> http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#widl-EntrySync-toURI
>> 
>> And as for string constants, you're right about AsyncFileSystem (its .cpp is not referred in chromium build).  In addition to the fact you pointed out, the URI stuff is not platform dependent stuff but spec based fact, so we should put them in WebCore/fileapi as a common implementation.
>> If we're going to put this code in EntryBase, I'd think about putting the constants in DOMFileSystemBase.{h,cpp}.
>> Or if we're sure we won't use the constants from anywhere else in webcore, it'd be also ok to leave them as literals.
>> So my take is to put them in DOMFileSystemBase or just leave them as is.
>> Thanks for taking your extra time on this issue!
> 
> Hooking this up to EntrySync will require a bit more work, since neither EntryBase nor EntrySync currently have access to the scriptExecutionContext->securityOrigin(). Getting EntryBase access to that would make this patch significantly bigger, I fear.  Might it make sense to do that in a followup?

I see, yes it makes sense.  Let's do it separately.

[Darin Fisher (:fishd, Google)]

Comment on attachment 81663 [details]
Patch

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

> Source/WebCore/page/SecurityOrigin.cpp:86
> +            m_protocol = originURL.protocol().lower();

I suspect that these calls to .lower() may be redundant.  Doesn't KURL canonicalize the protocol and host to lowercase?

[Adam Barth]

> Doesn't KURL canonicalize the protocol and host to lowercase?

As of about two days ago, yes.  Before that, no.  Let's keep the toLower for a bit while we make sure that change sticks.

[Darin Fisher (:fishd, Google)]

Comment on attachment 81663 [details]
Patch

OK, makes sense.  R=me

[WebKit Commit Bot]

Comment on attachment 81663 [details]
Patch

Rejecting attachment 81663 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2

Last 500 characters of output:
......................................................................................................................................................................
compositing ...........
compositing/animation ...
compositing/color-matching ..
compositing/geometry ......
compositing/geometry/empty-embed-rects.html -> crashed

Exiting early after 1 failures. 962 tests run.
103.11s total testing time

961 test cases (99%) succeeded
1 test case (<1%) crashed
2 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7886413

[Adam Klein]

Looks like  a flaky crash...Darin, can you resubmit to the commit queue?

[Eric Seidel (no email)]

Comment on attachment 81663 [details]
Patch

suspicious.

[WebKit Commit Bot]

Comment on attachment 81663 [details]
Patch

Rejecting attachment 81663 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2

Last 500 characters of output:
......................................................................................................................................................................
compositing ...........
compositing/animation ...
compositing/color-matching ..
compositing/geometry ......
compositing/geometry/empty-embed-rects.html -> crashed

Exiting early after 1 failures. 962 tests run.
103.00s total testing time

961 test cases (99%) succeeded
1 test case (<1%) crashed
2 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7886508

[Adam Barth]

(In reply to comment #37)
> Looks like  a flaky crash...Darin, can you resubmit to the commit queue?

The commit-queue should be robust to flaky crashes...  Have you tried to reproduce locally?

[Eric Seidel (no email)]

The commit-queue never lies. :)  I'm certain this is a crash from this patch.

[Adam Klein]

Created attachment 82163 [details]
Patch for commit, no layout test crashes

[Darin Fisher (:fishd, Google)]

I see this diff between the patches:

 bool SchemeRegistry::canDisplayOnlyIfCanRequest(const String& scheme)
 {
+    if (scheme.isEmpty())
+        return false;
     return canDisplayOnlyIfCanRequestSchemes().contains(scheme);
 }

LGTM!

[Adam Klein]

Indeed the commit queue was right. Very little code in this change exists without ENABLE_FILE_SYSTEM, but the bit that does was crashing when called with an empty scheme (as we apparently do for <embed>s).  I've fixed that in this change, and also sent out https://bugs.webkit.org/show_bug.cgi?id=54304 to add safety checks to the rest of the methods in SchemeRegistry.

[WebKit Commit Bot]

Comment on attachment 82163 [details]
Patch for commit, no layout test crashes

Clearing flags on attachment: 82163

Committed r78362: <http://trac.webkit.org/changeset/78362>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.