Repository navigation
Conversation
When a deep link into the app's singleTask activity clears the controller off the task, it is destroyed only later. A close() in that window creates a new BrowserControllerActivity, whose onCreate ran the open() listener again and launched the last URL a second time. If the old controller's onDestroy cleared the listener first, the new one launched nothing and stayed on top of the app. - finish at once in onCreate when started with the close extra - make the open() listener one-shot - onDestroy forgets only its own instance, not the listener - set the listener before startActivity(), and make both statics volatile Fixes ionic-team#2610
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
On Android,
Browser.close()can re-open the URL from the lastBrowser.open(), and sometimes leaves aBrowserControllerActivitythat launched nothing stuck on top of the app. This PR fixes the controller's lifecycle handling so neither can happen:BrowserControllerActivity.onCreatefinishes at once when it was started with thecloseextra.close()only creates a new instance when the previous one has already been cleared off the task (by a deep link into the app'ssingleTaskactivity) but not destroyed yet, so there is nothing left to close.open()is one-shot: the controller that serves it clears it, so no later controller can run the sameopen()again.onDestroyforgets only its own instance (onControllerDestroyed) and leaves the listener alone. By the time an old controller is destroyed, the listener may already belong to a neweropen()whose controller hasn't been created yet; clearing it is what left that controller with nothing to launch.open()sets the listener beforestartActivity(), and both statics arevolatile, since they are written on the plugin thread and read on the main thread.No public API changes:
setBrowserControllerListener()and thebrowserControllerListenerfield are still there.Fixes #2610
Change Type
Rationale / Problems Fixed
This is the usual OAuth sign-in or sign-out flow:
Browser.open()a URL that redirects back to the app's custom scheme, and callBrowser.close()fromappUrlOpen. When the redirect comes back while the Custom Tab is still starting (about 0.5 s on an emulator, which a fast or local auth server easily beats), the deep link clears the old controller off the task, but Android destroys it only later. Aclose()in that window creates a new controller. ItsonCreateignored thecloseextra and ran the still-set listener, which launched the last URL again. The app then receives every deep link twice; in our app, every Android sign-out sent the OIDC end-session request twice.When the old controller's
onDestroycleared the listener betweenclose()starting a new controller and that controller'sonCreate, the new controller launched nothing and stayed on top of the app. It is translucent, so the app looks normal, but taps no longer reach it, the nextBrowser.open()opens nothing and its promise never settles, and only Back gets rid of it.The full trace, with logcat, is in #2610.
Tests or Reproductions
Minimal repro: https://github.com/pramoth/capacitor-browser-close-repro. It uses a local 302 server plus
adb reverse, so the redirect comes back fast enough; the steps are in its README.On an API 35 emulator (Chrome 124),
@capacitor/browserfrom this branch, installed vianpm pack:close()appUrlOpenWith this PR,
close()still landed in the window on 55 of the 60 taps: logcat shows it creating a newBrowserControllerActivity(result code=0), and that instance now finishes without launching anything. An earlier, longer run of the same patch on 8.0.5 gave 150 deep links for 150 taps at 0, 80 and 100 ms. Unpatched, we saw 2 stuck controllers in about 180 taps, both at 80–100 ms; they are too rare to show up in a 10-tap baseline. Stuck controllers were checked withdumpsys activity activitiesafter every tap.A normal
Browser.close()of a Custom Tab that is still showing (https://capacitorjs.com) still closes it and returns to the app, andbrowserFinishedfires once per close (3 of 3 rounds).npm run verify:androidandnpm run eslint/prettier --checkpass inbrowser/.Screenshots / Media
Platforms Affected
Notes / Comments
iOS isn't affected:
SFSafariViewControllerstays presented untilclose(), so there is no equivalent window.#2095 (closed, no repro) describes a
Browser.open()shortly after returning through a deep link that closes again at once, with a 1.5 ssetTimeoutas the workaround. That timing fits the same "old controller not destroyed yet" window, but I haven't verified that it's the same cause.🤖 Generated with Claude Code