Bug 152593

Summary: Modern IDB: Only fire blocked events after all open connections have handled their versionchange events
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, aestes, alecflett, cdumez, commit-queue, esprehn+autocc, jsbell, kangil.han
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 149117, 150882, 152600    
Attachments:
Description Flags
Patch v1
aestes: review+, aestes: commit-queue-
Downcast followup none

Description Brady Eidson 2015-12-29 21:23:49 PST
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.
Comment 1 Brady Eidson 2015-12-29 21:32:44 PST
Created attachment 267998 [details]
Patch v1
Comment 2 Andy Estes 2015-12-30 15:08:41 PST
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.
Comment 3 Brady Eidson 2015-12-30 20:48:25 PST
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.
Comment 4 Brady Eidson 2015-12-30 20:56:36 PST
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. =/
Comment 5 Brady Eidson 2015-12-30 21:44:28 PST
https://trac.webkit.org/changeset/194452
Comment 6 Andy Estes 2015-12-30 22:28:30 PST
(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.
Comment 7 Andy Estes 2015-12-30 22:34:54 PST
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.
Comment 8 Brady Eidson 2015-12-31 10:08:38 PST
(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.
Comment 9 Brady Eidson 2015-12-31 10:10:22 PST
(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
Comment 10 Brady Eidson 2015-12-31 10:10:47 PST
And yes, the comment above that assert does say exactly how to fix it.
Comment 11 Brady Eidson 2015-12-31 10:17:11 PST
Created attachment 268045 [details]
Downcast followup
Comment 12 Andy Estes 2015-12-31 12:10:28 PST
(In reply to comment #11)
> Created attachment 268045 [details]
> Downcast followup

👍
Comment 13 Brady Eidson 2015-12-31 12:26:35 PST
Neither CQ nor EWS seem willing to chew on this patch... will land manually.
Comment 14 Brady Eidson 2015-12-31 13:30:26 PST
Followup landed in https://trac.webkit.org/changeset/194468