Bug 199302 - ASSERT that a sessionID is valid when encoding it
Summary: ASSERT that a sessionID is valid when encoding it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-27 17:53 PDT by youenn fablet
Modified: 2019-08-07 09:09 PDT (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2019-06-27 17:53:49 PDT
ASSERT that a sessionID is valid when encoding it
Comment 1 youenn fablet 2019-06-27 17:56:38 PDT
Created attachment 373074 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2019-06-27 17:58:00 PDT
<rdar://problem/52298011>
Comment 3 youenn fablet 2019-06-28 09:30:35 PDT
I plan to do a follow-up that will remove the sessionID from IDBValue.
Comment 4 Darin Adler 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?
Comment 5 youenn fablet 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.
Comment 6 youenn fablet 2019-07-03 17:38:59 PDT
Created attachment 373444 [details]
Patch
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2019-07-05 09:24:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Ryan Haddad 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
Comment 10 Ryan Haddad 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>
Comment 11 youenn fablet 2019-08-06 22:37:05 PDT
Created attachment 375687 [details]
Fixing assertion
Comment 12 WebKit Commit Bot 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>
Comment 13 WebKit Commit Bot 2019-08-07 09:09:06 PDT
All reviewed patches have been landed.  Closing bug.