Use V8RecursionScope::MicrotaskSuppression at ChromeClinetImpl::postAccessibilityNotification

Use V8RecursionScope::MicrotaskSuppression at ChromeClinetImpl::postAccessibilityNotification

Since ChromeClientImpl::postAccessibilityNotification may call JS
callbacks, it is necessary to modify the recursion state.

This is related to https://codereview.chromium.org/172263002,
which moves AccessibilityController from CppBound to
gin::Wrappable. When V8::Function::Call is used, the
recursion state should be cared at the blink side.

BUG=297480, 331301

[hajimehoshi, 2014-02-24 10:47:40]

PTAL

[jochen (gone - plz use gerrit), 2014-02-24 10:56:43]

+dcarney

happy to rubberstamp

[haraken, 2014-02-24 11:06:08]

+adamk

https://codereview.chromium.org/177553002/diff/1/Source/web/ChromeClientImpl.cpp
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/177553002/diff/1/Source/web/ChromeClientImpl....
Source/web/ChromeClientImpl.cpp:682: V8RecursionScope::MicrotaskSuppression
scope;

hmm, this looks fragile. We don't want to add MicrotaskSuppression to all web/
methods that can potentially invoke Function::Call(), Script::Run() etc.

BTW, what is MicrotaskSuppression for? MicrotaskSuppression is doing nothing
except for checking recursion level. I think that V8 already has a mechanism to
throw an exception when a recursion level is exceeded, so probably can we simply
remove MicrotaskSuppression completely?

[haraken, 2014-02-24 11:16:25]

On 2014/02/24 11:06:08, haraken wrote:
> +adamk
> 
>
https://codereview.chromium.org/177553002/diff/1/Source/web/ChromeClientImpl.cpp
> File Source/web/ChromeClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/177553002/diff/1/Source/web/ChromeClientImpl....
> Source/web/ChromeClientImpl.cpp:682: V8RecursionScope::MicrotaskSuppression
> scope;
> 
> hmm, this looks fragile. We don't want to add MicrotaskSuppression to all web/
> methods that can potentially invoke Function::Call(), Script::Run() etc.
> 
> BTW, what is MicrotaskSuppression for? MicrotaskSuppression is doing nothing
> except for checking recursion level. I think that V8 already has a mechanism
to
> throw an exception when a recursion level is exceeded, so probably can we
simply
> remove MicrotaskSuppression completely?

Wait :)

Given that you're going to call user scripts from AccessibilityController, what
you should create here is not MicrotaskSuppression but V8RecursionScope, right?

Then it seems what you need to do is:

- Expose V8RecursionScope to content_shell.
- Make content_shell enter the V8RecursionScope before calling Function::Call().

I'd like to hear opinions of Adamk, Jochen and Dan.

[adamk, 2014-02-24 19:00:26]

My memory is that kalman already did something to expose V8RecursionScope
through the API...hopefully he remembers?

[not at google - send to devlin, 2014-02-24 19:02:23]

On 2014/02/24 19:00:26, adamk wrote:
> My memory is that kalman already did something to expose V8RecursionScope
> through the API...hopefully he remembers?

I don't really remember, maybe you're referring to this?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...

[adamk, 2014-02-24 19:10:28]

On 2014/02/24 19:02:23, kalman wrote:
> On 2014/02/24 19:00:26, adamk wrote:
> > My memory is that kalman already did something to expose V8RecursionScope
> > through the API...hopefully he remembers?
> 
> I don't really remember, maybe you're referring to this?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...

Hmm, guess that ended up using the suppression scope too. Sounds like we just
need to create a WebScriptRecursionScope alongside the
WebScopedMicrotaskSuppression?

[esprehn, 2014-02-24 19:47:43]

It's definitely not safe to run script from inside postNotification, why does
that happen? We call it all over the tree in places where script is not safe,
ex. AXMenuList::didUpdateActiveOption.

This is a _huge_ security bug. We need to stop calling scripts from inside
postAccessibilityNotification.

