WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
111805
DatabaseTracker::origins() is missing some databases in a multi-process scenario
https://bugs.webkit.org/show_bug.cgi?id=111805
Summary
DatabaseTracker::origins() is missing some databases in a multi-process scenario
Mark Lam
Reported
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
>.
Attachments
the patch.
(9.54 KB, patch)
2013-03-09 01:26 PST
,
Mark Lam
ggaren
: review-
Details
Formatted Diff
Diff
updated patch with Geoff’s comments addressed.
(8.63 KB, patch)
2013-03-11 15:20 PDT
,
Mark Lam
ggaren
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Geoffrey Garen
Comment 1
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?
Mark Lam
Comment 2
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.
Mark Lam
Comment 3
2013-03-09 01:26:34 PST
Created
attachment 192340
[details]
the patch.
Geoffrey Garen
Comment 4
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.
Mark Lam
Comment 5
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.
Mark Lam
Comment 6
2013-03-11 15:20:25 PDT
Created
attachment 192573
[details]
updated patch with Geoff’s comments addressed.
Geoffrey Garen
Comment 7
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".
Mark Lam
Comment 8
2013-03-11 16:46:55 PDT
Thanks for the review. Landed in
r145431
: <
http://trac.webkit.org/changeset/145431
>.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug