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.

Sign up for GitHub

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

Jump to bottom

Bugfix FXIOS-8105 [v121] Fix Send to Device failing intermittently #18042

Merged
merged 4 commits into from
Jan 9, 2024
Merged

Bugfix FXIOS-8105 [v121] Fix Send to Device failing intermittently #18042

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98cd032
[8105] Fix issue where Send to Device could fail because RustFXAccoun…
mattreaganmozilla Jan 8, 2024
f65713d
[8105] Additional work to fix issue with Send to Device sometimes fai…
mattreaganmozilla Jan 9, 2024
51525b5
[8105] Cleanup, remove unnecessary code and explicit self references
mattreaganmozilla Jan 9, 2024
4849335
[8105] Additional cleanup, restore self references where needed to av…
mattreaganmozilla Jan 9, 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
61 changes: 47 additions & 14 deletions firefox-ios/Extensions/ShareTo/ShareViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ class ShareViewController: UIViewController {
var shareItem: ExtensionUtils.ExtractedShareItem?
private var viewsShownDuringDoneAnimation = [UIView]()
private var stackView: UIStackView!
private var spinner: UIActivityIndicatorView?
private var actionDoneRow: (row: UIStackView, label: UILabel)!
private var sendToDevice: SendToDevice?
private var pageInfoHeight: NSLayoutConstraint?
Expand All @@ -93,6 +94,25 @@ class ShareViewController: UIViewController {

setupNavBar()
setupStackView()

if RustFirefoxAccounts.shared.accountManager == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know how this can be not nil? Is there a sequence of events where the ShareViewController is made in a context where the initialization already happened? I’m mostly curious nothing is functionally off here!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tarikeshaq (cc @OrlaM) Just answering based on observation, there is a lot going on with this code that I still don't have full context/familiarity with. But what I could see was:

  • The accountManager starts off as nil until startup() completes
  • Startup is an async process and so there are potential races/timing issues
  • Repeated shares in Safari will reload the UI components but RustFirefoxAccounts.shared.accountManager is non-nil if startup() has already previously completed (unless, presumably, the app extension is jetsammed or terminated by the OS etc.)

I observed that in some situations the accountManager was also seemingly non-nil when I would expect it to be nil (on an initial test of the extension etc.). However I can't say for certain if this was simply due to side effects of how I was testing.

I should also add that I still am not entirely certain what the original bug reporters were actually seeing since at the time of the fix there were no reliable repro steps or screen captures. But based on all the available info this is, AFAICT, the most likely cause of the issue and if so should hopefully correct it.

LMK if any further questions, I'm happy to investigate or dig into this further.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeated shares in Safari will reload the UI components but RustFirefoxAccounts.shared.accountManager is non-nil if startup() has already previously completed (unless, presumably, the app extension is jetsammed or terminated by the OS etc.)

Ahh that makes a ton of sense, thanks a ton @mattreaganmozilla - really appreciate you digging into this one!!

// Show brief spinner in UI while startup is finishing
showProgressIndicator()

let profile = BrowserProfile(localName: "profile")
Viaduct.shared.useReqwestBackend()
RustFirefoxAccounts.startup(prefs: profile.prefs) { [weak self] _ in
// Hide spinner and finish UI setup (Note: this completion
// block is currently guaranteed to arrive on main thread.)
self?.hideProgressIndicator()
self?.finalizeUISetup()
}
} else {
finalizeUISetup()
}
}

private func finalizeUISetup() {
setupRows()

guard let shareItem = shareItem else { return }
Expand All @@ -104,10 +124,6 @@ class ShareViewController: UIViewController {
case .rawText(let text):
self.pageInfoRowTitleLabel?.text = text.quoted
}

let profile = BrowserProfile(localName: "profile")
Viaduct.shared.useReqwestBackend()
RustFirefoxAccounts.startup(prefs: profile.prefs) { _ in }
}

private func setupRows() {
Expand Down Expand Up @@ -383,6 +399,27 @@ class ShareViewController: UIViewController {
stackView.trailingAnchor.constraint(equalTo: view.trailingAnchor)
])
}

private func showProgressIndicator() {
let indicator = UIActivityIndicatorView(style: .large)
let defaultSize = CGSize(width: 40.0, height: 40.0)
view.addSubview(indicator)
indicator.translatesAutoresizingMaskIntoConstraints = false
NSLayoutConstraint.activate([
indicator.centerXAnchor.constraint(equalTo: view.centerXAnchor),
indicator.centerYAnchor.constraint(equalTo: view.centerYAnchor),
indicator.widthAnchor.constraint(equalToConstant: defaultSize.width),
indicator.heightAnchor.constraint(equalToConstant: defaultSize.height),
])
indicator.startAnimating()
spinner = indicator
}

private func hideProgressIndicator() {
spinner?.stopAnimating()
spinner?.removeFromSuperview()
spinner = nil
}
}

extension ShareViewController {
Expand Down Expand Up @@ -448,16 +485,12 @@ extension ShareViewController {
guard let shareItem = shareItem, case .shareItem(let item) = shareItem else { return }

gesture.isEnabled = false
view.isUserInteractionEnabled = false
if RustFirefoxAccounts.shared.accountManager != nil {
self.view.isUserInteractionEnabled = true
self.sendToDevice = SendToDevice()
guard let sendToDevice = self.sendToDevice else { return }
sendToDevice.sharedItem = item
sendToDevice.delegate = self.delegate
let vc = sendToDevice.initialViewController()
self.navigationController?.pushViewController(vc, animated: true)
}
self.sendToDevice = SendToDevice()
guard let sendToDevice = self.sendToDevice else { return }
sendToDevice.sharedItem = item
sendToDevice.delegate = self.delegate
let vc = sendToDevice.initialViewController()
navigationController?.pushViewController(vc, animated: true)
}

func openFirefox(withUrl url: String, isSearch: Bool) {
Expand Down