expo / expo

An open-source framework for making universal native apps with React. Expo runs on Android, iOS, and the web.
https://docs.expo.dev
MIT License
34.31k stars 5.48k forks source link

SDK 51 Critical dev client bug - dev client with jsEngine: "hermes" loads android app bundle twice in development #17355

Open evelant opened 2 years ago

evelant commented 2 years ago

Summary

On SDK45 beta if you use dev client and hermes then in development mode the app bundle gets loaded and run twice on the device.

This causes all sorts of havoc with development, breaking some native modules, some parts of logging, dev tooling, causing undefined behaviors, etc as well as potentially harming performance.

Managed or bare workflow? If you have ios/ or android/ directories in your project, the answer is bare!

managed

What platform(s) does this occur on?

Android

SDK Version (managed workflow only)

"expo": "~45.0.0-beta.4",

Environment

  expo-env-info 1.0.3 environment info:
    System:
      OS: macOS 12.3.1
      Shell: 3.3.1 - /opt/homebrew/bin/fish
    Binaries:
      Node: 16.14.2 - /opt/homebrew/opt/node@16/bin/node
      Yarn: 1.22.17 - /opt/homebrew/bin/yarn
      npm: 7.18.1 - /opt/homebrew/bin/npm
      Watchman: 2022.03.21.00 - /opt/homebrew/bin/watchman
    Managers:
      CocoaPods: 1.11.3 - /opt/homebrew/bin/pod
    SDKs:
      iOS SDK:
        Platforms: DriverKit 21.4, iOS 15.4, macOS 12.3, tvOS 15.4, watchOS 8.5
    IDEs:
      Android Studio: Bumblebee 2021.1.1 Patch 3 Bumblebee 2021.1.1 Patch 3
      Xcode: 13.3.1/13E500a - /usr/bin/xcodebuild
    npmPackages:
      expo: ~45.0.0-beta.4 => 45.0.0-beta.9
      react: 17.0.2 => 17.0.2
      react-dom: 17.0.2 => 17.0.2
      react-native: 0.68.1 => 0.68.1
      react-native-web: 0.17.1 => 0.17.1
    npmGlobalPackages:
      eas-cli: 0.52.0
      expo-cli: 5.4.3
    Expo Workflow: managed

Reproducible demo

https://github.com/evelant/expo_45_android_double_load

Expo SDK 45 with dev client and jsEngine: "hermes" loads the app bundle twice at startup in Android development mode. This causes all sorts of problems in anything beyond trivial apps.

To reproduce:

eas build -p android --profile development
//connect an android device
adb install ./build-123123123.apk
yarn start
//open app on device and connect to dev server

following these steps there will be output like

Android Bundling complete 23ms
loaded App.tsx
Android Bundling complete 52ms
Android Running app on SM-G965U
rendered App
loaded App.tsx
Android Running app on SM-G965U
rendered App

The app bundle is built twice and run twice on the device.

ajsmth commented 2 years ago

I think this is something we are already aware of - can you clarify what issues you are running into specifically?

evelant commented 2 years ago

@ajsmth It causes issues with native modules that don't expect to be called twice from the same running app. react-native-firebase emulator support is a big one affecting us. The double load seems to cause it to somewhat randomly lose authentication with the emulator suite leading to all firestore queries throwing permission-denied. That eats up a lot of time to then force close the app and reopen it until it works.

I think it may also be causing some dev mode app crashes with reanimated.

hirbod commented 2 years ago

That was just added to prevent some JSI race conditions crashes. For me, android isn't running anymore, I had to patch-package the reloads completely out of it. https://github.com/expo/expo/pull/17282#issuecomment-1118318522

Is there really no other way to fix hermes picking up the correct bundle without forced reloads? This feels so dirty. All of this happens cause the dev menu is an own bundle, right? (which would otherwise be picked up by devtools and flipper)

evelant commented 2 years ago

OK I can confirm that the bug is due to #17282 which forces the app to try to reload while it's already loading. Perhaps #17282 should be reverted until another solution can be found? Double loading the bundle to try to trick hermes/flipper into picking it up doesn't seem like a great solution. I think maybe a patch to hermes debugger or flipper to allow specifying which bundle it should target could possibly work better? I don't know enough about how those systems work to tell for sure though.

edit: actually it looks like the double load was introduced all the way back in https://github.com/expo/expo/pull/13041

Until then here is a patch to disable the double-load bug that can be used with patch-package

expo-dev-menu+0.10.5.patch

diff --git a/node_modules/expo-dev-menu/android/src/debug/java/expo/modules/devmenu/DevMenuManager.kt b/node_modules/expo-dev-menu/android/src/debug/java/expo/modules/devmenu/DevMenuManager.kt
index 3fbf7fc..b97b356 100644
--- a/node_modules/expo-dev-menu/android/src/debug/java/expo/modules/devmenu/DevMenuManager.kt
+++ b/node_modules/expo-dev-menu/android/src/debug/java/expo/modules/devmenu/DevMenuManager.kt
@@ -174,17 +174,6 @@ object DevMenuManager : DevMenuManagerInterface, LifecycleEventListener {
       devMenuHost = DevMenuHost(application)
       UiThreadUtil.runOnUiThread {
         devMenuHost.reactInstanceManager.createReactContextInBackground()
-
-        if (currentReactInstanceManager.get()?.jsExecutorName?.contains("Hermes") == true) {
-          // We have to switch thread to js queue to unload the event loop, otherwise, the app will crash.
-          currentReactInstanceManager.get()?.currentReactContext?.runOnJSQueueThread {
-            // Hermes inspector will use latest executed script for Chrome DevTools Protocol.
-            // It will be EXDevMenuApp.android.js in our case.
-            // To let Hermes aware target bundle, we try to reload here as a workaround solution.
-            // @see <a href="https://github.com/facebook/react-native/blob/0.63-stable/ReactCommon/hermes/inspector/Inspector.cpp#L231>code here</a>
-            currentReactInstanceManager.get()?.devSupportManager?.handleReloadJS()
-          }
-        }
       }
     }
   }
hirbod commented 2 years ago

@lukmccall

tcdavis commented 2 years ago

@evelant Thanks for the patch! We're figuring out how to address the issues being caused by reloading, but confirmation whether you see any difference in behavior between expo-dev-client@0.9.3 and 0.9.4 would be helpful.

evelant commented 2 years ago

@tcdavis @lukmccall This may still be an issue on 0.10.6 of dev-menu. Not sure where it's coming from but pressing r in the expo cli window to reload the app always results in what appears to be two bundles

› Reloading apps
Android Bundling complete 116ms
Android Bundling complete 121ms

I also see strange behavior in my app after reloads, things only work consistently upon on a complete force close and reopen of the android development app. Not sure where this is coming from yet but it seems there are deeper issues here with dev-client and hermes on SDK 45.

evelant commented 2 years ago

Another clue is that "production mode" does nothing, toggling production bundling on/off in the devtools has no effect. The CLI says it's now bundling in production mode but I get a dev bundle anyway.

evelant commented 2 years ago

A reload with r also does not include the latest code, it only loads an updated bundle after two reloads.

evelant commented 2 years ago

Hmm perhaps removing the double load actually uncovered this bug where app code doesn't get updated until 2 refreshes have happened?

evelant commented 7 months ago

@lukmccall Could this be reopened? It seems like it's still happening. Sometimes the android emulator just won't reload when pressing r in the terminal. Sometimes metro spits out one more Android Bundled 113ms (index.js) for each time you press r in the terminal (ex: 3rd reload says Android Bundled 4 times). Each reload gets slower. After a few reloads the app stops working and has to be force closed. This is especially problematic due to the bug with hermes profiler picking up the expo dev menu instead of profiling the app, you must reload 2x before trying to take a profile or you get a profile of the dev menu doing nothing in the background.

evelant commented 6 months ago

@ajsmth @tcdavis @lukmccall if anybody could please reopen this issue it would be appreciated. This is not fixed so I think it would be good to reopen so as to not lose context. This is probably related to other issues such a crashes on reloads, debugger not connecting, and sampling profiler picking up the dev menu instead of the app.

rikur commented 6 months ago

Since updating to SDK51, opening the DevMenu on iOS causes "Connecting to Metro" banner to appear. Furthermore, if I reload the app from the DevMenu, it only works once. After reloading, all the buttons (except Go home) are disabled and/or simply not responding.

Thought I'd mention it here if it's related. If not, I can create a new issue. Let me know.

Updating to expo@51.0.7 and friends resolved the iOS DevMenu issue 🙌

github-actions[bot] commented 3 months ago

This issue is stale because it has been open for 90 days with no activity. If there is no activity in the next 7 days, the issue will be closed.

rikur commented 1 week ago

Possibly related: https://github.com/expo/expo/issues/24104