Repository navigation
onActivityResult can hang forever if OK but data == null, and pending… - #70
MegaManSec wants to merge 2 commits into
Conversation
5250548 to
c5820a2
Compare
… promise is not cleared (cherry picked from commit a330c1ee2261f192103ac4e7a2fa5d86e2c59b7f)
c5820a2 to
26e0ebf
Compare
|
@MegaManSec may you resolve conflicts with main please? |
|
@JakubKorytko done |
JakubKorytko
left a comment
JakubKorytko
left a comment
There was a problem hiding this comment.
Tested on an emulator by calling onActivityResult directly with a fake promise:
RESULT_OK+ null data: rejects (hangs on main)- unknown
resultCode: rejects (hangs on main) RESULT_CANCELED: resolvesCANCELED, same as main
| ) | ||
| localPromise?.resolve(TokenizationStatus.SUCCESS.code) | ||
| if (data == null) { | ||
| localPromise?.reject(E_OPERATION_FAILED, "Tokenization returned RESULT_OK but intent data was null") |
There was a problem hiding this comment.
Is RESULT_OK with null data a real failure? Google says OK, so the card might be in the wallet already, just without the token id. Rejecting here would show an error to the user while the card was added. Do you have a repro for this, or is it a safety net?
| localPromise?.reject(E_OPERATION_FAILED, "Tokenization returned RESULT_OK but intent data was null") | ||
| return | ||
| } | ||
| val tokenId = data.getStringExtra(TapAndPay.EXTRA_ISSUER_TOKEN_ID).toString() |
There was a problem hiding this comment.
If the extra is missing, .toString() turns null into the string "null" and that goes out as tokenId in the event. Since this line is touched anyway, maybe drop .toString() and pass the nullable value, OnCardActivatedEvent already takes null for the canceled case.
| ) | ||
| localPromise?.resolve(TokenizationStatus.CANCELED.code) | ||
| } else { | ||
| localPromise?.reject(E_OPERATION_FAILED, "Tokenization failed with resultCode=$resultCode") |
There was a problem hiding this comment.
We already have TokenizationStatus.ERROR (-1), and JS maps it to 'error' in getTokenizationStatus, but nothing returns it yet. Would localPromise?.resolve(TokenizationStatus.ERROR.code) fit the API better here? Callers would get 'error' through the normal path instead of a thrown error.
2 participants
… promise is not cleared