Database access in worker threads results in WebKit SPI notifications being posted from worker threads

[Mark Rowe (bdash)]

Database access from a worker thread results in WebDatabaseTrackerClient being called on a background thread.  dispatchDidModifyOrigin / dispatchDidModifyDatabase both post notifications using NSNotification.  This means that with workers these notifications are now being posted from background threads, rather than only from the main thread as in the past.  I don’t think it’s safe to post these notifications from a background thread since existing applications may be observing them and not expect their code to be invoked in background threads.  For sake of compatibility I’d think these need to be proxied over to the main thread to be posted.

[Alexey Proskuryakov]

I definitely agree - an embedder application only talks to WebKit from main thread, and thus should only receive delegate calls on main thread.

[Brady Eidson]

+2 in agreement.

[Dumitru Daniliuc]

I think this should be done in the client, not in DatabaseTracker: future clients might not care what thread they're called on or might actually prefer to be called on the worker thread.

Unfortunately, I'm not very familiar with the code specific to the win and mac ports, and I have zero experience with Objective C, so I can't fix the two clients, unless you can point me to some code that does something similar.

[Alexey Proskuryakov]

The client implementation in WebKit can probably just use callOnMainThread.

[Dumitru Daniliuc]

Created attachment 59356 [details]
patch

Do we need to worry that the WebDatabaseTrackerClient/WebDatabaseManager might be destroyed before the task runs on the main thread?

[Alexey Proskuryakov]

See also: bug 40654.

[Dumitru Daniliuc]

(In reply to comment #6)
> See also: bug 40654.

I'll upload a separate patch in that bug to (hopefully) fix that problem.

[Dumitru Daniliuc]

Does the patch look OK? Anything I need to add/change? Is this still a problem?

[Mark Rowe (bdash)]

Comment on attachment 59356 [details]
patch

Is it safe to pass String and SecurityOrigin instances between threads like this?

[Alexey Proskuryakov]

Strings and SecurityOrigin instances need to be copied.

[David Levin]

Comment on attachment 59356 [details]
patch

I wonder how the concept of isMainThread maps to the work being done in WebKit2 with respect to multiple threads. If someone chimes in on that, maybe it can be addressed (but I'm guessing that it may need a more systematic pass later).


> 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.)

> +    OriginModifiedInfo(WebDatabaseTrackerClient* client, SecurityOrigin* origin)
> +        : client(client), origin(origin) { }

Ideally on separate lines.


> +    WebDatabaseTrackerClient* client;

Is WebDatabaseTrackerClient threadsafe? (Not many things in WebKit are.)

> +    SecurityOrigin* origin;

Definitely not threadsafe.

> +};
> +
> +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).


> +        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.)

> +        return;
> +    }
> +
> +    RetainPtr<WebSecurityOrigin> webSecurityOrigin(AdoptNS, [[WebSecurityOrigin alloc] _initWithWebCoreSecurityOrigin:origin]);
>  
>      [[NSNotificationCenter defaultCenter] postNotificationName:WebDatabaseDidModifyOriginNotification 
>                                                          object:webSecurityOrigin.get()];
>  }
>  
> +struct DatabaseModifiedInfo {
> +    DatabaseModifiedInfo(WebDatabaseTrackerClient* client, SecurityOrigin* origin, const String& databaseName)
> +        : client(client), origin(origin), databaseIdentifier(databaseIdentifier) { }

Ideally separate lines.

> +    WebDatabaseTrackerClient* client;
> +    SecurityOrigin* origin;

Same as before.

> +    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?)


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

Same comments as before.


> Index: WebKit/win/WebDatabaseManager.cpp


It's déjà vu all over again. Is there anyway to share code here?

[David Levin]

andersca@ From my code review comment:
"I wonder how the concept of isMainThread maps to the work being done in WebKit2 with respect to multiple threads. If someone chimes in on that, maybe it can be addressed (but I'm guessing that it may need a more systematic pass later)."

This code is attempting to call back the embedder on the same thread that the embedder is using it on. It does that by proxying callbacks to the "main thread".

