Bug 37037 – [Chrome] Clear browsing data dialog never closes if one of the WebSQLDatabase that should be removed is open at the time.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Michael Nordman]

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.

[Michael Nordman]

Created attachment 52446 [details]
fix a bug regression

[Eric U.]

LGTM, FWIW.

[Nate Chapin]

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 @@
> [phone]  Michael Nordman  <[e-mail]>
> +
> +        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

[Michael Nordman]

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

[Michael Nordman]

Created attachment 52453 [details]
fix ChangeLog typos

[Nate Chapin]

Comment on attachment 52453 [details]
fix ChangeLog typos

Fair enough. r=me

[WebKit Commit Bot]

Comment on attachment 52453 [details]
fix ChangeLog typos

Clearing flags on attachment: 52453

Committed r57034: <http://trac.webkit.org/changeset/57034>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57034 might have broken Leopard Intel Release (Tests)

[Michael Nordman]

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

[Adam Barth]

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