Bug 111805 – DatabaseTracker::origins() is missing some databases in a multi-process scenario

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

[Mark Lam]

Currently, DatabaseTracker::origins() returns a list of database origins based on an in RAM quotaMap.  The quotaMap was loaded from the sql tracker database at one point in time.  In a scenario where a second browser process opens a second database / origin, DatabaseTracker::origins() in the first browser process would not be aware of the new database because its quotaMap was cached in memory before the new database existed.  

Note: the 2nd browser process would have updated the sql tracker database (shared by all browser processes) on disk with details about the new database that it created.  Hence, the fix is simply to have DatabaseTracker::origins() flush its quotaMap cache and re-read the information from the sql tracker database on disk.  This will ensure that DatabaseTracker::origins() returns an accurate list of origins no matter which process it is queried from.

ref: <rdar://problem/13318532>.

[Geoffrey Garen]

> Hence, the fix is simply to have DatabaseTracker::origins() flush its quotaMap cache and re-read the information from the sql tracker database on disk.

At this point, is there any reason to keep a quotaMap in memory? When is it ever valid?

[Mark Lam]

(In reply to comment #1)
> At this point, is there any reason to keep a quotaMap in memory? When is it ever valid?

After analyzing the code some more, I agree that there is no reason to keep quotaMap.  Will upload a patch with quotaMap removed.

[Mark Lam]

Created attachment 192340 [details]
the patch.

[Geoffrey Garen]

Comment on attachment 192340 [details]
the patch.

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

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:260
> +    statement.bindText(1, origin->databaseIdentifier());
> +    int result;
> +    int rows = 0;
> +    while ((result = statement.step()) == SQLResultRow)
> +        rows++;
> +    ASSERT(rows <= 1);
> +    if (result != SQLResultDone)
> +        LOG_ERROR("Failed to read in origins from the database.");

Is it an SQL API requirement that we step through each statement result like this, even though we only care about whether there were any results? This code looks strange.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:426
> +    size_t capacityIncrement = 50;
> +    originsResult.reserveCapacity(capacityIncrement);
>      while ((result = statement.step()) == SQLResultRow) {
>          RefPtr<SecurityOrigin> origin = SecurityOrigin::createFromDatabaseIdentifier(statement.getColumnText(0));
> -        m_quotaMap->set(origin.get()->isolatedCopy(), statement.getColumnInt64(1));
> +        size_t capacity = originsResult.capacity();
> +        if (capacity == originsResult.size())
> +            originsResult.reserveCapacity(capacity + capacityIncrement);
> +
> +        originsResult.append(origin->isolatedCopy());

It looks like you've duplicated the Vector growth algorithm by hand here, and in a way that changes the built-in O(log(n)) algorithm into an O(N^2) algorithm. This is almost certainly wrong. Why can't we just use the built-in algorithm?

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:744
> +    openTrackerDatabase(DontCreateIfDoesNotExist);
> +    if (!m_database.isOpen())
> +        return quota;
> +
> +    SQLiteStatement statement(m_database, "SELECT quota FROM Origins where origin=?;");
> +    if (statement.prepare() != SQLResultOk) {
> +        LOG_ERROR("Failed to prepare statement.");
> +        return quota;
> +    }
> +
> +    statement.bindText(1, origin->databaseIdentifier());

This code should be a helper function, shared with the function above.

[Mark Lam]

Comment on attachment 192340 [details]
the patch.

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

>> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:260
>> +        LOG_ERROR("Failed to read in origins from the database.");
> 
> Is it an SQL API requirement that we step through each statement result like this, even though we only care about whether there were any results? This code looks strange.

The SQLiteStatement destructor will call sqlite3_finalize() on the under sqlite statement.  Hence, there is no need to run the steps to completion.  I will change this code to reflect that.

>> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:426
>> +        originsResult.append(origin->isolatedCopy());
> 
> It looks like you've duplicated the Vector growth algorithm by hand here, and in a way that changes the built-in O(log(n)) algorithm into an O(N^2) algorithm. This is almost certainly wrong. Why can't we just use the built-in algorithm?

I will change the code to use the default append().

>> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:744
>> +    statement.bindText(1, origin->databaseIdentifier());
> 
> This code should be a helper function, shared with the function above.

We talked about this offline.  There’s no clean way to factor this initialization code out into a common function.  So, we’ll leave these as is.

[Mark Lam]

Created attachment 192573 [details]
updated patch with Geoff’s comments addressed.

[Geoffrey Garen]

Comment on attachment 192573 [details]
updated patch with Geoff’s comments addressed.

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

r=me

> Source/WebCore/ChangeLog:8
> +        This changes is needed because using the quotaMap cache can result in

Typo: Should be "change is needed".

[Mark Lam]

Thanks for the review.  Landed in r145431: <http://trac.webkit.org/changeset/145431>.