Bug 178274

Summary:

We should wrap the removal of information from the tracker database in a transaction in DatabaseTracker::deleteOrigin()

Product:

WebKit

Reporter:

Maureen Daum <mdaum>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, beidson, bweinstein, commit-queue, mdaum, thorton

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

Unspecified

Attachments:

Description	Flags
patch	none
patch using SQLiteTransaction	thorton: review+, thorton: commit-queue-
patch	none

Maureen Daum

Reported 2017-10-13 11:23:00 PDT

We should wrap the removal of information from the tracker database in a transaction in DatabaseTracker::deleteOrigin(). Otherwise we could end up removing information from just one of the tables if something goes wrong.

Attachments
patch (5.32 KB, patch) 2017-10-13 11:51 PDT, Maureen Daum	no flags	Details Formatted Diff Diff
patch using SQLiteTransaction (3.37 KB, patch) 2017-10-13 14:14 PDT, Maureen Daum	thorton: review+ thorton: commit-queue-	Details Formatted Diff Diff
patch (3.29 KB, patch) 2017-10-16 11:39 PDT, Maureen Daum	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Maureen Daum

Comment 1 2017-10-13 11:28:26 PDT

This is part of <rdar://problem/34576132>

Maureen Daum

Comment 2 2017-10-13 11:51:08 PDT

Created attachment 323719 [details] patch

Brady Eidson

Comment 3 2017-10-13 13:36:20 PDT

Comment on attachment 323719 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=323719&action=review > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:848 > + auto rollbackTransaction = [this]() { > + if (!m_database.executeCommand("ROLLBACK TRANSACTION")) > + RELEASE_LOG_ERROR(DatabaseTracker, "Failed to rollback transaction after trying to remove origin from tracker"); > + }; I think you should be able to use a SQLiteTransaction object here instead of this lambda and manually calling it on exit paths.

Maureen Daum

Comment 4 2017-10-13 14:14:38 PDT

Created attachment 323746 [details] patch using SQLiteTransaction

Maureen Daum

Comment 5 2017-10-13 14:15:30 PDT

Comment on attachment 323719 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=323719&action=review >> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:848 >> + }; > > I think you should be able to use a SQLiteTransaction object here instead of this lambda and manually calling it on exit paths. We don't even have to call rollback() on exit paths because the object destructor does that for us!

Brian Weinstein

Comment 6 2017-10-16 11:15:20 PDT

Comment on attachment 323746 [details] patch using SQLiteTransaction View in context: https://bugs.webkit.org/attachment.cgi?id=323746&action=review > Source/WebCore/ChangeLog:18 > + goes wrong during deletion. Is this part of a different patch?

Maureen Daum

Comment 7 2017-10-16 11:19:06 PDT

Comment on attachment 323746 [details] patch using SQLiteTransaction View in context: https://bugs.webkit.org/attachment.cgi?id=323746&action=review >> Source/WebCore/ChangeLog:18 >> + goes wrong during deletion. > > Is this part of a different patch? OOPS, this is leftover from v1 … I forgot to pull it out of the changelog

Maureen Daum

Comment 8 2017-10-16 11:39:56 PDT

Created attachment 323916 [details] patch

WebKit Commit Bot

Comment 9 2017-10-16 12:21:11 PDT

Comment on attachment 323916 [details] patch Clearing flags on attachment: 323916 Committed r223423: <https://trac.webkit.org/changeset/223423>

WebKit Commit Bot

Comment 10 2017-10-16 12:21:12 PDT

All reviewed patches have been landed. Closing bug.

Alex Christensen

Comment 11 2017-10-16 12:43:19 PDT

This broke the iOS build.