[hajimehoshi, 2014-02-25 03:56:09]

On 2014/02/24 19:47:43, esprehn wrote:
> It's definitely not safe to run script from inside postNotification, why does
> that happen? We call it all over the tree in places where script is not safe,
> ex. AXMenuList::didUpdateActiveOption.
> 
> This is a _huge_ security bug. We need to stop calling scripts from inside
> postAccessibilityNotification.

Here is a part of the backtrace with the breakpoint at
WebTestRunner::AccessibilityController::notificationReceived:

gdb --args ./out/Debug/content_shell --dump-render-tree --single-process
accessibility/accessibility-controller-notificationlistener-leak-callback.html

#0  WebTestRunner::AccessibilityController::notificationReceived
(this=0x34755a7cfaa0, target=..., notificationName=0x5b6569 <.L.str20>
"LayoutComplete") at
../../content/shell/renderer/test_runner/AccessibilityController.cpp:116
#1  0x0000000000489719 in
WebTestRunner::WebTestProxyBase::postAccessibilityEvent (this=0x34755b6ef148,
obj=..., event=blink::WebAXEventLayoutComplete) at
../../content/shell/renderer/test_runner/WebTestProxy.cpp:849
#2  0x00000000004add04 in WebTestRunner::WebTestProxy<content::RenderViewImpl,
content::RenderViewImplParams*>::postAccessibilityEvent (this=0x34755b6ee220,
object=..., event=blink::WebAXEventLayoutComplete) at
../../content/shell/renderer/test_runner/WebTestProxy.h:283
#3  0x00000000004aef35 in non-virtual thunk to
WebTestRunner::WebTestProxy<content::RenderViewImpl,
content::RenderViewImplParams*>::postAccessibilityEvent(blink::WebAXObject
const&, blink::WebAXEvent) () at ../../content/test/layouttest_support.cc:285
#4  0x00007fffeffbb913 in blink::ChromeClientImpl::postAccessibilityNotification
(this=0x38a5f454060, obj=0x38a5f488e50,
notification=WebCore::AXObjectCache::AXLayoutComplete) at
../../third_party/WebKit/Source/web/ChromeClientImpl.cpp:688

And WebTestRunner::AccessibilityController::notificationReceived calls
user-registered JavaScript functions. Is this really a bug?

[hajimehoshi, 2014-02-25 10:08:40]

(adamk)
> Hmm, guess that ended up using the suppression scope too. Sounds like we just
> need to create a WebScriptRecursionScope alongside the
> WebScopedMicrotaskSuppression?

Sure, I'll try to create WebScriptRecursionScope which just use V8RecursiveScope
if there is no problem.

[adamk, 2014-02-25 18:24:51]

On 2014/02/25 03:56:09, hajimehoshi wrote:
> On 2014/02/24 19:47:43, esprehn wrote:
> > It's definitely not safe to run script from inside postNotification, why
does
> > that happen? We call it all over the tree in places where script is not
safe,
> > ex. AXMenuList::didUpdateActiveOption.
> > 
> > This is a _huge_ security bug. We need to stop calling scripts from inside
> > postAccessibilityNotification.
> 
> Here is a part of the backtrace with the breakpoint at
> WebTestRunner::AccessibilityController::notificationReceived:
> 
> gdb --args ./out/Debug/content_shell --dump-render-tree --single-process
> accessibility/accessibility-controller-notificationlistener-leak-callback.html
> 
> #0  WebTestRunner::AccessibilityController::notificationReceived
> (this=0x34755a7cfaa0, target=..., notificationName=0x5b6569 <.L.str20>
> "LayoutComplete") at
> ../../content/shell/renderer/test_runner/AccessibilityController.cpp:116
> #1  0x0000000000489719 in
> WebTestRunner::WebTestProxyBase::postAccessibilityEvent (this=0x34755b6ef148,
> obj=..., event=blink::WebAXEventLayoutComplete) at
> ../../content/shell/renderer/test_runner/WebTestProxy.cpp:849
> #2  0x00000000004add04 in WebTestRunner::WebTestProxy<content::RenderViewImpl,
> content::RenderViewImplParams*>::postAccessibilityEvent (this=0x34755b6ee220,
> object=..., event=blink::WebAXEventLayoutComplete) at
> ../../content/shell/renderer/test_runner/WebTestProxy.h:283
> #3  0x00000000004aef35 in non-virtual thunk to
> WebTestRunner::WebTestProxy<content::RenderViewImpl,
> content::RenderViewImplParams*>::postAccessibilityEvent(blink::WebAXObject
> const&, blink::WebAXEvent) () at ../../content/test/layouttest_support.cc:285
> #4  0x00007fffeffbb913 in
blink::ChromeClientImpl::postAccessibilityNotification
> (this=0x38a5f454060, obj=0x38a5f488e50,
> notification=WebCore::AXObjectCache::AXLayoutComplete) at
> ../../third_party/WebKit/Source/web/ChromeClientImpl.cpp:688
> 
> And WebTestRunner::AccessibilityController::notificationReceived calls
> user-registered JavaScript functions. Is this really a bug?

