[webkit-reviews] review denied: [Bug 79131] Use V8 completion callback API to assert that V8RecursionScope is on the stack whenever invoking script : [Attachment 134169] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Rafael Weinstein
<rafaelw at chromium.org>'s request for review:
Bug 79131: Use V8 completion callback API to assert that V8RecursionScope is on
the stack whenever invoking script
https://bugs.webkit.org/show_bug.cgi?id=79131

Attachment 134169: Patch
https://bugs.webkit.org/attachment.cgi?id=134169&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=134169&action=review


So, I have some really minor comments below.  The only one that's really of any
consequence is about ScriptController::callFunction (and the patching WebKit
API) being a trap in that it doesn't check canExecuteScripts.  As written, this
seems likely to lead to bugs where we execute JavaScript even if JavaScript is
supposed to be disabled.  We probably just need a better name.

Although I'm marking this patch R-, please feel free to land everything in here
except the WebKit API changes and ScriptController::callFunction with r=me.  If
you'd like to do that, that will simplify the remaining patch.	(You should
also feel free to continue to iterate on the full patch if that's easier for
you.)

> Source/WebCore/bindings/v8/ScriptController.cpp:168
> +ScriptValue ScriptController::callFunction(v8::Handle<v8::Function>
function,
> +						       v8::Handle<v8::Object>
receiver,
> +						       int argc,
> +						       v8::Handle<v8::Value>
argv[])

You can just combine this into one line.

> Source/WebCore/bindings/v8/ScriptController.cpp:171
> +    // FIXME: This should probably perform the same checks to
canExecuteScripts & isPaused that happen in ScriptController::executeScript.
> +    return ScriptValue(m_proxy->callFunction(function, receiver, argc,
argv));

In the bug you said that we shouldn't do that because extensions want to
execute script in sandboxed iframes.  Should we name this function something
that indicates that we're skilling the canExecuteScripts check? 
callFunctionEvenIfScriptsAreDisabled?

> Source/WebCore/bindings/v8/ScriptFunctionCall.cpp:136
> +	   V8RecursionScope recursionScope(getScriptExecutionContext());

getScriptExecutionContext() should be called scriptExecutionContext(), but
that's a problem for a future patch to solve.

> Source/WebCore/bindings/v8/V8RecursionScope.h:40
> +// C++ Calls into script contexts which are "owned" by WebKit (initialized
in

Calls -> calls

> Source/WebCore/bindings/v8/V8RecursionScope.h:41
> +// WebKit.cpp) must declare their type:

So, this comment isn't quite right.  The script contexts (i.e., v8::Context)
aren't initialized in WebKit.cpp.  The whole V8 engine is initialized there. 
That means this change affects V8 for the whole process.  I still think that's
ok, but we should clarify the comment.

> Source/WebKit/chromium/public/WebFrame.h:284
> +    // Call the function with the given receiver and arguments.
> +    virtual v8::Handle<v8::Value> callFunctionAndReturnValue(
> +	   v8::Handle<v8::Function>,
> +	   v8::Handle<v8::Object>,
> +	   int argc,
> +	   v8::Handle<v8::Value> argv[]) = 0;

It's really unclear that this function skips the canExecuteScripts check,
unlike executeScriptAndReturnValue, which seems very similar.  If we find a
good name for the ScriptController method, that will likely guide us towards a
good name here too.

> Source/WebKit/chromium/public/WebScriptInvocation.h:2
> + * Copyright (C) 2009 Google Inc. All rights reserved.

2009 -> 2012

> Source/WebKit/chromium/public/WebScriptInvocation.h:36
> +namespace WebScriptInvocation {

Is this a common pattern in the WebKit API?  I'm still learning about the
idioms of the WebKit API, so we might want to double-check with fishd about
what idiom he'd recommend here.

> Source/WebKit/chromium/public/WebScriptInvocation.h:46
> +    Impl* m_impl;

Can we use a WebPrivateOwnPtr to avoid calling delete manually?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:901
> +								
v8::Handle<v8::Object> receiver,
> +								 int argc,
> +								
v8::Handle<v8::Value> argv[])

Bad indent.

> Source/WebKit/chromium/src/WebScriptInvocation.cpp:48
> +#ifndef NDEBUG
> +class BypassMicrotaskCheckpoint::Impl {
> +public:
> +    Impl() { }
> +    ~Impl() { }
> +    WebCore::V8RecursionScope::BypassMicrotaskCheckpoint m_scope;
> +};
> +#endif

Why not just have BypassMicrotaskCheckpoint::Impl inherit from
WebCore::V8RecursionScope::BypassMicrotaskCheckpoint ?