Bug 45721 – Add a client interface to allow V8Proxy to ask whether an extension should run on a page

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Matt Perry]

This tracks chrome bug: http://code.google.com/p/chromium/issues/detail?id=53610

[Matt Perry]

Created attachment 67499 [details]
add method to FrameLoaderClient

[Adam Barth]

Comment on attachment 67499 [details]
add method to FrameLoaderClient

View in context: https://bugs.webkit.org/attachment.cgi?id=67499&action=prettypatch

This looks fine to me.  We can change how we grab the URL in a followup patch.  We'll need fishd to give the R+ since this touches the WebKit API.

> WebCore/bindings/v8/V8DOMWindowShell.cpp:343
> +        if (m_frame->loader()->client()->allowScriptExtension(extensions[i]->name(), m_frame->loader()->activeDocumentLoader()->url(), extensionGroup))
Ugg... That's not the right way to grab the URL.  I see that's what the old code does, but we should get it from the m_frame->document()->url().

[Adam Barth]

+fishd for WebKit API review.

[Darin Fisher (:fishd, Google)]

Comment on attachment 67499 [details]
add method to FrameLoaderClient

View in context: https://bugs.webkit.org/attachment.cgi?id=67499&action=prettypatch

> WebKit/chromium/public/WebFrameClient.h:305
> +    virtual bool allowScriptExtension(WebFrame*, const WebString& extension, const WebURL& url, int extensionGroup) { return false; }
nit: extension -> extensionName?

url -> documentURL?  actually, is this parameter necessary given that one can
just use WebFrame::url() to access the same value?

> WebKit/chromium/public/WebKitClient.h:243
> +    // given URL and script context group.
nit: "script context group" -> "extension group"

should there be some caller of this function?

[Matt Perry]

Comment on attachment 67499 [details]
add method to FrameLoaderClient

View in context: https://bugs.webkit.org/attachment.cgi?id=67499&action=prettypatch

> WebCore/bindings/v8/V8DOMWindowShell.cpp:343
> +        if (m_frame->loader()->client()->allowScriptExtension(extensions[i]->name(), m_frame->loader()->activeDocumentLoader()->url(), extensionGroup))
See the note above: we used to use the document()->url(), but it didn't work due to window.open navigating to about:blank first.

[Adam Barth]

(In reply to comment #5)
> (From update of attachment 67499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67499&action=prettypatch
> 
> > WebCore/bindings/v8/V8DOMWindowShell.cpp:343
> > +        if (m_frame->loader()->client()->allowScriptExtension(extensions[i]->name(), m_frame->loader()->activeDocumentLoader()->url(), extensionGroup))
> See the note above: we used to use the document()->url(), but it didn't work due to window.open navigating to about:blank first.

I suspect using the DocumentLoader isn't the right fix for that bug, but that's an issue for another patch.

[Matt Perry]

Comment on attachment 67499 [details]
add method to FrameLoaderClient

View in context: https://bugs.webkit.org/attachment.cgi?id=67499&action=prettypatch

I'm removing the URL parameter from the FrameLoaderClient interface, and moving the call to get the URL up into the WebKit/chromium layer. That's the highest layer where I still have access to the activeDocumentLoader, and if I can fix the bug in a different way and remove the need for the URL parameter entirely, there will be less interface to change.

> WebKit/chromium/public/WebFrameClient.h:305
> +    virtual bool allowScriptExtension(WebFrame*, const WebString& extension, const WebURL& url, int extensionGroup) { return false; }
Name changes made. See my reply to Adam about why we need the URL parameter.

> WebKit/chromium/public/WebKitClient.h:243
> +    // given URL and script context group.
Good catch - this was left over from an abandoned change. I'll reverted this file.

[Matt Perry]

Created attachment 67608 [details]
fixed with fishd's comments

[Darin Fisher (:fishd, Google)]

Comment on attachment 67608 [details]
fixed with fishd's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=67608&action=prettypatch

> WebKit/chromium/src/FrameLoaderClientImpl.cpp:156
> +    WebURL documentURL = m_webFrame->frame()->loader()->activeDocumentLoader()->url();
isn't this the same exact thing as:

  WebFrame* frame = ...;
  WebDataSource* ds = frame->provisionalDataSource();
  if (!ds)
    ds = frame->dataSource();
  WebURL url = ds->request().url();

In other words, you shouldn't need to pass documentURL here.

[Matt Perry]

Created attachment 67702 [details]
remove URL param

Thanks for the tip! I removed the URL param from the interface. (FYI, the chromium side of this change is here: http://codereview.chromium.org/3398001/show )

[Darin Fisher (:fishd, Google)]

Comment on attachment 67702 [details]
remove URL param

r=me

[Matt Perry]

Created attachment 68138 [details]
try #2, this time with a multi-stage patch

My last patch broke some things and got reverted. I'm going to try to land it again, in multiple stages this time.

The differences from the previous patch are:
- This is an additive change only. I added a registerExtension2 API that will be unused until a corresponding Chromium change lands.
- WebFrameClient defaults to allowing all script extensions. I think this is what broke the previous patch - test_shell and other test clients were probably preventing important v8 extensions from running.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/bindings/v8/V8Proxy.cpp:856:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/V8Proxy.h:131:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 12 files


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

[Matt Perry]

Created attachment 68139 [details]
style fixes

[Darin Fisher (:fishd, Google)]

Comment on attachment 68139 [details]
style fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=68139&action=review

> WebCore/bindings/v8/V8DOMWindowShell.cpp:344
> +            // Ensure our extension is always allowed.

nit: Ensure our date extension is always allowed.

> WebKit/chromium/public/WebFrameClient.h:305
> +    virtual bool allowScriptExtension(WebFrame*, const WebString& extensionName, int extensionGroup) { return true; }

should the magic extensionGroup with value 0 be defined here?

> WebKit/chromium/public/WebScriptController.h:62
> +    WEBKIT_API static void registerExtension2(v8::Extension*);

it may be easier to just call this registerExtension.  overriding based on arguments is fine.
that way you do not need to rename registerExtension2 back to registerExtension later.

[Matt Perry]

Created attachment 68287 [details]
fixed review comments

[Matt Perry]

Comment on attachment 68139 [details]
style fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=68139&action=review

>> WebCore/bindings/v8/V8DOMWindowShell.cpp:344
>> +            // Ensure our extension is always allowed.
> 
> nit: Ensure our date extension is always allowed.

Done.

>> WebKit/chromium/public/WebFrameClient.h:305
>> +    virtual bool allowScriptExtension(WebFrame*, const WebString& extensionName, int extensionGroup) { return true; }
> 
> should the magic extensionGroup with value 0 be defined here?

I added some comments.

>> WebKit/chromium/public/WebScriptController.h:62
>> +    WEBKIT_API static void registerExtension2(v8::Extension*);
> 
> it may be easier to just call this registerExtension.  overriding based on arguments is fine.
> that way you do not need to rename registerExtension2 back to registerExtension later.

Done. I was a bit wary to do this at first, since registerExtension already exists with the same signature. But I think the new version will have the same behavior as long as allowScriptExtension returns true.

[Matt Perry]

Created attachment 68288 [details]
fixed ChangeLog

[Darin Fisher (:fishd, Google)]

Comment on attachment 68288 [details]
fixed ChangeLog

r=me

[WebKit Commit Bot]

Comment on attachment 68288 [details]
fixed ChangeLog

Clearing flags on attachment: 68288

Committed r68061: <http://trac.webkit.org/changeset/68061>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.