chromium would like to measure how often pages request properties from frames from other origins.
Created attachment 50067 [details] patch to pass accesses through chromiumbridge
Created attachment 50068 [details] same patch without debug print
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.
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.
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.)
Created attachment 50454 [details] patch with style updates style nits fixed, except header ordering which I believe is a false-positive.
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.
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.
Comment on attachment 50454 [details] patch with style updates this will break chromium
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.
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.
Attachment 50856 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/916019
Created attachment 50916 [details] corrected patch
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.
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.
Created attachment 51468 [details] patch using FrameLoaderClient submitting new patch which uses FrameLoaderClient and does not use ChromiumBridge
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.
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.
(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.
> > 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.
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.
> > 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.
Created attachment 52067 [details] patch with renames and Frame parameter
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.
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.
Created attachment 52079 [details] patch with rename and reinterpret_cast
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.
Comment on attachment 52079 [details] patch with rename and reinterpret_cast R=me, but cq- since there is some style issue.
the style issue is a non-issue (see comments 6-8), unless that would cause the commit queue to not commit it?
Comment on attachment 52079 [details] patch with rename and reinterpret_cast OK. sorry, i missed the earlier discussion!
Comment on attachment 52079 [details] patch with rename and reinterpret_cast Clearing flags on attachment: 52079 Committed r56829: <http://trac.webkit.org/changeset/56829>
All reviewed patches have been landed. Closing bug.
Rolled out in http://trac.webkit.org/changeset/56849 due to inspector tests breakage.
LayoutTest failures for Chromium are being marked WontFix. The Bug is still accessible and referenced from TestExpectations.