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

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

Open
evelant opened this issue May 5, 2022 · 13 comments · Fixed by #17377
Labels
Development Builds Hermes needs review Issue is ready to be reviewed by a maintainer

Comments

@evelant
Copy link

evelant commented May 5, 2022

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.

@evelant evelant added the needs validation Issue needs to be validated label May 5, 2022
@evelant evelant changed the title SDK 45 with dev client and jsEngine: "hermes" loads android app bundle twice in development SDK 45 Critical dev client bug - with dev client and jsEngine: "hermes" loads android app bundle twice in development May 5, 2022
@evelant evelant changed the title SDK 45 Critical dev client bug - with dev client and jsEngine: "hermes" loads android app bundle twice in development SDK 45 Critical dev client bug - dev client with jsEngine: "hermes" loads android app bundle twice in development May 5, 2022
@ajsmth
Copy link
Contributor

ajsmth commented May 5, 2022

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

@evelant
Copy link
Author

evelant commented May 5, 2022

@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
Copy link
Contributor

hirbod commented May 5, 2022

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. #17282 (comment)

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
Copy link
Author

evelant commented May 5, 2022

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 #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
Copy link
Contributor

hirbod commented May 5, 2022

@lukmccall

@tcdavis
Copy link
Contributor

tcdavis commented May 6, 2022

@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.

lukmccall added a commit that referenced this issue May 6, 2022
# Why

Fixes #17355.
Remove reload workaround for the Hermes debugger. 

> Note:
if a user needs to inspect the right js bundle in flipper or devtools, they could reload manually.
lukmccall added a commit that referenced this issue May 6, 2022
# Why

Fixes #17355.
Remove reload workaround for the Hermes debugger. 

> Note:
if a user needs to inspect the right js bundle in flipper or devtools, they could reload manually.
@evelant
Copy link
Author

evelant commented May 10, 2022

@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
Copy link
Author

evelant commented May 10, 2022

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
Copy link
Author

evelant commented May 10, 2022

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

@evelant
Copy link
Author

evelant commented May 10, 2022

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

@evelant
Copy link
Author

evelant commented Apr 11, 2024

@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
Copy link
Author

evelant commented May 10, 2024

@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.

@lukmccall lukmccall reopened this May 12, 2024
@lukmccall lukmccall changed the title SDK 45 Critical dev client bug - dev client with jsEngine: "hermes" loads android app bundle twice in development SDK 51 Critical dev client bug - dev client with jsEngine: "hermes" loads android app bundle twice in development May 12, 2024
@expo-bot expo-bot added needs review Issue is ready to be reviewed by a maintainer and removed needs validation Issue needs to be validated labels May 13, 2024
@rikur
Copy link

rikur commented May 15, 2024

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 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development Builds Hermes needs review Issue is ready to be reviewed by a maintainer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants