Bug 111805

Summary: DatabaseTracker::origins() is missing some databases in a multi-process scenario
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: WebCore Misc.Assignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, ggaren, sam
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch.
ggaren: review-
updated patch with Geoff’s comments addressed. ggaren: review+

Description Mark Lam 2013-03-07 18:31:41 PST
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>.
Comment 1 Geoffrey Garen 2013-03-07 18:35:40 PST
> 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?
Comment 2 Mark Lam 2013-03-09 01:25:39 PST
(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.
Comment 3 Mark Lam 2013-03-09 01:26:34 PST
Created attachment 192340 [details]
the patch.
Comment 4 Geoffrey Garen 2013-03-11 13:06:34 PDT
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.
Comment 5 Mark Lam 2013-03-11 14:44:55 PDT
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.
Comment 6 Mark Lam 2013-03-11 15:20:25 PDT
Created attachment 192573 [details]
updated patch with Geoff’s comments addressed.
Comment 7 Geoffrey Garen 2013-03-11 16:39:33 PDT
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".
Comment 8 Mark Lam 2013-03-11 16:46:55 PDT
Thanks for the review.  Landed in r145431: <http://trac.webkit.org/changeset/145431>.