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

Fix failed downloads (file coordination) #2467

Merged
merged 27 commits into from
Apr 2, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
e0fdf98
add sandbox test tool
mallexxx Mar 14, 2024
f4f2caa
downloads file presenters
mallexxx Mar 21, 2024
1cd3048
Merge branch 'main' into alex/fix-failed-downloads
mallexxx Mar 21, 2024
9df235d
commit missing file contents
mallexxx Mar 21, 2024
e041179
Merge remote-tracking branch 'origin/alex/fix-failed-downloads' into …
mallexxx Mar 21, 2024
ada3b50
fix resuming issues, fixing some tests
mallexxx Mar 21, 2024
45155e3
fix release build, some fixes
mallexxx Mar 22, 2024
0947e9e
open downloads popup when opening a "duckload" file
mallexxx Mar 22, 2024
ec922ca
fix PR comments, corner cases and tests
mallexxx Mar 22, 2024
09e1d7a
validate destination is accessible before firing the download
mallexxx Mar 22, 2024
c175e61
replace breakByRaisingSigInt with raise(SIGINT) and log message
mallexxx Mar 22, 2024
378a48e
fix non-sandbox file presenter tests
mallexxx Mar 22, 2024
b1d69f0
disable sandbox tool logger
mallexxx Mar 22, 2024
f778d98
fix linter issues
mallexxx Mar 22, 2024
38d9321
Merge remote-tracking branch 'origin/main' into alex/fix-failed-downl…
mallexxx Apr 1, 2024
cbeeff2
fix integration tests
mallexxx Apr 1, 2024
915f0ec
fix UserDialogRequest leak
mallexxx Apr 1, 2024
556574a
extend FilePresenterTests timeout
mallexxx Apr 1, 2024
2d94fe7
add app group entitlement
mallexxx Apr 1, 2024
e8bc8dd
fix changing file letter case causing file removal detection
mallexxx Apr 1, 2024
1b78e1a
disable FilePresenterTests on CI
mallexxx Apr 1, 2024
8866fdf
fix app hanging when cancelling downloads on Quit in RELEASE
mallexxx Apr 1, 2024
a6d291b
extend fileReadPromise timeout
mallexxx Apr 1, 2024
f7ffaec
fix flaky test
mallexxx Apr 1, 2024
0d59476
fix warnings
mallexxx Apr 2, 2024
6ac1784
cleanup
mallexxx Apr 2, 2024
b357e49
fix linter warning
mallexxx Apr 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions Configuration/Tests/SandboxTestTool.xcconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@

// Copyright © 2024 DuckDuckGo. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

#include "../Common.xcconfig"
#include "../App/AppTargetsBase.xcconfig"
#include "../AppStore.xcconfig"

PRODUCT_BUNDLE_IDENTIFIER = com.duckduckgo.sandbox-test-tool

CODE_SIGN_ENTITLEMENTS = sandbox-test-tool/sandbox_test_tool.entitlements

CODE_SIGN_IDENTITY[sdk=macosx*] = 3rd Party Mac Developer Application
CODE_SIGN_IDENTITY[config=Debug][sdk=macosx*] = Apple Development
CODE_SIGN_IDENTITY[config=CI][sdk=macosx*] =

PROVISIONING_PROFILE_SPECIFIER[config=Debug][sdk=macosx*] =

ENABLE_APP_SANDBOX = YES
PRODUCT_NAME = $(TARGET_NAME);

INFOPLIST_FILE = sandbox-test-tool/Info.plist
INFOPLIST_KEY_NSPrincipalClass = SandboxTestToolApp

