[webkit-reviews] review denied: [Bug 35773] [chromium] add logging of cross-frame property accesses for site isolation : [Attachment 51468] patch using FrameLoaderClient

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied John Gregg
<johnnyg at google.com>'s request for review:
Bug 35773: [chromium] add logging of cross-frame property accesses for site
isolation
https://bugs.webkit.org/show_bug.cgi?id=35773

Attachment 51468: patch using FrameLoaderClient
https://bugs.webkit.org/attachment.cgi?id=51468&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
ndex: WebCore/bindings/v8/V8Utilities.cpp
...
> +void logSiteIsolationAccess(v8::Local<v8::String> name, const
v8::AccessorInfo& info)
> +{
> +    Frame* target = V8DOMWindow::toNative(info.Holder())->frame();
> +    Frame* active = V8BindingState::Only()->getActiveWindow()->frame();
> +    if (target == active)
> +	   return;
> +
> +    bool crossSite =
!V8BindingSecurity::canAccessFrame(V8BindingState::Only(), target, false);
> +    String propName = toWebCoreString(name);
> +
> +    // For cross-site, we also want to identify the current event to record
repeat accesses.

It would be helpful if this comment explained why we want to do this for
cross-site only.

What happens if this code is reached from within an isolated world?


> +    unsigned long long eventId = 0;
> +    if (crossSite) {
> +	   v8::HandleScope handleScope;
> +	   v8::Handle<v8::Context> v8Context =
V8Proxy::mainWorldContext(active);
> +	   if (!v8Context.IsEmpty()) {
> +	       v8::Context::Scope scope(v8Context);
> +	       v8::Handle<v8::Object> global = v8Context->Global();
> +	       v8::Handle<v8::Value> jsEvent =
global->Get(v8::String::NewSymbol("event"));
> +	       if (V8DOMWrapper::isValidDOMObject(jsEvent))
> +		   eventId = (unsigned long long)
V8Event::toNative(v8::Handle<v8::Object>::Cast(jsEvent));
> +	   }
> +    }
> +    active->loader()->client()->didMakeCrossFrameAccess(crossSite, propName,
eventId);
> +}

it might be nice to make these function names, logSiteIsolationAccess and
didMakeCrossFrameAccess, be related.  also, isn't this code running *before*
the cross frame access?  probably that "did" should be a "will"?

could it be useful to pass the target frame as a parameter to
didMakeCrossFrameAccess?  then, the embedder could gather additional
information from the target frame if needed.  maybe that is not useful?


> Index: WebKit/chromium/public/WebFrameClient.h
...
> +    // Notifies that a cross-frame access was made.
> +    virtual void didMakeCrossFrameAccess(WebFrame*, bool crossOrigin, const
WebString& property, unsigned long long) { }

nit: please name the 'unsigned long long' argument.
nit: the comment is redundant with the method name, so it should probably be
eliminated.