Bug 153912

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 -autoincrement flag -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] Patch v1

Comment on attachment 270760 [details] Patch v1 Clearing flags on attachment: 270760 Committed r196191: <http://trac.webkit.org/changeset/196191>

All reviewed patches have been landed. Closing bug.