Created attachment 263122 [details] Patch v1

Comment on attachment 263122 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=263122&action=review > Source/WebCore/Modules/indexeddb/IDBTransaction.h:64 > - virtual IDBDatabase* db() const = 0; > + virtual IDBDatabase* db() = 0; Why no const? > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:62 > -Ref<UniqueIDBDatabaseTransaction> UniqueIDBDatabaseConnection::createVersionChangeTransaction(uint64_t newVersion) > +UniqueIDBDatabaseTransaction* UniqueIDBDatabaseConnection::createVersionChangeTransaction(uint64_t newVersion) Why are you making this a raw pointer? > Source/WebCore/Modules/indexeddb/shared/IDBError.cpp:63 > + result.m_code = m_code; > + result.m_message = m_message.isolatedCopy(); Would return { m_code, m_message.isolatedCopy() }; work here? Setting members individually is annoying, and we forget one sometimes.

(In reply to comment #2) > Comment on attachment 263122 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=263122&action=review > > > Source/WebCore/Modules/indexeddb/IDBTransaction.h:64 > > - virtual IDBDatabase* db() const = 0; > > + virtual IDBDatabase* db() = 0; > > Why no const? Modern version - that keeps a Ref<> instead of a RefPtr<> - can't cope with the const. No real advantage, in practice. > > > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:62 > > -Ref<UniqueIDBDatabaseTransaction> UniqueIDBDatabaseConnection::createVersionChangeTransaction(uint64_t newVersion) > > +UniqueIDBDatabaseTransaction* UniqueIDBDatabaseConnection::createVersionChangeTransaction(uint64_t newVersion) > > Why are you making this a raw pointer? Because the connection itself now owns the object, and the object handles cleaning itself up. > > > Source/WebCore/Modules/indexeddb/shared/IDBError.cpp:63 > > + result.m_code = m_code; > > + result.m_message = m_message.isolatedCopy(); > > Would return { m_code, m_message.isolatedCopy() }; work here? Maybe.

Comment on attachment 263122 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=263122&action=review r=me with comments: > Source/WebCore/Modules/indexeddb/client/IDBOpenDBRequestImpl.cpp:75 > + Ref<IDBDatabase> database = IDBDatabase::create(*scriptExecutionContext(), connection(), resultData); Early return if (!scriptExecutionContext()) There should at least be an ASSERT(scriptExecutionContext()) >>> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:62 >>> +UniqueIDBDatabaseTransaction* UniqueIDBDatabaseConnection::createVersionChangeTransaction(uint64_t newVersion) >> >> Why are you making this a raw pointer? > > Because the connection itself now owns the object, and the object handles cleaning itself up. I really think this is a step in the wrong direction. UniqueIDBDatabase::startVersionChangeTransaction sets m_versionChangeTransaction as the result of this and then dereferences it without checking it (a separate issue that should be addressed), but I don't think this stays inside of UniqueIDBDatabaseConnection, and I think it should stay a Ref to be safe so we don't accidentally use anything after freeing.

(In reply to comment #4) > >>> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:62 > >>> +UniqueIDBDatabaseTransaction* UniqueIDBDatabaseConnection::createVersionChangeTransaction(uint64_t newVersion) > >> > >> Why are you making this a raw pointer? > > > > Because the connection itself now owns the object, and the object handles cleaning itself up. > > I really think this is a step in the wrong direction. I disagree. > UniqueIDBDatabase::startVersionChangeTransaction sets > m_versionChangeTransaction as the result of this and then dereferences it > without checking it (a separate issue that should be addressed) When we use "create" in naming, we're guaranteeing the return of a valid object. So using it right away in UniqueIDBDatabase is a non-issue. The corollary you're alluding to is the "maybeCreate" pattern. To make it more clear what "create" means in established style in WebCore, I'll change it to return a c++ reference. > but I don't think this stays inside of UniqueIDBDatabaseConnection, and I think it > should stay a Ref to be safe so we don't accidentally use anything after > freeing. There's a lot of objects with references to things here, and I think it would harder to correctly make sure everybody releases their Ref and objects don't leak indefinitely than it would be to clean up appropriately. I'll add addition cleanup in the UniqueIDBDatabaseTransaction d'tor.