Bug 199302 – ASSERT that a sessionID is valid when encoding it

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

ASSERT that a sessionID is valid when encoding it

[youenn fablet]

Created attachment 373074 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/52298011>

[youenn fablet]

I plan to do a follow-up that will remove the sessionID from IDBValue.

[Darin Adler]

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]

(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]

Created attachment 373444 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 373444 [details]
Patch

Clearing flags on attachment: 373444

Committed r247159: <https://trac.webkit.org/changeset/247159>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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]

Reverted r247159 for reason:

IndexedDB layout tests are failing the new assert.

Committed r247163: <https://trac.webkit.org/changeset/247163>

[youenn fablet]

Created attachment 375687 [details]
Fixing assertion

[WebKit Commit Bot]

Comment on attachment 375687 [details]
Fixing assertion

Clearing flags on attachment: 375687

Committed r248366: <https://trac.webkit.org/changeset/248366>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.