[webkit-reviews] review requested: [Bug 40655] Database access in worker threads results in WebKit SPI notifications being posted from worker threads : [Attachment 64525] patch

[bugzilla-daemon at webkit.org]

Dumitru Daniliuc <dumi at chromium.org> has asked	for review:
Bug 40655: Database access in worker threads results in WebKit SPI
notifications being posted from worker threads
https://bugs.webkit.org/show_bug.cgi?id=40655

Attachment 64525: patch
https://bugs.webkit.org/attachment.cgi?id=64525&action=review

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
WebDatabaseManager and WebDatabaseTrackerClient seem to be initialized as
static variables in a function, so i don't think we need to worry that they'll
be destroyed while a dispatchDidModifyOrigin task is pending on the main
thread.

> > Index: WebKit/mac/Storage/WebDatabaseTrackerClient.mm
> > +struct OriginModifiedInfo {
> 
> I can't parse this name (but my attempt was comical).
> ...
> Now that I read lower, I get it. I personally like DidModifyOriginData
better. What do you think? (I chose Data instead of Info to avoid an
abbreviation per WebKit style while keeping it short.)

changed.

> > +	 OriginModifiedInfo(WebDatabaseTrackerClient* client, SecurityOrigin*
origin)
> > +	     : client(client), origin(origin) { }
> 
> Ideally on separate lines.

done.

> > +	 WebDatabaseTrackerClient* client;
> 
> Is WebDatabaseTrackerClient threadsafe? (Not many things in WebKit are.)

i think it is. it doesn't have any field, only methods.

> > +	 SecurityOrigin* origin;
> 
> Definitely not threadsafe.

fixed.

> > +static void dispatchDidModifyOriginOnMainThread(void* context)
> > +{
> > +	 OriginModifiedInfo* info = static_cast<OriginModifiedInfo*>(context);
> > +	 info->client->dispatchDidModifyOrigin(info->origin);
> > +	 delete info;
> > +}
> > +
> >  void WebDatabaseTrackerClient::dispatchDidModifyOrigin(SecurityOrigin*
origin)
> >  {
> > -	  RetainPtr<WebSecurityOrigin> webSecurityOrigin(AdoptNS,
[[WebSecurityOrigin alloc] _initWithWebCoreSecurityOrigin:origin]);
> > +	 if (!isMainThread()) {
> > +	     OriginModifiedInfo* context = new OriginModifiedInfo(this,
origin);
> 
> Use origin->threadsafeCopy(); to get something appropriate for another thread
(and change the member variable to be a RefPtr).

done.

> > +	     callOnMainThread(dispatchDidModifyOriginOnMainThread,
static_cast<void*>(context));
> 
> I would just make "dispatchDidModifyOriginOnMainThread" a static method on
the class above and then make the variables private.
> 
> Taking it a step further you could just have the class have just one exposed
static method: DidModifyOriginData::dispatchToMainThread which does the new and
then the callOnMainThread. (It also makes it clear who should do the copies --
the class itself since it is doing the call to the main thread.)

done.

> > +struct DatabaseModifiedInfo {
> > +	 DatabaseModifiedInfo(WebDatabaseTrackerClient* client, SecurityOrigin*
origin, const String& databaseName)
> > +	     : client(client), origin(origin),
databaseIdentifier(databaseIdentifier) { }
> 
> Ideally separate lines.

done.

> > +	 WebDatabaseTrackerClient* client;
> > +	 SecurityOrigin* origin;
> 
> Same as before.

done.

> > +	 String databaseIdentifier;
> 
> Use databaseIdentifier->crossThreadString(); (I'm sorry about the name. It
doesn't provide a string that can be used on multiple threads at the same time.
It does provide a string to be given to another thread. Maybe otherThreadString
would be a better name?)

looks like this parameter went away.

> >  void WebDatabaseTrackerClient::dispatchDidModifyDatabase(SecurityOrigin*
origin, const String& databaseIdentifier)
> >  {
> > +	 if (!isMainThread()) {
> > +	     DatabaseModifiedInfo* context = new DatabaseModifiedInfo(this,
origin, databaseIdentifier);
> 
> Same comments as before.

done.

> > Index: WebKit/win/WebDatabaseManager.cpp
> 
> 
> It's déjà vu all over again. Is there anyway to share code here?

not sure. the mac implementation uses objective-c and implements only the
methods defined in DatabaseTrackerClient.h. the win implementation has a bunch
of other methods too, and uses win-specific types. in addition, even if we
wanted to share only the DidModifyOriginData class, i'm not sure where we'd put
that file: there's no WebKit/generic (or similar) directory.