Remove jsc dep from blob native library #25720
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
approved these changes
Jul 19, 2019
There was a problem hiding this comment.
@mdvacca is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@janicduplessis The merge failed internally because BlobCollector.cpp depends on JSI: java/com/facebook/react/modules/blob/jni/BlobCollector.cpp:22: error: undefined reference to 'facebook::jsi::HostObject::~HostObject()' |
|
@mdvacca This is when building with BUCK right? Weird that it doesn't happen on oss CI. I tried updating the dep to jsi instead of jsiexecutor. Works when building locally with buck. |
|
Yes, this is when building with BUCK, I will take a deeper look later today and I will let you know if we need any change in the PR |
There was a problem hiding this comment.
@osdnk has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@osdnk I think the re-import failed. Could you import it again, try to fix up the BUCK file in phabricator and then land this PR? We need it before cutting 0.61. |
|
@janicduplessis Need to resolve conflict. |
|
the blob collector pr was reverted yesterday so we’ll have to figure that out first. |
|
Going to work on re-landing the blob pr with this fix included instead. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Sep 18, 2019
… 2 (#26155) Summary: Reland #24767 The commit had to be reverted because it caused a crash when using remote debugging in chrome. This is normal since jsi is not available in that environment. The crash was caused by `jsContext.get()` being 0, then being dereferenced later in c++. We can simply skip initializing the blob collector in this case. This also includes the fix from #25720 to fix a crash when using hermes. ## Changelog [Android] [Fixed] - Release underlying resources when JS instance is GC'ed on Android Pull Request resolved: #26155 Test Plan: Test using RN tester with jsc and hermes Test remote debugging Reviewed By: mdvacca, fred2028 Differential Revision: D17072644 Pulled By: makovkastar fbshipit-source-id: 079d1d43501e854297fbbe586ba229920c892584
mcuelenaere
pushed a commit
to getdelta/react-native
that referenced
this pull request
Sep 24, 2019
… 2 (facebook#26155) Summary: Reland facebook#24767 The commit had to be reverted because it caused a crash when using remote debugging in chrome. This is normal since jsi is not available in that environment. The crash was caused by `jsContext.get()` being 0, then being dereferenced later in c++. We can simply skip initializing the blob collector in this case. This also includes the fix from facebook#25720 to fix a crash when using hermes. [Android] [Fixed] - Release underlying resources when JS instance is GC'ed on Android Pull Request resolved: facebook#26155 Test Plan: Test using RN tester with jsc and hermes Test remote debugging Reviewed By: mdvacca, fred2028 Differential Revision: D17072644 Pulled By: makovkastar fbshipit-source-id: 079d1d43501e854297fbbe586ba229920c892584
grabbou
pushed a commit
that referenced
this pull request
Oct 11, 2019
… 2 (#26155) Summary: Reland #24767 The commit had to be reverted because it caused a crash when using remote debugging in chrome. This is normal since jsi is not available in that environment. The crash was caused by `jsContext.get()` being 0, then being dereferenced later in c++. We can simply skip initializing the blob collector in this case. This also includes the fix from #25720 to fix a crash when using hermes. ## Changelog [Android] [Fixed] - Release underlying resources when JS instance is GC'ed on Android Pull Request resolved: #26155 Test Plan: Test using RN tester with jsc and hermes Test remote debugging Reviewed By: mdvacca, fred2028 Differential Revision: D17072644 Pulled By: makovkastar fbshipit-source-id: 079d1d43501e854297fbbe586ba229920c892584
douglowder
pushed a commit
to react-native-tvos/react-native-tvos
that referenced
this pull request
Dec 11, 2019
… 2 (#26155) Summary: Reland facebook/react-native#24767 The commit had to be reverted because it caused a crash when using remote debugging in chrome. This is normal since jsi is not available in that environment. The crash was caused by `jsContext.get()` being 0, then being dereferenced later in c++. We can simply skip initializing the blob collector in this case. This also includes the fix from facebook/react-native#25720 to fix a crash when using hermes. ## Changelog [Android] [Fixed] - Release underlying resources when JS instance is GC'ed on Android Pull Request resolved: facebook/react-native#26155 Test Plan: Test using RN tester with jsc and hermes Test remote debugging Reviewed By: mdvacca, fred2028 Differential Revision: D17072644 Pulled By: makovkastar fbshipit-source-id: 079d1d43501e854297fbbe586ba229920c892584
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The blob native lib should not include JSC, otherwise it causes a crash when using Hermes.
This actually doesn't depend on the Hermes PR so we can land it now.
Changelog
[Android] [Fixed] - Remove jsc dep from blob native library
Test Plan
Tested that is no crash with hermes enabled and disabled