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.

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

iOS: Clear Linking.getInitialURL during bridge reload #22659

Closed
wants to merge 3 commits into from

Conversation

venik
Copy link

@venik venik commented Dec 15, 2018

On iOS platform, RN retains launchOptions dictionary after bridge reload which can lead to unexpected consequences to a developer. The app will receive the same value for Linking.getInitialURL during initial launch and during bridge reload. Here's an example from our application. We use deeplinks via custom URL scheme so a user can open the app via link. Also, we reload the bridge when a user signs out. So if a user opens the app via URL, logs out, and a second user logs into the app, the app will behave as though the second user launched the app via the same deeplink. Because reload destroys the JS engine, there's nowhere for our app to remember that it already handled the deeplink activation.

On iOS Linking.getInitialURL() gets URL from the _launchOptions dictionary, so by setting it to nil we prevent retention of initialURL after reload.

This change makes iOS's behavior consistent with Android's. On Android, the launch URL is stored on the Intent and reloading the app involves creating a new Intent. Consequently, the launch URL is dropped as desired during the reload process.

Test plan:

I wrote a toy application to show the problem
https://github.com/venik/launchUrlExample/blob/fa8542b57b4331ee6be226db24048efe0d957b3e/App.js

Repro Steps

  1. Ensure the app isn't running (swipe it out if it is running)
  2. Open Safari, type in the URL mypoc://some_text, and hit enter.
  3. OS asks if you want to open the app. Choose to open the app.
  4. Tap on the button. Alert dialog shows "url: mypoc://some_text"
  5. Now reload the app (cmd+R). This simulates an app in production reloading the bridge.
  6. Tap the button again

Unexpected result (i.e. before this fix): Alert dialog shows "url: mypoc://some_text"

Expected result (i.e. after this fix): Alert dialog shows "url: null"

Here's the code from my toy application which repros the bug:

export default class App extends Component {
  render() {
    return (
      <View style={styles.container}>
        <Button
          onPress={
            () => {
              Linking.getInitialURL().then((url) => {
                AlertIOS.alert('url: ' + url);
              }).catch(err => console.error('An error occurred', err));
            }
          }
          style={styles.welcome}
          title="Learn More"
          accessibilityLabel="Learn more about this purple button"
        />
      </View>
    );
  }
}

Changelog:

[iOS] [Fixed] [RCTBridge] - Set _launchOptions to nil when app reloads and treat reload as a fresh start. Do not respect previous launchOptions, so Linking.getInitialURL() will return null after reload.

@venik venik requested a review from shergin as a code owner December 15, 2018 01:15
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 15, 2018
@pull-bot
Copy link

Warnings
⚠️

📋 Changelog - This PR may have incorrectly formatted Changelog.

Generated by 🚫 dangerJS

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@cpojer
Copy link
Contributor

cpojer commented Jan 2, 2019

This makes sense to me. Thank you for submitting a pull request with a fix and providing an extensive test plan :)

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jan 2, 2019
@cpojer
Copy link
Contributor

cpojer commented Jan 2, 2019

Ah, it seems like at FB we are using a warning about retaining references in blocks:

js/react-native-github/React/Base/RCTBridge.m:292:5: error: block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior [-Werror,-Wimplicit-retain-self]
    _launchOptions = nil;

Do you mind fixing up your PR to remove this warning? Thank you!

@venik
Copy link
Author

venik commented Jan 3, 2019

I will fix and resubmit soon. Thanks

@venik
Copy link
Author

venik commented Jan 8, 2019

@cpojer I fixed the PR and tested locally against POC application. It doesn't show the warning now.

Thanks in advance.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

Alexander Nikiforov merged commit 19d04a3 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Jan 15, 2019
@react-native-bot react-native-bot added the Merged This PR has been merged. label Jan 15, 2019
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019
matt-oakes pushed a commit to matt-oakes/react-native that referenced this pull request Feb 7, 2019
Summary:
On iOS platform, RN retains launchOptions dictionary after bridge reload which can lead to unexpected consequences to a developer. The app will receive the same value for `Linking.getInitialURL` during initial launch and during bridge reload. Here's an example from our application. We use deeplinks via custom URL scheme so a user can open the app via link. Also, we reload the bridge when a user signs out. So if a user opens the app via URL, logs out, and a second user logs into the app, the app will behave as though the second user launched the app via the same deeplink. Because reload destroys the JS engine, there's nowhere for our app to remember that it already handled the deeplink activation.

On iOS Linking.getInitialURL() gets URL from the _launchOptions dictionary, so by setting it to nil we prevent retention of initialURL after reload.

This change makes iOS's behavior consistent with Android's. On Android, the launch URL is stored on the `Intent` and reloading the app involves creating a new `Intent`. Consequently, the launch URL is dropped as desired during the reload process.
Pull Request resolved: facebook#22659

Differential Revision: D13564251

Pulled By: cpojer

fbshipit-source-id: 4c6d81f1775eb3c41b100582436f1c0f1ee6dc36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API: Linking CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants