WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
199302
ASSERT that a sessionID is valid when encoding it
https://bugs.webkit.org/show_bug.cgi?id=199302
Summary
ASSERT that a sessionID is valid when encoding it
youenn fablet
Reported
2019-06-27 17:53:49 PDT
ASSERT that a sessionID is valid when encoding it
Attachments
Patch
(7.14 KB, patch)
2019-06-27 17:56 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(6.85 KB, patch)
2019-07-03 17:38 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Fixing assertion
(8.95 KB, patch)
2019-08-06 22:37 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2019-06-27 17:56:38 PDT
Created
attachment 373074
[details]
Patch
Radar WebKit Bug Importer
Comment 2
2019-06-27 17:58:00 PDT
<
rdar://problem/52298011
>
youenn fablet
Comment 3
2019-06-28 09:30:35 PDT
I plan to do a follow-up that will remove the sessionID from IDBValue.
Darin Adler
Comment 4
2019-06-30 13:27:08 PDT
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?
youenn fablet
Comment 5
2019-07-02 12:48:02 PDT
(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.
youenn fablet
Comment 6
2019-07-03 17:38:59 PDT
Created
attachment 373444
[details]
Patch
WebKit Commit Bot
Comment 7
2019-07-05 09:24:32 PDT
Comment on
attachment 373444
[details]
Patch Clearing flags on attachment: 373444 Committed
r247159
: <
https://trac.webkit.org/changeset/247159
>
WebKit Commit Bot
Comment 8
2019-07-05 09:24:34 PDT
All reviewed patches have been landed. Closing bug.
Ryan Haddad
Comment 9
2019-07-05 10:59:41 PDT
IndexedDB layout tests are failing the new assert:
https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r247159%20(3424)/results.html
Ryan Haddad
Comment 10
2019-07-05 11:00:59 PDT
Reverted
r247159
for reason: IndexedDB layout tests are failing the new assert. Committed
r247163
: <
https://trac.webkit.org/changeset/247163
>
youenn fablet
Comment 11
2019-08-06 22:37:05 PDT
Created
attachment 375687
[details]
Fixing assertion
WebKit Commit Bot
Comment 12
2019-08-07 09:09:04 PDT
Comment on
attachment 375687
[details]
Fixing assertion Clearing flags on attachment: 375687 Committed
r248366
: <
https://trac.webkit.org/changeset/248366
>
WebKit Commit Bot
Comment 13
2019-08-07 09:09:06 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug