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

[expo-modules-core][1.5.6] Crash when launching project through dev-client #23451

Closed
thespacemanatee opened this issue Jul 11, 2023 · 12 comments · Fixed by #23491
Closed

[expo-modules-core][1.5.6] Crash when launching project through dev-client #23451

thespacemanatee opened this issue Jul 11, 2023 · 12 comments · Fixed by #23491
Assignees

Comments

@thespacemanatee
Copy link
Contributor

thespacemanatee commented Jul 11, 2023

Minimal reproducible example

https://github.com/thespacemanatee/expo-dev-menu-repro/tree/regression/expo-modules-core-url-crash

Summary

There is a regression caused by (EDIT: #23405). I'm on expo@49.0.2 and expo-modules-core@1.5.6 and on Debug builds I'm getting the following error after launching the dev-client via deep-link or through the UI:

Thread 7 Crashed::  Dispatch queue: com.apple.NSURLSession-delegate
0   libswiftCore.dylib            	       0x18bcdc794 closure #1 in closure #1 in closure #1 in _assertionFailure(_:_:file:line:flags:) + 288
1   libswiftCore.dylib            	       0x18bcdc544 closure #1 in closure #1 in _assertionFailure(_:_:file:line:flags:) + 196
2   libswiftCore.dylib            	       0x18bcdc43c closure #1 in _assertionFailure(_:_:file:line:flags:) + 328
3   libswiftCore.dylib            	       0x18bcdc034 _assertionFailure(_:_:file:line:flags:) + 168
4   Padlet                        	       0x10450ba80 closure #1 in FileDownloader.downloadData(withRequest:successBlock:errorBlock:) + 1588 (FileDownloader.swift:1016)
5   Padlet                        	       0x10450e2e4 partial apply for closure #1 in FileDownloader.downloadData(withRequest:successBlock:errorBlock:) + 44
6   Padlet                        	       0x10450c0e4 thunk for @escaping @callee_guaranteed @Sendable (@guaranteed Data?, @guaranteed NSURLResponse?, @guaranteed Error?) -> () + 280
7   Padlet                        	       0x1047e608c __InstrumentDataTaskWithRequestCompletionHandler_block_invoke_2 + 252 (FPRNSURLSessionInstrument.m:307)
8   Padlet                        	       0x1047e608c __InstrumentDataTaskWithRequestCompletionHandler_block_invoke_2 + 252 (FPRNSURLSessionInstrument.m:307)
9   CFNetwork                     	       0x183d30cf8 0x183d2a000 + 27896
10  CFNetwork                     	       0x183d4c170 0x183d2a000 + 139632
11  libdispatch.dylib             	       0x180132ee4 _dispatch_call_block_and_release + 24
12  libdispatch.dylib             	       0x180134708 _dispatch_client_callout + 16
13  libdispatch.dylib             	       0x18013c77c _dispatch_lane_serial_drain + 776
14  libdispatch.dylib             	       0x18013d414 _dispatch_lane_invoke + 448
15  libdispatch.dylib             	       0x180149608 _dispatch_workloop_worker_thread + 768
16  libsystem_pthread.dylib       	       0x1b1834878 _pthread_wqthread + 284
17  libsystem_pthread.dylib       	       0x1b183363c start_wqthread + 8

FileDownLoader.swift is in the expo-updates project, and on this particular line, an URL is created, but fails and goes to the errorBlock on line 1016.

Release build is not affected.

Environment

expo-env-info 1.0.5 environment info:
    System:
      OS: macOS 13.3.1
      Shell: 5.9 - /bin/zsh
    Binaries:
      Node: 16.15.0 - ~/.asdf/installs/nodejs/16.15.0/bin/node
      Yarn: 1.22.19 - ~/.asdf/installs/nodejs/16.15.0/bin/yarn
      npm: 8.5.5 - ~/.asdf/plugins/nodejs/shims/npm
      Watchman: 2023.04.17.00 - /opt/homebrew/bin/watchman
    Managers:
      CocoaPods: 1.12.1 - /Users/honeycomb/.asdf/shims/pod
    SDKs:
      iOS SDK:
        Platforms: DriverKit 22.4, iOS 16.4, macOS 13.3, tvOS 16.4, watchOS 9.4
    IDEs:
      Android Studio: Hedgehog 2023.1.1 Canary 11 Hedgehog 2023.1.1 Canary 11
      Xcode: 14.3/14E222b - /usr/bin/xcodebuild
    npmPackages:
      expo: ~49.0.2 => 49.0.2 
      react: 18.2.0 => 18.2.0 
      react-native: 0.72.1 => 0.72.1 
    npmGlobalPackages:
      eas-cli: 3.13.3
      expo-cli: 6.3.7
    Expo Workflow: bare
@thespacemanatee thespacemanatee added the needs validation Issue needs to be validated label Jul 11, 2023
@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 Jul 11, 2023
@alanjhughes
Copy link
Collaborator

alanjhughes commented Jul 11, 2023

@thespacemanatee - Unclear to me how this related to that PR? data.write(to: URL(fileURLWithPath: destinationPath) is a system api. It has nothing to do with expo-modules-core.

@thespacemanatee
Copy link
Contributor Author

thespacemanatee commented Jul 11, 2023

@alanjhughes it's unclear to me too, however the PR modifies URL in Convertibles.swift (see here) which is an extension of URL in Foundation, so it definitely has something to do with expo-modules-core. I've given the repro as well so it should be fairly straightforward to figure out what was the actual regression between 1.5.4 and 1.5.6.

@alanjhughes
Copy link
Collaborator

alanjhughes commented Jul 11, 2023

Yes and convertibles are used when converting arguments from Javascript. It's not being used here. Extensions don't affect the behaviour of default initializers. I've cloned your example and ran it without issue

Simulator Screenshot - iPhone 14 Pro - 2023-07-11 at 09 18 25

@thespacemanatee
Copy link
Contributor Author

Did you clone the correct branch? Check expo-modules-core-url-crash

@thespacemanatee
Copy link
Contributor Author

thespacemanatee commented Jul 11, 2023

@alanjhughes I'll add that doing

"resolutions": {
  "expo-modules-core": "1.5.5"
}

fixes it, so it should be a regression caused 1.5.6 instead, sorry for the mistake. It should be caused by #23405 @Kudo

@alanjhughes
Copy link
Collaborator

@thespacemanatee - I've reproduced it. I'll look into it 👍

@alanjhughes alanjhughes added Issue accepted and removed needs review Issue is ready to be reviewed by a maintainer labels Jul 11, 2023
@alanjhughes alanjhughes self-assigned this Jul 11, 2023
@expo-bot
Copy link
Collaborator

Thank you for filing this issue!
This comment acknowledges we believe this may be a bug and there’s enough information to investigate it.
However, we can’t promise any sort of timeline for resolution. We prioritize issues based on severity, breadth of impact, and alignment with our roadmap. If you’d like to help move it more quickly, you can continue to investigate it more deeply and/or you can open a pull request that fixes the cause.

@CruelMoney
Copy link

@alanjhughes I'll add that doing

"resolutions": {
  "expo-modules-core": "1.5.5"
}

fixes it, so it should be a regression caused 1.5.6 instead, sorry for the mistake. It should be caused by #23405 @Kudo

This fixed the issue for me as well.

@Kudo Kudo mentioned this issue Jul 12, 2023
3 tasks
@jozzzaa
Copy link

jozzzaa commented Jul 12, 2023

@alanjhughes I'll add that doing

"resolutions": {
  "expo-modules-core": "1.5.5"
}

fixes it, so it should be a regression caused 1.5.6 instead, sorry for the mistake. It should be caused by #23405 @Kudo

I am having issues with this resolution. I have added to package.json, deleted node_modules and reinstalled and rebuilt my dev client on EAS. Any idea how to get this workaround to work?

@davecalnan
Copy link

@alanjhughes I'll add that doing

"resolutions": {
  "expo-modules-core": "1.5.5"
}

fixes it, so it should be a regression caused 1.5.6 instead, sorry for the mistake. It should be caused by #23405 @Kudo

Note that if you're using pnpm with a monorepo you will need to add this to the root package.json:

{
  "resolutions": {
    "expo-modules-core": "1.5.5"
  }
}

This is an alias for pnpm.overrides so this is equivalent to:

{
  "pnpm": {
    "overrides": {
      "expo-modules-core": "1.5.5"
    }
  }
}

Run pnpm install and you should see a change to pnpm-lock.yaml. I found using pnpm why expo-modules-core helpful to check which version was being installed.

See https://pnpm.io/package_json#pnpmoverrides

@tsapeta
Copy link
Member

tsapeta commented Jul 12, 2023

While resolutions/overrides are sometimes ok as workarounds, please keep in mind that this is just a temporary thing and should eventually be removed when you upgrade the expo package. expo-modules-core is not intended to be a direct dependency of the project – expo package should be the only one that controls its version. Otherwise, this may lead to some other issues and dependency conflicts. In this specific case, downgrading the expo package would be a better choice than overriding expo-modules-core.

By the way, expo@49.0.3 that should fix this problem has been released 🥳

@thespacemanatee
Copy link
Contributor Author

@tsapeta agree. I only posted the overrides 'solution' to bring attention to the fact that 1.5.6 introduced a regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants