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

[android][web-browser] Fix WebBrowser sending 'dismiss' before opening #6743

Conversation

LucaColonnello
Copy link
Contributor

Why

Fixes #6679

How

#6679 (comment)

TL;DR
A wrong usage of AppState causes WebBrowser to resolve with
{ type: 'dismiss' } before opening the view.

This is related to AppState being not initialised yet,
triggering a change before the user leaves the app view
to focus on the browser view.

Test Plan

As this can be tested only in production (the bug does not happen in development), testing is hard to achieve.
I tested it in the app I'm building, other people (see comments in the issue) did the same, linking their forked package with the patch applied to their apps, and it seems to work fine.

@LucaColonnello
Copy link
Contributor Author

I'm looking for ways to test this in E2E 🤞
Open to suggestions!

@LucaColonnello
Copy link
Contributor Author

image
Tests are having issues in CI...

@LucaColonnello
Copy link
Contributor Author

@mczernek I'm unfortunately blocked on this one.
Could I get some support? I don't know how to unlock this CI error.
I read the contributor guidelines document but I can't find anything relevant...

@lukmccall
Copy link
Contributor

Hi @LucaColonnello, to fix the problem with SDK, you need to run yarn build in the expo-web-browser folder.

An issue with the client is strange. Maybe rebase help.

Moreover, please add an entry to CHANGELOG.md.

@LucaColonnello LucaColonnello force-pushed the @LucaColonnello/fix-web-browser-app-state-init branch from 8617d93 to 49f9b89 Compare March 17, 2020 19:56
@LucaColonnello
Copy link
Contributor Author

@lukmccall @mczernek @sjchmiela Can I ask for a re review here please? :)
I added the CHANGELOG.md and managed to do get the CI passing (all but the client 🤷‍♂)

Thanks in advance!

Copy link
Contributor

@lukmccall lukmccall left a comment

Choose a reason for hiding this comment

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

Please don't bother about failing CI job - this particular job always fails to people from community.

I'm not sure why you have changes in yarn.lock. Did you rebase your branch?

@@ -149,7 +149,18 @@ let _redirectHandler: ((event: RedirectEvent) => void) | null = null;
// returns to active
let _onWebBrowserCloseAndroid: null | (() => void) = null;

// Store previous app state to check whether the listener has ever been attached
let _previousAppState: null | string = AppState.currentState;
Copy link
Contributor

Choose a reason for hiding this comment

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

It'll be better if we change it to bool and rename it. We don't have to remember the previous app state. We need information if it the first call or not.

Fixes expo#6679

TL;DR
A wrong usage of AppState causes WebBrowser to resolve with
`{ type: 'dismiss' }` before opening the view.

This is related to AppState being not initialised yet,
triggering a change before the user leaves the app view
to focus on the browser view.
@LucaColonnello LucaColonnello force-pushed the @LucaColonnello/fix-web-browser-app-state-init branch from 8822e92 to e8190f1 Compare March 22, 2020 10:44
Copy link
Contributor

@lukmccall lukmccall left a comment

Choose a reason for hiding this comment

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

LGTM 🥇I'll merge when you fix one more thing ;)

packages/expo-web-browser/CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: Łukasz Kosmaty <lukasz.kosmaty@student.uj.edu.pl>
@LucaColonnello
Copy link
Contributor Author

🎉
Can someone help testing this in an Android machine?
Unfortunately my machine has some issues with Android VM.

@cruzach
Copy link
Contributor

cruzach commented Mar 24, 2020

This will resolve with {type: 'dismissed'} even if the browser was just backgrounded, and is still opened (which is a bit confusing, since the iOS side only resolves with {type: 'dismissed'} if the tab is closed, like with dismissBrowser)

It seems that unless we can know whether the browser is actually closed, or if it's backgrounded, then we're better off just returning {type: 'opened'}

@LucaColonnello
Copy link
Contributor Author

Hey @cruzach can you please explain your point better? Unfortunately I do not understand why it would result in dismiss.

Only thing I’m tackling here is that there’s no guarantee to have an initial event from AppState triggered ONLY when you do change focus.

The AppState could trigger the first time is currentState is called even if no change occurred.

How is this change impacting the current feature as you say?

Thanks in advance :)

@cruzach
Copy link
Contributor

cruzach commented Mar 25, 2020

Sorry, I didn't mean to indicate this PR was negatively impacting the feature at all.

Since, with a Chrome custom tab, we have no way of knowing whether the web browser was closed, or backgrounded, we state in the docs that:

On Android promise resolves with {type: 'opened'} if we were able to open browser.

I think we should revert back to this behavior, since resolving to {type: 'dismiss'} indicates that the web browser was successfully closed, which is the behavior on the iOS side:

On iOS: if the browser is closed using dismissBrowser , the Promise resolves with { type: 'dismiss' }.

The best option would be to tell whether the chrome custom tab was closed or not, but I'm not sure it's possible

@brentvatne brentvatne self-requested a review March 25, 2020 22:23
@brentvatne
Copy link
Member

brentvatne commented Mar 25, 2020

I can't reproduce the issue described here on SDK36 on Android.

import React from "react";
import { AppState, Button, StyleSheet, Text, View } from "react-native";
import * as WebBrowser from "expo-web-browser";

export default function App() {
  return (
    <View style={styles.container}>
      <Text>{AppState.currentState}</Text>
      <Button
        title="Press me"
        onPress={() => {
          WebBrowser.openAuthSessionAsync(
            "https://expo.io",
            "exp://127.0.0.1:19000"
          );
        }}
      />
    </View>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    backgroundColor: "#fff",
    alignItems: "center",
    justifyContent: "center"
  }
});
  1. the AppState.currentState text is initialized to active, which is what is to be expected for React Native 0.61 (the docs appear to be outdated and incorrect - see https://github.com/facebook/react-native/blob/master/Libraries/AppState/AppState.js#L42
  2. I added logs to the app state change function that you modified and it fired with the values I expected: first background and then active when I return to the app.

I tried this inside of Expo client and in a standalone app (download apk here)

Can you explain how I can reproduce this problem on SDK36?

@brentvatne brentvatne added needs more info To be used when awaiting reporter response needs repro labels Mar 25, 2020
@thisBrian
Copy link
Contributor

thisBrian commented Mar 26, 2020

@brentvatne

The reason is because you call AppState.currentState explicitly before opening the browser.
The problem will occur were you to start the auth flow directly and see if it resolves with AppState.currentState being omitted.

For the past few months I have been using this proposed fix in my own app and found it resolve the issue.

@LucaColonnello
Copy link
Contributor Author

Are we ok to merge this @lukmccall? :)

@brentvatne
Copy link
Member

brentvatne commented Mar 28, 2020

@thisBrian - that doesn't make any sense to me, i must be missing something -- accessing the property does not initialize it. it is initialized in the AppState constructor as i linked to. i would need more information on exactly what the problem is and how i can reproduce this before i'm comfortable merging

@thisBrian
Copy link
Contributor

@brentvatne

Apologies if it didn't make sense, let me clarify: your code block where you read from AppState prior to launching the auth session has side effects that won't reproduce the bug.

To demonstrate I forked your helloworld repo and quickly added a simple auth flow using reddit as an example.

  • Here is the bug where the first attempt silently fails, yet subsequent attempts succeed
  • Here is the same code but with the side effect of having read AppState.currentState prior to launching the browser.

The same holds true for any helper classes that use WebBrowser (such as AuthSession).

I hope that clears it up enough to merge the fix by @LucaColonnello, cheers!


Here's an excerpt from the aformentioned fork

import { AuthSession } from 'expo';
import React from 'react';
import { Button, StyleSheet, View } from 'react-native';
import { getAuthorizeUrl } from './helpers';

export default function App() {
  return (
    <View style={styles.container}>
      <Button
        title="Press me"
        onPress={async () => {
          const returnUrl = 'helloworld://any';
          const authUrl = getAuthorizeUrl(
            {
              client_id: 'INSERT_YOUR_CLIENT_ID', // https://www.reddit.com/prefs/apps/ + match redirect Url with aformentioned returnUrl
              duration: 'temporary',
              redirect_uri: returnUrl,
              response_type: 'code',
              scope: ['identity'],
            },
            '.compact'
          );
          const result = await AuthSession.startAsync({
            authUrl,
            returnUrl,
          }).catch(alert);
          alert(JSON.stringify(result));
        }}
      />
    </View>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    backgroundColor: '#fff',
    alignItems: 'center',
    justifyContent: 'center',
  },
});

@LucaColonnello
Copy link
Contributor Author

LucaColonnello commented Mar 29, 2020

@brentvatne I understand where you're coming from, it is misleading.
I'll try make it clearer.

The ReactContext LifecycleEventListener, used by AppState, triggers an onHostResume event when you add the first ever listener, according to this code.
This happens just before opening the browser. Because a change event is triggered, WebBrowser thinks that you returned from the browser to the app cancelling, and returns dismiss.

