Bug 49332 - Do not allow access to existing HTML5 databases in private browsing mode
Summary: Do not allow access to existing HTML5 databases in private browsing mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-10 11:12 PST by Anton D'Auria
Modified: 2010-12-14 19:02 PST (History)
9 users (show)

See Also:


Attachments
[PATCH] Denies read access to databases in private browsing mode (479 bytes, patch)
2010-11-10 11:14 PST, Anton D'Auria
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Denies read access to databases in private browsing mode (with ChangeLog) (1.01 KB, patch)
2010-11-10 17:29 PST, Anton D'Auria
no flags Details | Formatted Diff | Diff
[PATCH] separate read-only transaction state from private browsing no read-write state (28.94 KB, patch)
2010-12-07 11:18 PST, Anton D'Auria
no flags Details | Formatted Diff | Diff
[PATCH] separate read-only transaction state from private browsing no read-write state (renamed mask variables) (34.34 KB, patch)
2010-12-09 16:17 PST, Anton D'Auria
no flags Details | Formatted Diff | Diff
[PATCH] separate read-only transaction state from private browsing no read-write state (fixed ChangeLog conflict) (33.26 KB, patch)
2010-12-09 16:23 PST, Anton D'Auria
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anton D'Auria 2010-11-10 11:12:53 PST
WebKit shouldn't allow access to existing HTML5 databases when in private browsing mode.
Comment 1 Anton D'Auria 2010-11-10 11:14:28 PST
Created attachment 73515 [details]
[PATCH] Denies read access to databases in private browsing mode
Comment 2 Darin Adler 2010-11-10 17:05:02 PST
Comment on attachment 73515 [details]
[PATCH] Denies read access to databases in private browsing mode

Patch looks good, but it’s missing a change log entry.

review- because of lack of ChangeLog
Comment 3 Anton D'Auria 2010-11-10 17:29:01 PST
Created attachment 73560 [details]
[PATCH] Denies read access to databases in private browsing mode (with ChangeLog)

Patch now includes ChangeLog.
Comment 4 WebKit Commit Bot 2010-11-10 18:04:19 PST
Comment on attachment 73560 [details]
[PATCH] Denies read access to databases in private browsing mode (with ChangeLog)

Rejecting patch 73560 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
...
fast/tokenizer ........................................
fast/transforms .......................
fast/url ....................
fast/workers .............................................
fast/workers/storage ....................
fast/workers/storage/read-and-write-transactions-dont-run-together.html -> failed

Exiting early after 1 failures. 16196 tests run.
274.85s total testing time

16195 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/5610009
Comment 5 Anton D'Auria 2010-12-07 11:18:32 PST
Created attachment 75831 [details]
[PATCH] separate read-only transaction state from private browsing no read-write state

Previously, read-only transactions and private browsing mode were represented by the same SQLStatement and DatabaseAuthorizer states. This patch removes the m_readOnly member variable from SQLStatement and DatabaseAuthorizer, and replaces it with m_permissions whose bit fields are initialized by a DatabaseAuthorizer enum Permissions (ReadWrite, ReadOnly, NoAccess). A read-only transaction sets permissions to ReadOnly, and if !m_database->scriptExecutionContext()->allowDatabaseAccess(), then permissions also set to NoAccess.
Comment 6 WebKit Review Bot 2010-12-07 21:44:19 PST
Attachment 75831 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Anton D'Auria 2010-12-09 16:17:09 PST
Created attachment 76132 [details]
[PATCH] separate read-only transaction state from private browsing no read-write state (renamed mask variables)
Comment 8 Anton D'Auria 2010-12-09 16:23:25 PST
Created attachment 76135 [details]
[PATCH] separate read-only transaction state from private browsing no read-write state (fixed ChangeLog conflict)
Comment 9 Eric Seidel (no email) 2010-12-14 01:24:28 PST
Comment on attachment 73560 [details]
[PATCH] Denies read access to databases in private browsing mode (with ChangeLog)

Cleared Darin Adler's review+ from obsolete attachment 73560 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 10 Darin Adler 2010-12-14 18:04:26 PST
Comment on attachment 76135 [details]
[PATCH] separate read-only transaction state from private browsing no read-write state (fixed ChangeLog conflict)

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

r=me, but the enum/bit combo is a bit ugly, the bitifield use here is sloppy and there is some incorrectly formatted code

> WebCore/storage/DatabaseAuthorizer.h:97
> +    void setPermissions(int permissions);

It doesn’t seem right to use int for permissions, but use an enum for the values. I think we normally have a better idiom for this sort of thing.

> WebCore/storage/DatabaseAuthorizer.h:114
> +    int m_permissions;
>      bool m_securityEnabled : 1;

All these bools are set to be a single bit to save memory, I suppose, and then the three bit permissions is taking up an entire 32-bit int! I think the use of bitfields on the bools is excessive. You could use a bitfield for permissions, but because we are using a signed type, int, we’d need to use four bits. It would be better to use unsigned and three bits. Or not use bitfields at all in this class.

> WebCore/storage/SQLTransactionSync.cpp:94
> +    if (!m_database->scriptExecutionContext()->allowDatabaseAccess())
> +      permissions |= DatabaseAuthorizer::NoAccessMask;
> +    else if (m_readOnly)
> +      permissions |= DatabaseAuthorizer::ReadOnlyMask;

This is not indented correctly.
Comment 11 WebKit Commit Bot 2010-12-14 18:59:45 PST
The commit-queue encountered the following flaky tests while processing attachment 76135 [details]:

inspector/styles-new-API.html bug 50889 (authors: apavlov@chromium.org and pfeldman@chromium.org)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2010-12-14 19:02:14 PST
Comment on attachment 76135 [details]
[PATCH] separate read-only transaction state from private browsing no read-write state (fixed ChangeLog conflict)

Clearing flags on attachment: 76135

Committed r74093: <http://trac.webkit.org/changeset/74093>
Comment 13 WebKit Commit Bot 2010-12-14 19:02:21 PST
All reviewed patches have been landed.  Closing bug.