Release underlying resources when JS instance is GC'ed on Android try 2 #26155

janicduplessis · 2019-08-22T19:41:57Z

Summary

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

Test Plan

Test using RN tester with jsc and hermes
Test remote debugging

janicduplessis · 2019-08-22T19:43:54Z

cc @makovkastar

makovkastar · 2019-08-23T11:40:59Z

RNTester/android/app/BUCK

@@ -25,6 +25,7 @@ rn_android_library(
        react_native_target("java/com/facebook/react:react"),
        react_native_target("java/com/facebook/react/common:build_config"),
        react_native_target("java/com/facebook/react/modules/core:core"),
+        react_native_target("java/com/facebook/react/views/text:text"),


Why was this dependency introduced?

I reverted 983ba63 to start this pr so it came from that. I’m not sure why it was it was added in the original commit. Should I just remove it?

Yeah, let's remove it if it's not needed.

facebook-github-bot

@makovkastar has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@makovkastar has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

makovkastar · 2019-08-27T16:03:00Z

@janicduplessis I've imported this PR, but it failed to compile when I tried to build with Buck:

stderr: xplat/js/react-native-github/ReactAndroid/src/main/java/com/facebook/react/modules/blob/jni/BlobCollector.cpp:22: error: undefined reference to 'facebook::jsi::HostObject::~HostObject()'
buck-out/gen/xplat/jsi/jsiAndroid#header-mode-symlink-tree-with-header-map,headers/jsi/jsi.h:0: error: undefined reference to 'vtable for facebook::jsi::HostObject'

Could you please take a look?

janicduplessis · 2019-08-27T18:49:31Z

@makovkastar I'm not able to reproduce the issue with the OSS buck setup, could you investigate this one? I did some tweaks to the buck file but I can't tell if it will fix the issue.

janicduplessis · 2019-08-27T18:50:23Z

The text addition in the buck file was actually fixing the buck build for RN tester, there are quite a fe w more things that need to be fixed for the oss buck build of RN tester to work so I will open a separate PR for that.

janicduplessis · 2019-09-09T23:34:31Z

@makovkastar Any change you can have a look at this again?

makovkastar · 2019-09-11T10:28:45Z

Hi @janicduplessis, I've been busy lately with some Fabric work and didn't have time to look into it. I will try to check it this evening, but no promises.

facebook-github-bot

@makovkastar has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

makovkastar · 2019-09-12T14:23:15Z

I made a small change in the BUCK file and it's building now. I'm waiting for the code review from one of the teams that was affected last time we landed this PR. If everything is OK, I will merge it on Monday.

mcuelenaere · 2019-09-13T14:25:12Z

Will this be backported to 0.60? This was a huge issue on our app and has caused a lot of lost engineering resources for us..

This merges with relatively little merge conflicts on 0.60-stable, see https://github.com/mcuelenaere/react-native/tree/backport/android-blob-gc-0.60

talha-apps · 2019-09-18T10:17:59Z

Any idea how soon can we get this? We have been waiting for this one for a long time. Will be great if someone can look at the broken tests so this can go in?

makovkastar · 2019-09-18T10:22:13Z

This PR was accepted and now there are some internal tests being run before merging this. I will try to get this merged today.

react-native-bot · 2019-09-18T11:55:07Z

This pull request was successfully merged by @janicduplessis in 9b2374b.

^{When will my fix make it into a release? | Upcoming Releases}

talha-apps · 2019-09-18T16:21:45Z

Thanks @makovkastar and @janicduplessis for resolving this. Looking forward to having this in the upcoming release. Any idea when that might be?

janicduplessis · 2019-09-18T17:10:04Z

@Talha-Ahmed I'll try to get it cherry-picked in the next 0.60.x release

… 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

mcuelenaere · 2019-09-24T09:28:36Z

To people who are interested in this, we are keeping track of these changes at https://github.com/getdelta/react-native/tree/0.60-patched (prebuilt Android binaries are available at https://github.com/getdelta/react-native/tree/0.60-patched-prebuilt)

talha-apps · 2019-10-07T11:20:36Z

This didn't get picked in 0.60.6. Can this please be released soon? @janicduplessis @makovkastar

… 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

… 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

Release underlying resources when JS instance is GC'ed on Android try 2

d909864

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. labels Aug 22, 2019

react-native-bot added Platform: Android Android applications. Bug labels Aug 22, 2019

makovkastar reviewed Aug 23, 2019

View reviewed changes

Update BUCK

3a81d84

facebook-github-bot reviewed Aug 27, 2019

View reviewed changes

Tweak buck file

2e44fb1

facebook-github-bot reviewed Sep 11, 2019

View reviewed changes

facebook-github-bot closed this in 9b2374b Sep 18, 2019

react-native-bot added the Merged This PR has been merged. label Sep 18, 2019

janicduplessis deleted the blob-dealloc-android-2 branch September 18, 2019 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release underlying resources when JS instance is GC'ed on Android try 2 #26155

Release underlying resources when JS instance is GC'ed on Android try 2 #26155

janicduplessis commented Aug 22, 2019 •

edited

Loading

janicduplessis commented Aug 22, 2019

makovkastar Aug 23, 2019

janicduplessis Aug 23, 2019

makovkastar Aug 26, 2019

janicduplessis Aug 27, 2019

facebook-github-bot left a comment

facebook-github-bot left a comment

makovkastar commented Aug 27, 2019

janicduplessis commented Aug 27, 2019

janicduplessis commented Aug 27, 2019

janicduplessis commented Sep 9, 2019

makovkastar commented Sep 11, 2019 •

edited

Loading

facebook-github-bot left a comment

makovkastar commented Sep 12, 2019

mcuelenaere commented Sep 13, 2019

talha-apps commented Sep 18, 2019

makovkastar commented Sep 18, 2019

react-native-bot commented Sep 18, 2019

talha-apps commented Sep 18, 2019

janicduplessis commented Sep 18, 2019

mcuelenaere commented Sep 24, 2019

talha-apps commented Oct 7, 2019 •

edited

Loading

Release underlying resources when JS instance is GC'ed on Android try 2 #26155

Release underlying resources when JS instance is GC'ed on Android try 2 #26155

Conversation

janicduplessis commented Aug 22, 2019 • edited Loading

Summary

Changelog

Test Plan

janicduplessis commented Aug 22, 2019

makovkastar Aug 23, 2019

Choose a reason for hiding this comment

janicduplessis Aug 23, 2019

Choose a reason for hiding this comment

makovkastar Aug 26, 2019

Choose a reason for hiding this comment

janicduplessis Aug 27, 2019

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

makovkastar commented Aug 27, 2019

janicduplessis commented Aug 27, 2019

janicduplessis commented Aug 27, 2019

janicduplessis commented Sep 9, 2019

makovkastar commented Sep 11, 2019 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

makovkastar commented Sep 12, 2019

mcuelenaere commented Sep 13, 2019

talha-apps commented Sep 18, 2019

makovkastar commented Sep 18, 2019

react-native-bot commented Sep 18, 2019

talha-apps commented Sep 18, 2019

janicduplessis commented Sep 18, 2019

mcuelenaere commented Sep 24, 2019

talha-apps commented Oct 7, 2019 • edited Loading

janicduplessis commented Aug 22, 2019 •

edited

Loading

makovkastar commented Sep 11, 2019 •

edited

Loading

talha-apps commented Oct 7, 2019 •

edited

Loading