Bug 35773 – [chromium] add logging of cross-frame property accesses for site isolation

• Component: WebKit Misc.
• Status: RESOLVED WONTFIX

[John Gregg]

chromium would like to measure how often pages request properties from frames from other origins.

[John Gregg]

Created attachment 50067 [details]
patch to pass accesses through chromiumbridge

[John Gregg]

Created attachment 50068 [details]
same patch without debug print

[Charles Reis]

A little context: knowing whether many pages access cross-origin frames will help us decide about some potential changes to improve site isolation in Chromium.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebCore/bindings/v8/V8Utilities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/v8/V8Utilities.cpp:152:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 50068 [details]
same patch without debug print

Please fix the stylebot's nits.  Also you'll need to remove the following line before we can actually commit your patch:

+ No new tests. (OOPS!)

(It's ok not to have tests for this patch because it doesn't change any functionality.)

[John Gregg]

Created attachment 50454 [details]
patch with style updates

style nits fixed, except header ordering which I believe is a false-positive.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/V8Utilities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 50454 [details]
patch with style updates

LGTM.  I think the header ordering issue is cased by the #include <v8.h>, which we can probably just remove.

[John Gregg]

Comment on attachment 50454 [details]
patch with style updates

this will break chromium

[John Gregg]

Created attachment 50856 [details]
patch with event id

new version which also passes an ID for the current event when there is a cross-site access, so we can track when a page makes repeated accesses during a single event.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/V8Utilities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 50856 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/916019

[John Gregg]

Created attachment 50916 [details]
corrected patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/V8Utilities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 50916 [details]
corrected patch

> 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) { }

> Index: WebKit/chromium/src/ChromiumBridge.cpp
...
> +void ChromiumBridge::recordCrossFrameAccess(Frame* frame, bool crossOrigin, const String& name, unsigned long long eventId)
> +{
> +    WebFrameImpl* frameImpl = WebFrameImpl::fromFrame(frame);
> +    frameImpl->client()->didMakeCrossFrameAccess(frameImpl, crossOrigin, name, eventId);
> +}
> +

If it belongs on WebFrameClient, then it also belongs on FrameLoaderClient.
Also, we should avoid dumping things into ChromiumBridge that are not purely
the domain of WebCore/platform.  The easy test:  is the method parameterized
by a Frame?  If so, then it should actually be on FrameLoaderClient.

[John Gregg]

Created attachment 51468 [details]
patch using FrameLoaderClient

submitting new patch which uses FrameLoaderClient and does not use ChromiumBridge

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/V8Utilities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 51468 [details]
patch using FrameLoaderClient

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.

[John Gregg]

(In reply to comment #18)
> (From update of attachment 51468 [details])
> 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.
> 

Adding more context.

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

I'm not exactly sure what is different in an isolated world, but the code above is the same code that runs to do the actual access check; and the code below should just produce a 0 eventId if the mainWorldContext is empty.
 
> 
> > +    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"?

logSiteIsolationAccess is called before we know that it's cross frame (you'll see this code just returns if it turns out to be the same frame).  I can change it to logAccessToCrossFrameProperty, I guess that is still accurate and more related.

I would argue the code is running *during* the access and that 'did' is okay, but I'll change it if you feel strongly.

> 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?

Charlie Reis, any opinion?  

> 
> > 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.

done on #1; all the methods in that file have comments, so instead of removing it i'll make it more informative.

[Charles Reis]

> > 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?
> 
> Charlie Reis, any opinion?  

That's not a bad idea.  It'll unfortunately require an update to the Chromium side, but it would allow us to eventually check whether an event that makes multiple cross-site frame accesses is doing it on the same frame or different frames.

[Eric Seidel (no email)]

Comment on attachment 50454 [details]
patch with style updates

Cleared Adam Barth's review+ from obsolete attachment 50454 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Darin Fisher (:fishd, Google)]

> > 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"?
> 
> logSiteIsolationAccess is called before we know that it's cross frame (you'll
> see this code just returns if it turns out to be the same frame).  I can change
> it to logAccessToCrossFrameProperty, I guess that is still accurate and more
> related.
> 
> I would argue the code is running *during* the access and that 'did' is okay,
> but I'll change it if you feel strongly.

True.  How about logPropertyAccess and logCrossFramePropertyAccess?

The logSiteIsolationAccess name is a little awkward as well since it
is not about access to "site-isolation" that you are logging.

[John Gregg]

Created attachment 52067 [details]
patch with renames and Frame parameter

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/V8Utilities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 52067 [details]
patch with renames and Frame parameter

> Index: WebCore/bindings/v8/V8Utilities.cpp
...
> +void logPropertyAccess(v8::Local<v8::String> name, const v8::AccessorInfo& info)
...
> +                eventId = (unsigned long long) V8Event::toNative(v8::Handle<v8::Object>::Cast(jsEvent));

^^^ shouldn't that be a reinterpret_cast?


> Index: WebKit/chromium/public/WebFrameClient.h
> ===================================================================
> --- WebKit/chromium/public/WebFrameClient.h	(revision 56799)
> +++ WebKit/chromium/public/WebFrameClient.h	(working copy)
> @@ -284,6 +284,9 @@ public:
>      // scripts.
>      virtual void didCreateIsolatedScriptContext(WebFrame*) { }
>  
> +    // Notifies that a cross-frame access was made.
> +    virtual void didMakeCrossFrameAccess(WebFrame* active, WebFrame* target, bool crossOrigin, const WebString& property, unsigned long long eventId) { }

Any reason not to name this the same as the FrameLoaderClient method?
Unless there is a good reason, I think it would be better to make them
match.

[John Gregg]

Created attachment 52079 [details]
patch with rename and reinterpret_cast

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/V8Utilities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 52079 [details]
patch with rename and reinterpret_cast

R=me, but cq- since there is some style issue.

[John Gregg]

the style issue is a non-issue (see comments 6-8), unless that would cause the commit queue to not commit it?

[Darin Fisher (:fishd, Google)]

Comment on attachment 52079 [details]
patch with rename and reinterpret_cast

OK.  sorry, i missed the earlier discussion!

[WebKit Commit Bot]

Comment on attachment 52079 [details]
patch with rename and reinterpret_cast

Clearing flags on attachment: 52079

Committed r56829: <http://trac.webkit.org/changeset/56829>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Pavel Feldman]

Rolled out in http://trac.webkit.org/changeset/56849 due to inspector tests breakage.

[Stephen Chenney]

LayoutTest failures for Chromium are being marked WontFix. The Bug is still accessible and referenced from TestExpectations.