Bug 156321 – Modern IDB (Blob support): Write blobs to temporary files and move them to the correct location when storing them

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

[Brady Eidson]

Modern IDB (Blob support): Write blobs to temporary files and move them to the correct location when storing them

Still a no-op in trunk, as we still don't actually allow put/add with blobs at the IDBObjectStore level.

[Brady Eidson]

Created attachment 275835 [details]
WIP in case I have time to work on this at home tonight

[Brady Eidson]

Created attachment 275836 [details]
Updated WIP

[WebKit Commit Bot]

Attachment 275836 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:296:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:115:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:207:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/Modules/indexeddb/server/SQLiteIDBTransaction.cpp:90:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Could you please elaborate on why blobs need to be stored in files?

[Brady Eidson]

(In reply to comment #4)
> Could you please elaborate on why blobs need to be stored in files?

Follows in a non-exhaustive list of reasons:

1 - Blobs can be so large, that it's impractical to shuffle around their contents between processes in memory.
E.g. To get a 500MB blob from the Networking process to the Database process would take at LEAST 1GB of memory, and probably 1.5GB of memory, which is not possible on all supported platforms.

2 - If a Blob *is* a File, then we can get "storing it as a file" for free.

3 - If we did store Blobs in the SQLite database, we would then be unable to stream its contents in a way that our ResourceHandle/ResourceLoaders are designed for, and instead would have to make a full copy of the Blob in memory just to read it.
E.g. a 1GB File blob stored in the database would require 1GB of memory to read, instead of "streaming the file from disk little by little"

[Brady Eidson]

Created attachment 275897 [details]
Near-final WIP

No ChangeLogs yet, and maybe doing some final tweaks.

Just want to get the EWS run started.

[WebKit Commit Bot]

Attachment 275897 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:296:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:115:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:207:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/Modules/indexeddb/server/SQLiteIDBTransaction.cpp:90:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

For GTK:

Last 500 characters of output:
 undefined reference to 'WebCore::hardLinkOrCopyFile(WTF::String const&, WTF::String const&)'
lib/libWebCoreGTK.a(lib/../Source/WebCore/CMakeFiles/WebCore.dir/Modules/indexeddb/server/SQLiteIDBTransaction.cpp.o):SQLiteIDBTransaction.cpp:function WebCore::IDBServer::SQLiteIDBTransaction::moveBlobFilesIfNecessary(): error: undefined reference to 'WebCore::hardLinkOrCopyFile(WTF::String const&, WTF::String const&)'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.


Does GTK not use FileSystemPOSIX.cpp?

[Alex Christensen]

FileSystemGtk.cpp

[Brady Eidson]

(In reply to comment #8)
> For GTK:
> 
> Last 500 characters of output:
>  undefined reference to 'WebCore::hardLinkOrCopyFile(WTF::String const&,
> WTF::String const&)'
> lib/libWebCoreGTK.a(lib/../Source/WebCore/CMakeFiles/WebCore.dir/Modules/
> indexeddb/server/SQLiteIDBTransaction.cpp.o):SQLiteIDBTransaction.cpp:
> function
> WebCore::IDBServer::SQLiteIDBTransaction::moveBlobFilesIfNecessary(): error:
> undefined reference to 'WebCore::hardLinkOrCopyFile(WTF::String const&,
> WTF::String const&)'
> collect2: error: ld returned 1 exit status
> ninja: build stopped: subcommand failed.
> 
> 
> Does GTK not use FileSystemPOSIX.cpp?

It does not. Bummer. I'll stub out the new method for them.

[Brady Eidson]

Created attachment 275902 [details]
Near-final WIP again

[WebKit Commit Bot]

Attachment 275902 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:296:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:115:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:207:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Created attachment 275922 [details]
Patch v1

[WebKit Commit Bot]

Attachment 275922 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:296:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:115:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:207:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 275922 [details]
Patch v1

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

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBTransaction.cpp:94
> +        if (!hardLinkOrCopyFile(entry.first, pathByAppendingComponent(databaseDirectory, entry.second)))
> +            LOG_ERROR("Failed to link/copy temporary blob file '%s' to location '%s'", entry.first.utf8().data(), pathByAppendingComponent(databaseDirectory, entry.second).utf8().data());

