Bug 40655 - Database access in worker threads results in WebKit SPI notifications being posted from worker threads
Summary: Database access in worker threads results in WebKit SPI notifications being p...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Dumitru Daniliuc
URL:
Keywords:
Depends on:
Blocks: 40654
  Show dependency treegraph
 
Reported: 2010-06-15 23:04 PDT by Mark Rowe (bdash)
Modified: 2010-10-20 17:47 PDT (History)
8 users (show)

See Also:


Attachments
patch (7.37 KB, patch)
2010-06-22 02:53 PDT, Dumitru Daniliuc
levin: review-
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (9.39 KB, patch)
2010-08-16 14:56 PDT, Dumitru Daniliuc
levin: review-
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (8.04 KB, patch)
2010-08-17 12:50 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch (9.86 KB, patch)
2010-08-17 14:39 PDT, Dumitru Daniliuc
darin: review-
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (9.12 KB, patch)
2010-08-17 17:02 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (9.88 KB, patch)
2010-10-18 14:36 PDT, Dumitru Daniliuc
levin: review-
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (7.93 KB, patch)
2010-10-19 16:57 PDT, Dumitru Daniliuc
levin: review+
dumi: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Rowe (bdash) 2010-06-15 23:04:40 PDT
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.
Comment 1 Alexey Proskuryakov 2010-06-15 23:18:16 PDT
I definitely agree - an embedder application only talks to WebKit from main thread, and thus should only receive delegate calls on main thread.
Comment 2 Brady Eidson 2010-06-16 00:32:27 PDT
+2 in agreement.
Comment 3 Dumitru Daniliuc 2010-06-16 14:40:16 PDT
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.
Comment 4 Alexey Proskuryakov 2010-06-16 15:44:41 PDT
The client implementation in WebKit can probably just use callOnMainThread.
Comment 5 Dumitru Daniliuc 2010-06-22 02:53:19 PDT
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?
Comment 6 Alexey Proskuryakov 2010-06-22 13:41:37 PDT
See also: bug 40654.
Comment 7 Dumitru Daniliuc 2010-06-26 04:10:09 PDT
(In reply to comment #6)
> See also: bug 40654.

I'll upload a separate patch in that bug to (hopefully) fix that problem.
Comment 8 Dumitru Daniliuc 2010-07-08 18:26:03 PDT
Does the patch look OK? Anything I need to add/change? Is this still a problem?
Comment 9 Mark Rowe (bdash) 2010-07-08 21:08:12 PDT
Comment on attachment 59356 [details]
patch

Is it safe to pass String and SecurityOrigin instances between threads like this?
Comment 10 Alexey Proskuryakov 2010-07-08 22:00:38 PDT
Strings and SecurityOrigin instances need to be copied.
Comment 11 David Levin 2010-07-09 00:32:06 PDT
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?
Comment 12 David Levin 2010-07-09 00:34:50 PDT
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".
Comment 13 David Levin 2010-07-09 00:37:25 PDT
(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?
Comment 14 Dumitru Daniliuc 2010-08-16 14:56:21 PDT
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.
Comment 15 David Levin 2010-08-17 01:15:47 PDT
Comment on attachment 64525 [details]
patch

None of the changes suggested for DatabaseModifiedInfo were done (in either file).
Comment 16 Dumitru Daniliuc 2010-08-17 12:50:14 PDT
Created attachment 64623 [details]
patch

forgot to clean up the old struct in the last patch... sorry about that.
Comment 17 Darin Adler 2010-08-17 13:45:16 PDT
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
Comment 18 Dumitru Daniliuc 2010-08-17 13:57:17 PDT
(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?
Comment 19 Darin Adler 2010-08-17 13:58:45 PDT
(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.
Comment 20 Dumitru Daniliuc 2010-08-17 14:39:34 PDT
Created attachment 64633 [details]
patch

same patch, removed the destructors. no linker errors on either win or mac.
Comment 21 Darin Adler 2010-08-17 14:52:20 PDT
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.
Comment 22 Dumitru Daniliuc 2010-08-17 17:02:58 PDT
Created attachment 64654 [details]
patch

made the destructors private.
Comment 23 Darin Adler 2010-08-17 17:07:41 PDT
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!
Comment 24 Dumitru Daniliuc 2010-08-17 18:05:36 PDT
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.
Comment 25 David Levin 2010-08-17 18:14:53 PDT
(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.
Comment 26 Dumitru Daniliuc 2010-08-17 22:54:36 PDT
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.
Comment 27 Mark Rowe (bdash) 2010-08-17 23:05:03 PDT
(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;
Comment 28 Dumitru Daniliuc 2010-08-17 23:07:22 PDT
(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?
Comment 29 Mark Rowe (bdash) 2010-08-17 23:10:03 PDT
(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>.
Comment 30 Dumitru Daniliuc 2010-08-17 23:15:30 PDT
(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?
Comment 31 Mark Rowe (bdash) 2010-08-17 23:19:35 PDT
(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.
Comment 32 Dumitru Daniliuc 2010-08-17 23:53:02 PDT
if i remove "delete this" from Release(), i get exactly the same linker error.
Comment 33 David Levin 2010-08-18 00:21:22 PDT
(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).
Comment 34 Mark Rowe (bdash) 2010-08-18 00:28:54 PDT
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.
Comment 35 Eric Seidel (no email) 2010-08-20 15:31:11 PDT
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.
Comment 36 Darin Adler 2010-08-24 13:49:08 PDT
Lets not get hung up on that minor issue. We should land this and then eliminate the destructor later.
Comment 37 David Levin 2010-10-17 23:02:07 PDT
(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.
Comment 38 Dumitru Daniliuc 2010-10-18 14:36:05 PDT
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.
Comment 39 David Levin 2010-10-19 16:25:29 PDT
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.
Comment 40 Dumitru Daniliuc 2010-10-19 16:57:38 PDT
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.
Comment 41 Dumitru Daniliuc 2010-10-20 17:26:15 PDT
patch landed: r70195.
Comment 42 David Levin 2010-10-20 17:42:09 PDT
Did a bug get filed about removing the destructor?
Comment 43 Dumitru Daniliuc 2010-10-20 17:42:59 PDT
(In reply to comment #42)
> Did a bug get filed about removing the destructor?

No, I'll do that.
Comment 44 Dumitru Daniliuc 2010-10-20 17:47:02 PDT
(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.