DCHECK fixes

[ptomato]

Description

This is a series of commits to fix issues that cause V8 debug check (DCHECK) failures when running the test suite using debug-enabled V8 libraries. Each commit message contains a more specific description, so I recommend reviewing it commit-by-commit.

(The last commit is not related to DCHECK fixes — it pulls in an update to common-runtime-tests-app which will be needed to upgrade V8 to 11.x.)

Related Pull Requests

• https://github.com/NativeScript/common-runtime-tests-app/pull/23 (the common-runtime-tests-app update brought in by the last commit)

Does your pull request have unit tests?

Most of these changes are either covered by existing tests, or are validated by the DCHECK failures no longer occurring. The commit "Avoid setting the same property on an ObjectTemplate twice" has tests specifically for an API that I found had both an Android definition and a Kotlin extension, just to confirm that the new code handles it.

[edusperoni]

LGTM! Actually quite surprised by the dated GetImplementedInterfaces implementation. Nice catch!

Regarding https://github.com/NativeScript/android/pull/1764/commits/7a479c81c0a19af27df64337b3013e9b26149274, do you know if the properties were overwritten even if the DCHECK failed? Asking mostly because if the DCHECK failed but it did overwrite anyway, it'd change the behavior (previously it would set to the last property written, now to the first).

[ptomato]

Good question, I assumed it was the case but it bears following up on.

The DCHECK fails because the ToLocalChecked() in ctorFuncTemplate->GetFunction(context).ToLocalChecked(); is called on an empty handle. So the failure happens quite a lot later than when the second property is added to the template. What I believe happens is that the template does "contain" both properties (you can see them in Object.keys of the prototype object) but only one can be accessed.

The test I added in https://github.com/NativeScript/android/pull/1764/files#diff-4187621cb8f9cc3730f47a10d0ba131f90f83cf0321251b0ea66ebdf9493b265R119 passes both with and without the patch, so that suggests that the behaviour of which function is called is the same.

For android.os.Handler.postAtTime there are three possible functions:

• Java, 2-argument overload: https://developer.android.com/reference/android/os/Handler#postAtTime(java.lang.Runnable,%20long)
• Java, 3-argument overload: https://developer.android.com/reference/android/os/Handler#postAtTime(java.lang.Runnable,%20java.lang.Object,%20long)
• Kotlin extension: https://developer.android.com/reference/kotlin/androidx/core/os/package-summary#(android.os.Handler).postAtTime(kotlin.Long,%20kotlin.Any,%20kotlin.Function0)

What I've observed is that the 2-argument Java version is called if you pass 2 arguments, and the Kotlin extension overshadows the 3-argument Java version, so the Kotlin one is called if you pass 3 arguments. The patch doesn't seem to change this.

I'm not 100% sure of this, but I think from my understanding of the code, it doesn't matter whether the first or last property is kept - they both have the same MethodCallbackData. The distinction is only made at runtime in MetadataNode::MethodCallback() where the overload is selected based on the number of arguments passed to the JS function.

Here's the only difference in behaviour that I see with and without the patch.

console.log(Object.keys(android.os.Handler.prototype).filter(name => name === 'postAtTime'));
// output before: [postAtTime, postAtTime]
// output after: [postAtTime]

I believe that additionally, without the patch, the instantiation of the constructor function template is never added to the cache in MetadataNode::GetConstructorFunctionTemplate() either. I'm not certain what that affects, though, because you can still use the constructor function like new android.os.Handler().