SWIFT_OPTIMIZATION_LEVEL[config=*][arch=*][sdk=*] = -Onone
FEATURE_FLAGS[arch=*][sdk=*] = SANDBOX_TEST_TOOL
210 changes: 208 additions & 2 deletions DuckDuckGo.xcodeproj/project.pbxproj

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
ActionType = "Xcode.IDEStandardExecutionActionsCore.ExecutionActionType.ShellScriptAction">
<ActionContent
title = "Run Script"
scriptText = "killall tests-server&#10;pushd &quot;${METAL_LIBRARY_OUTPUT_DIR}&quot;&#10;&quot;${BUILT_PRODUCTS_DIR}/tests-server&quot; &amp;&#10;popd&#10;">
scriptText = "killall tests-server&#10;killall sandbox-test-tool&#10;pushd &quot;${METAL_LIBRARY_OUTPUT_DIR}&quot;&#10;&quot;${BUILT_PRODUCTS_DIR}/tests-server&quot; &amp;&#10;popd&#10;">
<EnvironmentBuildable>
<BuildableReference
BuildableIdentifier = "primary"
Expand All @@ -79,7 +79,7 @@
ActionType = "Xcode.IDEStandardExecutionActionsCore.ExecutionActionType.ShellScriptAction">
<ActionContent
title = "Run Script"
scriptText = "killall tests-server&#10;">
scriptText = "killall tests-server&#10;killall sandbox-test-tool&#10;">
</ActionContent>
</ExecutionAction>
</PostActions>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@
ActionType = "Xcode.IDEStandardExecutionActionsCore.ExecutionActionType.ShellScriptAction">
<ActionContent
title = "Run Script"
scriptText = "killall tests-server&#10;# integration tests resources dir&#10;pushd &quot;${METAL_LIBRARY_OUTPUT_DIR}&quot;&#10;&quot;${BUILT_PRODUCTS_DIR}/tests-server&quot; &amp;&#10;popd&#10;">
scriptText = "killall tests-server&#10;killall sandbox-test-tool&#10;# integration tests resources dir&#10;pushd &quot;${METAL_LIBRARY_OUTPUT_DIR}&quot;&#10;&quot;${BUILT_PRODUCTS_DIR}/tests-server&quot; &amp;&#10;popd&#10;">
<EnvironmentBuildable>
<BuildableReference
BuildableIdentifier = "primary"
Expand All @@ -93,7 +93,7 @@
ActionType = "Xcode.IDEStandardExecutionActionsCore.ExecutionActionType.ShellScriptAction">
<ActionContent
title = "Run Script"
scriptText = "killall tests-server&#10;">
scriptText = "killall tests-server&#10;killall sandbox-test-tool&#10;">
</ActionContent>
</ExecutionAction>
</PostActions>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
<?xml version="1.0" encoding="UTF-8"?>
<Scheme
LastUpgradeVersion = "1530"
version = "1.7">
<BuildAction
parallelizeBuildables = "YES"
buildImplicitDependencies = "YES"
buildArchitectures = "Automatic">
<BuildActionEntries>
<BuildActionEntry
buildForTesting = "YES"
buildForRunning = "YES"
buildForProfiling = "YES"
buildForArchiving = "YES"
buildForAnalyzing = "YES">
<BuildableReference
BuildableIdentifier = "primary"
BlueprintIdentifier = "B6E6B9F22BA1FD90008AA7E1"
BuildableName = "sandbox-test-tool.app"
BlueprintName = "sandbox-test-tool"
ReferencedContainer = "container:DuckDuckGo.xcodeproj">
</BuildableReference>
</BuildActionEntry>
</BuildActionEntries>
</BuildAction>
<TestAction
buildConfiguration = "Debug"
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
shouldUseLaunchSchemeArgsEnv = "YES"
shouldAutocreateTestPlan = "YES">
</TestAction>
<LaunchAction
buildConfiguration = "Debug"
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
launchStyle = "0"
useCustomWorkingDirectory = "NO"
ignoresPersistentStateOnLaunch = "NO"
debugDocumentVersioning = "YES"
debugServiceExtension = "internal"
allowLocationSimulation = "YES"
viewDebuggingEnabled = "No">
<BuildableProductRunnable
runnableDebuggingMode = "0">
<BuildableReference
BuildableIdentifier = "primary"
BlueprintIdentifier = "B6E6B9F22BA1FD90008AA7E1"
BuildableName = "sandbox-test-tool.app"
BlueprintName = "sandbox-test-tool"
ReferencedContainer = "container:DuckDuckGo.xcodeproj">
</BuildableReference>
</BuildableProductRunnable>
</LaunchAction>
<ProfileAction
buildConfiguration = "Release"
shouldUseLaunchSchemeArgsEnv = "YES"
savedToolIdentifier = ""
useCustomWorkingDirectory = "NO"
debugDocumentVersioning = "YES">
<BuildableProductRunnable
runnableDebuggingMode = "0">
<BuildableReference
BuildableIdentifier = "primary"
BlueprintIdentifier = "B6E6B9F22BA1FD90008AA7E1"
BuildableName = "sandbox-test-tool.app"
BlueprintName = "sandbox-test-tool"
ReferencedContainer = "container:DuckDuckGo.xcodeproj">
</BuildableReference>
</BuildableProductRunnable>
</ProfileAction>
<AnalyzeAction
buildConfiguration = "Debug">
</AnalyzeAction>
<ArchiveAction
buildConfiguration = "Release"
revealArchiveInOrganizer = "YES">
</ArchiveAction>
</Scheme>
19 changes: 2 additions & 17 deletions DuckDuckGo/Application/AppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import Subscription
#endif