[David Levin]

(In reply to comment #5)
> Created an attachment (id=59356) [details]
> patch
> 
> Do we need to worry that the WebDatabaseTrackerClient/WebDatabaseManager might be destroyed before the task runs on the main thread?

Possibly. What are the lifetimes of these objects? How is the lifetime controlled?

[Dumitru Daniliuc]

Created attachment 64525 [details]
patch

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.

[David Levin]

Comment on attachment 64525 [details]
patch

None of the changes suggested for DatabaseModifiedInfo were done (in either file).

[Dumitru Daniliuc]

Created attachment 64623 [details]
patch

forgot to clean up the old struct in the last patch... sorry about that.

[Darin Adler]

Comment on attachment 64623 [details]
patch

What guarantees these main-thread calls don't outlive the WebDatabaseTrackerClient and WebDatabaseManager objects? Are those immortal objects? I see a destructor for WebDatabaseTrackerClient, so there seems to be some code that thinks it can destroy one. It would be bad if it destroyed one that had an outstanding DidModifyOriginData object.

> +        DidModifyOriginData* context = new DidModifyOriginData(client, origin->threadsafeCopy());
> +        callOnMainThread(&DidModifyOriginData::dispatchDidModifyOriginOnMainThread, context);

Soon we'll be implementing a rule where you have to call adoptPtr any time you do new. We might want to come up with a new idiom for this that avoids the raw new/delete because we'll have to come back to all these call sites some day.

review- because of the lifetime issue

[Dumitru Daniliuc]

(In reply to comment #17)
> (From update of attachment 64623 [details])
> What guarantees these main-thread calls don't outlive the WebDatabaseTrackerClient and WebDatabaseManager objects? Are those immortal objects? I see a destructor for WebDatabaseTrackerClient, so there seems to be some code that thinks it can destroy one. It would be bad if it destroyed one that had an outstanding DidModifyOriginData object.

> 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.

doesn't that take care of the problem?

[Darin Adler]

(In reply to comment #18)
> > 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.
> 
> doesn't that take care of the problem?

Yes. If they’re immortal objects, then I suggest we get rid of their destructors entirely. Don't define them. Then we'll get a linker failure.

[Dumitru Daniliuc]

Created attachment 64633 [details]
patch

same patch, removed the destructors. no linker errors on either win or mac.

[Darin Adler]

Comment on attachment 64633 [details]
patch

>  class WebDatabaseTrackerClient : public WebCore::DatabaseTrackerClient {
>  public:
>      static WebDatabaseTrackerClient* sharedWebDatabaseTrackerClient();
> -    
> -    virtual ~WebDatabaseTrackerClient();
> +

> Index: WebKit/win/WebDatabaseManager.h
> ===================================================================
> --- WebKit/win/WebDatabaseManager.h	(revision 65540)
> +++ WebKit/win/WebDatabaseManager.h	(working copy)
> @@ -83,7 +83,6 @@ public:
>  
>  private:
>      WebDatabaseManager();
> -    ~WebDatabaseManager();

By removing the destructor's declaration, you are making it use the default destructor. If you kept the declaration, but had no definition, then we'd get a link error if someone tries to destroy the object.

You could make the destructor private, but if you leave it out entirely, the compiler will create a default one for you.

[Dumitru Daniliuc]

Created attachment 64654 [details]
patch

made the destructors private.

[Darin Adler]

Comment on attachment 64654 [details]
patch

This is fine, r=me

But you should remove the destructors from the .cpp files. Leave them in the headers. I’m sorry if I wasn’t clear enough about this before. This patch leaves them in .cpp files, which is not good enough to ensure the objects never get destroyed. They could be destroyed!

[Dumitru Daniliuc]

I get an error if I remove the implementation:

unresolved external symbol "private: virtual __thiscall WebDatabaseManager::~WebDatabaseManager(void)" (??1WebDatabaseManager@@EAE@XZ) referenced in function "private: virtual void * __thiscall WebDatabaseManager::`scalar deleting destructor'(unsigned int)" (??_GWebDatabaseManager@@EAEPAXI@Z)

I'm not sure what exactly this error means, but I'm guessing it has something to do with the ULONG member variable in WebDatabaseManager.

[David Levin]

(In reply to comment #24)
> I get an error if I remove the implementation:
> 
> unresolved external symbol "private: virtual __thiscall WebDatabaseManager::~WebDatabaseManager(void)" (??1WebDatabaseManager@@EAE@XZ) referenced in function "private: virtual void * __thiscall WebDatabaseManager::`scalar deleting destructor'(unsigned int)" (??_GWebDatabaseManager@@EAEPAXI@Z)
> 
> I'm not sure what exactly this error means, but I'm guessing it has something to do with the ULONG member variable in WebDatabaseManager.

It means that the destructor for WebDatabaseManager is being called and there is no implementation, so it sounds like you need to figure out why this class is getting desctructed and get rid of that.

[Dumitru Daniliuc]

I can't find any place where WebDatabaseManager is created. It looks like this class is trying to be ref-counted, but it definitely wasn't designed to be used on multiple threads (the ref-count variable is not protected by a lock, even though it looks thread-safe otherwise). The fact that there are zero comments doesn't help either.

I think it would be best if the original author of this code took a look at this bug and fixed it, especially since he's still an active contributor to WebKit and knows a lot more about Safari's WebKit APIs than I do.

[Mark Rowe (bdash)]

(In reply to comment #26)
> I can't find any place where WebDatabaseManager is created.

WebDatabaseManager* WebDatabaseManager::createInstance()
{
    WebDatabaseManager* manager = new WebDatabaseManager();


As for where it’s destroyed:

ULONG STDMETHODCALLTYPE WebDatabaseManager::Release()
{
    ULONG newRef = --m_refCount;
    if (!newRef)
        delete this;

[Dumitru Daniliuc]

(In reply to comment #27)
> (In reply to comment #26)
> > I can't find any place where WebDatabaseManager is created.
> 
> WebDatabaseManager* WebDatabaseManager::createInstance()
> {
>     WebDatabaseManager* manager = new WebDatabaseManager();

and who calls this method?

[Mark Rowe (bdash)]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > I can't find any place where WebDatabaseManager is created.
> > 
> > WebDatabaseManager* WebDatabaseManager::createInstance()
> > {
> >     WebDatabaseManager* manager = new WebDatabaseManager();
> 
> and who calls this method?

WebDatabaseManager::sharedWebDatabaseManager… <http://trac.webkit.org/browser/trunk/WebKit/win/WebDatabaseManager.cpp#L197>.

[Dumitru Daniliuc]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > (In reply to comment #26)
> > > > I can't find any place where WebDatabaseManager is created.
> > > 
> > > WebDatabaseManager* WebDatabaseManager::createInstance()
> > > {
> > >     WebDatabaseManager* manager = new WebDatabaseManager();
> > 
> > and who calls this method?
> 
> WebDatabaseManager::sharedWebDatabaseManager… <http://trac.webkit.org/browser/trunk/WebKit/win/WebDatabaseManager.cpp#L197>.

hmm, i missed that... didn't notice that codesearch showed only the first 3 references to "WebDatabaseManager" in this file...

still, if it's a static variable, i don't understand why it needs to be AddRef()'d and Release()'d. i understand that those methods need to be implemented because IWebDatabaseManager has them, but why not have them be empty functions?

[Mark Rowe (bdash)]

(In reply to comment #30)
> still, if it's a static variable, i don't understand why it needs to be AddRef()'d and Release()'d. i understand that those methods need to be implemented because IWebDatabaseManager has them, but why not have them be empty functions?

::Release is required to return the new reference count, so at a minimum AddRef / Release needs to keep the reference count up to date.  The “delete this” code should never be hit due to our permanent reference kept in s_sharedWebDatabaseManager.  If the “delete this” case were to be hit it would indicate a programming error in the client application, so it may be useful to CRASH() in that scenario instead.  At the very least you’ll need to remove the “delete this” in order to avoid a link error if you’re removing the destructor.

[Dumitru Daniliuc]

if i remove "delete this" from Release(), i get exactly the same linker error.

[David Levin]

(In reply to comment #32)
> if i remove "delete this" from Release(), i get exactly the same linker error.

If all else fails, you can fallback to the tried and true technique of commenting things out until you figure out what is causing the delete of WebDatabaseManager (linker error).

[Mark Rowe (bdash)]

I’d just disassemble the object files and see which function references the relevant symbol.  I’m not sure exactly how one would do this on Windows, but I’m sure it’s possible.

[Eric Seidel (no email)]

Comment on attachment 64623 [details]
patch

Cleared Darin Adler's review+ from obsolete attachment 64623 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Darin Adler]

Lets not get hung up on that minor issue. We should land this and then eliminate the destructor later.

[David Levin]

(In reply to comment #36)
> Lets not get hung up on that minor issue. We should land this and then eliminate the destructor later.

Dumi, ping.

[Dumitru Daniliuc]

Created attachment 71084 [details]
patch

Removed the destructors, the patch seems to build on both Windows and Mac. Not sure why it didn't build before.

Please don't r+/r- the patch before the EWS bots are done with it.

[David Levin]

Comment on attachment 71084 [details]
patch

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

> WebKit/mac/ChangeLog:5
> +        Repost the DatabaseTracker notifications to the main thread, if needed.

This would probably be best as a function level comment. (And leave this area for the bug title.)

> WebKit/mac/ChangeLog:8
> +        Removing WebDatabaseTrackerClient::~WebDatabaseTrackerClient(),

If that is true, then leave the destructor declaration and remove the implementation. Otherwise, you'll get the default destructor implementation.

(btw, this will likely generate the link error you had before, so you'll likely need an implementation, but then this ChangeLog can be updated to be more accurate.)

Also this type of comment would be better at the function level.

> WebKit/mac/Storage/WebDatabaseTrackerClient.mm:51
> +class DidModifyOriginData {

Inherit from Noncopyable.

> WebKit/win/ChangeLog:8
> +        Removing WebDatabaseManager::~WebDatabaseManager(), because

Same comments as before.

[Dumitru Daniliuc]

Created attachment 71228 [details]
patch

> > WebKit/mac/ChangeLog:5
> > +        Repost the DatabaseTracker notifications to the main thread, if needed.
> 
> This would probably be best as a function level comment. (And leave this area for the bug title.)

to me, this looks like a pretty good description of the changes made to the code in this dir... also, i think it's redundant to add the bug description to the ChangeLog entry, since we already include a link to the bug. but i don't feel strongly about this, so please let me know what you want me to put in the "overall description" part of the ChangeLog entry (bug title?) and i'll do it.


> > WebKit/mac/ChangeLog:8
> > +        Removing WebDatabaseTrackerClient::~WebDatabaseTrackerClient(),
> 
> If that is true, then leave the destructor declaration and remove the implementation. Otherwise, you'll get the default destructor implementation.
> 
> (btw, this will likely generate the link error you had before, so you'll likely need an implementation, but then this ChangeLog can be updated to be more accurate.)
> 
> Also this type of comment would be better at the function level.

you're right, still getting those link errors. i added the destructors back.

> > WebKit/mac/Storage/WebDatabaseTrackerClient.mm:51
> > +class DidModifyOriginData {
> 
> Inherit from Noncopyable.

done.

> > WebKit/win/ChangeLog:8
> > +        Removing WebDatabaseManager::~WebDatabaseManager(), because
> 
> Same comments as before.

did the same things i did for the changes in WebKit/mac.

[Dumitru Daniliuc]

patch landed: r70195.

[David Levin]

Did a bug get filed about removing the destructor?

[Dumitru Daniliuc]

(In reply to comment #42)
> Did a bug get filed about removing the destructor?

No, I'll do that.

[Dumitru Daniliuc]

(In reply to comment #43)
> (In reply to comment #42)
> > Did a bug get filed about removing the destructor?
> 
> No, I'll do that.

opened bug 48033.