Missing lock in call to doneCreatingDatabase

[Eric U.]

The last call to doneCreatingDatabase in DatabaseTracker::canEstablishDatabase is called without locking m_databaseGuard.  This will assert in debug builds and use a supposedly-protected data structure without locking in release builds.  This is a bug in my last checkin [56293].

This wasn't caught by layout tests because we don't have one that tries to create a database with a large quota.

[Eric U.]

Created attachment 51382 [details]
Patch

[Dumitru Daniliuc]

LGTM, fwiw.

[Eric Seidel (no email)]

Comment on attachment 51382 [details]
Patch

The test seems missing from this patch.

[Eric U.]

Created attachment 51656 [details]
Patch

[Eric U.]

Thanks Eric; forgot the svn add.

[David Levin]

Comment on attachment 51656 [details]
Patch

> Index: WebCore/ChangeLog
> +        * storage/DatabaseTracker.cpp:  Added missing lock call.

Ideally your comment would be at the function level (which is where you did this change).

> +        (WebCore::DatabaseTracker::canEstablishDatabase):
> +

> Index: WebCore/storage/DatabaseTracker.cpp
> +    MutexLocker lockDatabase(m_databaseGuard);
>      doneCreatingDatabase(origin, name);

It seems nicer to do this:

  MutexLocker lockDatabase(m_databaseGuard);
  if (requirement <= quotaForOriginNoLock(origin))
      return true;
  
  doneCreatingDatabase(origin, name);

Opinion?

[Eric U.]

(In reply to comment #6)
> (From update of attachment 51656 [details])
> > Index: WebCore/ChangeLog
> > +        * storage/DatabaseTracker.cpp:  Added missing lock call.
> 
> Ideally your comment would be at the function level (which is where you did
> this change).
> 
> > +        (WebCore::DatabaseTracker::canEstablishDatabase):
> > +

Fixed.

> > Index: WebCore/storage/DatabaseTracker.cpp
> > +    MutexLocker lockDatabase(m_databaseGuard);
> >      doneCreatingDatabase(origin, name);
> 
> It seems nicer to do this:
> 
>   MutexLocker lockDatabase(m_databaseGuard);
>   if (requirement <= quotaForOriginNoLock(origin))
>       return true;
> 
>   doneCreatingDatabase(origin, name);
> 
> Opinion?

Sure, and I'll do you one better; I can let the previous lock call out of its braces.

[Eric U.]

Created attachment 51682 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 51682 [details]
Patch

Rejecting patch 51682 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', '--build-style=both', '--quiet', '51682', '--no-update']" exit_code: 1
Last 500 characters of output:
ne 60, in _run
    step(tool, options).run(state)
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/commit.py", line 34, in run
    commit_message = self._tool.checkout().commit_message_for_this_commit()
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/checkout/api.py", line 65, in commit_message_for_this_commit
    return CommitMessage("".join(changelog_messages).splitlines())
NameError: global name 'CommitMessage' is not defined

[Eric Seidel (no email)]

Comment on attachment 51682 [details]
Patch

I've restarted the commit-bot.  Sorry for the noise.

[WebKit Commit Bot]

Comment on attachment 51682 [details]
Patch

Clearing flags on attachment: 51682

Committed r56604: <http://trac.webkit.org/changeset/56604>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.