ASSERT that a sessionID is valid when encoding it
Created attachment 373074 [details] Patch
<rdar://problem/52298011>
I plan to do a follow-up that will remove the sessionID from IDBValue.
Comment on attachment 373074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373074&action=review > Source/WebCore/Modules/indexeddb/IDBValue.h:57 > - const PAL::SessionID& sessionID() const { return m_sessionID; } > + const PAL::SessionID& sessionID() const > + { > + // FIXME: We should assert m_sessionID is valid or remove m_sessionID. > + return m_sessionID; > + } When changing from a trivial to a multi-line inline function, I think it’s better style to put the function at the bottom of the file, outside the class definition, with other inline function and function template definitions. This helps keep class definitions smaller and easier to read. > Source/WebCore/Modules/indexeddb/IDBValue.h:78 > - encoder << m_sessionID; > + encoder << m_sessionID.isValid(); > + if (m_sessionID.isValid()) > + encoder << m_sessionID; Kind of inelegant to have the "invalid" concept built into SessionID but not work for encode/decode. Maybe change to not having "invalid" session IDs and use Optional<> instead as a cleanup later. That would make this code nicer like the m_databaseIdentifier example. > Source/WebCore/Modules/indexeddb/shared/IDBRequestData.h:74 > - const IDBDatabaseIdentifier& databaseIdentifier() const { return m_databaseIdentifier; } > + const IDBDatabaseIdentifier& databaseIdentifier() const > + { > + ASSERT(m_databaseIdentifier); > + if (!m_databaseIdentifier) > + m_databaseIdentifier = IDBDatabaseIdentifier { }; > + return *m_databaseIdentifier; > + } Same comment about moving definitions when making them non-trivial. > Source/WebCore/PAL/pal/SessionID.h:108 > + // FIXME: We should fail to decode an invalid sessionID. > + ASSERT(SessionID { *sessionID }.isValid()); When will we make that change? I understand that perhaps it’s too risky now, but when will it be possible?
(In reply to Darin Adler from comment #4) > Comment on attachment 373074 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=373074&action=review > > > Source/WebCore/Modules/indexeddb/IDBValue.h:57 > > - const PAL::SessionID& sessionID() const { return m_sessionID; } > > + const PAL::SessionID& sessionID() const > > + { > > + // FIXME: We should assert m_sessionID is valid or remove m_sessionID. > > + return m_sessionID; > > + } > > When changing from a trivial to a multi-line inline function, I think it’s > better style to put the function at the bottom of the file, outside the > class definition, with other inline function and function template > definitions. This helps keep class definitions smaller and easier to read. OK > > Source/WebCore/Modules/indexeddb/IDBValue.h:78 > > - encoder << m_sessionID; > > + encoder << m_sessionID.isValid(); > > + if (m_sessionID.isValid()) > > + encoder << m_sessionID; > > Kind of inelegant to have the "invalid" concept built into SessionID but not > work for encode/decode. Maybe change to not having "invalid" session IDs and > use Optional<> instead as a cleanup later. That would make this code nicer > like the m_databaseIdentifier example. Sure, the idea is to remove m_sessionID from IDBValue. See https://bugs.webkit.org/show_bug.cgi?id=199320 for such a patch. > > Source/WebCore/Modules/indexeddb/shared/IDBRequestData.h:74 > > - const IDBDatabaseIdentifier& databaseIdentifier() const { return m_databaseIdentifier; } > > + const IDBDatabaseIdentifier& databaseIdentifier() const > > + { > > + ASSERT(m_databaseIdentifier); > > + if (!m_databaseIdentifier) > > + m_databaseIdentifier = IDBDatabaseIdentifier { }; > > + return *m_databaseIdentifier; > > + } > > Same comment about moving definitions when making them non-trivial. > > > Source/WebCore/PAL/pal/SessionID.h:108 > > + // FIXME: We should fail to decode an invalid sessionID. > > + ASSERT(SessionID { *sessionID }.isValid()); > > When will we make that change? I understand that perhaps it’s too risky now, > but when will it be possible? I think we can do this in a couple of weeks.
Created attachment 373444 [details] Patch
Comment on attachment 373444 [details] Patch Clearing flags on attachment: 373444 Committed r247159: <https://trac.webkit.org/changeset/247159>
All reviewed patches have been landed. Closing bug.
IndexedDB layout tests are failing the new assert: https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r247159%20(3424)/results.html
Reverted r247159 for reason: IndexedDB layout tests are failing the new assert. Committed r247163: <https://trac.webkit.org/changeset/247163>
Created attachment 375687 [details] Fixing assertion
Comment on attachment 375687 [details] Fixing assertion Clearing flags on attachment: 375687 Committed r248366: <https://trac.webkit.org/changeset/248366>