Skip to content

Commit

Permalink
Fix #6721: FxAccountManger crash filling deferred 2x, make re-entran…
Browse files Browse the repository at this point in the history 
…cy-safe (#6724)

* Fix #6721: FxAcountManger crash filling deferred 2x, make re-entrancy-safe

* Unrelated: fix warning about this unused code being unsafe
  • Loading branch information
@garvankeeley
garvankeeley committed Jun 1, 2020
1 parent 23c1426 commit 7e7aebb
Show file tree
Hide file tree
Showing 5 changed files with 71 additions and 85 deletions.
104 changes: 51 additions & 53 deletions Account/FxAPushMessageHandler.swift
View file
Expand Up @@ -62,64 +62,62 @@ extension FxAPushMessageHandler {

// return handle(plaintext: string)
let deferred = PushMessageResult()
RustFirefoxAccounts.startup(prefs: profile.prefs).uponQueue(.main) { _ in
RustFirefoxAccounts.reconfig { accountManager in
accountManager.deviceConstellation()?.processRawIncomingAccountEvent(pushPayload: string) {
result in
guard case .success(let events) = result, let firstEvent = events.first else {
let err: PushMessageError
if case .failure(let error) = result {
err = PushMessageError.messageIncomplete(error.localizedDescription)
} else {
err = PushMessageError.messageIncomplete("empty message")
}
deferred.fill(Maybe(failure: err))
return
RustFirefoxAccounts.reconfig(prefs: profile.prefs).uponQueue(.main) { accountManager in
accountManager.deviceConstellation()?.processRawIncomingAccountEvent(pushPayload: string) {
result in
guard case .success(let events) = result, let firstEvent = events.first else {
let err: PushMessageError
if case .failure(let error) = result {
err = PushMessageError.messageIncomplete(error.localizedDescription)
} else {
err = PushMessageError.messageIncomplete("empty message")
}
if events.count > 1 {
// Log to the console for debugging release builds
os_log("%{public}@", log: OSLog(subsystem: "org.mozilla.firefox", category: "firefoxnotificationservice"), type: OSLogType.debug, "Multiple events arrived, only handling the first event.")
deferred.fill(Maybe(failure: err))
return
}
if events.count > 1 {
// Log to the console for debugging release builds
os_log("%{public}@", log: OSLog(subsystem: "org.mozilla.firefox", category: "firefoxnotificationservice"), type: OSLogType.debug, "Multiple events arrived, only handling the first event.")
}
switch firstEvent {
case .incomingDeviceCommand(let deviceCommand):
switch deviceCommand {
case .tabReceived(_, let tabData):
let title = tabData.last?.title ?? ""
let url = tabData.last?.url ?? ""
let message = PushMessage.commandReceived(tab: ["title": title, "url": url])
deferred.fill(Maybe(success: message))
}
switch firstEvent {
case .incomingDeviceCommand(let deviceCommand):
switch deviceCommand {
case .tabReceived(_, let tabData):
let title = tabData.last?.title ?? ""
let url = tabData.last?.url ?? ""
let message = PushMessage.commandReceived(tab: ["title": title, "url": url])
deferred.fill(Maybe(success: message))
}
case .deviceConnected(let deviceName):
let message = PushMessage.deviceConnected(deviceName)
case .deviceConnected(let deviceName):
let message = PushMessage.deviceConnected(deviceName)
deferred.fill(Maybe(success: message))
case let .deviceDisconnected(deviceId, isLocalDevice):
if isLocalDevice {
// We can't disconnect the device from the account until we have access to the application, so we'll handle this properly in the AppDelegate (as this code in an extension),
// by calling the FxALoginHelper.applicationDidDisonnect(application).
self.profile.prefs.setBool(true, forKey: PendingAccountDisconnectedKey)
let message = PushMessage.thisDeviceDisconnected
deferred.fill(Maybe(success: message))
case let .deviceDisconnected(deviceId, isLocalDevice):
if isLocalDevice {
// We can't disconnect the device from the account until we have access to the application, so we'll handle this properly in the AppDelegate (as this code in an extension),
// by calling the FxALoginHelper.applicationDidDisonnect(application).
self.profile.prefs.setBool(true, forKey: PendingAccountDisconnectedKey)
let message = PushMessage.thisDeviceDisconnected
deferred.fill(Maybe(success: message))
return
}

guard let profile = self.profile as? BrowserProfile else {
// We can't look up a name in testing, so this is the same as not knowing about it.
let message = PushMessage.deviceDisconnected(nil)
deferred.fill(Maybe(success: message))
return
}

profile.remoteClientsAndTabs.getClient(fxaDeviceId: deviceId).uponQueue(.main) { result in
guard let device = result.successValue else { return }
let message = PushMessage.deviceDisconnected(device?.name)
if let id = device?.guid {
profile.remoteClientsAndTabs.deleteClient(guid: id).uponQueue(.main) { _ in
print("deleted client")
}
}
return
}

deferred.fill(Maybe(success: message))
guard let profile = self.profile as? BrowserProfile else {
// We can't look up a name in testing, so this is the same as not knowing about it.
let message = PushMessage.deviceDisconnected(nil)
deferred.fill(Maybe(success: message))
return
}

profile.remoteClientsAndTabs.getClient(fxaDeviceId: deviceId).uponQueue(.main) { result in
guard let device = result.successValue else { return }
let message = PushMessage.deviceDisconnected(device?.name)
if let id = device?.guid {
profile.remoteClientsAndTabs.deleteClient(guid: id).uponQueue(.main) { _ in
print("deleted client")
}
}

deferred.fill(Maybe(success: message))
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion Client/Frontend/Settings/AdvancedAccountSettingViewController.swift
View file
Expand Up @@ -55,7 +55,7 @@ class AdvancedAccountSettingViewController: SettingsTableViewController {

override func viewWillDisappear(_ animated: Bool) {
super.viewWillDisappear(animated)
RustFirefoxAccounts.reconfig()
RustFirefoxAccounts.reconfig(prefs: profile.prefs)
}

override func generateSettings() -> [SettingSection] {
Expand Down
4 changes: 2 additions & 2 deletions Client/Frontend/Settings/AppSettingsOptions.swift
View file
Expand Up @@ -922,7 +922,7 @@ class ChinaSyncServiceSetting: Setting {
UnifiedTelemetry.recordEvent(category: .action, method: .tap, object: .chinaServerSwitch)
guard profile.rustFxA.hasAccount() else {
prefs.setObject(toggle.isOn, forKey: prefKey)
RustFirefoxAccounts.reconfig()
RustFirefoxAccounts.reconfig(prefs: profile.prefs)
return
}

Expand All @@ -933,7 +933,7 @@ class ChinaSyncServiceSetting: Setting {
let ok = UIAlertAction(title: Strings.OKString, style: .default) { _ in
self.prefs.setObject(toggle.isOn, forKey: self.prefKey)
self.profile.removeAccount()
RustFirefoxAccounts.reconfig()
RustFirefoxAccounts.reconfig(prefs: self.profile.prefs)
}
let cancel = UIAlertAction(title: Strings.CancelString, style: .default) { _ in
toggle.setOn(!toggle.isOn, animated: true)
Expand Down
31 changes: 17 additions & 14 deletions RustFxA/RustFirefoxAccounts.swift
View file
Expand Up @@ -23,11 +23,11 @@ final class Unknown: NSObject, NSCoding {
// TODO: renamed FirefoxAccounts.swift once the old code is removed fully.
open class RustFirefoxAccounts {
public static let prefKeyLastDeviceName = "prefKeyLastDeviceName"

private static let clientID = "1b1a3e44c54fbb58"
public static let redirectURL = "urn:ietf:wg:oauth:2.0:oob:oauth-redirect-webchannel"
public static var shared = RustFirefoxAccounts()
public var accountManager = Deferred<FxAccountManager>()
private static var isInitializingAccountManager = false
public var avatar: Avatar?
public let syncAuthState: SyncAuthState
fileprivate static var prefs: Prefs?
Expand All @@ -50,15 +50,21 @@ open class RustFirefoxAccounts {
hook into notifications like `.accountProfileUpdate` to refresh once initialize() is complete.
Or they can wait on the accountManager deferred to fill.
*/
public static func startup(prefs _prefs: Prefs) -> Deferred<FxAccountManager> {
prefs = _prefs
if let manager = RustFirefoxAccounts.shared.accountManager.peek() {
return Deferred(value: manager)
public static func startup(prefs: Prefs) -> Deferred<FxAccountManager> {
assert(Thread.isMainThread)
RustFirefoxAccounts.prefs = prefs
if RustFirefoxAccounts.shared.accountManager.isFilled || isInitializingAccountManager {
return RustFirefoxAccounts.shared.accountManager
}

let deferred = Deferred<FxAccountManager>()
// Setup the re-entrancy guard so consecutive calls to startup() won't do manager.initialize(). This flag is cleared when initialize is complete, or by calling reconfig().
isInitializingAccountManager = true

let manager = RustFirefoxAccounts.shared.createAccountManager()
manager.initialize { result in
assert(Thread.isMainThread)
isInitializingAccountManager = false

let hasAttemptedMigration = UserDefaults.standard.bool(forKey: "hasAttemptedMigration")

// Note this checks if startup() is called in an app extensions, and if so, do not try account migration
Expand All @@ -72,14 +78,13 @@ open class RustFirefoxAccounts {
}

RustFirefoxAccounts.shared.accountManager.fill(manager)
deferred.fill(manager)

// After everthing is setup, register for push notifications
if manager.hasAccount() {
NotificationCenter.default.post(name: .RegisterForPushNotifications, object: nil)
}
}
return deferred
return RustFirefoxAccounts.shared.accountManager
}

private static let prefKeySyncAuthStateUniqueID = "PrefKeySyncAuthStateUniqueID"
Expand All @@ -95,13 +100,11 @@ open class RustFirefoxAccounts {
return id
}

public static func reconfig(_ completion: ((FxAccountManager) -> Void)? = nil) {
@discardableResult
public static func reconfig(prefs: Prefs) -> Deferred<FxAccountManager> {
isInitializingAccountManager = false
shared.accountManager = Deferred<FxAccountManager>()
let manager = shared.createAccountManager()
manager.initialize() { _ in
shared.accountManager.fill(manager)
completion?(manager)
}
return startup(prefs: prefs)
}

public var isChinaSyncServiceEnabled: Bool {
Expand Down
15 changes: 0 additions & 15 deletions Shared/Extensions/HashExtensions.swift
View file
Expand Up @@ -57,18 +57,3 @@ extension Data {
}
}

extension Data {
public func xoredWith(_ other: Data) -> Data? {
if self.count != other.count {
return nil
}
var xoredBytes = [UInt8](repeating: 0, count: self.count)
let selfBytes = (self as NSData).bytes.bindMemory(to: UInt8.self, capacity: self.count)
let otherBytes = (other as NSData).bytes.bindMemory(to: UInt8.self, capacity: other.count)
for i in 0..<self.count {
xoredBytes[i] = selfBytes[i] ^ otherBytes[i]
}
return Data(bytes: UnsafePointer<UInt8>(xoredBytes), count: self.count)
}

}

0 comments on commit 7e7aebb

Please sign in to comment.