Bug 32326 – Refactor some security code out of V8 bindings

• Component: New Bugs
• Status: RESOLVED FIXED

[Charles Reis]

As a first step toward sharing code between the V8 and JSC bindings, we should try to refactor some of V8's code for enforcing the Same Origin Policy.  I suggest moving canAccessFrame and checkNodeSecurity from V8Proxy to a new BindingsSecurity class that can be used by multiple bindings.  Since JSC passes around execution state explicitly and V8 does not, the BindingsSecurity interface should be able to take in execution state as an argument.

There has been some discussion about starting to share bindings code in this thread:
http://lists.macosforge.org/pipermail/webkit-dev/2009-May/007935.html

[Charles Reis]

Created attachment 44542 [details]
Moves V8's canAccessFrame and checkNodeSecurity to a new BindingsSecurity class

[WebKit Review Bot]

Attachment 44542 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/custom/V8LocationCustom.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/v8/custom/V8LocationCustom.cpp:222:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bindings/v8/custom/V8LocationCustom.cpp:240:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bindings/v8/custom/V8LocationCustom.cpp:259:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 4

[Charles Reis]

Created attachment 44544 [details]
Fixes issues from the style-queue bot

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44544 [details] without any errors.

[Adam Barth]

Comment on attachment 44544 [details]
Fixes issues from the style-queue bot

Thanks for fixing the style nits.  At a high level, this looks great. I have a few questions:

+ virtual DOMWindow* getActiveWindow() = 0;

Does this need to be virtual, or can we do link-time dispatch here?

+ // Implement BindingsSecurity methods that depend on V8-specific code.

Don't we want to implement this in a V8BindingsSecurity.cpp file instead of
V8Proxy?  V8Proxy is an all-sing, all-dance dumping ground.

