From 82908360da7d2ed7c55415f02d2a1a8fa1a8e458 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mickae=CC=88l=20Menu?= Date: Fri, 20 May 2022 16:10:06 +0200 Subject: [PATCH 1/5] Fix crash when `progression` is out of range --- Sources/Navigator/EPUB/EPUBNavigatorViewController.swift | 2 +- Sources/Navigator/EPUB/EPUBReflowableSpreadView.swift | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift b/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift index e296b8ed9..5cf741aa9 100644 --- a/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift +++ b/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift @@ -504,7 +504,7 @@ open class EPUBNavigatorViewController: UIViewController, VisualNavigator, Selec let link = spreadView.focusedResource ?? spreadView.spread.leading let href = link.href - let progression = spreadView.progression(in: href) + let progression = min(max(spreadView.progression(in: href), 0.0), 1.0) // The positions are not always available, for example a Readium WebPub doesn't have any // unless a Publication Positions Web Service is provided. diff --git a/Sources/Navigator/EPUB/EPUBReflowableSpreadView.swift b/Sources/Navigator/EPUB/EPUBReflowableSpreadView.swift index 6ff91dd3c..29f829e48 100644 --- a/Sources/Navigator/EPUB/EPUBReflowableSpreadView.swift +++ b/Sources/Navigator/EPUB/EPUBReflowableSpreadView.swift @@ -321,9 +321,11 @@ final class EPUBReflowableSpreadView: EPUBSpreadView { // Called by the javascript code to notify that scrolling ended. private func progressionDidChange(_ body: Any) { - guard spreadLoaded, let bodyString = body as? String, let newProgression = Double(bodyString) else { + guard spreadLoaded, let bodyString = body as? String, var newProgression = Double(bodyString) else { return } + newProgression = min(max(newProgression, 0.0), 1.0) + if previousProgression == nil { previousProgression = progression } From 715d7349a7b365a9a926d01cbbbe130ab9ced540 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mickae=CC=88l=20Menu?= Date: Fri, 20 May 2022 17:48:47 +0200 Subject: [PATCH 2/5] Fix data race in DefaultHTTPClient --- .../Toolkit/HTTP/DefaultHTTPClient.swift | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/Sources/Shared/Toolkit/HTTP/DefaultHTTPClient.swift b/Sources/Shared/Toolkit/HTTP/DefaultHTTPClient.swift index b8b883e5b..ffad284d9 100644 --- a/Sources/Shared/Toolkit/HTTP/DefaultHTTPClient.swift +++ b/Sources/Shared/Toolkit/HTTP/DefaultHTTPClient.swift @@ -253,44 +253,45 @@ public final class DefaultHTTPClient: NSObject, HTTPClient, Loggable, URLSession /// On-going tasks. private var tasks: [Task] = [] + + /// Protects `tasks` against data races. + private let tasksQueue = DispatchQueue(label: "org.readium.DefaultHTTPClient.tasksQueue") - private func findTaskIndex(_ task: URLSessionTask) -> Int? { - let i = tasks.firstIndex(where: { $0.task == task}) - if i == nil { - log(.error, "Cannot find on-going HTTP task for \(task)") + private func start(_ task: Task) -> Cancellable { + tasksQueue.sync { + tasks.append(task) + task.start() + return task } - return i } - private func start(_ task: Task) -> Cancellable { - tasks.append(task) - task.start() - return task + private func findTask(for urlTask: URLSessionTask) -> Task? { + tasksQueue.sync { + let task = tasks.first { $0.task == urlTask} + if task == nil { + log(.error, "Cannot find on-going HTTP task for \(urlTask)") + } + return task + } } // MARK: - URLSessionDataDelegate public func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive response: URLResponse, completionHandler: @escaping (URLSession.ResponseDisposition) -> ()) { - guard let i = findTaskIndex(dataTask) else { + guard let task = findTask(for: dataTask) else { completionHandler(.cancel) return } - tasks[i].urlSession(session, didReceive: response, completionHandler: completionHandler) + task.urlSession(session, didReceive: response, completionHandler: completionHandler) } public func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive data: Data) { - guard let i = findTaskIndex(dataTask) else { - return - } - tasks[i].urlSession(session, didReceive: data) + findTask(for: dataTask)?.urlSession(session, didReceive: data) } public func urlSession(_ session: URLSession, task: URLSessionTask, didCompleteWithError error: Error?) { - guard let i = findTaskIndex(task) else { - return - } - tasks[i].urlSession(session, didCompleteWithError: error) + findTask(for: task)?.urlSession(session, didCompleteWithError: error) } From 09e714e8e78e9e3d5c9f650a3eb89607937d4637 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mickae=CC=88l=20Menu?= Date: Sat, 21 May 2022 11:59:18 +0200 Subject: [PATCH 3/5] Allow concurrent reads on `DefaultHTTPClient.tasks` --- Sources/Shared/Toolkit/HTTP/DefaultHTTPClient.swift | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Sources/Shared/Toolkit/HTTP/DefaultHTTPClient.swift b/Sources/Shared/Toolkit/HTTP/DefaultHTTPClient.swift index ffad284d9..c529f76b9 100644 --- a/Sources/Shared/Toolkit/HTTP/DefaultHTTPClient.swift +++ b/Sources/Shared/Toolkit/HTTP/DefaultHTTPClient.swift @@ -255,10 +255,13 @@ public final class DefaultHTTPClient: NSObject, HTTPClient, Loggable, URLSession private var tasks: [Task] = [] /// Protects `tasks` against data races. - private let tasksQueue = DispatchQueue(label: "org.readium.DefaultHTTPClient.tasksQueue") + private let tasksQueue = DispatchQueue(label: "org.readium.DefaultHTTPClient.tasksQueue", attributes: .concurrent) private func start(_ task: Task) -> Cancellable { - tasksQueue.sync { + // The `barrier` flag here guarantees that we will never have a + // concurrent read on `tasks` while we are modifying it. This prevents + // a data race. + tasksQueue.sync(flags: .barrier) { tasks.append(task) task.start() return task From 33d22bb68cb5e0a6795a94897882cad5da794513 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mickae=CC=88l=20Menu?= Date: Sat, 21 May 2022 18:57:08 +0200 Subject: [PATCH 4/5] Refactor data race prevention into an `@Atomic` property wrapper --- Sources/Shared/Toolkit/Atomic.swift | 76 +++++++++++++++++++ .../Toolkit/HTTP/DefaultHTTPClient.swift | 26 ++----- Support/Carthage/.xcodegen | 4 +- .../Readium.xcodeproj/project.pbxproj | 4 + 4 files changed, 91 insertions(+), 19 deletions(-) create mode 100644 Sources/Shared/Toolkit/Atomic.swift diff --git a/Sources/Shared/Toolkit/Atomic.swift b/Sources/Shared/Toolkit/Atomic.swift new file mode 100644 index 000000000..cea2aa2a4 --- /dev/null +++ b/Sources/Shared/Toolkit/Atomic.swift @@ -0,0 +1,76 @@ +// +// Copyright 2022 Readium Foundation. All rights reserved. +// Use of this source code is governed by the BSD-style license +// available in the top-level LICENSE file of the project. +// + +import Foundation + +/// Smart pointer protecting concurrent access to its memory to avoid data races. +/// +/// This is also a property wrapper, which makes it easy to use as: +/// ``` +/// @Atomic var data: Int +/// ``` +/// +/// The property becomes read-only, to prevent a common error when modifying the property using its +/// previous value. For example: +/// ``` +/// data += 1 +/// ``` +/// This is not safe, because it's actually two operations: a read and a write. The value might have changed +/// between the moment you read it and when you write the result of incrementing the value. +/// +/// Instead, you must use `write()` to mutate the property: +/// ``` +/// $data.write { value in +/// value += 1 +/// } +/// ``` +@propertyWrapper +public final class Atomic { + private var value: Value + + /// Queue used to protect accesses to `value`. + /// + /// We could use a serial queue but that would impact performances as concurrent reads would not be + /// possible. To make sure we don't get data races, writes are done using a `.barrier` flag. + private let queue = DispatchQueue(label: "org.readium.Atomic", attributes: .concurrent) + + public init(wrappedValue value: Value) { + self.value = value + } + + public var wrappedValue: Value { + get { read() } + set { fatalError("Use $property.write { $0 = ... } to mutate this property") } + } + + public var projectedValue: Atomic { + return self + } + + /// Reads the current value synchronously. + public func read() -> Value { + queue.sync { + value + } + } + + /// Reads the current value asynchronously. + public func read(completion: @escaping (Value) -> Void) { + queue.async { + completion(self.value) + } + } + + /// Writes the value synchronously in a safe way. + public func write(_ changes: (inout Value) -> Void) { + // The `barrier` flag here guarantees that we will never have a + // concurrent read on `value` while we are modifying it. This prevents + // a data race. + queue.sync(flags: .barrier) { + changes(&value) + } + } +} diff --git a/Sources/Shared/Toolkit/HTTP/DefaultHTTPClient.swift b/Sources/Shared/Toolkit/HTTP/DefaultHTTPClient.swift index c529f76b9..7b81cf246 100644 --- a/Sources/Shared/Toolkit/HTTP/DefaultHTTPClient.swift +++ b/Sources/Shared/Toolkit/HTTP/DefaultHTTPClient.swift @@ -252,30 +252,20 @@ public final class DefaultHTTPClient: NSObject, HTTPClient, Loggable, URLSession // MARK: - Task Management /// On-going tasks. - private var tasks: [Task] = [] + @Atomic private var tasks: [Task] = [] - /// Protects `tasks` against data races. - private let tasksQueue = DispatchQueue(label: "org.readium.DefaultHTTPClient.tasksQueue", attributes: .concurrent) - private func start(_ task: Task) -> Cancellable { - // The `barrier` flag here guarantees that we will never have a - // concurrent read on `tasks` while we are modifying it. This prevents - // a data race. - tasksQueue.sync(flags: .barrier) { - tasks.append(task) - task.start() - return task - } + $tasks.write { $0.append(task) } + task.start() + return task } private func findTask(for urlTask: URLSessionTask) -> Task? { - tasksQueue.sync { - let task = tasks.first { $0.task == urlTask} - if task == nil { - log(.error, "Cannot find on-going HTTP task for \(urlTask)") - } - return task + let task = tasks.first { $0.task == urlTask} + if task == nil { + log(.error, "Cannot find on-going HTTP task for \(urlTask)") } + return task } diff --git a/Support/Carthage/.xcodegen b/Support/Carthage/.xcodegen index d3207b9b6..991c877cd 100644 --- a/Support/Carthage/.xcodegen +++ b/Support/Carthage/.xcodegen @@ -1,5 +1,5 @@ # XCODEGEN VERSION -2.25.0 +2.28.0 # SPEC { @@ -7504,6 +7504,7 @@ ../../Sources/Shared/Publication/Services/Locator/DefaultLocatorService.swift ../../Sources/Shared/Publication/Services/Locator/LocatorService.swift ../../Sources/Shared/Publication/Services/Positions +../../Sources/Shared/Publication/Services/Positions/InMemoryPositionsService.swift ../../Sources/Shared/Publication/Services/Positions/PerResourcePositionsService.swift ../../Sources/Shared/Publication/Services/Positions/PositionsService.swift ../../Sources/Shared/Publication/Services/PublicationService.swift @@ -7525,6 +7526,7 @@ ../../Sources/Shared/Toolkit/Archive/ExplodedArchive.swift ../../Sources/Shared/Toolkit/Archive/Minizip.swift ../../Sources/Shared/Toolkit/Archive/ZIPFoundation.swift +../../Sources/Shared/Toolkit/Atomic.swift ../../Sources/Shared/Toolkit/Cancellable.swift ../../Sources/Shared/Toolkit/CancellableResult.swift ../../Sources/Shared/Toolkit/ControlFlow.swift diff --git a/Support/Carthage/Readium.xcodeproj/project.pbxproj b/Support/Carthage/Readium.xcodeproj/project.pbxproj index 8b2068588..208ed9f78 100644 --- a/Support/Carthage/Readium.xcodeproj/project.pbxproj +++ b/Support/Carthage/Readium.xcodeproj/project.pbxproj @@ -157,6 +157,7 @@ 825642E013351C922B6510AD /* UTI.swift in Sources */ = {isa = PBXBuildFile; fileRef = 48B28C65845F0575C40877F6 /* UTI.swift */; }; 82BAA3EB081DD29A928958AC /* ContentLayout.swift in Sources */ = {isa = PBXBuildFile; fileRef = E4A496C959F870BAFDB447DA /* ContentLayout.swift */; }; 837C0BC3151E302508B4BC44 /* LCPAuthenticating.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2CB0BFECA8236412881393AA /* LCPAuthenticating.swift */; }; + 861C71906603180ABD01E8FA /* Atomic.swift in Sources */ = {isa = PBXBuildFile; fileRef = CBB57FCAEE605484A7290DBB /* Atomic.swift */; }; 862098954DE3522E87E806CC /* Minizip.xcframework in Frameworks */ = {isa = PBXBuildFile; fileRef = CFFEBDFE931745C07DACD4A3 /* Minizip.xcframework */; }; 874BD412CBA1D392451B952B /* Assets in Resources */ = {isa = PBXBuildFile; fileRef = 251275D0DF87F85158A5FEA9 /* Assets */; }; 88A171A36700ACF5A4AD6305 /* PublicationService.swift in Sources */ = {isa = PBXBuildFile; fileRef = 667B76C4766DFF58D066D40B /* PublicationService.swift */; }; @@ -538,6 +539,7 @@ C5E7CEDF6EA681FE8119791B /* Feed.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Feed.swift; sourceTree = ""; }; C7931CB2A5658CAAECD150B0 /* NSRegularExpression.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NSRegularExpression.swift; sourceTree = ""; }; CAD79372361D085CA0500CF4 /* Properties+OPDS.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Properties+OPDS.swift"; sourceTree = ""; }; + CBB57FCAEE605484A7290DBB /* Atomic.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Atomic.swift; sourceTree = ""; }; CC925E451D875E5F74748EDC /* Optional.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Optional.swift; sourceTree = ""; }; CDA8111A330AB4D7187DD743 /* LocatorService.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LocatorService.swift; sourceTree = ""; }; CE641F78FD99A426A80B3495 /* Zip.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = Zip.h; sourceTree = ""; }; @@ -1287,6 +1289,7 @@ C42B511253C3D9C6DA8AA5CC /* Toolkit */ = { isa = PBXGroup; children = ( + CBB57FCAEE605484A7290DBB /* Atomic.swift */, 7BBD54FD376456C1925316BC /* Cancellable.swift */, 0CB0D3EE83AE0CE1F0B0B0CF /* CancellableResult.swift */, 55BC4119B8937D17ED80B1AB /* ControlFlow.swift */, @@ -1842,6 +1845,7 @@ 09B7475BC8E63C940BD5881A /* Archive.swift in Sources */, A3EBB38968F8EB4ABC560678 /* ArchiveFetcher.swift in Sources */, AA218336FBD1C23959542515 /* Array.swift in Sources */, + 861C71906603180ABD01E8FA /* Atomic.swift in Sources */, 5C8ED4151A6C7EF6608A03F8 /* AudioSession.swift in Sources */, 8E4C9F5A53A6F9B8FC28B7D4 /* BufferedResource.swift in Sources */, 6D27F5B8C7DBFBF5FB99A4BE /* Bundle.swift in Sources */, From ee97fe0c7cc905113c43da91f1717152c00d4e5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mickae=CC=88l=20Menu?= Date: Sat, 21 May 2022 19:46:15 +0200 Subject: [PATCH 5/5] Update Carthage project --- Support/Carthage/.xcodegen | 2 +- Support/Carthage/Readium.xcodeproj/project.pbxproj | 2 +- .../xcshareddata/xcschemes/R2Navigator.xcscheme | 2 +- .../Readium.xcodeproj/xcshareddata/xcschemes/R2Shared.xcscheme | 2 +- .../xcshareddata/xcschemes/R2Streamer.xcscheme | 2 +- .../xcshareddata/xcschemes/ReadiumLCP.xcscheme | 2 +- .../xcshareddata/xcschemes/ReadiumOPDS.xcscheme | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Support/Carthage/.xcodegen b/Support/Carthage/.xcodegen index 991c877cd..d10703e09 100644 --- a/Support/Carthage/.xcodegen +++ b/Support/Carthage/.xcodegen @@ -1,5 +1,5 @@ # XCODEGEN VERSION -2.28.0 +2.29.0 # SPEC { diff --git a/Support/Carthage/Readium.xcodeproj/project.pbxproj b/Support/Carthage/Readium.xcodeproj/project.pbxproj index 208ed9f78..2390dc5fd 100644 --- a/Support/Carthage/Readium.xcodeproj/project.pbxproj +++ b/Support/Carthage/Readium.xcodeproj/project.pbxproj @@ -1690,7 +1690,7 @@ }; }; buildConfigurationList = 5A872BCD95ECE5673BC89051 /* Build configuration list for PBXProject "Readium" */; - compatibilityVersion = "Xcode 10.0"; + compatibilityVersion = "Xcode 11.0"; developmentRegion = en; hasScannedForEncodings = 0; knownRegions = ( diff --git a/Support/Carthage/Readium.xcodeproj/xcshareddata/xcschemes/R2Navigator.xcscheme b/Support/Carthage/Readium.xcodeproj/xcshareddata/xcschemes/R2Navigator.xcscheme index 0d36feb0c..faba467b2 100644 --- a/Support/Carthage/Readium.xcodeproj/xcshareddata/xcschemes/R2Navigator.xcscheme +++ b/Support/Carthage/Readium.xcodeproj/xcshareddata/xcschemes/R2Navigator.xcscheme @@ -1,7 +1,7 @@ + version = "1.7"> + version = "1.7"> + version = "1.7"> + version = "1.7"> + version = "1.7">