Android: fix JSC crash in dev #11804
Conversation
…looping over object keys
facebook-github-bot
added
CLA Signed
|
Tracing back the native (c++) execution suggests that the error occurs in the JIT-ed code. Because JIT is disabled on iOS, this bug cannot occur there. Additionally, the fact that the crash only occurs when many calls are made points towards a bug in optimizations within JavascriptCore. If we can reproduce the problem outside of react native it would be interesting to port back upstream. |
| for (var key in object) { | ||
| var keys = Object.keys(object); | ||
|
|
||
| for (var i = 0; i < keys.length; i++) { |
There was a problem hiding this comment.
Because you copied the keys from the object, i.e.: there is no direct reference anymore, I think you can still use the enhanced for loop. So it could look like this:
var keys = Object.keys(object); for (var key in keys) { if (object.hasOwnProperty(key)) { object.__defineGetter__(key, identity.bind(null, object[key])); ... }}
There was a problem hiding this comment.
key will be a numeric array index in your code.
|
+1 |
|
ok let’s try this out |
facebook-github-bot
added
the
Import Started
|
@davidaurelio has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
It fixed my issue! Thank you @AlbertBrand 👍 |
Summary: On Android with dev mode on, we're seeing a regular SIGSEGV when pushing a lot of animation declarations over the bridge. We tracked this down to being not specific to animations, but the crash is caused in `deepFreezeAndThrowOnMutationInDev`. Specifically: the provided object to freeze is modified while looping, replacing the current key access to a getter/setter. After the modification, JSC crashes during retrieval of the next key - but only when there are a lot of events passing over the bridge. We have a hunch that this is due to a bug in JSC object enumeration but did we not look into it further yet. Any help here is welcome. The JS code seems all right at first sight and shouldn't cause a segmentation crash. The workaround in this PR is to retrieve the keys first from the object and then looping over that array. In our app and in a reduced app test case this fixes the crash. If needed I can provide the reduced app test case. It's really tricky to make a test for this as it requires to be run Closes facebook#11804 Differential Revision: D4403483 Pulled By: davidaurelio fbshipit-source-id: a31e5cff734e96bfec56e4a39dc1c6854798e245
On Android with dev mode on, we're seeing a regular SIGSEGV when pushing a lot of animation declarations over the bridge. We tracked this down to being not specific to animations, but the crash is caused in
deepFreezeAndThrowOnMutationInDev.Specifically: the provided object to freeze is modified while looping, replacing the current key access to a getter/setter. After the modification, JSC crashes during retrieval of the next key - but only when there are a lot of events passing over the bridge.
We have a hunch that this is due to a bug in JSC object enumeration but did we not look into it further yet. Any help here is welcome. The JS code seems all right at first sight and shouldn't cause a segmentation crash.
The workaround in this PR is to retrieve the keys first from the object and then looping over that array. In our app and in a reduced app test case this fixes the crash.
If needed I can provide the reduced app test case. It's really tricky to make a test for this as it requires to be run on Android and causes a segmentation crash.
Crash log: