Rework DBP XPC interface #2758

THISISDINOSAUR · 2024-05-08T17:54:45Z

Task/Issue URL: https://app.asana.com/0/0/1207231867963535/f
Tech Design URL: https://app.asana.com/0/0/1207198317317333/f
CC:

Description:

Moves the existing XPC interface ("DataBrokerProtectionScheduler") from the scheduler into the BackgroundAgentManager
Renames said manager to AgentManager
Renames the XPC interface to DataBrokerProtectionAgentInterface
Completely changes the composition of the interface, separating it out into two different protocols: DataBrokerProtectionAgentAppEvents, DataBrokerProtectionAgentDebugCommands
As those protocols imply, changes the XPC interface methods to be events based, to move decision making from the main app to the agent. Except for the debug commands, which are now clearly separated as for debugging purposes only.
Removes two of the many XPC layers since they were operating solely as a passthrough (i.e. boilerplate for nothing)
Uses protocol composition to further cut down on the repetition in those layers
Changes LoginItemScheduler to rather than be a class, a protocol that inherits the AgentInterface
Removes existing completions from the XPC interface since they were unused
Replaces them with new completions that are solely for reporting the reliability of XPC

It still needs the pixels for #9, including clean up of the old pixels

Steps to test this PR:

Test DBP still works, particularly removing the login item, entirely and starting fresh (since when testing at one point I managed to break that)
Otherwise don't test too much since this PR is not intended to stand alone/be merged into main as is.

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

github-actions · 2024-05-08T17:56:41Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 968ba2a

THISISDINOSAUR · 2024-05-08T18:13:24Z

DuckDuckGo/DBP/DataBrokerProtectionLoginItemInterface.swift

@@ -0,0 +1,100 @@
+//
+//  DataBrokerProtectionLoginItemInterface.swift


Despite what GitHub thinks, this class isn't really new, just renamed (although it has changed a bit to give it an explicit protocol)

THISISDINOSAUR · 2024-05-08T18:14:08Z

DuckDuckGo/DBP/DataBrokerProtectionLoginItemInterface.swift

+    func profileSaved() {
+        enableLoginItem()
+        ipcClient.profileSaved { error in
+            // TODO


Need to implement pixels here. It's my next item on my todo list, but could take a while so I don't think the PR should wait for it since it's not merging to main anyway

THISISDINOSAUR · 2024-05-08T18:15:03Z

DuckDuckGoDBPBackgroundAgent/DataBrokerProtectionAgentManager.swift

    }

-    public func runOperationsAndStartSchedulerIfPossible() {
+    public func agentFinishedLaunching() {


also changed this to be more event based, so it's all the manager making the decisions

THISISDINOSAUR · 2024-05-08T18:15:45Z

DuckDuckGoDBPBackgroundAgent/DataBrokerProtectionAgentManager.swift

-                return
-            }
-
+        scheduler.runQueuedOperations(showWebView: false) { [weak self] _ in
            self?.pixelHandler.fire(.backgroundAgentRunOperationsAndStartSchedulerIfPossibleRunQueuedOperationsCallbackStartScheduler)
            self?.scheduler.startScheduler()


There's some more work to do in this completion, but it should all change with the final PR so pointless doing it here

THISISDINOSAUR · 2024-05-08T18:18:06Z

...BrokerProtection/Sources/DataBrokerProtection/Database/DataBrokerProtectionDataManager.swift

+        // This is now unusused as we decided the web UI shouldn't issue commands directly
+        // The background agent itself instead decides to start scans based on events
+        // This should be removed once we can remove it from the web side
+        return true


Worth noting this always returned true anyway unless the scan delegate was nil

THISISDINOSAUR · 2024-05-08T18:18:33Z

...taBrokerProtection/Sources/DataBrokerProtection/IPC/DataBrokerProtectionAgentInterface.swift

+}
+
+@objc
+public class DataBrokerProtectionAgentErrorCollection: NSObject, NSSecureCoding {


This bits not new, but newly in its own file and renamed

THISISDINOSAUR · 2024-05-08T18:19:44Z

...es/DataBrokerProtection/Sources/DataBrokerProtection/IPC/DataBrokerProtectionIPCClient.swift

        })
    }
+
+    public func getDebugMetadata() async -> DBPBackgroundAgentMetadata? {
+        await withCheckedContinuation { continuation in


I thought this might be a bad idea across the XPC boundary, but netP is also doing the same thing

THISISDINOSAUR · 2024-05-08T18:20:31Z

...es/DataBrokerProtection/Sources/DataBrokerProtection/IPC/DataBrokerProtectionIPCServer.swift

@@ -75,40 +75,18 @@ public final class DBPBackgroundAgentMetadata: NSObject, NSSecureCoding {

 /// This protocol describes the server-side IPC interface for controlling the tunnel
 ///
-public protocol IPCServerInterface: AnyObject {
+public protocol IPCServerInterface: AnyObject, DataBrokerProtectionAgentDebugCommands {


I'm slightly on the fence about this protocol conformance versus duplicating it again, I don't want to make it more confusing

I think the conformance is fine 👍🏼

THISISDINOSAUR · 2024-05-08T18:21:13Z

...es/DataBrokerProtection/Sources/DataBrokerProtection/IPC/DataBrokerProtectionIPCServer.swift

-    func getDebugMetadata(completion: @escaping (DBPBackgroundAgentMetadata?) -> Void)
+    func profileSaved(xpcMessageReceivedCompletion: @escaping (Error?) -> Void)
+    func dataDeleted(xpcMessageReceivedCompletion: @escaping (Error?) -> Void)
+    func appLaunched(xpcMessageReceivedCompletion: @escaping (Error?) -> Void)


These are declared again rather than conforming to the AppEvents protocol to have the xpcMessageReceivedCompletion, and to avoid having it in the AppEvents protocol.

THISISDINOSAUR · 2024-05-08T18:22:11Z

...es/DataBrokerProtection/Sources/DataBrokerProtection/IPC/DataBrokerProtectionIPCServer.swift


    /// Opens a browser window with the specified domain
    ///
    func openBrowser(domain: String)

+    func startManualScan(showWebView: Bool)
+    func runQueuedOperations(showWebView: Bool)
+    func runAllOptOuts(showWebView: Bool)
    func getDebugMetadata(completion: @escaping (DBPBackgroundAgentMetadata?) -> Void)


These are declared again because this version of getDebugMetadata has to use a completion to go inside xpc.execute, but the higher level interfaces hide this and use a more modern async

THISISDINOSAUR · 2024-05-08T18:22:43Z

...es/DataBrokerProtection/Sources/DataBrokerProtection/IPC/DataBrokerProtectionIPCServer.swift

    func register() {
-        serverDelegate?.register()


The register method this was calling was empty

THISISDINOSAUR · 2024-05-08T18:25:00Z

DuckDuckGoDBPBackgroundAgent/DataBrokerProtectionAgentManager.swift

+
+extension DataBrokerProtectionAgentManager: DataBrokerProtectionAgentAppEvents {
+
+    public func profileSaved() {


I think it makes sense to unit test that when certain events happen the correct scheduler method is called, but given the next PR is gonna change all that, I don't think it's worth doing until the next PR

aataraxiaa

Overall LGTM @THISISDINOSAUR, nice work! Validated that background agent is started on fresh launch. Left a few comments, main one being that I think we should add tests for the DataBrokerProtectionAgentManager.

aataraxiaa · 2024-05-09T08:40:49Z

...taBrokerProtection/Sources/DataBrokerProtection/IPC/DataBrokerProtectionAgentInterface.swift

+    func getDebugMetadata() async -> DBPBackgroundAgentMetadata?
+}
+
+public protocol DataBrokerProtectionAgentInterface: AnyObject, DataBrokerProtectionAgentAppEvents, DataBrokerProtectionAgentDebugCommands {


Based on my understanding of intention, I find the name DataBrokerProtectionAgentInterface a bit misleading/confusing. On one level we refer to the ‘background agent’ as the ‘agent’. However, this interface is used on both sides of the app -> agent communication. Do we think then maybe something like DataBrokerProtectionClientServerInterface….or DataBrokerProtectionAppAgentInterface…?

I think that's worse since it's strictly one directional (completions or return types not withstanding). It could be DataBrokerProtectionAppToAgentInterface?

The more I think about it, I think I do prefer that, since it is specific to the app (what with the appLaunched event and that)

aataraxiaa · 2024-05-09T08:42:52Z

...taBrokerProtection/Sources/DataBrokerProtection/IPC/DataBrokerProtectionAgentInterface.swift

+    public static var supportsSecureCoding: Bool {
+        return true
+    }


Minor, and not new code maybe, but while we are here this could be:

public static let supportsSecureCoding = true

Just to remove a line or two.

aataraxiaa · 2024-05-09T08:46:33Z

...taBrokerProtection/Sources/DataBrokerProtection/IPC/DataBrokerProtectionAgentInterface.swift

+public protocol DataBrokerProtectionAgentDebugCommands {
+    func openBrowser(domain: String)
+    func startManualScan(showWebView: Bool)
+    func runQueuedOperations(showWebView: Bool)
+    func runAllOptOuts(showWebView: Bool)
+    func getDebugMetadata() async -> DBPBackgroundAgentMetadata?
+}


As mentioned on MM, I was also adding something similar based on previous suggestions, and the approach I came up with using enum cases as debug actions, as follows:

enum DataBrokerDebugCommand { case startOptOutOperations(showWebView: Bool, operationDependencies: DataBrokerOperationDependencies, completion: ((DataBrokerProtectionSchedulerErrorCollection?) -> Void)?) } protocol DataBrokerDebugCommandExecutor { func execute(_ command: DataBrokerDebugCommand) }

Any desire to go the enum route? The API it provides is succinct. Just my preference, not a strong opinion.

It's not possible because it to be able to cross the XPC boundary which means it needs to be representable in Objective-C, i.e. can't have associated values. It's what I originally tried but it didn't work
It might be possible to have the higher level interface be an enum and then have a translation layer in the middle, but I don't think it would be worth the complexity

Ah yeah, or course. Apologies, you already highlighted this. I was trying for something similar, but only on the agent side, and so the boundary issue slipped my mind. Agree, translation layer etc. not worth the complexity.

aataraxiaa · 2024-05-09T08:48:46Z

...es/DataBrokerProtection/Sources/DataBrokerProtection/IPC/DataBrokerProtectionIPCClient.swift

@@ -24,7 +24,6 @@ import XPCHelper
 /// This protocol describes the server-side IPC interface for controlling the tunnel
 ///
 public protocol IPCClientInterface: AnyObject {
-    func schedulerStatusChanges(_ status: DataBrokerProtectionSchedulerStatus)


Assuming these empty interfaces will be removed as some point?

I think we still need to give the xpc server something. At the very least our own generic XPCClient requires it

Regardless of if we need it or not, I think I prefer explicitly declaring that the interface in the other direction is empty.

aataraxiaa · 2024-05-09T08:52:17Z

...es/DataBrokerProtection/Sources/DataBrokerProtection/IPC/DataBrokerProtectionIPCServer.swift

@@ -75,40 +75,18 @@ public final class DBPBackgroundAgentMetadata: NSObject, NSSecureCoding {

 /// This protocol describes the server-side IPC interface for controlling the tunnel
 ///
-public protocol IPCServerInterface: AnyObject {
+public protocol IPCServerInterface: AnyObject, DataBrokerProtectionAgentDebugCommands {


I think the conformance is fine 👍🏼

aataraxiaa · 2024-05-09T09:02:22Z

DuckDuckGoDBPBackgroundAgent/DataBrokerProtectionAgentManager.swift

@@ -22,24 +22,29 @@ import BrowserServicesKit
 import DataBrokerProtection
 import PixelKit

-public final class DataBrokerProtectionBackgroundManager {
+public final class DataBrokerProtectionAgentManager {


I think we should take the time now to make this type testable. It’s currently a singleton with lazy vars, which is not ideal for dependency injection. Does it need to be a singleton? I see it is only referenced once in DuckDuckGoDBPBackgroundAgentAppDelegate. Can we remove the singleton behavior, make the init public, and then inject what is needed to be able to mock dependencies? I think we should be testing that the DataBrokerProtectionAgentAppEvents perform the right actions.

Somehow I missed your comment above re: testing DataBrokerProtectionAgentAppEvents, and so if we are gonna test in next PR that’s fine with me.

Yeah agreed on testing the app events in that way (#2758 (comment)). I'm indifferent to if we do that particular groundwork in this PR or the next, although think writing the actual tests should wait for the next one since the expected behaviour will change significantly

Sure thing. Indifferent on order of work also. Whatever is most pragmatic to you.

jotaemepereira

Looks great 👍🏼

Task/Issue URL: https://app.asana.com/0/1199230911884351/1207338264132623/f Tech Design URL: https://app.asana.com/0/481882893211075/1207174884557414/f CC: **Description**: This PR has the effect of mergeing four different PRs into main: #2777 #2758 #2757 #2743 This covers significant changes to the XPC interface and how the main app uses it, the background manager, the scheduler, and the processor. See individual PRs for details. There's no code changes here that haven't been reviewed separately as part of those PRs, so it's up to you how you want to review it code wise. The more important step at this stage is manual testing. **Steps to test this PR**: 1. Because this is a fairly significant set of changes, we need to be as thorough as we can in testing DBP generally works and really try to break it. Because of this, I'm keen that anyone testing things of their own ways to break it before trying the steps below. 2. Edit/add profile data, and before scans finish, edit it again. Scans should start afresh, and you should see an interrupted pixel fire. Check that you don't get a scans completed notification 3. Edit/add profile data, and before scans finish, close the app, and reopen it. The same set of manual scans should still continue, and a blocked pixel should fire. 4. With profile data already existing but initial scans having finished, close the app and reopen it. Scheduled scans should run. 5. With profile data already existing, and with the app closed, launch the background agent, scheduled scans should run 6. Edit/add profile data, put the laptop to sleep/lock it/restart it, check that scans continue as expected. 7. Check that the background agent activity lines up with the UI, I.e. the progress bar and when we sent the scans completed notification is accurate  --- ###### Internal references: [Pull Request Review Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f) [Software Engineering Expectations](https://app.asana.com/0/59792373528535/199064865822552) [Technical Design Template](https://app.asana.com/0/59792373528535/184709971311943) [Pull Request Documentation](https://app.asana.com/0/1202500774821704/1204012835277482/f) --------- Co-authored-by: Pete Smith <[email protected]>

Make broad sweeping changes to DBP XPC interface

9bd359b

THISISDINOSAUR commented May 8, 2024

View reviewed changes

THISISDINOSAUR requested a review from aataraxiaa May 8, 2024 18:25

Fix typos in comments

968ba2a

aataraxiaa approved these changes May 9, 2024

View reviewed changes

jotaemepereira self-requested a review May 9, 2024 11:48

jotaemepereira approved these changes May 9, 2024

View reviewed changes

THISISDINOSAUR merged commit 74e390d into feature/pir-stabilization May 9, 2024
22 of 23 checks passed

THISISDINOSAUR deleted the elle/dbp-xpc-changes branch May 9, 2024 20:44

THISISDINOSAUR mentioned this pull request May 16, 2024

Merge pir stabilization feature branch into main #2789

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework DBP XPC interface #2758

Rework DBP XPC interface #2758

THISISDINOSAUR commented May 8, 2024 •

edited

Loading

github-actions bot commented May 8, 2024 •

edited

Loading

THISISDINOSAUR May 8, 2024

THISISDINOSAUR May 8, 2024

THISISDINOSAUR May 8, 2024

THISISDINOSAUR May 8, 2024

THISISDINOSAUR May 8, 2024

THISISDINOSAUR May 8, 2024

THISISDINOSAUR May 8, 2024

THISISDINOSAUR May 8, 2024

aataraxiaa May 9, 2024

THISISDINOSAUR May 8, 2024

THISISDINOSAUR May 8, 2024

THISISDINOSAUR May 8, 2024

THISISDINOSAUR May 8, 2024

aataraxiaa left a comment

aataraxiaa May 9, 2024 •

edited

Loading

THISISDINOSAUR May 9, 2024

THISISDINOSAUR May 9, 2024

aataraxiaa May 9, 2024

aataraxiaa May 9, 2024

THISISDINOSAUR May 9, 2024

aataraxiaa May 9, 2024

aataraxiaa May 9, 2024

THISISDINOSAUR May 9, 2024

THISISDINOSAUR May 9, 2024

aataraxiaa May 9, 2024

aataraxiaa May 9, 2024

aataraxiaa May 9, 2024

THISISDINOSAUR May 9, 2024

aataraxiaa May 9, 2024

jotaemepereira left a comment

		@@ -0,0 +1,100 @@
		//
		// DataBrokerProtectionLoginItemInterface.swift


		extension DataBrokerProtectionAgentManager: DataBrokerProtectionAgentAppEvents {

		public func profileSaved() {

Rework DBP XPC interface #2758

Rework DBP XPC interface #2758

Conversation

THISISDINOSAUR commented May 8, 2024 • edited Loading

Internal references:

github-actions bot commented May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aataraxiaa left a comment

Choose a reason for hiding this comment

aataraxiaa May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jotaemepereira left a comment

Choose a reason for hiding this comment

THISISDINOSAUR commented May 8, 2024 •

edited

Loading

github-actions bot commented May 8, 2024 •

edited

Loading

aataraxiaa May 9, 2024 •

edited

Loading