mozilla-mobile
Multiple calls to RustLogins `logins.addLogin()` one after another cause a crash with unrecognized selector sent to instance
Multiple calls to RustLogins logins.addLogin() one after another cause a crash with unrecognized selector sent to instance.
For example:
func test...() {
...
// Add three logins to delete
logins.addLogin(login: login1) { result in
switch result {
case .success(let login):
XCTAssertNotNil(login)
addLogin1Expectation.fulfill()
case .failure:
XCTFail("Add login 1 failed")
}
}
logins.addLogin(login: login2) { result in
switch result {
case .success(let login):
XCTAssertNotNil(login)
addLogin2Expectation.fulfill()
case .failure:
XCTFail("Add login 2 failed")
}
}
logins.addLogin(login: login3) { result in
switch result {
case .success(let login):
XCTAssertNotNil(login)
addLogin3Expectation.fulfill()
case .failure:
XCTFail("Add login 3 failed")
}
}
// Wait for both logins to be added
wait(for: [addLogin1Expectation, addLogin2Expectation, addLogin3Expectation])
...
}
We should look into why this crashes the app during tests and fix the issue.
┆Issue is synchronized with this Jira Bug
I dug into the crash and it looks like the issue happens because getStoredKey(...) can complete on a background thread, but addLogin, updateLogin, and verifyLogins were touching self.storage directly inside that callback without hopping back to the serial queue.
So when multiple addLogin() calls run back-to-back, some storage operations were executed off the intended queue, leading to racy FFI calls and eventually the “unrecognized selector sent to instance” crash.
Great investigation @Aniket-pd! I haven't dug into this myself but it definitely sounds like this could be the culprit. If you'd like to take a stab at some reworking to solve this, let me know and I can assign this one to you!
I stumbled across this bug myself as I was trying to write a unit test to create and then delete multiple logins. See this PR: #31133
If you do work on a fix, you can borrow that logic to add a unit test with multiple concurrent calls to addLogin. For now, I opted to use an expectation to essentially throttle the addLogins call in testDeleteMultipleLogins until we have a fix. It was very easy to reproduce with a unit test, but I don't imagine this is likely to happen in production. 🤞
