Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Observations database clearing improvements #495

Merged
merged 3 commits into from
Apr 1, 2022

Conversation

samsymons
Copy link
Collaborator

Task/Issue URL: https://app.asana.com/0/0/1201945748431037/f
Tech Design URL:
CC: @ladamski

Description:

This PR updates the logic used to clear Apple's observations.db. There are three changes:

  1. Before clearing, a checkpoint call is made to ensure that the data in the wal file is moved into the main database. The observations database uses Write-Ahead Logging, hence the need for this call.
  2. After clearing, a VACUUM call is made to tell the database to clean up after itself.
  3. Despite these first two points, sometimes domains would still be present when running strings over the database, so the delete call is run more time - it's unclear to me why, but this final step triggers SQLite to fully clean up after itself in my testing.

For further context: this database is used by Intelligent Tracking Protection, and stores a list of domains used by that feature. We have no official way to clear or disable it, hence the direct SQL usage.

Steps to test this PR:

  1. Open ~/Library/WebKit/com.duckduckgo.macos.browser.debug/WebsiteData/ResourceLoadStatistics in your terminal
  2. Run strings observations.db-wal observations.db | grep ".com", which will show you a list of domains that you've visited - gross!
  3. Run the app from this branch and then use the Fire button2.
  4. Run strings observations.db-wal observations.db | grep ".com again, you should get no results back - if you do, then this PR should not be approved and further investigation is needed
  5. Check the size of the files in the directory with ls -la - it's expected that the observations.db-wal file is totally empty. This isn't a hard requirement of the PR (we only care if any data is left over), but is expected due to the checkpoint call
  6. Lastly, do some browsing in the app and then run strings observations.db-wal observations.db | grep ".com" one last time, it should once again show you some of the domains you've visited. This is unfortunately expected, and unavoidable, they'll get cleared again the next time you use the Fire button.

Testing checklist:

  • Test with Release configuration
  • Test proper deallocation of tabs
  • Make sure committed submodule changes are desired

Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍 💯 I did not test anything in this PR but Lucas did! 😆

@samsymons samsymons merged commit 59114ac into develop Apr 1, 2022
@samsymons samsymons deleted the sam/clean-observations-db branch April 1, 2022 16:03
samsymons added a commit that referenced this pull request Apr 4, 2022
# By Tomas Strba (5) and others
# Via GitHub
* develop:
  Refresh the Autofill panel after importing (#496)
  Require authentication for login export (#494)
  Observations database clearing improvements (#495)
  Fix handling external schemes in address bar (#493)
  New autoconsent rules and move tab cleanup out of onCommitted (#489)
  Safari CSV Passwords import +LastPass, 1Password (#483)
  Split preferences into multiple screens and add an option to show full URL (#487)
  Color of hover label fixed (#497)
  Add AMP links protection (#488)
  Enhancements of fire button options (#484)
  Rename Logins+ to Autofill (#492)
  new tab page low hanging fruit (#491)
  Fix threading issues in Autoconsent (#478)
  Navigation bar improvement: CMD + back or CMD + forward opens a new tab (#486)
  TextField refactored to TextView in order to make it scrollable (#481)
  More Xcode cleaning (#485)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants