Modern IDB: UniqueIDBDatabase's m_databaseInfo is used from multiple threads
It's meant to be assigned and used on the main thread, but in UniqueIDBDatabase::performPutOrAdd, which is a database thread method:
auto objectStoreInfo = m_databaseInfo->infoForExistingObjectStore(objectStoreIdentifier);
Noticed this in a random audit, don't know of any actual fallout, but it's obviously not safe.
Created attachment 270728 [details]
Patch for moving between machines
This is the patch I plan on fixing up and landing tomorrow from a different machine - attaching it here so I don't lose it.
In the meanwhile - while fixing this specific instance is easy - I'm trying to come up with a good design pattern to apply here to actually prevent this type of coding mistake.
One idea is that UniqueIDBDatabase inherits from some class - like UniqueIDBDatabaseBase - and that class is the keeper of thread-sensitive objects, such as m_databaseInfo.
Additionally, the data members themselves are private, and access to them is only allowed through protected methods.
The protector methods that allow access could have ASSERT_WITH_SECURITY_IMPLICATION checks making sure the access is on the expected thread (e.g., m_databaseInfo only ever on the main thread, m_backingStore only ever on the database server thread, etc etc)
It would still be *possible* for the caller to save off their own pointer to the object that could be incorrectly used on another thread, but that would appear much more suspicious in any patch review and/or audit.
CC'ing Alexey here for comment and ideas, since he's spent a lot of time dwelling on thread safety issues.
Sending the entire IDBObjectStoreInfo over for every put/add might be overkill - I'll look closely and see if sending some pieces over piecemeal might be lighter weight.
For example - We don't need a copy of each IDBIndexInfo that might exist in the object store.
(In reply to comment #3)
> Sending the entire IDBObjectStoreInfo over for every put/add might be
> overkill - I'll look closely and see if sending some pieces over piecemeal
> might be lighter weight.
> For example - We don't need a copy of each IDBIndexInfo that might exist in
> the object store.
We can go much lighter weight for the Memory backing store, but the SQLite backing store requires the whole thing to walk the indexes.
The Memory backing store does NOT need this because it saves its own copy of IDBIndexInfos in each MemoryIndex when it is created.
That seems much more efficient than passing over a full copy for every put or add.
All UniqueIDBDatabase needs for its logic is:
-object store identifier
-object store key path
If the SQLite backing store maintained its own copy of the database info, it could also get buy with just that.
(In reply to comment #5)
> If the SQLite backing store maintained its own copy of the database info, it
> could also get buy with just that.
There is, of course, complexity in doing this properly which is:
-Keeping it updated during version change transactions
-Reverting it on version change abort
(In reply to comment #6)
> (In reply to comment #5)
> > If the SQLite backing store maintained its own copy of the database info, it
> > could also get buy with just that.
> There is, of course, complexity in doing this properly which is:
> -Keeping it updated during version change transactions
> -Reverting it on version change abort
Both are things we already do in the memory backing store, and will be covered by a myriad of tests, so it's definitely not insurmountable.
Created attachment 270760 [details]
Comment on attachment 270760 [details]
Clearing flags on attachment: 270760
Committed r196191: <http://trac.webkit.org/changeset/196191>
All reviewed patches have been landed. Closing bug.