+ class V8State : public BindingsState {

I'd call this V8BindingsState to mirror the DOMWindow / V8DOMWindow name
convention.

In general, it's slightly unclear to me whether we actually want a
V8BindingsState object that derives from BindingsState or whether we want to
have a single class where some of the methods are implemented by the different
bindings, like ScriptController.  The advantage of the latter is that we'll get
compile-time dispatch instead of runtime dispatch.

Another option is to have a shared BindingsStateBase and V8 implement a
subclass BindingsState that the shared code refers to.  That way, the bindings
can override whatever methods they need without incurring a virtual method
call.

A final option is to use template namespaces.  These shared classes would be

template<classname Bindings>
class ...

and then refer to Bindings::State.  Then we can have V8Binding::State derive
from GenericBinding::State.  That's more code, but it might make it easier to
support things like re-using this code for ObjCBindings::State, etc.

Thoughts?

[Adam Barth]

Anding some more folks in case they'd like to have input at this early stage.  We're starting small to make sure we get started in the right direction.

[Charles Reis]

(In reply to comment #5)
> (From update of attachment 44544 [details])
> Thanks for fixing the style nits.  At a high level, this looks great. I have a
> few questions:
> 
> + virtual DOMWindow* getActiveWindow() = 0;
> 
> Does this need to be virtual, or can we do link-time dispatch here?
> 

(See below)

> + // Implement BindingsSecurity methods that depend on V8-specific code.
> 
> Don't we want to implement this in a V8BindingsSecurity.cpp file instead of
> V8Proxy?  V8Proxy is an all-sing, all-dance dumping ground.
> 

Well, reportUnsafeAccessTo (and the enum it relies on) looks like it's only definied within V8Proxy.cpp, but I could move it to be a static member of V8Proxy that we can call outside V8Proxy.cpp.

I could also move the immediatelyReportUnsafeAccessTo method to the BindingsState class, since that already has bindings-specific code.  That'll clean up BindingsSecurity so that it's all shared code.


> + class V8State : public BindingsState {
> 
> I'd call this V8BindingsState to mirror the DOMWindow / V8DOMWindow name
> convention.
> 
> In general, it's slightly unclear to me whether we actually want a
> V8BindingsState object that derives from BindingsState or whether we want to
> have a single class where some of the methods are implemented by the different
> bindings, like ScriptController.  The advantage of the latter is that we'll get
> compile-time dispatch instead of runtime dispatch.
> 
> Another option is to have a shared BindingsStateBase and V8 implement a
> subclass BindingsState that the shared code refers to.  That way, the bindings
> can override whatever methods they need without incurring a virtual method
> call.
> 
> A final option is to use template namespaces.  These shared classes would be
> 
> template<classname Bindings>
> class ...
> 
> and then refer to Bindings::State.  Then we can have V8Binding::State derive
> from GenericBinding::State.  That's more code, but it might make it easier to
> support things like re-using this code for ObjCBindings::State, etc.
> 
> Thoughts?

 I think the template approach is probably the most flexible (while still giving us compile-time dispatch).  I'll look into it.

[Charles Reis]

Created attachment 44696 [details]
Update BindingSecurity class to use templates

This updated patch makes BindingSecurity a template class so that each binding will be able to implement things it depends on, like execution state, without needing virtual methods.  I threw in a few typedefs to make the code easier to use at the call sites.

I also made the error reporting function visible outside V8Proxy.cpp so that we can call it.

Thoughts?

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44696 [details] without any errors.

[Adam Barth]

Comment on attachment 44696 [details]
Update BindingSecurity class to use templates

This looks like a great first step.  I've noted a few formatting issues below.  There are a couple of things that I'm not sure how they'll scale up when we convert more of the bindings, but they're fine for now.  We can revisit them if the code isn't growing the right direction.

+ template <class Binding> class BindingSecurity {

I think we'd prefer

+ template <class Binding>
+ class BindingSecurity {

(meaning a line break)

+ class BindingSecurityHelper

I'm not entirely sold on this class.  We might need to see more examples to see what the best general pattern is for this kind of thing.

+ typedef BindingSecurity<V8Binding> V8BindingSecurity;

I'm also not sure how this will scale.  It's fine here though.

+ template <> class State<V8Binding> : public State<GenericBinding> {

We'd also like a line break here.

[Charles Reis]

(In reply to comment #10)
> (From update of attachment 44696 [details])
> This looks like a great first step.  I've noted a few formatting issues below. 
> There are a couple of things that I'm not sure how they'll scale up when we
> convert more of the bindings, but they're fine for now.  We can revisit them if
> the code isn't growing the right direction.
> 
> + template <class Binding> class BindingSecurity {
> 
> I think we'd prefer
> 
> + template <class Binding>
> + class BindingSecurity {
> 
> (meaning a line break)
> 

Sure, I'll upload a patch in just a minute with linebreaks on all the templates.

> + class BindingSecurityHelper
> 
> I'm not entirely sold on this class.  We might need to see more examples to see
> what the best general pattern is for this kind of thing.
> 

I wasn't thrilled about this, but it seemed the best approach.  That is, I don't think files like DOMWindow.h are supposed to be included in header files like BindingSecurity.h.  For example, it was causing stack corruption problems for me in V8 code.

On the flip side, templates won't link unless you forward declare all your instantiations in the cpp file (which is undesirable), or unless you include the method implementations in the header file (which appears to be common in WebKit).

This helper class at least solves both problems-- it lets us call methods on the WebCore types while keeping them opaque in BindingSecurity.h.


> + typedef BindingSecurity<V8Binding> V8BindingSecurity;
> 
> I'm also not sure how this will scale.  It's fine here though.
> 

I was just using it to make the call sites a little shorter-- I'm not too attached to it in the long run.

> + template <> class State<V8Binding> : public State<GenericBinding> {
> 
> We'd also like a line break here.

Sure.

[Adam Barth]

> I wasn't thrilled about this, but it seemed the best approach.

Another option to consider is a non-template base class.  That can implement its methods in its own cpp file.  I'm not sure if that scales either.  I'm inclined to try this.  As we do move of this work, the right answer will likely reveal itself.

Another option, by the way, is to move this code into WebCore proper.  If it doesn't need to call back into the bindings, the dependencies allow it escape from the bindings directory.

[Adam Barth]

There's also the ScriptState abstraction, but I think this code is different.  Basically ScriptState is part of WebCore's view of the bindings.  This code is internal to the bindings themselves.

[Charles Reis]

Created attachment 44709 [details]
Adds newlines and a BindingSecurityBase class

I took Adam's suggestion to make BindingSecurityHelper into BindingSecurityBase, and I added the requested newlines.

I'm inclined to think that this code should stay in the bindings directory, but if WebCore proper has a better place for it, that's fine.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44709 [details] without any errors.

[Adam Barth]

Comment on attachment 44709 [details]
Adds newlines and a BindingSecurityBase class

This looks great.  I'm going to wait a bit before landing this in case someone else wants to give their opinion.

+ DEFINE_STATIC_LOCAL(State, globalV8BindingState, ());

I'm surprised that compiles without the template paramater, but if it does that's fine.  :)

[Adam Barth]

Comment on attachment 44709 [details]
Adds newlines and a BindingSecurityBase class

Clearing flags on attachment: 44709

Committed r52080: <http://trac.webkit.org/changeset/52080>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.