Do not handle JSI host object in deepFreezeAndThrowOnMutationInDev

[Kudo]

Summary

That is specific for react-native-v8 issue Kudo/react-native-v8#27, but I do think that V8 is semantic right and to introduce the change for RN upstream.

JSI host object is a JavaScript Object that C++ side could control getter or setter.
In react-native-v8, I use V8 object interceptor to implement JSI host object.
V8 does not allow Object.freeze() for objects with interceptor and will throw exception.
https://chromium.googlesource.com/v8/v8.git/+/lkgr/src/objects/js-objects.cc#3845
That is, you cannot truly freeze the JavaScript object because C++ side may add new properties in interceptor.

In this CL, I introduced some JSI helper functions to determine if the target object is JSI host object and skip the freezing.

function global.__jsiUtils.isHostObject(target: Object): boolean
function global.__jsiUtils.isHostFunction(target: Function): boolean

Changelog

[General] [Changed] - Do not handle JSI host object in deepFreezeAndThrowOnMutationInDev

Test Plan

1. Test react-native-v8 + BlobCollector JSI object change and the test case:

const TestCase = async () => {
  const resp = await fetch('http://www.africau.edu/images/default/sample.pdf', {
    method: 'GET',
  });
  const respBlob = await resp.blob();
  await AsyncStorage.setItem('foo', respBlob);
};

And see if there is JS exception.

2. Verify the JSI helper function by

global.__jsiUtils.isHostObject(global.nativeModuleProxy);

That the returned value should be true.

[winseyli]

Will this PR move onward?

[Alaa-Ben]

Any news on this ?

[Kudo]

Not sure if this global.__jsiUtils.isHostObject() helps or not.
I've workaround from react-native-v8 side.
If that helper does not make sense, please feel free to let me know and I will close this PR.
Thanks.

[facebook-github-bot]

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

[JoshuaGross]

Hi @Kudo!

I don't understand why we need two additional methods, can you clarify? instead of isHostObject and isHostFunction, would it be helpful to just have "isHostValue"? You should still be able to determine if it's a function or object by using typeof, right?

It's possible I'm mistaken but I believe you'd only need one additional method:

isHostObject == typeof x === 'object' && !Array.isArray(x) && isHostValue(x)
isHostArray == Array.isArray(x) && isHostValue(x)
isHostFunction == typeof x === 'function' && isHostValue(x)

[motiz88]

Instead of trying to precisely detect host objects, can we guard the Object.freeze() calls with try...catch? This is DEV-only to begin with, so I don't think the performance overhead of exception handling will be a problem. It would be slightly less precise - in the sense that carefully crafted / "malicious" code could cause deepFreezeAndThrowOnMutationInDev to return without necessarily freezing everything that could be frozen - but I think that is acceptable.

[Kudo]

the story is that hosted objects are not freezable.
as the time past, i have workaround in react-native-v8 and acts stable. so the value of this pr right now is to determine if introducing some utilities like global.__jsiUtils.isHostObject making sense and we could revise the deepFreezeAndThrowOnMutationInDev code precisely right. otherwise, i am to okay to close this pr, too.

[motiz88]

the story is that hosted objects are not freezable.

Yes - my idea is to generalise that into "some objects are not freezable" and handle it in a standard, portable way, without adding new introspection APIs just for the JSI host object case.

[sota000]

It sounds like that we don't have a strong use case for the utilities added in this PR due to 1. existence of the workaround and 2. no need for now to be precise when some objects are not freezable. If anyone wants to create a PR to add the error handling, but for now, I am closing this PR. Please re-open if you think this is still important. Thank you so much for the discussion!