Should we continue if this fails?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBTransaction.h:88
> +    Vector<std::pair<String, String>> m_blobFiles;

Could these be typedefed so we know what they mean?

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:371
> +    bool appendResult = appendFileContentsToFileHandle(source, handle);

This is a strange function to call when copying a file to a destination we know is empty.

> Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:314
> +        ASSERT_UNUSED(result, result.isNewEntry);

Will this be true if we have multiple blobs of the same file?

> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:307
> +    if (handler)

handler could be declared inside the if.  Same thing with extension earlier.

> Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:280
> +        SandboxExtension::createHandle(paths[i], SandboxExtension::ReadWrite, extensions[i]);

Will this always succeed?

[Brady Eidson]

(In reply to comment #15)
> Comment on attachment 275922 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275922&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBTransaction.cpp:94
> > +        if (!hardLinkOrCopyFile(entry.first, pathByAppendingComponent(databaseDirectory, entry.second)))
> > +            LOG_ERROR("Failed to link/copy temporary blob file '%s' to location '%s'", entry.first.utf8().data(), pathByAppendingComponent(databaseDirectory, entry.second).utf8().data());
> 
> Should we continue if this fails?

At this point, the transaction is committed.

There's room for improvement in the future if we find situations where it becomes a problem.

> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBTransaction.h:88
> > +    Vector<std::pair<String, String>> m_blobFiles;
> 
> Could these be typedefed so we know what they mean?

In person we decided typedef isn't necessary, but an improved variable name is.

> > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:371
> > +    bool appendResult = appendFileContentsToFileHandle(source, handle);
> 
> This is a strange function to call when copying a file to a destination we
> know is empty.

Maybe room for improvement in the function name, but it's also already used like this.

> > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:314
> > +        ASSERT_UNUSED(result, result.isNewEntry);
> 
> Will this be true if we have multiple blobs of the same file?

If you have a File, and you put it in IDB 10 times, we're required by the structured clone algorithm to make 10 unique copies of it.

Therefore the filenames will always be unique.

There's room for improvement here, as well, but worrying about it now seems like premature optimization.

> > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:307
> > +    if (handler)
> 
> handler could be declared inside the if.  Same thing with extension earlier.

Yup.

> > Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:280
> > +        SandboxExtension::createHandle(paths[i], SandboxExtension::ReadWrite, extensions[i]);
> 
> Will this always succeed?

In practice, yes.

The UIProcess had already granted the NetworkProcess extensions to these locations when it was launched. So if the UIProcess can't create an extension here, it means the NetworkProcess never could've gotten up and running, and we never would've reached this point.

But I'm definitely going to add an ASSERT.

[Brady Eidson]

Created attachment 275930 [details]
Patch v2

I have Alex's review, but since this is a "touches the filesystem" patch I'm waiting for more.

[WebKit Commit Bot]

Attachment 275930 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:296:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:115:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:207:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Comment on attachment 275930 [details]
Patch v2

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

Instead of WebCore keeping a IDBBackingStoreFileHandler* that is null for WebKit1 clients, why don't we have a default handler that can also handle the deletion issue I mentioned above. This'll also let you store the handler by reference and get rid of a bunch of branches. r=me with these changes.

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:122
> +    IDBBackingStoreFileHandler* m_backingStoreFileHandler;

Looks like this isn't initialized when the default constructor is called. You can just initialize it to nullptr here.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:340
> +        } else if (sqliteResult != SQLITE_ROW) {
> +            LOG_ERROR("Error executing statement to fetch schema for the BlobRecords table.");
> +            return false;
> +        } else
> +            currentSchema = statement.getColumnText(1);

No need for the else since you return in the else if.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBTransaction.cpp:133
> +    m_blobTemporaryAndStoredFilenames.clear();

You told me that the only callers of reset() are abort() and commit(), so you can just assert that m_blobTemporaryAndStoredFilenames is empty.

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:364
> +    int result = link(fsSource.data(), fsDestination.data());
> +    if (!result)
> +        return true;

This could be more concise by removing the local.

> Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:329
> +    // We've either hard linked the temporary blob file to the database directory, copied it there,
> +    // or the transaction is being aborted.
> +    // In any of those cases, we can delete the temporary blob file now.
> +    deleteFile(path);

It's strange that we link/copy the file in WebCore but delete it in WebKit2. Even worse, it looks like we never delete it in WebKit1!

> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:296
> +void NetworkProcess::grantSandboxExtensionsToDatabaseProcessForBlobs(const Vector<String> filenames, std::function<void ()> completionHandler)

filenames should be by reference to const.

[Andy Estes]

It's worth noting that my review of the SQL statements in this patch is essentially a rubber-stamp.

[Andy Estes]

Comment on attachment 275930 [details]
Patch v2

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

> Source/WebCore/Modules/indexeddb/IDBValue.cpp:42
> +    m_blobURLs = scriptValue.blobURLsIsolatedCopy();

You should move this into the initialization list.

[Brady Eidson]

Created attachment 275952 [details]
Patch v3 - Awaiting a potential final review

[Brady Eidson]

(In reply to comment #22)
> Created attachment 275952 [details]
> Patch v3 - Awaiting a potential final review

I'll work on landing this in the morning if I hear no additional feedback.

[WebKit Commit Bot]

Attachment 275952 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:296:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:115:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:207:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

(In reply to comment #21)
> Comment on attachment 275930 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275930&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBValue.cpp:42
> > +    m_blobURLs = scriptValue.blobURLsIsolatedCopy();
> 
> You should move this into the initialization list.

Fixed in my local copy, won't upload a new patch just for it though.

[Darin Adler]

Comment on attachment 275952 [details]
Patch v3 - Awaiting a potential final review

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

I didn’t get a chance to think deeply about the patch but did spot a few small things.

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:122
> +    IDBBackingStoreTemporaryFileHandler& m_backingStoreTemporaryFileHandler;

Does not seem safe to have a reference counted object hold a reference to another object without any influence over the other object’s lifetime. What guarantees this handler will outlive the server? Perhaps only immortal objects are passed as these handlers?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBTransaction.cpp:202
> +    m_blobTemporaryAndStoredFilenames.append(std::make_pair(temporaryPath, storedFilename));

In new code I think we can just use braces instead of std::make_pair:

    m_blobTemporaryAndStoredFilenames.append({ temporaryPath, storedFilename });

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2759
> +Vector<String> SerializedScriptValue::blobURLsIsolatedCopy() const
> +{
> +    Vector<String> result;
> +    result.reserveInitialCapacity(m_blobURLs.size());
> +    for (auto& url : m_blobURLs)
> +        result.uncheckedAppend(url.isolatedCopy());
> +
> +    return result;
> +}

I don’t think this should be a member function. This is just isolatedCopy for a Vector<String>; nothing specific to URLs here, or to blob URLs, or to SerializedScriptValue. Should be a free function named isolatedCopy and could call isolatedCopy(value.blobURLs()). Not sure exactly where to put it, though.

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:352
> +    if (fileExists(destination))
> +        return false;

We should let the file system calls fail instead of trying to preflight. We just need to use O_EXCL. It’s normally bad practice to preflight.

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:355
> +    if (!fsSource.data() || fsSource.data()[0] == '\0')

Do we really need to check for both a null pointer and an empty string? Why exactly? Other code seems to do this, which is super annoying since there’s no reason fileSystemRepresentation has two have two different failure modes. I think maybe the check for the empty string is unnecessary and bogus.

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:359
> +    if (!fsDestination.data() || fsDestination.data()[0] == '\0')

Ditto.

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:366
> +    auto handle = openFile(destination, OpenForWrite);

I think we can call open directly here; it’s in the POSIX-specific file, not platform independent code, and we already have the file name converted to a C string. Also, that lets use use O_EXCL instead of O_TRUNC.

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:371
> +    closeFile(handle);

Should just call close directly here for similar reasons.

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:372
> +    return appendResult;

If this returns false, it’s not good that it leaves a file behind.

> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:299
> +    static uint64_t nextRequestID;
> +    m_sandboxExtensionForBlobsCompletionHandlers.set(++nextRequestID, completionHandler);

Note that given how this uses the global variable with pre-increment, it’s not nextRequestID, it’s lastUsedRequestID.

> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:301
> +    parentProcessConnection()->send(Messages::NetworkProcessProxy::GrantSandboxExtensionsToDatabaseProcessForBlobs(nextRequestID, filenames), 0);

I think it’s cleaner to put the current requestID into a local variable so we don’t have to think so hard about why the one above does pre increment and this one does not.

[Brady Eidson]

(In reply to comment #26)
> Comment on attachment 275952 [details]
> Patch v3 - Awaiting a potential final review
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275952&action=review
> 
> I didn’t get a chance to think deeply about the patch but did spot a few
> small things.
> 
> > Source/WebCore/Modules/indexeddb/server/IDBServer.h:122
> > +    IDBBackingStoreTemporaryFileHandler& m_backingStoreTemporaryFileHandler;
> 
> Does not seem safe to have a reference counted object hold a reference to
> another object without any influence over the other object’s lifetime. What
> guarantees this handler will outlive the server? Perhaps only immortal
> objects are passed as these handlers?

Correct. The only two handlers are immortal.

> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBTransaction.cpp:202
> > +    m_blobTemporaryAndStoredFilenames.append(std::make_pair(temporaryPath, storedFilename));
> 
> In new code I think we can just use braces instead of std::make_pair:
> 
>     m_blobTemporaryAndStoredFilenames.append({ temporaryPath, storedFilename
> });

Exciting, will try.

> > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2759
> > +Vector<String> SerializedScriptValue::blobURLsIsolatedCopy() const
> > +{
> > +    Vector<String> result;
> > +    result.reserveInitialCapacity(m_blobURLs.size());
> > +    for (auto& url : m_blobURLs)
> > +        result.uncheckedAppend(url.isolatedCopy());
> > +
> > +    return result;
> > +}
> 
> I don’t think this should be a member function. This is just isolatedCopy
> for a Vector<String>; nothing specific to URLs here, or to blob URLs, or to
> SerializedScriptValue. Should be a free function named isolatedCopy and
> could call isolatedCopy(value.blobURLs()). Not sure exactly where to put it,
> though.

We actually explicitly and purposefully do *not* expose the blobURLs Vector on SerializedScriptValue *because* of the potential thread safety issue.
e.g., a SerializedScriptValue is created on a worker and then the blobURLs is accessed from an outside object on the main thread.

This patch requires the blobURLs object to be exposed, but tries to make sure it's thread safe.

> > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:352
> > +    if (fileExists(destination))
> > +        return false;
> 
> We should let the file system calls fail instead of trying to preflight. We
> just need to use O_EXCL. It’s normally bad practice to preflight.

Yup. Will get rid of it.

> > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:355
> > +    if (!fsSource.data() || fsSource.data()[0] == '\0')
> 
> Do we really need to check for both a null pointer and an empty string? Why
> exactly? Other code seems to do this, which is super annoying since there’s
> no reason fileSystemRepresentation has two have two different failure modes.
> I think maybe the check for the empty string is unnecessary and bogus.

You called out why I did this - It seems to be established practice in our other fileSystem function code.

I will look into it a bit to see if I can get a conclusive answer.

> > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:366
> > +    auto handle = openFile(destination, OpenForWrite);
> 
> I think we can call open directly here; it’s in the POSIX-specific file, not
> platform independent code, and we already have the file name converted to a
> C string. Also, that lets use use O_EXCL instead of O_TRUNC.

Sure!

> > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:371
> > +    closeFile(handle);
> 
> Should just call close directly here for similar reasons.

Sure!

> > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:372
> > +    return appendResult;
> 
> If this returns false, it’s not good that it leaves a file behind.

Good call.

> > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:299
> > +    static uint64_t nextRequestID;
> > +    m_sandboxExtensionForBlobsCompletionHandlers.set(++nextRequestID, completionHandler);
> 
> Note that given how this uses the global variable with pre-increment, it’s
> not nextRequestID, it’s lastUsedRequestID.

Touché.

> > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:301
> > +    parentProcessConnection()->send(Messages::NetworkProcessProxy::GrantSandboxExtensionsToDatabaseProcessForBlobs(nextRequestID, filenames), 0);
> 
> I think it’s cleaner to put the current requestID into a local variable so
> we don’t have to think so hard about why the one above does pre increment
> and this one does not.

Okay!

[Brady Eidson]

(In reply to comment #27)
> (In reply to comment #26)

> > > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:355
> > > +    if (!fsSource.data() || fsSource.data()[0] == '\0')
> > 
> > Do we really need to check for both a null pointer and an empty string? Why
> > exactly? Other code seems to do this, which is super annoying since there’s
> > no reason fileSystemRepresentation has two have two different failure modes.
> > I think maybe the check for the empty string is unnecessary and bogus.
> 
> You called out why I did this - It seems to be established practice in our
> other fileSystem function code.
> 
> I will look into it a bit to see if I can get a conclusive answer.

The answer is that if you pass an empty string to CFStringGetFileSystemRepresentation, it will return "True" and give you back an empty, null terminated C string buffer.

So I changed it to explicitly check if source or destination are empty before getting the file system representation.

[Brady Eidson]

Created attachment 276006 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 276006 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:296:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:115:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:207:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 276006 [details]
Patch for landing

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2758
> +    Vector<String> result;
> +    result.reserveInitialCapacity(m_blobURLs.size());
> +    for (auto& url : m_blobURLs)
> +        result.uncheckedAppend(url.isolatedCopy());
> +
> +    return result;

I still think the isolated copy algorithm should be in a separate function. This function’s body should be:

    return isolatedCopy(m_blobURLs);

[WebKit Commit Bot]

Comment on attachment 276006 [details]
Patch for landing

Clearing flags on attachment: 276006

Committed r199230: <http://trac.webkit.org/changeset/199230>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

(In reply to comment #31)
> Comment on attachment 276006 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276006&action=review
> 
> > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2758
> > +    Vector<String> result;
> > +    result.reserveInitialCapacity(m_blobURLs.size());
> > +    for (auto& url : m_blobURLs)
> > +        result.uncheckedAppend(url.isolatedCopy());
> > +
> > +    return result;
> 
> I still think the isolated copy algorithm should be in a separate function.
> This function’s body should be:
> 
>     return isolatedCopy(m_blobURLs);

I see. That makes sense.

Like you, not sure where it'd go. =/

[Brady Eidson]

EFL build failure is interesting...

In file included from /usr/include/fcntl.h:279:0,
                 from ../../Source/WebCore/platform/posix/FileSystemPOSIX.cpp:35:
In function 'int open(const char*, int, ...)',
    inlined from 'bool WebCore::hardLinkOrCopyFile(const WTF::String&, const WTF::String&)' at ../../Source/WebCore/platform/posix/FileSystemPOSIX.cpp:366:23:
/usr/include/x86_64-linux-gnu/bits/fcntl2.h:50:26: error: call to '__open_missing_mode' declared with attribute error: open with O_CREAT in second argument needs 3 arguments
    __open_missing_mode ();


open() apparently requires a mode when O_CREAT is used, but Mac (and the GTK bots, are they Linux?) let me *not* do that.

Looking into it now.

[Brady Eidson]

(In reply to comment #35)
> open() apparently requires a mode when O_CREAT is used, but Mac (and the GTK
> bots, are they Linux?) let me *not* do that.
> 
> Looking into it now.

The only other time in the POSIX implementation that we call open() with O_CREAT, we pass 0666 as the mode/umask.

Will do the same here.

[Brady Eidson]

http://trac.webkit.org/changeset/199232

[Csaba Osztrogonác]

(In reply to comment #37)
> http://trac.webkit.org/changeset/199232

It broke the Apple Mac cmake build.

[Alex Christensen]

Build fix in r199256

[Csaba Osztrogonác]

(In reply to comment #39)
> Build fix in r199256

Build is still broken:

/Volumes/Data/slave/elcapitan-cmake-debug/build/WebKitBuild/Debug/DerivedSources/WebKit2/DatabaseProcessMessageReceiver.cpp:71:120: error: no member named 'grantSandboxExtensionsForBlobs' in 'WebKit::DatabaseProcess'
        IPC::handleMessage<Messages::DatabaseProcess::GrantSandboxExtensionsForBlobs>(decoder, this, &DatabaseProcess::grantSandboxExtensionsForBlobs);
                                                                                                      ~~~~~~~~~~~~~~~~~^


https://trac.webkit.org/changeset/199256 said it is a "Build fix with 
IndexedDB disabled but DatabaseProcess enabled after r199230", but both
INDEXED_DATABASE and DATABASE_PROCESS are enabled on Apple Mac cmake build.

Otherwise is DatabaseProcess used for anything if IndexedDB is disabled?

[Csaba Osztrogonác]

just to document, Alex fixed it in http://trac.webkit.org/changeset/199532

[Csaba Osztrogonác]

(In reply to comment #40)
> https://trac.webkit.org/changeset/199256 said it is a "Build fix with 
> IndexedDB disabled but DatabaseProcess enabled after r199230", but both
> INDEXED_DATABASE and DATABASE_PROCESS are enabled on Apple Mac cmake build.

(In reply to comment #41)
> just to document, Alex fixed it in http://trac.webkit.org/changeset/199532
Which says "Fix build without IndexedDB."

But IndexedDB should be enabled on Apple Mac cmake build, see
http://trac.webkit.org/browser/trunk/Source/cmake/OptionsMac.cmake#L48
How come if it is disabled on the Apple Mac cmake bot?


> Otherwise is DatabaseProcess used for anything if IndexedDB is disabled?
I'm still waiting the answer for this question.

[Csaba Osztrogonác]

Alex, ping?

[Csaba Osztrogonác]

I think bug156902 shows how is it possible that INDEXED_DATABASE is disabled on Apple Mac cmake.

But the question is still valid: Does it make sense to build DatabaseProcess
when IndexedDB is disabled?

[Brady Eidson]

(In reply to comment #44)
> I think bug156902 shows how is it possible that INDEXED_DATABASE is disabled
> on Apple Mac cmake.
> 
> But the question is still valid: Does it make sense to build DatabaseProcess
> when IndexedDB is disabled?

Yes - The DatabaseProcess should remain a separate compile time flag from IndexedDB

[Csaba Osztrogonác]

(In reply to comment #45)
> (In reply to comment #44)
> > I think bug156902 shows how is it possible that INDEXED_DATABASE is disabled
> > on Apple Mac cmake.
> > 
> > But the question is still valid: Does it make sense to build DatabaseProcess
> > when IndexedDB is disabled?
> 
> Yes - The DatabaseProcess should remain a separate compile time flag from
> IndexedDB

I remember a previous discussion that separated compile time guards should
remain to be able to use DatabaseProcess for a database thing which is not
IndexedDB in the near/far future.

But my question was if it makes sense to build DatabaseProcess
without IndexedDB right now. Shouldn't we disable it if IndexedDB
is disabled? (Until we don't have a new user of the DatabaseProcess)

[Brady Eidson]

(In reply to comment #46)
> (In reply to comment #45)
> > (In reply to comment #44)
> > > I think bug156902 shows how is it possible that INDEXED_DATABASE is disabled
> > > on Apple Mac cmake.
> > > 
> > > But the question is still valid: Does it make sense to build DatabaseProcess
> > > when IndexedDB is disabled?
> > 
> > Yes - The DatabaseProcess should remain a separate compile time flag from
> > IndexedDB
> 
> I remember a previous discussion that separated compile time guards should
> remain to be able to use DatabaseProcess for a database thing which is not
> IndexedDB in the near/far future.
> 
> But my question was if it makes sense to build DatabaseProcess
> without IndexedDB right now. Shouldn't we disable it if IndexedDB
> is disabled? (Until we don't have a new user of the DatabaseProcess)

There's already a #if ENABLE(DATABASE_PROCESS) flag.

If a port wants it disabled, they can already do so.

[Brady Eidson]

(In reply to comment #47)
> (In reply to comment #46)
> > (In reply to comment #45)
> > > (In reply to comment #44)
> > > > I think bug156902 shows how is it possible that INDEXED_DATABASE is disabled
> > > > on Apple Mac cmake.
> > > > 
> > > > But the question is still valid: Does it make sense to build DatabaseProcess
> > > > when IndexedDB is disabled?
> > > 
> > > Yes - The DatabaseProcess should remain a separate compile time flag from
> > > IndexedDB
> > 
> > I remember a previous discussion that separated compile time guards should
> > remain to be able to use DatabaseProcess for a database thing which is not
> > IndexedDB in the near/far future.
> > 
> > But my question was if it makes sense to build DatabaseProcess
> > without IndexedDB right now. Shouldn't we disable it if IndexedDB
> > is disabled? (Until we don't have a new user of the DatabaseProcess)
> 
> There's already a #if ENABLE(DATABASE_PROCESS) flag.
> 
> If a port wants it disabled, they can already do so.

Said differently,

If a port is interested in both disabling INDEXED_DATABASE and DATABASE_PROCESS, they can already do so today.