Modern IDB: Only fire blocked events after all open connections have handled their versionchange events

[Brady Eidson]

Modern IDB: Only fire blocked events after all open connections have handled their version change events

Currently, whenever we send all open connections the versionchange event, we immediately fire the blocked event on the request.

That's not right, as in their versionchange event handlers the connections can close themselves, rendering the request *not* blocked.

This behavior is explicitly denoted in the spec and is covered by existing failing tests.

[Brady Eidson]

Created attachment 267998 [details]
Patch v1

[Andy Estes]

Comment on attachment 267998 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=267998&action=review

> Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:382
> +        serverConnection().didFireVersionChangeEvent(m_databaseConnectionIdentifier, static_cast<IDBVersionChangeEvent&>(event).requestIdentifier());

You should use downcast here instead of static_cast.

> Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.h:92
> +    virtual bool dispatchEvent(Event&) override final;

No need to specify virtual here.

> Source/WebCore/Modules/indexeddb/client/IDBVersionChangeEventImpl.h:51
> +    virtual bool isVersionChangeEvent() const override final { return true; }

Ditto.

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:350
> +    if (!databaseConnection)
> +        return;
> +
> +    databaseConnection->didFireVersionChangeEvent(requestIdentifier);

It'd be more concise to write:

if (auto databaseConnection = m_databaseConnections.get(databaseConnectionIdentifier))
    databaseConnection->didFireVersionChangeEvent(requestIdentifier);

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:377
> +    RefPtr<InProcessIDBServer> self(this);
> +    RunLoop::current().dispatch([this, self, databaseConnectionIdentifier, requestIdentifier] {
> +        m_server->didFireVersionChangeEvent(databaseConnectionIdentifier, requestIdentifier);
> +    });

Could you use self->m_server so you don't have to capture this?

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.h:75
> +    virtual void didFireVersionChangeEvent(uint64_t databaseConnectionIdentifier, const IDBResourceIdentifier& requestIdentifier) override final;

No need for virtual here, or for any other function in this file that's already marked override.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.h:94
> -    virtual void fireVersionChangeEvent(IDBServer::UniqueIDBDatabaseConnection&, uint64_t requestedVersion) override final;
> +    virtual void fireVersionChangeEvent(IDBServer::UniqueIDBDatabaseConnection&, const IDBResourceIdentifier& requestIdentifier, uint64_t requestedVersion) override final;

Ditto.

[Brady Eidson]

I'll do *some* of the "virtual not needed"s, but not all (where the rest of the file is already littered with them.

(In reply to comment #2)
> Comment on attachment 267998 [details]
> Patch v1
> 
> > Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:377
> > +    RefPtr<InProcessIDBServer> self(this);
> > +    RunLoop::current().dispatch([this, self, databaseConnectionIdentifier, requestIdentifier] {
> > +        m_server->didFireVersionChangeEvent(databaseConnectionIdentifier, requestIdentifier);
> > +    });
> 
> Could you use self->m_server so you don't have to capture this?

Nope, need to capture this to have access to private variables.

[Brady Eidson]

Forgot to reply to this one:

(In reply to comment #2)
> Comment on attachment 267998 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267998&action=review
> 
> > Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:382
> > +        serverConnection().didFireVersionChangeEvent(m_databaseConnectionIdentifier, static_cast<IDBVersionChangeEvent&>(event).requestIdentifier());
> 
> You should use downcast here instead of static_cast.

It's an upcast, not a downcast. =/

[Brady Eidson]

https://trac.webkit.org/changeset/194452

[Andy Estes]

(In reply to comment #4)
> Forgot to reply to this one:
> 
> (In reply to comment #2)
> > Comment on attachment 267998 [details]
> > Patch v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=267998&action=review
> > 
> > > Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:382
> > > +        serverConnection().didFireVersionChangeEvent(m_databaseConnectionIdentifier, static_cast<IDBVersionChangeEvent&>(event).requestIdentifier());
> > 
> > You should use downcast here instead of static_cast.
> 
> It's an upcast, not a downcast. =/

IDBVersionChangeEvent is a subclass of Event, right? This is what downcast is for.

[Andy Estes]

Comment on attachment 267998 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=267998&action=review

>>>> Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:382
>>>> +        serverConnection().didFireVersionChangeEvent(m_databaseConnectionIdentifier, static_cast<IDBVersionChangeEvent&>(event).requestIdentifier());
>>> 
>>> You should use downcast here instead of static_cast.
>> 
>> It's an upcast, not a downcast. =/
> 
> IDBVersionChangeEvent is a subclass of Event, right? This is what downcast is for.

Arguably this isn't necessary since you're already calling isVersionChangeEvent(), but it wouldn't hurt to have downcast's assertions.

[Brady Eidson]

(In reply to comment #6)
> (In reply to comment #4)
> > Forgot to reply to this one:
> > 
> > (In reply to comment #2)
> > > Comment on attachment 267998 [details]
> > > Patch v1
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=267998&action=review
> > > 
> > > > Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:382
> > > > +        serverConnection().didFireVersionChangeEvent(m_databaseConnectionIdentifier, static_cast<IDBVersionChangeEvent&>(event).requestIdentifier());
> > > 
> > > You should use downcast here instead of static_cast.
> > 
> > It's an upcast, not a downcast. =/
> 
> IDBVersionChangeEvent is a subclass of Event, right? This is what downcast
> is for.

It's not a direct subclass, but it is a descendent class.

I guess I misunderstand the direction.

Regardless, I changed it to downcast<> and it wouldn't compile. Maybe I just don't understand how downcast<> is supposed to be used.

[Brady Eidson]

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > Forgot to reply to this one:
> > > 
> > > (In reply to comment #2)
> > > > Comment on attachment 267998 [details]
> > > > Patch v1
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=267998&action=review
> > > > 
> > > > > Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:382
> > > > > +        serverConnection().didFireVersionChangeEvent(m_databaseConnectionIdentifier, static_cast<IDBVersionChangeEvent&>(event).requestIdentifier());
> > > > 
> > > > You should use downcast here instead of static_cast.
> > > 
> > > It's an upcast, not a downcast. =/
> > 
> > IDBVersionChangeEvent is a subclass of Event, right? This is what downcast
> > is for.
> 
> It's not a direct subclass, but it is a descendent class.
> 
> I guess I misunderstand the direction.
> 
> Regardless, I changed it to downcast<> and it wouldn't compile. Maybe I just
> don't understand how downcast<> is supposed to be used.

The build failure I got was caused by:
        static_assert(std::is_void<ExpectedType>::value, "Missing TypeCastTraits specialization");
in TypeCasts.h

[Brady Eidson]

And yes, the comment above that assert does say exactly how to fix it.

[Brady Eidson]

Created attachment 268045 [details]
Downcast followup

[Andy Estes]

(In reply to comment #11)
> Created attachment 268045 [details]
> Downcast followup

👍

[Brady Eidson]

Neither CQ nor EWS seem willing to chew on this patch... will land manually.

[Brady Eidson]

Followup landed in https://trac.webkit.org/changeset/194468

[Sihui Liu]

*** Bug 71130 has been marked as a duplicate of this bug. ***