[api] Add safe API to inspect monkey-patched functions [v8/v8 : main]
Yao Xiao (Gerrit)
Yao Xiao added 3 comments![]( "Open in Gerrit")
GetAssessedFunctionForPathNoJavascriptExecution(Yao XiaoV8 reviewers will know better but I'd suggest removing the "NoJavascriptExecution" suffix and just mark that as a note in the comment about the behavior.
Done
*/
V8_WARN_UNUSED_RESULT MaybeLocal<Value> GetPropertyNoJavascriptExecution(
Local<Context> context, Local<Name> key);Yao XiaoWhy define this here? Can't it just be a helper function in unnamed namespace in cc?
Done. (Originally, I thought it would be beneficial to expose it as a separate API because it abstracts a potentially reusable part. But given that there is currently no embedder using it, I agree it's better to remove it.)
bool is_user_defined =Yao XiaoLooks like we can improve upon our existing heuristic here.
The current heuristic will miss monkey-patches that use `.bind()`, such as `window.history.pushState = adWrapper.bind(window.history);`. In V8, the result of `.bind()` is a `JSBoundFunction`, which does not have a script ID, causing `is_user_defined` to be `false`.
Since native platform APIs are never exposed as `JSBoundFunction` objects by default, I think we can safely assume that if the resolved function is a bound function, it is user-defined/monkey-patched.
I think you can udpate to this:
```cpp
i::Tagged<i::Object> internal_func = *Utils::OpenDirectHandle(*api_function);
bool is_user_defined = i::IsJSBoundFunction(internal_func) ||
api_function->ScriptId() != v8::Message::kNoScriptIdInfo;
```Please then also add a test.
Done
Related details
- Josh Karlin
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josh Karlin (Gerrit)
Josh Karlin added 3 comments![]( "Open in Gerrit")
* function bails out and returns an empty function. It allows native C++suggest saying "returns a default constructed AssessedFunction" or stating that is_user_defined is false in such cases.
* function bails out and returns an empty function. It allows native C++nit: remove unnecessary words
Local<Context> context, MemorySpan<const char* const> property_path) {Suggest taking a span of v8::Name so that v8 doesn't have to assume that the incoming string is UTF8 and create a new string each call. This way the embedder can create the string once and hold onto it for better performance if it wants.
Related details
- Yao Xiao
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yao Xiao (Gerrit)
Yao Xiao added 3 comments![]( "Open in Gerrit")
* function bails out and returns an empty function. It allows native C++suggest saying "returns a default constructed AssessedFunction" or stating that is_user_defined is false in such cases.
Done
* function bails out and returns an empty function. It allows native C++Yao Xiaonit: remove unnecessary words
Done
Local<Context> context, MemorySpan<const char* const> property_path) {Suggest taking a span of v8::Name so that v8 doesn't have to assume that the incoming string is UTF8 and create a new string each call. This way the embedder can create the string once and hold onto it for better performance if it wants.
- Josh Karlin
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josh Karlin (Gerrit)
Josh Karlin added 1 comment![]( "Open in Gerrit")
V8_WARN_UNUSED_RESULT AssessedFunction GetAssessedFunctionForPath(So the caller doesn't really need both values returned. Basically, what Chrome wants is a "GetUserDefinedFunctionIfExists" which returns an MaybeLocal function, right?
Related details
- Yao Xiao
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |