Bug 178274 - We should wrap the removal of information from the tracker database in a transaction in DatabaseTracker::deleteOrigin()
Summary: We should wrap the removal of information from the tracker database in a tran...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: All Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-13 11:23 PDT by Maureen Daum
Modified: 2017-10-16 12:46 PDT (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Maureen Daum 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.
Comment 1 Maureen Daum 2017-10-13 11:28:26 PDT
This is part of <rdar://problem/34576132>
Comment 2 Maureen Daum 2017-10-13 11:51:08 PDT
Created attachment 323719 [details]
patch
Comment 3 Brady Eidson 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.
Comment 4 Maureen Daum 2017-10-13 14:14:38 PDT
Created attachment 323746 [details]
patch using SQLiteTransaction
Comment 5 Maureen Daum 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!
Comment 6 Brian Weinstein 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?
Comment 7 Maureen Daum 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
Comment 8 Maureen Daum 2017-10-16 11:39:56 PDT
Created attachment 323916 [details]
patch
Comment 9 WebKit Commit Bot 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>
Comment 10 WebKit Commit Bot 2017-10-16 12:21:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Alex Christensen 2017-10-16 12:43:19 PDT
This broke the iOS build.
Comment 12 Alex Christensen 2017-10-16 12:44:05 PDT
Or something related to this:
https://webkit-queues.webkit.org/results/4873526
Comment 13 Alex Christensen 2017-10-16 12:46:54 PDT
JK, https://trac.webkit.org/changeset/223422/webkit broke the build.