@MainActor
final class AppDelegate: NSObject, NSApplicationDelegate, FileDownloadManagerDelegate {
final class AppDelegate: NSObject, NSApplicationDelegate {

#if DEBUG
let disableCVDisplayLinkLogs: Void = {
Expand Down Expand Up @@ -230,7 +230,6 @@ final class AppDelegate: NSObject, NSApplicationDelegate, FileDownloadManagerDel
FaviconManager.shared.loadFavicons()
}
ConfigurationManager.shared.start()
FileDownloadManager.shared.delegate = self
_ = DownloadListCoordinator.shared
_ = RecentlyClosedCoordinator.shared

Expand Down Expand Up @@ -349,7 +348,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate, FileDownloadManagerDel
func applicationShouldTerminate(_ sender: NSApplication) -> NSApplication.TerminateReply {
if !FileDownloadManager.shared.downloads.isEmpty {
// if there‘re downloads without location chosen yet (save dialog should display) - ignore them
if FileDownloadManager.shared.downloads.contains(where: { $0.location.destinationURL != nil }) {
if FileDownloadManager.shared.downloads.contains(where: { $0.state.isDownloading }) {
let alert = NSAlert.activeDownloadsTerminationAlert(for: FileDownloadManager.shared.downloads)
if alert.runModal() == .cancel {
return .terminateCancel
Expand All @@ -363,20 +362,6 @@ final class AppDelegate: NSObject, NSApplicationDelegate, FileDownloadManagerDel
return .terminateNow
}

func askUserToGrantAccessToDestination(_ folderUrl: URL) {
if FileManager.default.urls(for: .downloadsDirectory, in: .userDomainMask).first?.lastPathComponent == folderUrl.lastPathComponent {
let alert = NSAlert.noAccessToDownloads()
if alert.runModal() != .cancel {
let preferencesLink = URL(string: "x-apple.systempreferences:com.apple.preference.security?Privacy_DownloadsFolder")!
NSWorkspace.shared.open(preferencesLink)
return
}
} else {
let alert = NSAlert.noAccessToSelectedFolder()
alert.runModal()
}
}

func applicationShouldHandleReopen(_ sender: NSApplication, hasVisibleWindows flag: Bool) -> Bool {
if WindowControllersManager.shared.mainWindowControllers.isEmpty,
case .normal = sender.runType {
Expand Down
14 changes: 14 additions & 0 deletions DuckDuckGo/Common/Extensions/DispatchQueueExtensions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,18 @@
}
}

/// executes the work item synchronously when running on the main thread, otherwise - schedules asynchronous dispatch
func asyncOrNow(execute workItem: @escaping @MainActor () -> Void) {
assert(self == .main)
if Thread.isMainThread {
MainActor.assumeIsolated {

Check failure on line 35 in DuckDuckGo/Common/Extensions/DispatchQueueExtensions.swift

View workflow job for this annotation

GitHub Actions / Make Release Build (DuckDuckGo Privacy Browser)

'assumeIsolated(_:file:line:)' is only available in macOS 14.0 or newer

Check failure on line 35 in DuckDuckGo/Common/Extensions/DispatchQueueExtensions.swift

View workflow job for this annotation

GitHub Actions / Make Release Build (DuckDuckGo Privacy Pro)

'assumeIsolated(_:file:line:)' is only available in macOS 14.0 or newer

Check failure on line 35 in DuckDuckGo/Common/Extensions/DispatchQueueExtensions.swift

View workflow job for this annotation

GitHub Actions / Export Notarized App

'assumeIsolated(_:file:line:)' is only available in macOS 14.0 or newer

Check failure on line 35 in DuckDuckGo/Common/Extensions/DispatchQueueExtensions.swift

View workflow job for this annotation

GitHub Actions / Export Notarized App

'assumeIsolated(_:file:line:)' is only available in macOS 14.0 or newer
workItem()
}
} else {
DispatchQueue.main.async {
workItem()
}
}
}

}
78 changes: 44 additions & 34 deletions DuckDuckGo/Common/Extensions/FileManagerExtension.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,62 +23,72 @@ extension FileManager {

@discardableResult
func moveItem(at srcURL: URL, to destURL: URL, incrementingIndexIfExists flag: Bool, pathExtension: String? = nil) throws -> URL {
return try self.perform(self.moveItem, from: srcURL, to: destURL, incrementingIndexIfExists: flag, pathExtension: pathExtension)
return try withNonExistentUrl(for: destURL, incrementingIndexIfExistsUpTo: flag ? 10000 : 0, pathExtension: pathExtension) { url in
try moveItem(at: srcURL, to: url)
return url
}
}

@discardableResult
func copyItem(at srcURL: URL, to destURL: URL, incrementingIndexIfExists flag: Bool, pathExtension: String? = nil) throws -> URL {
return try self.perform(self.copyItem, from: srcURL, to: destURL, incrementingIndexIfExists: flag, pathExtension: pathExtension)
return try withNonExistentUrl(for: destURL, incrementingIndexIfExistsUpTo: flag ? 10000 : 0, pathExtension: pathExtension) { url in
try copyItem(at: srcURL, to: url)
return url
}
}

private func perform(_ operation: (URL, URL) throws -> Void,
from srcURL: URL,
to destURL: URL,
incrementingIndexIfExists: Bool,
pathExtension: String?) throws -> URL {
func withNonExistentUrl<T>(for desiredURL: URL,
incrementingIndexIfExistsUpTo limit: UInt,
pathExtension: String? = nil,
continueOn shouldContinue: (Error) -> Bool = { ($0 as? CocoaError)?.code == .fileWriteFileExists },
perform operation: (URL) throws -> T) throws -> T {

guard incrementingIndexIfExists else {
try operation(srcURL, destURL)
return destURL
}

var suffix = pathExtension ?? destURL.pathExtension
var suffix = pathExtension ?? desiredURL.pathExtension
if !suffix.hasPrefix(".") {
suffix = "." + suffix
}
if !destURL.pathExtension.isEmpty {
if !destURL.path.hasSuffix(suffix) {
suffix = "." + destURL.pathExtension
if !desiredURL.pathExtension.isEmpty {
if !desiredURL.path.hasSuffix(suffix) {
suffix = "." + desiredURL.pathExtension
}
} else {
suffix = ""
}

let ownerDirectory = destURL.deletingLastPathComponent()
let fileNameWithoutExtension = destURL.lastPathComponent.dropping(suffix: suffix)
let ownerDirectory = desiredURL.deletingLastPathComponent()
let fileNameWithoutExtension = desiredURL.lastPathComponent.dropping(suffix: suffix)

for copy in 0... {
let destURL: URL = {
var index: UInt = 0
repeat {
let desiredURL: URL = {
// Zero means we haven't tried anything yet, so use the suggested name.
// Otherwise, simply append the file name with the copy number.
guard copy > 0 else { return destURL }
return ownerDirectory.appendingPathComponent("\(fileNameWithoutExtension) \(copy)\(suffix)")
guard index > 0 else { return desiredURL }
return ownerDirectory.appendingPathComponent("\(fileNameWithoutExtension) \(index)\(suffix)")
}()

do {
try operation(srcURL, destURL)
return destURL

} catch CocoaError.fileWriteFileExists {
// This is expected, as moveItem throws an error if the file already exists
guard copy <= 1000 else {
// If it gets to 1000 of these then chances are something else is wrong
os_log("Failed to move file to Downloads folder, attempt %d", type: .error, copy)
throw CocoaError(.fileWriteFileExists)
if !self.fileExists(atPath: desiredURL.path) {
do {
return try operation(desiredURL)
} catch {
guard shouldContinue(error) else { throw error }
// This is expected, as moveItem throws an error if the file already exists
index += 1
}
}
}
fatalError("Unexpected flow")
index += 1
} while index <= limit
// If it gets beyond the limit then chances are something else is wrong
os_log("Failed to move file to %s, attempt: %d", type: .error, desiredURL.deletingLastPathComponent().path, index)
throw CocoaError(.fileWriteFileExists)
}

func isInTrash(_ url: URL) -> Bool {
let resolvedUrl = url.resolvingSymlinksInPath()
guard let trashUrl = (try? self.url(for: .trashDirectory, in: .allDomainsMask, appropriateFor: resolvedUrl, create: false))
?? urls(for: .trashDirectory, in: .userDomainMask).first else { return false }

return resolvedUrl.path.hasPrefix(trashUrl.path)
}

}
2 changes: 1 addition & 1 deletion DuckDuckGo/Common/Extensions/NSApplicationExtension.swift
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ extension NSApplication {
}()
var runType: RunType { Self.runType }

#if !NETWORK_EXTENSION
#if !NETWORK_EXTENSION && !SANDBOX_TEST_TOOL
var mainMenuTyped: MainMenu {
return mainMenu as! MainMenu // swiftlint:disable:this force_cast
}
Expand Down
12 changes: 12 additions & 0 deletions DuckDuckGo/Common/Extensions/NSWorkspaceExtension.swift
Original file line number Diff line number Diff line change
Expand Up @@ -67,3 +67,15 @@ extension NSWorkspace {
}

}

extension NSWorkspace.OpenConfiguration {

convenience init(newInstance: Bool, environment: [String: String]? = nil) {
self.init()
self.createsNewApplicationInstance = newInstance
if let environment {
self.environment = environment
}
}

}
9 changes: 9 additions & 0 deletions DuckDuckGo/Common/Extensions/StringExtension.swift
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,15 @@ extension String {
(self as NSString).pathExtension
}

func appendingPathComponent(_ component: String) -> String {
(self as NSString).appendingPathComponent(component)
}

func appendingPathExtension(_ pathExtension: String?) -> String {
guard let pathExtension, !pathExtension.isEmpty else { return self }
return self + "." + pathExtension
}

// MARK: - Mutating

@inlinable mutating func prepend(_ string: String) {
Expand Down
Loading
Loading