Is this only an issue when running with the TestRunner? If so, it seems
esprehn's concern is likely not so bad, as the "user" is trusted.

[jochen (gone - plz use gerrit), 2014-02-25 19:22:26]

What does the callstack with the CppBoundClass look like?

Maybe it's an option to postTask() the script execution?

[hajimehoshi, 2014-02-26 05:21:10]

(adamk)
> Is this only an issue when running with the TestRunner? If so, it seems
> esprehn's concern is likely not so bad, as the "user" is trusted.

I guess so. I understand the current patch (#1) is evil in terms of that this is
executed on both chromium and content_shell (TestRunner). IIUC,
WebScriptRecursionScope seems acceptable because the responsibility is at the
caller of WebScriptRecursionScope.

On 2014/02/25 19:22:26, jochen wrote:
> What does the callstack with the CppBoundClass look like?

Here is the stack trace by hand:

v8::Function::Call
(third_party/WebKit/Source/bindings/v8/V8ScriptRunner.cpp:135)
V8ScriptRunner::callFunction
(third_party/WebKit/Source/bindings/v8/ScriptController.cpp:183)
ScriptController::callFunction
(third_party/WebKit/Source/bindings/v8/ScriptController.cpp:155)
ScriptController::callFunction
(third_party/WebKit/Source/bindings/v8/NPV8Object.cpp:309)
_NPN_InvokeDefault (third_party/WebKit/Source/web/WebBindings.cpp:134)
WebBindings::invokeDefault
(content/shell/renderer/test_runner/CppVariant.cpp:291)
CppVariant::invokeDefault
(content/shell/renderer/test_runner/AccessibilityController.cpp:126)
AccessibilityController::notificationReceived

I couldn't get a good stack trace in V8 world...

> Maybe it's an option to postTask() the script execution?

Hm, let me think about this.

[hajimehoshi, 2014-02-26 05:32:14]

The problem is originally that we don't have any proper way to call JavaScript
functions when we use gin. I'm afraid WebScriptRecursionScope could be a
temporary hack. jochen, what do you think?

[haraken, 2014-02-26 05:44:24]

I'd like to understand how gin is going to be architectured.

As esprehn mentioned, we need to be careful about the timing when a script is
executed. Currently we're controlling the timing in V8 binding. If we want to
allow gin to call scripts, we need to be careful about it as well. Specifically,
I think we'll need to take either of the following approaches:

(a) Reimplement the careful control in gin to allow gin to call scripts. (e.g.,
exposing V8RecursionScope to gin, limiting the script execution to trusted
TestRunner etc)

(b) Not allow gin to call scripts. When gin calls scripts, gin delegates it to
V8 bindings (just like the current gin delegates it to CppBoundClass).

(c) Move the careful control from V8 binding to gin. When V8 binding calls
scripts, V8 binding delegates it to gin.

Which approach are we going to take?

[jochen (gone - plz use gerrit), 2014-02-26 06:09:20]

On 2014/02/26 05:44:24, haraken wrote:
> I'd like to understand how gin is going to be architectured.
> 
> As esprehn mentioned, we need to be careful about the timing when a script is
> executed. Currently we're controlling the timing in V8 binding. If we want to
> allow gin to call scripts, we need to be careful about it as well.
Specifically,
> I think we'll need to take either of the following approaches:
> 
> (a) Reimplement the careful control in gin to allow gin to call scripts.
(e.g.,
> exposing V8RecursionScope to gin, limiting the script execution to trusted
> TestRunner etc)
> 
> (b) Not allow gin to call scripts. When gin calls scripts, gin delegates it to
> V8 bindings (just like the current gin delegates it to CppBoundClass).
> 
> (c) Move the careful control from V8 binding to gin. When V8 binding calls
> scripts, V8 binding delegates it to gin.
> 
> Which approach are we going to take?

I think (c) is the way to go

[haraken, 2014-02-26 06:19:49]

On 2014/02/26 06:09:20, jochen wrote:
> On 2014/02/26 05:44:24, haraken wrote:
> > I'd like to understand how gin is going to be architectured.
> > 
> > As esprehn mentioned, we need to be careful about the timing when a script
is
> > executed. Currently we're controlling the timing in V8 binding. If we want
to
> > allow gin to call scripts, we need to be careful about it as well.
> Specifically,
> > I think we'll need to take either of the following approaches:
> > 
> > (a) Reimplement the careful control in gin to allow gin to call scripts.
> (e.g.,
> > exposing V8RecursionScope to gin, limiting the script execution to trusted
> > TestRunner etc)
> > 
> > (b) Not allow gin to call scripts. When gin calls scripts, gin delegates it
to
> > V8 bindings (just like the current gin delegates it to CppBoundClass).
> > 
> > (c) Move the careful control from V8 binding to gin. When V8 binding calls
> > scripts, V8 binding delegates it to gin.
> > 
> > Which approach are we going to take?
> 
> I think (c) is the way to go

Yeah, I agree.

Also I think that at the moment it would be better to implement a logic that
allows gin to delegate script execution to V8 binding, instead of trying to
re-implement the logic in gin by exposing the current V8 binding infrastructure
to gin (e.g., V8RecursionScope). Once we're ready, we can migrate the V8 binding
infrastructure to gin and remove the delegation.

So my preference is:

(1) Take the approach (a) at the moment.
(2) Move the infrastructure of script execution from V8 binding to gin.
(3) Switch to the approach (c).

What do you think?

[jochen (gone - plz use gerrit), 2014-02-26 08:39:44]

On 2014/02/26 06:19:49, haraken wrote:
> On 2014/02/26 06:09:20, jochen wrote:
> > On 2014/02/26 05:44:24, haraken wrote:
> > > I'd like to understand how gin is going to be architectured.
> > > 
> > > As esprehn mentioned, we need to be careful about the timing when a script
> is
> > > executed. Currently we're controlling the timing in V8 binding. If we want
> to
> > > allow gin to call scripts, we need to be careful about it as well.
> > Specifically,
> > > I think we'll need to take either of the following approaches:
> > > 
> > > (a) Reimplement the careful control in gin to allow gin to call scripts.
> > (e.g.,
> > > exposing V8RecursionScope to gin, limiting the script execution to trusted
> > > TestRunner etc)
> > > 
> > > (b) Not allow gin to call scripts. When gin calls scripts, gin delegates
it
> to
> > > V8 bindings (just like the current gin delegates it to CppBoundClass).
> > > 
> > > (c) Move the careful control from V8 binding to gin. When V8 binding calls
> > > scripts, V8 binding delegates it to gin.
> > > 
> > > Which approach are we going to take?
> > 
> > I think (c) is the way to go
> 
> Yeah, I agree.
> 
> Also I think that at the moment it would be better to implement a logic that
> allows gin to delegate script execution to V8 binding, instead of trying to
> re-implement the logic in gin by exposing the current V8 binding
infrastructure
> to gin (e.g., V8RecursionScope). Once we're ready, we can migrate the V8
binding
> infrastructure to gin and remove the delegation.
> 
> So my preference is:
> 
> (1) Take the approach (a) at the moment.
> (2) Move the infrastructure of script execution from V8 binding to gin.
> (3) Switch to the approach (c).
> 
> What do you think?

I think (a) will be complicated, because gin can't depend on blink.

Anyways, if you feel that (a) lets you iterate faster, go for it

[haraken, 2014-02-26 08:44:48]

On 2014/02/26 08:39:44, jochen wrote:
> On 2014/02/26 06:19:49, haraken wrote:
> > On 2014/02/26 06:09:20, jochen wrote:
> > > On 2014/02/26 05:44:24, haraken wrote:
> > > > I'd like to understand how gin is going to be architectured.
> > > > 
> > > > As esprehn mentioned, we need to be careful about the timing when a
script
> > is
> > > > executed. Currently we're controlling the timing in V8 binding. If we
want
> > to
> > > > allow gin to call scripts, we need to be careful about it as well.
> > > Specifically,
> > > > I think we'll need to take either of the following approaches:
> > > > 
> > > > (a) Reimplement the careful control in gin to allow gin to call scripts.
> > > (e.g.,
> > > > exposing V8RecursionScope to gin, limiting the script execution to
trusted
> > > > TestRunner etc)
> > > > 
> > > > (b) Not allow gin to call scripts. When gin calls scripts, gin delegates
> it
> > to
> > > > V8 bindings (just like the current gin delegates it to CppBoundClass).
> > > > 
> > > > (c) Move the careful control from V8 binding to gin. When V8 binding
calls
> > > > scripts, V8 binding delegates it to gin.
> > > > 
> > > > Which approach are we going to take?
> > > 
> > > I think (c) is the way to go
> > 
> > Yeah, I agree.
> > 
> > Also I think that at the moment it would be better to implement a logic that
> > allows gin to delegate script execution to V8 binding, instead of trying to
> > re-implement the logic in gin by exposing the current V8 binding
> infrastructure
> > to gin (e.g., V8RecursionScope). Once we're ready, we can migrate the V8
> binding
> > infrastructure to gin and remove the delegation.
> > 
> > So my preference is:
> > 
> > (1) Take the approach (a) at the moment.
> > (2) Move the infrastructure of script execution from V8 binding to gin.
> > (3) Switch to the approach (c).
> > 
> > What do you think?
> 
> I think (a) will be complicated, because gin can't depend on blink.
> 
> Anyways, if you feel that (a) lets you iterate faster, go for it

I'm sorry, I meant (b).

However, (b) also requires to call Blink from gin, so we have the same problem.
Let us discuss offline a bit.

[hajimehoshi, 2014-02-26 09:52:05]

We discussed offline. We agreed that in long term (c) is the way to go, but for
the current situation, it'd better to take another way. The first idea
WebScriptRecursionScope seems too ad hoc, and IMO this is unsuitable. We think
there are two options:

(1) Expose V8ScriptRunner::callFunction and use it at TestRunner.
(2) As jochen suggested, use WebKitTestRunner::postTask. IIUC, this is same as
setTimeout(func, 0); and func is called in a proper environment.

In case of (1), it is difficult to prevent the modules other than content_shell
from executing JS functions, isn't it? So (2) seems to be better for now.

[jochen (gone - plz use gerrit), 2014-02-26 15:37:04]

Scott on a different CL pointed out that V8ScriptRunner::callFunction is already
exposed in the public blink API: WebFrame::callFunctionEvenIfScriptDisabled()

[hajimehoshi, 2014-02-27 03:36:38]

On 2014/02/26 15:37:04, jochen (OOO from March 1) wrote:
> Scott on a different CL pointed out that V8ScriptRunner::callFunction is
already
> exposed in the public blink API: WebFrame::callFunctionEvenIfScriptDisabled()

Thanks, this worked well!

I'll close this patch.