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

RNTester: Crash loading <Image> page #3738

Closed
kmelmon opened this issue Dec 7, 2019 · 6 comments
Closed

RNTester: Crash loading <Image> page #3738

kmelmon opened this issue Dec 7, 2019 · 6 comments

Comments

@kmelmon
Copy link
Contributor

kmelmon commented Dec 7, 2019

The page in RNTester is having issues and crashes when you try to load it.

Callstack to the crash:

React.UWP.dll!winrt::to_hresult() Line 4298	C++
React.UWP.dll!winrt::terminate() Line 4372	C++
React.UWP.dll!std::experimental::coroutine_traits<winrt::fire_and_forget,react::uwp::ReactImage *,react::uwp::ImageSource>::promise_type::unhandled_exception() Line 7942	C++

React.UWP.dll!react::uwp::ReactImage::Source$_ResumeCoro$2() Line 112 C++
React.UWP.dll!react::uwp::ReactImage::Source$_InitCoro$1() Line 112 C++
React.UWP.dll!react::uwp::ReactImage::Source(react::uwp::ImageSource source) Line 112 C++
React.UWP.dll!react::uwp::ImageViewManager::setSource(winrt::Windows::UI::Xaml::Controls::Canvas canvas, const folly::dynamic & data) Line 162 C++
React.UWP.dll!react::uwp::ImageViewManager::UpdateProperties(react::uwp::ShadowNodeBase * nodeToUpdate, const folly::dynamic & reactDiffMap) Line 124 C++
React.UWP.dll!react::uwp::ShadowNodeBase::updateProperties(const folly::dynamic && props) Line 28 C++
React.UWP.dll!facebook::react::UIManager::createView(__int64 tag, std::string && className, __int64 __formal, folly::dynamic && props) Line 260 C++
React.UWP.dll!facebook::react::UIManagerModule::getMethods::__l2::(folly::dynamic args) Line 451 C++

Debug spew says:
Exception thrown at 0x764A35D2 (KernelBase.dll) in Playground.exe: WinRT originate error - 0x80070057 : 'legacy_image is not a valid absolute URI.'.
Exception thrown at 0x764A35D2 in Playground.exe: Microsoft C++ exception: winrt::hresult_invalid_argument at memory location 0x04AFC88C.

Exception thrown at 0x764A35D2 in Playground.exe: Microsoft C++ exception: [rethrow] at memory location 0x00000000.

Debugging back to the property setter, we have this:

  Name Value Type
sources[0] {uri="legacy_image" method="" bundleRootPath="ms-appx:///Bundle/" ...} react::uwp::ImageSource
  ▶ uri "legacy_image" std::string
  ▶ method "" std::string
  ▶ bundleRootPath "ms-appx:///Bundle/" std::string

So, we have an with a uri of "legacy_image" and that's failing to resolve to a valid resource in our appx package. It looks like we either aren't packaging image resources for RNTester properly, or aren't resolving the URI propertly.

@ghost ghost added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Dec 7, 2019
@kmelmon kmelmon added Area: Image Area: Infrastructure and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Dec 9, 2019
@kmelmon kmelmon added this to the MVP+ (vNext M5) milestone Dec 9, 2019
@kmelmon
Copy link
Contributor Author

kmelmon commented Dec 9, 2019

This may be a dupe of #2452

@kmelmon
Copy link
Contributor Author

kmelmon commented Dec 18, 2019

Marlene thinks the image won't load because the test was just copied from Android and the image is only part of the Android bundle.

@chrisglein chrisglein removed the vnext label Mar 18, 2020
@jonthysell
Copy link
Contributor

This test assumes that the host app has some kind of native image/resource and that legacy_image is a valid identifier.

For our code, we automatically resolve the whole uri to ms-appx://Bundle/legacy_image. If RNTester had its own proper host app, we could do either of the following:

  1. Include an image file named legacy_image in the Bundle folder, so that it gets resolved. OR
  2. Include an image resource with the identifier legacy_image and update our lookup code to look for that resource (can UWP apps even do that, or do we still need an ms-appx:// uri?)

However, we don't have a proper RNTester app, we have Playground. So we could do either of the above to the Playground app, but it would a weird dependency for Playground.

We should, however, not ever crash because of missing images. Is there a standard image not found image we could use?

@jonthysell
Copy link
Contributor

Ok, it looks like it's expected to be an unsafe operation, and there are different rules for iOS and Android: https://reactnative.dev/docs/images#images-from-hybrid-apps-resources

We should update the logic in ImageViewManager::setSource() to properly handle all of the supported image uri types:

  • Regular URI (just load)
  • Filesystem URI (just load)
  • Bundled asset URI (append bundle root)
  • Explicit package URI

I think that's all of them, but we should test the different configs (debug, release, with/without bundles, with/without assets)

@jonthysell
Copy link
Contributor

jonthysell commented Mar 23, 2020

For this particular bug, I think the right thing to do is not support a plain legacy_image identifier. We should just not load an image.

Potentially though we could add an example where we explicitly set an ms-appx:// uri and it should work.

@jonthysell
Copy link
Contributor

PR out to prevent crashing when trying to load invalid uris, just fail.

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

No branches or pull requests

3 participants