Bug 49332

Created attachment 73515 [details] [PATCH] Denies read access to databases in private browsing mode

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

Created attachment 73560 [details] [PATCH] Denies read access to databases in private browsing mode (with ChangeLog) Patch now includes ChangeLog.

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

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.

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.

Created attachment 76132 [details] [PATCH] separate read-only transaction state from private browsing no read-write state (renamed mask variables)

Created attachment 76135 [details] [PATCH] separate read-only transaction state from private browsing no read-write state (fixed ChangeLog conflict)

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 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.

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 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>

All reviewed patches have been landed. Closing bug.