Bug 125258 – DatabaseProcess IndexedDB: Establish a metadata backing store on disk

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

DatabaseProcess IndexedDB: Establish a metadata backing store on disk

The directory structure for each unique Indexed Database is:
<The WebKit2 Databases path> /___IndexedDB/<main frame security origin>/<name of indexed database>/

This patch adds a SQLite backing store and actually establishes files on disk.

Each IDB will have its metadata stored in a backing store database in that directory.
Going forward, each ObjectStore will also live in a separate database in that directory.

[Brady Eidson]

Created attachment 218462 [details]
Patch v1

[Brady Eidson]

Since this patch touches the filesystem, I'll be trying to get 3 reviews on at least those specific areas of the code.

[Alexey Proskuryakov]

> ___IndexedDB

What are the underscores good for?

[Brady Eidson]

(In reply to comment #3)
> > ___IndexedDB
> 
> What are the underscores good for?

This was established in an earlier patch.

The "Databases" directory contains both WebSQL and now IndexedDB.
Currently WebSQL stuff is dumped directly in the Databases directory.  the ___IndexedDB name guarantees a lack of collision with any WebSQL names because they will never have a ___ prefix.

It's a hack, but hopefully a very temporary one - WebSQL should be migrated into a .../Databases/WebSQL directory, and then IDB can be in .../Databases/IndexedDB instead of ___IndexedDB

[Alexey Proskuryakov]

Comment on attachment 218462 [details]
Patch v1

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

I only looked at a small part of the patch so far, will continue tomorrow.

> Source/WebKit2/ChangeLog:45
> +        To help with thread safety, add an isolatedCopy to UniqueIDBDatabaseIdentifier:

Can you elaborate please, what does this fix? It's clear that isolatedCopy always serves this purpose, so this comment doesn't add much.

> Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:102
> +    ensureIndexedDatabaseRelativePathExists("");

Not sure if it's something DatabaseProcess should do, or UI process. From sandboxing point of view, having access above the root level is excessive and dangerous, although I think that we mitigate this by only allowing creating certain types of nodes.

We'll need to more closely look at what WebProcess does for similar directories.

Also, String::empty().

> Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:111
> +        LOG_ERROR("DatabaseProcess::ensureDatabaseRelativePathExists() - Complete path is empty");

We were just calling this function with an empty argument above? Is the problem actually with null strings?

> Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:121
> +    ASSERT(!isMainThread());

I think that a comment would help here. Why is calling this on main thread a problem?

This is a faceless background process, so this is not going to block UI or something.

[Brady Eidson]

(In reply to comment #5)
> (From update of attachment 218462 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218462&action=review
> 
> I only looked at a small part of the patch so far, will continue tomorrow.
> 
> > Source/WebKit2/ChangeLog:45
> > +        To help with thread safety, add an isolatedCopy to UniqueIDBDatabaseIdentifier:
> 
> Can you elaborate please, what does this fix? It's clear that isolatedCopy always serves this purpose, so this comment doesn't add much.
> 
> > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:102
> > +    ensureIndexedDatabaseRelativePathExists("");
> 
> Not sure if it's something DatabaseProcess should do, or UI process. From sandboxing point of view, having access above the root level is excessive and dangerous, although I think that we mitigate this by only allowing creating certain types of nodes.

Maybe I'm misunderstanding your concern - I think the idea here is that the DatabaseProcess only has access to the database directory, and not anything above it.
It should be able to mkdir and create files underneath that directory.

> Also, String::empty().

Indeed.

> 
> > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:111
> > +        LOG_ERROR("DatabaseProcess::ensureDatabaseRelativePathExists() - Complete path is empty");
> 
> We were just calling this function with an empty argument above? Is the problem actually with null strings?

The relative path can be empty - e.g. "/Users/myname/WebKit/Databases/" with a relative path of "" resolves to a complete path of "/Users/myname/WebKit/Databases/"

The complete path cannot be empty.

> 
> > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:121
> > +    ASSERT(!isMainThread());
> 
> I think that a comment would help here. Why is calling this on main thread a problem?

Because the main thread should not do I/O.
 
> This is a faceless background process, so this is not going to block UI or something.

I'm not willing to make the leap of "this is a faceless background process therefore main thread i/o is okay"

If there was a simple process-wide way to enforce "disk i/o on the main thread is a fatal error" I would support enabling it.  Until then, we have a tradition of asserting !isMainThread() for known i/o work.

In this concrete example, main thread i/o would block the DatabaseProcess from processing non-i/o messages and requests from any number of WebProcesses

[Alexey Proskuryakov]

>It should be able to mkdir and create files underneath that directory.

Creating the top level database directory requires write access to its parent directory.

> In this concrete example, main thread i/o would block the DatabaseProcess from processing non-i/o messages and requests from any number of WebProcesses

Ok. How is this a problem?

I think that there needs to be a real reason for writing more complicated code, not just a mantra that remains a failure even for UI processes.

[Brady Eidson]

(In reply to comment #7)
> >It should be able to mkdir and create files underneath that directory.
> 
> Creating the top level database directory requires write access to its parent directory.

Fair enough.  When we get to sandboxing, we'll have to make sure UI Process makes that directory.

> > In this concrete example, main thread i/o would block the DatabaseProcess from processing non-i/o messages and requests from any number of WebProcesses
> 
> Ok. How is this a problem?

WebProcess A triggers an IDB database to be opened for the first time, requiring i/o to complete.
WebProcess B opens a connection to an already-open IDB database and needs to get the database's metadata to continue.

The operation requested by WebProcess B could occur immediately without i/o...  as long as WebProcess A is doing its i/o on a background thread.

> I think that there needs to be a real reason for writing more complicated code, not just a mantra that remains a failure even for UI processes.

"...not just a mantra that remains a failure even for UI processes."

I'm not sure what you mean here...  are you suggesting that work we put in to moving i/o to non-main threads is wasted effort?

I'm surprised to hear this.

[Alexey Proskuryakov]

Comment on attachment 218462 [details]
Patch v1

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

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:49
> +    m_inMemory = identifier.openingOrigin() != identifier.mainFrameOrigin();

Not sure if comparing origins for equality is what we want here.

Should document.domain relaxing allow using database in a subframe? How can we make this decision? How will we regression test it?

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:54
> +    m_databaseRelativeDirectory = pathByAppendingComponent(identifier.openingOrigin().databaseFilenameIdentifier(), encodeForFileName(identifier.databaseName()));

Where are path components sanitized, is that explicit? 

I think that things probably work well implicitly, but they might be fragile. For example, encodeForFileName("..") just returns "..", which is not a good component to build paths from.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:114
> +    if (m_metadata) {

Is m_metadata accessed from multiple threads without locking?

It is very difficult to analyze this code, because we have a class with methods run on multiple threads, which doesn't even have threading asserts in most functions.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:115
> +        completionCallback(true, IDBDatabaseMetadata(*m_metadata));

What is the purpose of this copy? Is the metadata mutable? But in that case, returning a possibly stale copy makes racy code.

Or if it's for passing data across threads, then a regular copy isn't sufficient, there are strings inside IDBDatabaseMetadata.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:61
> +    String fullDatabaseDirectory() const;

"Absolute path" might be a little clearer than "full".

> Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:41
> +// Current version of the metadata schema being used in the metadata database

Please add a period at the end.

> Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:46
> +    : m_identifier(database.identifier().isolatedCopy())
> +    , m_fullDatabaseDirectory(database.fullDatabaseDirectory().isolatedCopy())

As you said, we need a better way for ensuring that isolatedCopy is called.

We have some existing and fairly well established code for that, CrossThreadCopier.h. It is automatically invoked in some existing message passing designs (see CrossThreadTask, FileThreadTask, MainThreadTask).

> Source/WebKit2/Shared/SecurityOriginData.h:63
>  bool operator==(const SecurityOriginData&, const SecurityOriginData&);
> +bool operator!=(const SecurityOriginData&, const SecurityOriginData&);

As alluded to above, equality is rarely (if ever) the right operation for security origins, which is why we have individual "can" functions for everything.

[Alexey Proskuryakov]

> > Ok. How is this a problem?
> 
> WebProcess A triggers an IDB database to be opened for the first time, requiring i/o to complete.
> WebProcess B opens a connection to an already-open IDB database and needs to get the database's metadata to continue.
> 
> The operation requested by WebProcess B could occur immediately without i/o...  as long as WebProcess A is doing its i/o on a background thread.

What is the scenario where this would happen, in user observable terms? Won't there be other I/O blocking response to any reasonable user action anyway?

Presumably a process that opens a new connection does so to read some data from the DB, and won't respond to user action unlit that data is available.

I feel that the opportunity cost for this kind of effort is unreasonable by many orders of magnitude, considering all the other things we could be fixing. It's just a background process that's invoked by web process, and web process happily blocks its own main thread for HTML parsing and JavaScript execution!

> I'm not sure what you mean here...  are you suggesting that work we put in to moving i/o to non-main threads is wasted effort?

Yes. We put a lot of effort into moving explicit open/read/write/mkdir calls to secondary threads, but little effort into moving system API calls that perform blocking I/O internally. That just doesn't make sense.

> I'm surprised to hear this.

Anyway, this bug is not a good place to discuss this in general terms.

[Brady Eidson]

(In reply to comment #9)
> (From update of attachment 218462 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218462&action=review
> 
> > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:49
> > +    m_inMemory = identifier.openingOrigin() != identifier.mainFrameOrigin();
> 
> Not sure if comparing origins for equality is what we want here.

Right now it is our design.  As the comment says, we can change policies at any time.

> Should document.domain relaxing allow using database in a subframe? 

Databases can always be used in a subframe.  They just get partitioned.  I don't think document.domain relaxing breaks you out of the partition, but that's a policy greater in scope than just IDB.

> How can we make this decision? 

By actually knowing the design of 3rd party partitioning, which I don't think should hold up the task of getting IDB working...

> How will we regression test it?

Once we know the design, I'm not sure how it would be difficult to regression test?

> > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:54
> > +    m_databaseRelativeDirectory = pathByAppendingComponent(identifier.openingOrigin().databaseFilenameIdentifier(), encodeForFileName(identifier.databaseName()));
> 
> Where are path components sanitized, is that explicit? 

No, but:

> I think that things probably work well implicitly, but they might be fragile. For example, encodeForFileName("..") just returns "..", which is not a good component to build paths from.

If encodeForFileName("..") just returns ".." then encodeForFileName has a pretty terrible bug that should be fixed.

> 
> > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:114
> > +    if (m_metadata) {
> 
> Is m_metadata accessed from multiple threads without locking?

Excellent catch.

> It is very difficult to analyze this code, because we have a class with methods run on multiple threads, which doesn't even have threading asserts in most functions.

This class actually doesn't have very many methods yet, but I agree there should be more threading ASSERTs.  Willdo.

> 
> > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:115
> > +        completionCallback(true, IDBDatabaseMetadata(*m_metadata));
> 
> What is the purpose of this copy? Is the metadata mutable? 

Yes, in limited circumstances.

> But in that case, returning a possibly stale copy makes racy code.

Those limited circumstances (a version change transaction being in progress) are not implemented yet, but they preempt anything else happening.

Once version change transactions are implemented, the case where the metadata is mutable will not get the easy return like this.

> > Or if it's for passing data across threads, then a regular copy isn't sufficient, there are strings inside IDBDatabaseMetadata.

It's also for this, and therefore it should also be an isolatedcopy().

> > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:61
> > +    String fullDatabaseDirectory() const;
> 
> "Absolute path" might be a little clearer than "full".

I agree.

> > Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:46
> > +    : m_identifier(database.identifier().isolatedCopy())
> > +    , m_fullDatabaseDirectory(database.fullDatabaseDirectory().isolatedCopy())
> 
> As you said, we need a better way for ensuring that isolatedCopy is called.
> 
> We have some existing and fairly well established code for that, CrossThreadCopier.h. It is automatically invoked in some existing message passing designs (see CrossThreadTask, FileThreadTask, MainThreadTask).

Interesting.  This is new since the last time I worked in heavily MT code. I will take a look.
> 
> > Source/WebKit2/Shared/SecurityOriginData.h:63
> >  bool operator==(const SecurityOriginData&, const SecurityOriginData&);
> > +bool operator!=(const SecurityOriginData&, const SecurityOriginData&);
> 
> As alluded to above, equality is rarely (if ever) the right operation for security origins, which is why we have individual "can" functions for everything.

As answered above, currently equality *is* the design.  But I agree the design can be encapsulated by a "can" method on SecurityOriginData.

[Brady Eidson]

(In reply to comment #10)
> > > Ok. How is this a problem?
> > 
> > WebProcess A triggers an IDB database to be opened for the first time, requiring i/o to complete.
> > WebProcess B opens a connection to an already-open IDB database and needs to get the database's metadata to continue.
> > 
> > The operation requested by WebProcess B could occur immediately without i/o...  as long as WebProcess A is doing its i/o on a background thread.
> 
> What is the scenario where this would happen, in user observable terms? Won't there be other I/O blocking response to any reasonable user action anyway?
> 
> Presumably a process that opens a new connection does so to read some data from the DB, and won't respond to user action unlit that data is available.

There are some synchronous operations that can take place on an open IDB database handle in JS that don't require I/O.
Those operations can't be made until the database handle is open.

So the user observation is that the web page is "slow" (if not actually blocked) when there's no reason for it to be slow.

> I feel that the opportunity cost for this kind of effort is unreasonable by many orders of magnitude, considering all the other things we could be fixing. It's just a background process that's invoked by web process, and web process happily blocks its own main thread for HTML parsing and JavaScript execution!

As you probably know, there's various efforts at various stages of completion to move more and more types of work off the main thread.  Yes, the WebProcess main thread currently does HTML parsing and JS execution, but we also take great strides to speed those tasks up and - whenever we identify where they might block for unreasonable amounts of time - break them up so the main thread doesn't stay blocked.

That's a reasonable solution for now since we have absolute control over the characteristics of HTML parsing and JS execution.

Long ago in the project we identified known file I/O as something to not allow on the main thread since it both has potentially terrible characteristics in comparison to HTML/JS, and since we don't have anywhere near absolutely control over improving those characteristics.

> > I'm not sure what you mean here...  are you suggesting that work we put in to moving i/o to non-main threads is wasted effort?
> 
> Yes. We put a lot of effort into moving explicit open/read/write/mkdir calls to secondary threads, but little effort into moving system API calls that perform blocking I/O internally. That just doesn't make sense.

Sounds like we should expand the effort to make it more comprehensive and effective, instead of giving up on it since we haven't been 100% successful yet.

> Anyway, this bug is not a good place to discuss this in general terms.

No argument here.

[Alexey Proskuryakov]

> If encodeForFileName("..") just returns ".." then encodeForFileName has a pretty terrible bug that should be fixed.

It does, but it's not a bug - .. is a valid file name as far as file system goes. We can't escape periods, because "mydatabase.db" has a period in he name!

encodeForFileName does not sanitize - it does not check for reserved names like "COM1" on Windows, does not check for "..", and so on.

> Sounds like we should expand the effort to make it more comprehensive and effective, instead of giving up on it since we haven't been 100% successful yet.

Again, please consider opportunity cost. Most things we *should* do are OK to perpetually sit as open known bugs that don't affect users.

[Brady Eidson]

(In reply to comment #13)
> > If encodeForFileName("..") just returns ".." then encodeForFileName has a pretty terrible bug that should be fixed.
> 
> It does, but it's not a bug - .. is a valid file name as far as file system goes. We can't escape periods, because "mydatabase.db" has a period in he name!

We can scan for equality to one or two periods, though.  "." is an invalid filename, and ".." is an invalid filename, but "..." and so on are valid.

> encodeForFileName does not sanitize - it does not check for reserved names like "COM1" on Windows, does not check for "..", and so on.

Fair enough.  
> 
> > Sounds like we should expand the effort to make it more comprehensive and effective, instead of giving up on it since we haven't been 100% successful yet.
> 
> Again, please consider opportunity cost. Most things we *should* do are OK to perpetually sit as open known bugs that don't affect users.

If you're objecting to the mkdir being on a background thread, which is where you made the original comment, I wonder if we've spend more time discussing it instead of pointing out anything wrong with the already written code and having me fix it?

If you're objecting to the entire notion of trying to make all DatabaseProcess i/o be on a background thread, then you have an extremely steep mountain to climb to convince me threading isn't worth it, considering the short and long term plans for the DatabaseProcess.

[Brady Eidson]

> > encodeForFileName does not sanitize - it does not check for reserved names like "COM1" on Windows, does not check for "..", and so on.
> 
> Fair enough.  

And to elaborate on this point, do we have any sanitization like this anywhere?  Or should I just build it in to WK2/DatabaseProcess locally?

[Alexey Proskuryakov]

> I wonder if we've spend more time discussing it

Not my fault that you disagreed with my short and reasonable comment ;-)

> And to elaborate on this point, do we have any sanitization like this anywhere?

We sanitize paths with realpath in some places, although that's not quite right API to use, and not really relevant here.

Also, we have custom sanitization in code like this <http://trac.webkit.org/changeset/146902/trunk/Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm> (which is not really relevant either, as slashes are already escaped by encodeForFileName).

> Or should I just build it in to WK2/DatabaseProcess locally?

I can't think of any existing code, WebKit rarely creates files or directories. And I don't see how anything really bad could happen (famous last words). As there aren't slashes, it looks like the only thing a rogue page WebProcess might do is replace one of the components with "..", and thus manipulate files at a higher level than it should (but still within the root directory, because there will be sandbox).

So maybe it could prevent another page from using IndexedDB by preemptively creating a file with the name reserved for that page's origin. Or if there are any additional files WebKit creates in the hierarchy, it could mess with those.

Again, I don't think that there is any immediate impact, it's just that the code looks fragile without explicit sanitization.

[Brady Eidson]

Created attachment 218554 [details]
Patch v2 - Incorporate all feedback besides CrossThreadCopier

This incorporates all feedback except for looking in to CrossThreadCopier.  I'm looking more closely at that now, but just wanted another EWS pass on this.

[Alexey Proskuryakov]

Comment on attachment 218554 [details]
Patch v2 - Incorporate all feedback besides CrossThreadCopier

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

> Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:109
> +    // FIXME: We should come up with a better idiom for safely copying strings across threads.

As discussed in person, the better idiom is to use tasks that invoke CrossThreadCopier automatically, not a WorkQueue.

> Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:129
> +    return pathByAppendingComponent(m_indexedDatabaseDirectory, relativePath);

This is a misuse of pathByAppendingComponent - this function only adds one component, not a subpath.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:148
> +            MutexLocker locker(m_metadataMutex);

I'm not sure what the design here is. As noted below, m_metadataMutex is not taken by any code outside this function. But even if it were, I don't see how it would help. Did you intend to use a condition variable?

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:174
> +    ASSERT(!m_metadata);

No m_metadataMutex locking?

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:182
> +    m_metadata = m_backingStore->getOrEstablishMetadata();

No m_metadataMutex locking?

> Source/WebKit2/Shared/SecurityOriginData.h:53
> +    // Returns a serialized string that is appropriate for use as a unique filename
> +    String databaseFilenameIdentifier() const;
> +
> +    // Returns true if this origin can use the same databases as the given origin.
> +    bool canShareDatabases(const SecurityOriginData&) const;

What is the difference between SecurityOrigin and SecurityOriginData? I didn't expect to see any policy decisions made by a "Data" class, which sounds like maybe it's for temporary objects used in message passing only.

[Brady Eidson]

(In reply to comment #18)
> (From update of attachment 218554 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218554&action=review
> 
> > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:109
> > +    // FIXME: We should come up with a better idiom for safely copying strings across threads.
> 
> As discussed in person, the better idiom is to use tasks that invoke CrossThreadCopier automatically, not a WorkQueue.

New patch coming up that does this.

> > Source/WebKit2/Shared/SecurityOriginData.h:53
> > +    // Returns a serialized string that is appropriate for use as a unique filename
> > +    String databaseFilenameIdentifier() const;
> > +
> > +    // Returns true if this origin can use the same databases as the given origin.
> > +    bool canShareDatabases(const SecurityOriginData&) const;
> 
> What is the difference between SecurityOrigin and SecurityOriginData? I didn't expect to see any policy decisions made by a "Data" class, which sounds like maybe it's for temporary objects used in message passing only.

In general WK2 works in terms of SecurityOriginData objects and tries not to pretend they are SecurityOrigins because:
1 - Don't have the full fidelity of a SecurityOrigin
2 - They often exist in a different process from the originating SecurityOrigin
3 - Changes to the originating SecurityOrigin (via events in the WebProcess) are not reflected in the SecurityOriginData.

The policy could just as easily be put on WebCore::SecurityOrigin, but I think we'd fall into a trap of converting SecurityOriginData back into SecurityOrigin then assuming we can do more with it than we can.

[Brady Eidson]

(In reply to comment #18)
> > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:129
> > +    return pathByAppendingComponent(m_indexedDatabaseDirectory, relativePath);
> 
> This is a misuse of pathByAppendingComponent - this function only adds one component, not a subpath.

Perhaps this is a concern of semantics - How is a "subpath" not a "component"?

I could rename the method to pathByAppendingSubpath(), but it's implementation would remain the same (I believe)

[Alexey Proskuryakov]

> Perhaps this is a concern of semantics - How is a "subpath" not a "component"?

A path component is a file or directory name I think.

> I could rename the method to pathByAppendingSubpath(), but it's implementation would remain the same (I believe)

A function with that name would have to be more resilient - what will it do when called as pathByAppendingSubpath("/foo/bar")?

[Brady Eidson]

(In reply to comment #21)
> > Perhaps this is a concern of semantics - How is a "subpath" not a "component"?
> 
> A path component is a file or directory name I think.

Those are atomic components, sure, but I would argue a subpath is a non-atomic component.

> > I could rename the method to pathByAppendingSubpath(), but it's implementation would remain the same (I believe)
> 
> A function with that name would have to be more resilient - what will it do when called as pathByAppendingSubpath("/foo/bar")?

Probably fail the same way that pathByAppendingComponent("/foo") fails today.

[Brady Eidson]

Created attachment 218994 [details]
Patch v3 - Message passing for threading with cross thread copying.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/text/WTFString.h', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/indexeddb/IDBDatabaseMetadata.cpp', u'Source/WebCore/Modules/indexeddb/IDBDatabaseMetadata.h', u'Source/WebCore/Modules/indexeddb/IDBKeyPath.cpp', u'Source/WebCore/Modules/indexeddb/IDBKeyPath.h', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/CrossThreadCopier.cpp', u'Source/WebCore/platform/CrossThreadCopier.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp', u'Source/WebKit2/DatabaseProcess/DatabaseProcess.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabaseBackingStore.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabaseIdentifier.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabaseIdentifier.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.h', u'Source/WebKit2/Shared/AsyncTask.h', u'Source/WebKit2/Shared/SecurityOriginData.cpp', u'Source/WebKit2/Shared/SecurityOriginData.h', u'Source/WebKit2/Shared/WebCrossThreadCopier.cpp', u'Source/WebKit2/Shared/WebCrossThreadCopier.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', '--commit-queue']" exit_code: 1
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/Shared/AsyncTask.h:50:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 29 files


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

[Alexey Proskuryakov]

> Probably fail the same way that pathByAppendingComponent("/foo") fails today.

Well, "/foo" is not a component I think, so it's not expected that this will work.

[Alexey Proskuryakov]

Comment on attachment 218994 [details]
Patch v3 - Message passing for threading with cross thread copying.

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

Thank you on iterating on this code, I find it much, much easier to follow now.

Looks good to me - did you want additional reviewers to take a look?

One thing I don't quite understand is what guarantees continuing existence of target objects while async requests are in flight. Are there implicit RefPtrs somewhere?

> Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:103
> +    ensureIndexedDatabaseRelativePathExists(StringImpl::empty());

Still not sure if it's a great idea do this in DatabaseProcess. UI process has to build the path anyway, so it would be easy for it to also create the directory.

> Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:110
> +    postDatabaseTask(createAsyncTask(this, &DatabaseProcess::ensurePathExists, absoluteIndexedDatabasePathFromDatabaseRelativePath(relativePath)));

Can createAsyncTask be inside postDatabaseTask?

> Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:123
> +    return pathByAppendingComponent(m_indexedDatabaseDirectory, relativePath);

Still unhappy. Please at least add a FIXME saying that this is an abuse of this function, and should be made nicer.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:62
> +String UniqueIDBDatabase::filenameForDatabaseName()

I wonder if we should block .DS_Store or any other strange names. Not sure what the best approach is.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:148
> +    if (success)
> +        m_metadata = std::make_unique<IDBDatabaseMetadata>(metadata);
> +
> +    while (!m_pendingMetadataRequests.isEmpty()) {
> +        RefPtr<AsyncRequest> request = m_pendingMetadataRequests.takeFirst();
>          request->completeRequest();
>      }

I think that this code can enter an infinite loop if success is false, so there is no m_metadata. Client could call getOrEstablishIDBDatabaseMetadata again from its completeRequest() function, and if m_metadata is null, that would just append to m_pendingMetadataRequests, or even create a new task, depending on whether this was the last request.

I think that we need separate bits of data for m_metadata and for whether an attempt to establish it already finished (successfully or not).

> Source/WebKit2/Shared/AsyncTask.h:100
> +

Blank line.

> Source/WebKit2/Shared/SecurityOriginData.cpp:79
> +bool SecurityOriginData::canShareDatabases(const SecurityOriginData& other) const

We might need a better name for SecurityOriginData now (or eventually), something that explains how it's a SecurityOrigin with limited knowledge about things, just for secondary processes.

> Source/WebKit2/Shared/SecurityOriginData.h:49
> +    // Returns a serialized string that is appropriate for use as a unique filename

I think that "serialized string" is that same thing as "string" :)

> Source/WebKit2/Shared/WebCrossThreadCopier.cpp:36
> +CrossThreadCopierBase<false, false, UniqueIDBDatabaseIdentifier>::Type CrossThreadCopierBase<false, false, UniqueIDBDatabaseIdentifier>::copy(const UniqueIDBDatabaseIdentifier& identifier)

Can we just say what the return type is here, explicitly?

[Brady Eidson]

(In reply to comment #26)
> (From update of attachment 218994 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218994&action=review
> 
> Thank you on iterating on this code, I find it much, much easier to follow now.
> 
> Looks good to me - did you want additional reviewers to take a look?

Once you're happy, I will be having 2 others take a look at filesystem bits.

> 
> One thing I don't quite understand is what guarantees continuing existence of target objects while async requests are in flight. Are there implicit RefPtrs somewhere?

Doh!  We need to abort any outstanding AsyncRequests before the UniqueIDBDatabase goes away.

But I'm not sure what a good solution is for AsyncTasks.  Maybe they need to implicitly Ref the UniqueIDBDatabase...

> > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:103
> > +    ensureIndexedDatabaseRelativePathExists(StringImpl::empty());
> 
> Still not sure if it's a great idea do this in DatabaseProcess. UI process has to build the path anyway, so it would be easy for it to also create the directory.
> 
> > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:110
> > +    postDatabaseTask(createAsyncTask(this, &DatabaseProcess::ensurePathExists, absoluteIndexedDatabasePathFromDatabaseRelativePath(relativePath)));
> 
> Can createAsyncTask be inside postDatabaseTask?

If I can easily do it with a single variadic template, then I think it's worthwhile.
If not, I don't think it's worthwhile.

> > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:123
> > +    return pathByAppendingComponent(m_indexedDatabaseDirectory, relativePath);
> 
> Still unhappy. Please at least add a FIXME saying that this is an abuse of this function, and should be made nicer.

Willdo.

> > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:62
> > +String UniqueIDBDatabase::filenameForDatabaseName()
> 
> I wonder if we should block .DS_Store or any other strange names. Not sure what the best approach is.

I'm not sure, either.
 
> > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:148
> > +    if (success)
> > +        m_metadata = std::make_unique<IDBDatabaseMetadata>(metadata);
> > +
> > +    while (!m_pendingMetadataRequests.isEmpty()) {
> > +        RefPtr<AsyncRequest> request = m_pendingMetadataRequests.takeFirst();
> >          request->completeRequest();
> >      }
> 
> I think that this code can enter an infinite loop if success is false, so there is no m_metadata. Client could call getOrEstablishIDBDatabaseMetadata again from its completeRequest() function, and if m_metadata is null, that would just append to m_pendingMetadataRequests, or even create a new task, depending on whether this was the last request.
> 
> I think that we need separate bits of data for m_metadata and for whether an attempt to establish it already finished (successfully or not).

I agree on the theoretical problem with future changes, but don't see it happening with the code as-is.

It makes sense to keep a separate bit.  Will do.

> > Source/WebKit2/Shared/SecurityOriginData.cpp:79
> > +bool SecurityOriginData::canShareDatabases(const SecurityOriginData& other) const
> 
> We might need a better name for SecurityOriginData now (or eventually), something that explains how it's a SecurityOrigin with limited knowledge about things, just for secondary processes.

I agree.  Suggestions?

> > Source/WebKit2/Shared/SecurityOriginData.h:49
> > +    // Returns a serialized string that is appropriate for use as a unique filename
> 
> I think that "serialized string" is that same thing as "string" :)

Fixed

> > Source/WebKit2/Shared/WebCrossThreadCopier.cpp:36
> > +CrossThreadCopierBase<false, false, UniqueIDBDatabaseIdentifier>::Type CrossThreadCopierBase<false, false, UniqueIDBDatabaseIdentifier>::copy(const UniqueIDBDatabaseIdentifier& identifier)
> 
> Can we just say what the return type is here, explicitly?

Sure. I originally was copying the pattern from WebCore/CrossThreadCopier for consistency, but the WebKit file can be a clean break.

[Brady Eidson]

Created attachment 219019 [details]
Patch v4 - More code is better, right?

[Brady Eidson]

Patch v4 answers the AsyncRequest question, but doesn't answer the AsyncTask question...

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/text/WTFString.h', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/indexeddb/IDBDatabaseMetadata.cpp', u'Source/WebCore/Modules/indexeddb/IDBDatabaseMetadata.h', u'Source/WebCore/Modules/indexeddb/IDBKeyPath.cpp', u'Source/WebCore/Modules/indexeddb/IDBKeyPath.h', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/CrossThreadCopier.cpp', u'Source/WebCore/platform/CrossThreadCopier.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp', u'Source/WebKit2/DatabaseProcess/DatabaseProcess.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabaseBackingStore.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabaseIdentifier.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabaseIdentifier.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.h', u'Source/WebKit2/Shared/AsyncTask.h', u'Source/WebKit2/Shared/FileSystemHelper.cpp', u'Source/WebKit2/Shared/FileSystemHelper.h', u'Source/WebKit2/Shared/SecurityOriginData.cpp', u'Source/WebKit2/Shared/SecurityOriginData.h', u'Source/WebKit2/Shared/WebCrossThreadCopier.cpp', u'Source/WebKit2/Shared/WebCrossThreadCopier.h', u'Source/WebKit2/UIProcess/WebContext.cpp', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', '--commit-queue']" exit_code: 1
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:128:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/Shared/AsyncTask.h:50:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 32 files


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

[Brady Eidson]

Filed https://bugs.webkit.org/show_bug.cgi?id=125616 for the check-webkit-style issue.

[Brady Eidson]

(In reply to comment #29)
> Patch v4 answers the AsyncRequest question, but doesn't answer the AsyncTask question...

I have a solution for the AsyncTask question with clean database shutdown.  Patch v5 on its way.

[Brady Eidson]

Created attachment 219094 [details]
Patch v5 - Landing candidate

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/text/WTFString.h', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/indexeddb/IDBDatabaseMetadata.cpp', u'Source/WebCore/Modules/indexeddb/IDBDatabaseMetadata.h', u'Source/WebCore/Modules/indexeddb/IDBKeyPath.cpp', u'Source/WebCore/Modules/indexeddb/IDBKeyPath.h', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/CrossThreadCopier.cpp', u'Source/WebCore/platform/CrossThreadCopier.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp', u'Source/WebKit2/DatabaseProcess/DatabaseProcess.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabaseBackingStore.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabaseIdentifier.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabaseIdentifier.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.h', u'Source/WebKit2/Shared/AsyncTask.h', u'Source/WebKit2/Shared/FileSystemHelper.cpp', u'Source/WebKit2/Shared/FileSystemHelper.h', u'Source/WebKit2/Shared/SecurityOriginData.cpp', u'Source/WebKit2/Shared/SecurityOriginData.h', u'Source/WebKit2/Shared/WebCrossThreadCopier.cpp', u'Source/WebKit2/Shared/WebCrossThreadCopier.h', u'Source/WebKit2/UIProcess/WebContext.cpp', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', '--commit-queue']" exit_code: 1
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:148:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/Shared/AsyncTask.h:50:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 32 files


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

[Brady Eidson]

Created attachment 219099 [details]
Patch v6 - Move database directory creation back into the database process.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/text/WTFString.h', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/indexeddb/IDBDatabaseMetadata.cpp', u'Source/WebCore/Modules/indexeddb/IDBDatabaseMetadata.h', u'Source/WebCore/Modules/indexeddb/IDBKeyPath.cpp', u'Source/WebCore/Modules/indexeddb/IDBKeyPath.h', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/CrossThreadCopier.cpp', u'Source/WebCore/platform/CrossThreadCopier.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp', u'Source/WebKit2/DatabaseProcess/DatabaseProcess.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabaseBackingStore.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabaseIdentifier.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabaseIdentifier.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.h', u'Source/WebKit2/Shared/AsyncTask.h', u'Source/WebKit2/Shared/SecurityOriginData.cpp', u'Source/WebKit2/Shared/SecurityOriginData.h', u'Source/WebKit2/Shared/WebCrossThreadCopier.cpp', u'Source/WebKit2/Shared/WebCrossThreadCopier.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', '--commit-queue']" exit_code: 1
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:148:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/Shared/AsyncTask.h:50:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 29 files


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

[Anders Carlsson]

Comment on attachment 219099 [details]
Patch v6 - Move database directory creation back into the database process.

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

> Source/WebCore/Modules/indexeddb/IDBDatabaseMetadata.cpp:45
> +    ObjectStoreMap::const_iterator i = objectStores.begin();
> +    ObjectStoreMap::const_iterator end = objectStores.end();
> +    for (; i != end; ++i)
> +        result.objectStores.set(i->key, i->value.isolatedCopy());

Please move the declarations inside the loop:

for (auto it = objectStores.begin(), end = objectStores.end(); it != end; ++it)

> Source/WebCore/Modules/indexeddb/IDBDatabaseMetadata.cpp:63
> +    IndexMap::const_iterator i = indexes.begin();
> +    IndexMap::const_iterator end = indexes.end();
> +    for (; i != end; ++i)
> +        result.indexes.set(i->key, i->value.isolatedCopy());

Same thing here.

> Source/WebCore/Modules/indexeddb/IDBKeyPath.cpp:257
> +    result.m_array.reserveCapacity(m_array.size());

This should use reserveInitialCapacity.

> Source/WebCore/Modules/indexeddb/IDBKeyPath.cpp:259
> +        result.m_array.append(m_array[i].isolatedCopy());

This should use uncheckedAppend.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:80
> +    ASSERT(!m_identifier.databaseName().isNull());
> +
> +    if (m_identifier.databaseName().isEmpty())
> +        return "%00";
> +
> +    String filename = encodeForFileName(m_identifier.databaseName());
> +    if (filename == ".")
> +        return "%2E";
> +
> +    if (filename == "..")
> +        return "%2E%2E";
> +
> +    return filename;

Escape all the dots (Says Tim).

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:255
> +        ref();

I think you should use a protection variable here instead of explicitly calling ref()/deref().

> Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:147
> +std::unique_ptr<WebCore::SQLiteDatabase> UniqueIDBDatabaseBackingStoreSQLite::openSQLiteDatabaseAtPath(const String& path)

No need for WebCore:: here.

> Source/WebKit2/Shared/AsyncTask.h:48
> +    AsyncTaskImpl(T* const callee, void (T::*method)(Arguments...), Arguments&&... arguments)

I think callee should be a reference here (and everywhere else).

> Source/WebKit2/Shared/SecurityOriginData.h:53
> +    // Returns a string that is appropriate for use as a unique filename
> +    String databaseFilenameIdentifier() const;
> +
> +    // Returns true if this origin can use the same databases as the given origin.
> +    bool canShareDatabases(const SecurityOriginData&) const;

We think these should be moved to the IDB code.

[Brady Eidson]

> > Source/WebKit2/Shared/AsyncTask.h:48
> > +    AsyncTaskImpl(T* const callee, void (T::*method)(Arguments...), Arguments&&... arguments)
> 
> I think callee should be a reference here (and everywhere else).

This causes problems with deleted constructors, and I'm not sure how to explore the fix...  I changed the createAsyncTask() helpers to take a reference, but left the AsyncTaskImpl class taking a pointer.

I'm going to land it like this, but would love to have help resolving it later.

[Brady Eidson]

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