(read my comment here for details of why this is important in the context of AuthSession #6679 (comment), although lines have changed since then in ReactNative)

You're right to say that if that is the case it should not matter as AppState constructor creates a listener as soon as an instance is created, but there's a detail you're missing.

In the context of ReactNative, all modules are exported as getter of the module.exports object, requiring such modules only when and if required (most probably for performance reasons).
Here's the code responsible for that, you can see AppState being exported in such way at line 294.

The above might or might not have relevance, as WebBrowser imports AppState as well, so in theory should be the same as you importing it in your app. That said, depends on how Metro bundler is running those modules (which is unclear to me, I haven't seen a JS build from Metro yet)

I'm investigating what could be in this that makes it work when you call AppState.currentState in your app. That said, by testing and logging inside this module (from within node_modules), I can tell that the event listener gets called initially before opening the browser, which triggers the dismiss.

I'll keep digging as it's weird, also I think to fix this the code should be changed as the original commit, as the suggested change, which I made, checks for null as opposed to checking whether the state is changed or not based on the previous state (when I made the change as requested I forgot about this detail)...

@LucaColonnello
Copy link
Contributor Author

@thisBrian could you tell me how to reproduce this in a virtual machine if you did this in a VM to take the videos?
I was only able to debug this in a real device (old Android), but unfortunately I don't have it with me anymore...

Thanks :)

@thisBrian
Copy link
Contributor

@LucaColonnello

Oh I had used a screen recorder for those particular videos; however Expo has some good docs on setting up a virtual device.

Alternatively if you want a quick and dirty virtual Android device to run the apk, you could use BlueStacks too. I have tested it with both virtual variations and they could reproduce the bug.

Apologies, if I was able to get to the office I would upload my docker image that has it all set up.

@lukmccall
Copy link
Contributor

@LucaColonnello, I'm sorry it took so long. However, I think all doubts were resolved. So, we almost ready to merge it 🎉Please, just resolve conflicts and it will be good to go.

@vernondegoede
Copy link

vernondegoede commented Apr 15, 2020

Hey guys,

We ran into the same issue with expo-web-browser. When running openAuthSessionAsync from expo-web-browser, we always get returned to the app with {"type": "dismiss"}. This only happens on Android, both for successful and unsuccessful authentication attempts.

For us the reason this failed was because of a change in Linking.makeUrl that was introduced in SDK 37. I'll create a separate issue for that.

Copy link
Member

@brentvatne brentvatne left a comment

Choose a reason for hiding this comment

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

still not totally sure why this is happening but was able to repro it in a prod build of the app and not in development. thanks for the patience!

@brentvatne brentvatne merged commit 3686e36 into expo:master Apr 16, 2020
brentvatne added a commit that referenced this pull request Apr 16, 2020
#6743)

* [android][web-browser] Fix WebBrowser sending 'dismiss' before opening

Fixes #6679

TL;DR
A wrong usage of AppState causes WebBrowser to resolve with
`{ type: 'dismiss' }` before opening the view.

This is related to AppState being not initialised yet,
triggering a change before the user leaves the app view
to focus on the browser view.

* Build packages

* Updated CHANGELOG

* Store only if AppState is available instead of every state change

* Clean yarn.lock

* Update packages/expo-web-browser/CHANGELOG.md

Co-Authored-By: Łukasz Kosmaty <lukasz.kosmaty@student.uj.edu.pl>

Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@student.uj.edu.pl>
Co-authored-by: Brent Vatne <brentvatne@gmail.com>
@brentvatne
Copy link
Member

this is available in expo-web-browser@8.1.1

@Rohansi
Copy link

Rohansi commented Apr 17, 2020

this is available in expo-web-browser@8.1.1

Any way to use that in an SDK 37 app? I tried pulling it but it wasn't fixed, and expo installing it only gives 8.1.0. Maybe I need to build standalone again?

@brentvatne
Copy link
Member

if you already have expo-web-browser in your app 8.1.0 matches ~8.1.0 which is what is installed by expo install. if you remove it and then install it again you will get the latest version, or just run yarn add expo-web-browser@8.1.1

@Rohansi
Copy link

Rohansi commented Apr 17, 2020

Yeah, I installed expo-web-browser@8.1.1 and rebuilt the standalone app. Still immediately get a dismiss the first time around.

Expo CLI also said There were no new changes from the last build, you can download that build from here which makes me think these dependencies are bundled separately from my app's code?

@simplesthing
Copy link

I am getting this issue with expo-web-browser 8.2.1

@thisBrian
Copy link
Contributor

@simplesthing It seems you got your issue resolved. btw the ./helpers and getAuthorizeUrl were reddit-specific implementations for the demo to reproduce the bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more info To be used when awaiting reporter response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AuthSession returns dismiss result even before the browser is opened
8 participants