Skip to content

Commit

Permalink
Add HTTP errors and status codes to site breakage reports (#2205)
Browse files Browse the repository at this point in the history
Task/Issue URL:
https://app.asana.com/0/414235014887631/1206604819945148/f
Tech Design URL:
CC:

**Description**:

**Steps to test this PR**:
1. Load https://google.com/invalid
2. Set a breakpoint on Line 240 of
`PrivacyDashboardViewController.swift` and send a breakage report
3. Validate that the report contains httpErrorCodes

<!--
Tagging instructions
If this PR isn't ready to be merged for whatever reason it should be
marked with the `DO NOT MERGE` label (particularly if it's a draft)
If it's pending Product Review/PFR, please add the `Pending Product
Review` label.

If at any point it isn't actively being worked on/ready for
review/otherwise moving forward (besides the above PR/PFR exception)
strongly consider closing it (or not opening it in the first place). If
you decide not to close it, make sure it's labelled to make it clear the
PRs state and comment with more information.
-->

---
###### 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)
  • Loading branch information
SlayterDev authored Feb 23, 2024
1 parent 09ad818 commit b565485
Show file tree
Hide file tree
Showing 16 changed files with 85 additions and 24 deletions.
2 changes: 1 addition & 1 deletion DuckDuckGo.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -13538,7 +13538,7 @@
repositoryURL = "https://github.com/duckduckgo/BrowserServicesKit";
requirement = {
kind = exactVersion;
version = 109.0.2;
version = 110.0.0;
};
};
AA06B6B52672AF8100F541C5 /* XCRemoteSwiftPackageReference "Sparkle" */ = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
"kind" : "remoteSourceControl",
"location" : "https://github.com/duckduckgo/BrowserServicesKit",
"state" : {
"revision" : "da5f8ae73e7ad7fc47931f82f5ac6c4fafa6ac94",
"version" : "109.0.2"
"revision" : "d56b90bd229288f681f0a3a6a325ef25e3ce5f3c",
"version" : "110.0.0"
}
},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,15 @@ extension PrivacyDashboardViewController {
let configuration = ContentBlocking.shared.privacyConfigurationManager.privacyConfig
let protectionsState = configuration.isFeature(.contentBlocking, enabledForDomain: currentTab.content.url?.host)

var errors: [Error]?
var statusCodes: [Int]?
if let error = tabViewModel?.tab.lastWebError {
errors = [error]
}
if let httpStatusCode = tabViewModel?.tab.lastHttpStatusCode {
statusCodes = [httpStatusCode]
}

let websiteBreakage = WebsiteBreakage(siteUrl: currentURL,
category: category.lowercased(),
description: description,
Expand All @@ -288,8 +297,8 @@ extension PrivacyDashboardViewController {
urlParametersRemoved: urlParametersRemoved,
protectionsState: protectionsState,
reportFlow: source,
error: nil,
httpStatusCode: nil)
errors: errors,
httpStatusCodes: statusCodes)
return websiteBreakage
}
}
12 changes: 12 additions & 0 deletions DuckDuckGo/Tab/Model/Tab.swift
Original file line number Diff line number Diff line change
Expand Up @@ -702,6 +702,9 @@ protocol NewWindowPolicyDecisionMaker {
}
let permissions: PermissionModel

@Published private(set) var lastWebError: Error?
@Published private(set) var lastHttpStatusCode: Int?

@Published private(set) var isLoading: Bool = false
@Published private(set) var loadingProgress: Double = 0.0

Expand Down Expand Up @@ -1281,6 +1284,7 @@ extension Tab/*: NavigationResponder*/ { // to be moved to Tab+Navigation.swift
@MainActor
func willStart(_ navigation: Navigation) {
if error != nil { error = nil }
if lastWebError != nil { lastWebError = nil }

delegate?.tabWillStartNavigation(self, isUserInitiated: navigation.navigationAction.isUserInitiated)
}
Expand All @@ -1290,6 +1294,8 @@ extension Tab/*: NavigationResponder*/ { // to be moved to Tab+Navigation.swift
internalUserDecider?.markUserAsInternalIfNeeded(forUrl: webView.url,
response: navigationResponse.response as? HTTPURLResponse)

lastHttpStatusCode = navigationResponse.httpStatusCode

return .next
}

Expand All @@ -1300,6 +1306,7 @@ extension Tab/*: NavigationResponder*/ { // to be moved to Tab+Navigation.swift
userInteractionDialog = nil

// Unnecessary assignment triggers publishing
if lastWebError != nil { lastWebError = nil }
if error != nil,
navigation.navigationAction.navigationType != .alternateHtmlLoad { // error page navigation
error = nil
Expand Down Expand Up @@ -1339,6 +1346,11 @@ extension Tab/*: NavigationResponder*/ { // to be moved to Tab+Navigation.swift
}
}

@MainActor
func didFailProvisionalLoad(with request: URLRequest, in frame: WKFrameInfo, with error: Error) {
lastWebError = error
}

@MainActor
func webContentProcessDidTerminate(with reason: WKProcessTerminationReason?) {
let error = WKError(.webContentProcessTerminated, userInfo: [
Expand Down
2 changes: 1 addition & 1 deletion LocalPackages/DataBrokerProtection/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ let package = Package(
targets: ["DataBrokerProtection"])
],
dependencies: [
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "109.0.2"),
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "110.0.0"),
.package(path: "../PixelKit"),
.package(path: "../SwiftUIExtensions"),
.package(path: "../XPCHelper")
Expand Down
2 changes: 1 addition & 1 deletion LocalPackages/LoginItems/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ let package = Package(
),
],
dependencies: [
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "109.0.2"),
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "110.0.0"),
],
targets: [
.target(
Expand Down
2 changes: 1 addition & 1 deletion LocalPackages/NetworkProtectionMac/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ let package = Package(
.library(name: "NetworkProtectionUI", targets: ["NetworkProtectionUI"])
],
dependencies: [
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "109.0.2"),
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "110.0.0"),
.package(path: "../XPCHelper"),
.package(path: "../SwiftUIExtensions"),
.package(path: "../LoginItems")
Expand Down
2 changes: 1 addition & 1 deletion LocalPackages/PixelKit/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ let package = Package(
)
],
dependencies: [
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "109.0.2"),
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "110.0.0"),
],
targets: [
.target(
Expand Down
2 changes: 1 addition & 1 deletion LocalPackages/SubscriptionUI/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ let package = Package(
targets: ["SubscriptionUI"]),
],
dependencies: [
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "109.0.2"),
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "110.0.0"),
.package(path: "../SwiftUIExtensions")
],
targets: [
Expand Down
2 changes: 1 addition & 1 deletion LocalPackages/SwiftUIExtensions/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ let package = Package(
.library(name: "PreferencesViews", targets: ["PreferencesViews"]),
],
dependencies: [
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "109.0.2"),
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "110.0.0"),
],
targets: [
.target(
Expand Down
2 changes: 1 addition & 1 deletion LocalPackages/SyncUI/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ let package = Package(
],
dependencies: [
.package(path: "../SwiftUIExtensions"),
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "109.0.2"),
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "110.0.0"),
],
targets: [
.target(
Expand Down
2 changes: 1 addition & 1 deletion LocalPackages/SystemExtensionManager/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ let package = Package(
),
],
dependencies: [
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "109.0.2"),
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "110.0.0"),
],
targets: [
// Targets are the basic building blocks of a package, defining a module or a test suite.
Expand Down
2 changes: 1 addition & 1 deletion LocalPackages/XPCHelper/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ let package = Package(
.library(name: "XPCHelper", targets: ["XPCHelper"]),
],
dependencies: [
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "109.0.2"),
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "110.0.0"),
],
targets: [
.target(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,22 @@ import PrivacyDashboard
final class BrokenSiteReportingReferenceTests: XCTestCase {
private let testHelper = PrivacyReferenceTestHelper()

struct MockError: LocalizedError {
let description: String

init(_ description: String) {
self.description = description
}

var errorDescription: String? {
description
}

var localizedDescription: String? {
description
}
}

private enum Resource {
static let tests = "privacy-reference-tests/broken-site-reporting/tests.json"
}
Expand All @@ -53,9 +69,14 @@ final class BrokenSiteReportingReferenceTests: XCTestCase {

os_log("Testing [%s]", type: .info, test.name)

var errors: [Error]?
if let errs = test.errorDescriptions {
errors = errs.map { MockError($0) }
}

let breakage = WebsiteBreakage(siteUrl: test.siteURL,
category: "test",
description: "description",
category: test.category,
description: test.providedDescription,
osVersion: test.os ?? "",
manufacturer: "Apple",
upgradedHttps: test.wasUpgraded,
Expand All @@ -67,8 +88,8 @@ final class BrokenSiteReportingReferenceTests: XCTestCase {
urlParametersRemoved: false,
protectionsState: test.protectionsEnabled,
reportFlow: .appMenu,
error: nil,
httpStatusCode: nil)
errors: errors,
httpStatusCodes: test.httpErrorCodes ?? [])

let request = makeURLRequest(with: breakage.requestParameters)

Expand All @@ -94,7 +115,23 @@ final class BrokenSiteReportingReferenceTests: XCTestCase {

let match = regex.matches(in: absoluteURL, range: NSRange(location: 0, length: absoluteURL.count))

XCTAssertEqual(match.count, 1, "Param [\(param.name)] with value [\(param.value)] not found in [\(absoluteURL)]")
if param.name == "errorDescriptions" {
// `localizedDescription` adds class information to the error. The value is not standardized across platforms
// so we'll just check the result is an array of strings
guard let params = URLComponents(string: absoluteURL)?.queryItems else {
XCTFail("Unable to parse query parameters from \(absoluteURL)")
return
}
var errorsFound = false
for queryItem in params {
if queryItem.name != param.name { continue }
errorsFound = true
XCTAssert((queryItem.value?.split(separator: ",").count ?? 0) > 1, "Error descriptions should return an array of strings. Parsed: \(queryItem.value ?? "")")
}
XCTAssert(errorsFound, "Param [\(param.name)] with value [\(param.value)] not found in [\(absoluteURL)]")
} else {
XCTAssertEqual(match.count, 1, "Param [\(param.name)] with value [\(param.value)] not found in [\(absoluteURL)]")
}
}
}
}
Expand All @@ -121,6 +158,7 @@ private struct Test: Codable {
let siteURL: URL
let wasUpgraded: Bool
let category: String
let providedDescription: String?
let blockedTrackers, surrogates: [String]
let atb, blocklistVersion: String
let expectReportURLPrefix: String
Expand All @@ -129,6 +167,8 @@ private struct Test: Codable {
let manufacturer, model, os: String?
let gpcEnabled: Bool?
let protectionsEnabled: Bool
let errorDescriptions: [String]?
let httpErrorCodes: [Int]?
}

// MARK: - ExpectReportURLParam
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ class WebsiteBreakageReportTests: XCTestCase {
urlParametersRemoved: false,
protectionsState: true,
reportFlow: .appMenu,
error: nil,
httpStatusCode: nil
errors: nil,
httpStatusCodes: nil
)

let urlRequest = makeURLRequest(with: breakage.requestParameters)
Expand Down Expand Up @@ -87,8 +87,8 @@ class WebsiteBreakageReportTests: XCTestCase {
urlParametersRemoved: false,
protectionsState: true,
reportFlow: .appMenu,
error: nil,
httpStatusCode: nil
errors: nil,
httpStatusCodes: nil
)

let urlRequest = makeURLRequest(with: breakage.requestParameters)
Expand Down

0 comments on commit b565485

Please sign in to comment.