Bug 37037 - [Chrome] Clear browsing data dialog never closes if one of the WebSQLDatabase that should be removed is open at the time.
Summary: [Chrome] Clear browsing data dialog never closes if one of the WebSQLDatabase...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Michael Nordman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-02 12:02 PDT by Michael Nordman
Modified: 2010-04-02 20:57 PDT (History)
5 users (show)

See Also:


Attachments
fix a bug regression (5.27 KB, patch)
2010-04-02 13:33 PDT, Michael Nordman
japhet: review-
Details | Formatted Diff | Diff
fix ChangeLog typos (5.26 KB, patch)
2010-04-02 14:56 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Nordman 2010-04-02 12:02:26 PDT
See sister bug... http://code.google.com/p/chromium/issues/detail?id=40245

Not long ago in r56293, the behavior of DatabaseCloseTask was altered. One callsite was updated accordingly, but another callsite was not. The second callsite is busted due to the behavior change.
Comment 1 Michael Nordman 2010-04-02 13:33:15 PDT
Created attachment 52446 [details]
fix a bug regression
Comment 2 Eric U. 2010-04-02 14:36:24 PDT
LGTM, FWIW.
Comment 3 Nate Chapin 2010-04-02 14:40:32 PDT
Comment on attachment 52446 [details]
fix a bug regression

A rather pedantic r- since we'll need to update the ChangeLog before setting commit-queue+ anyway.

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 57019)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,21 @@
> +2010-04-02  Michael Nordman  <michaeln@google.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Set the close policy used the by the DatabaseCloseTask in a constructor argument

Typo here and in the other ChangeLog.

> @@ -110,18 +110,20 @@ private:
>  
>  class DatabaseCloseTask : public DatabaseTask {
>  public:
> -    static PassOwnPtr<DatabaseCloseTask> create(Database* db, DatabaseTaskSynchronizer* synchronizer)
> +    static PassOwnPtr<DatabaseCloseTask> create(Database* db, Database::ClosePolicy closePolicy, DatabaseTaskSynchronizer* synchronizer)
>      { 
> -        return new DatabaseCloseTask(db, synchronizer);
> +        return new DatabaseCloseTask(db, closePolicy, synchronizer);
>      }
>  
>  private:
> -    DatabaseCloseTask(Database*, DatabaseTaskSynchronizer*);
> +    DatabaseCloseTask(Database*, Database::ClosePolicy, DatabaseTaskSynchronizer*);
>  
>      virtual void doPerformTask();
>  #ifndef NDEBUG
>      virtual const char* debugTaskName() const;
>  #endif
> +
> +    Database::ClosePolicy m_closePolicy;
>  };

Perhaps we should support closePolicy as an optional parameter on the constructor/create function? I assume
in the vast majority of cases we will want DoNotRemoveFromDatabaseContext. I'm willing to be convinced otherwise
Comment 4 Michael Nordman 2010-04-02 14:53:26 PDT
> Perhaps we should support closePolicy as an optional parameter on the
> constructor/create function? I assume
> in the vast majority of cases we will want DoNotRemoveFromDatabaseContext. I'm
> willing to be convinced otherwise

I'd rather not do that.

If there are any other clients of this task out there that aren't in webkit's svn repository, i'd like them to have to figure out what value is appropiate for their callsite. For example, if this explicit approach had been done originally, chrome wouldn't have been built with this subtle bug in it.
Comment 5 Michael Nordman 2010-04-02 14:56:14 PDT
Created attachment 52453 [details]
fix ChangeLog typos
Comment 6 Nate Chapin 2010-04-02 15:02:05 PDT
Comment on attachment 52453 [details]
fix ChangeLog typos

Fair enough. r=me
Comment 7 WebKit Commit Bot 2010-04-02 17:07:03 PDT
Comment on attachment 52453 [details]
fix ChangeLog typos

Clearing flags on attachment: 52453

Committed r57034: <http://trac.webkit.org/changeset/57034>
Comment 8 WebKit Commit Bot 2010-04-02 17:07:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2010-04-02 17:46:27 PDT
http://trac.webkit.org/changeset/57034 might have broken Leopard Intel Release (Tests)
Comment 10 Michael Nordman 2010-04-02 18:01:23 PDT
(In reply to comment #9)
> http://trac.webkit.org/changeset/57034 might have broken Leopard Intel Release
> (Tests)

fyi: this change should be a noop for all but chromium, before and after the patch, WebCore::Database::markAsDeletedAndClose() is using the DoNotRemoveDatabaseFromContext policy.
Comment 11 Adam Barth 2010-04-02 20:57:17 PDT
> fyi: this change should be a noop for all but chromium, before and after the
> patch, WebCore::Database::markAsDeletedAndClose() is using the
> DoNotRemoveDatabaseFromContext policy.

My apologies, but I think you were bit by https://bugs.webkit.org/show_bug.cgi?id=36646 occurring twice in a row.