Bug 90501 – Implement HTML's External interface

• Status: ASSIGNED
• Component: WebCore Misc.

[George Staikos]

Implement the window.external object and it's methods AddSearchProvider and IsSearchProviderInstalled.  Needs to be announced on webkit-dev still.  Is implemented as an optional Module so that ports can decide to support it or not.  It is a bit unorthodox in design in my opinion, and I won't be surprised to see changes or lack of desire to support it.  Nevertheless, my port does want to use it.

[George Staikos]

Created attachment 150682 [details]
First draft implementing the feature.

Yes, tests are still needed too.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/CMakeLists.tx..." exit_code: 1
Source/WebCore/page/ChromeClient.h:357:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/Modules/external/External.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 23 files


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

[Gyuyoung Kim]

Comment on attachment 150682 [details]
First draft implementing the feature.

Attachment 150682 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13124663

[Build Bot]

Comment on attachment 150682 [details]
First draft implementing the feature.

Attachment 150682 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13137512

[George Staikos]

Created attachment 150687 [details]
Updated second draft, possibly fixing a few ports.

[Build Bot]

Comment on attachment 150687 [details]
Updated second draft, possibly fixing a few ports.

Attachment 150687 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13132599

[George Staikos]

Created attachment 150706 [details]
Updated third draft, possibly fixing a few ports.

[Build Bot]

Comment on attachment 150706 [details]
Updated third draft, possibly fixing a few ports.

Attachment 150706 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13134620

[George Staikos]

Created attachment 150709 [details]
Updated fourth draft, trying to fix win32

[George Staikos]

Looks like we just need tests and it will be good to go.

[Adam Barth]

Comment on attachment 150709 [details]
Updated fourth draft, trying to fix win32

This patch contains a use-after-free vulnerability.  Please make sure you have someone review this patch who understands how we manage memory for the major webcore objects.

[Nikolas Zimmermann]

Comment on attachment 150709 [details]
Updated fourth draft, trying to fix win32

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

Looks good to me on first sight, except missing tests and minor comments.
I guess Adam is worried that DOMWindow is destructed before External is, right?
If so we might want to clear the DOMWindow pointer in External, as soon as clearDOMWindowProperties() is called. "m_external = 0" doesn't necessarily free the External object, as JS may still hold a reference.

Is it possible/desired that External keeps DOMWindow alive? (Thinking about a custom mark() logic, if that still exists... JSC is transitioning fast... :-)

> Source/WebCore/ChangeLog:3
> +        Implement HTML5 6.5.2 "The External Interface" and its functions

Nitpicking: This should really only contain the bug title.
Bug title
https:///

Bug desc.

> Source/WebCore/ChangeLog:10
> + 

I wish the ChangeLog was a bit more descriptive.

> Source/WebCore/Modules/external/External.cpp:32
> +External::External(const DOMWindow *window)
> +: m_window(window)

Weird that style-ews doesn't complain here: "const DOMWindow* window"
and "    : m_window(window)" is the preferred style."

> Source/WebCore/Modules/external/External.cpp:40
> +void External::addSearchProvider(const String& url, ExceptionCode&)

Does the spec define to raise exceptions here? If not, can we remove the raises(DOMException) from the IDL?

> Source/WebCore/Modules/external/External.h:41
> +private:
> +    External(const DOMWindow*);
> +
> +    const DOMWindow* m_window;
> +

It's a bit unusual to place private: before public: in our headers, you could swap those sections.

> Source/WebCore/page/ChromeClient.h:359
> +#if ENABLE(WINDOW_EXTERNAL)
> +        virtual void addSearchProvider(const String&) { }
> +        virtual unsigned long isSearchProviderInstalled(const String&) { return 0; }
> +#endif

WINDOW_EXTERNAL is quite confusing. How about HTML5_EXTERNAL_INTERFACE? or even HTML5_SEARCH_PROVIDERS?

[Yong Li]

(In reply to comment #11)
> (From update of attachment 150709 [details])
> This patch contains a use-after-free vulnerability.  Please make sure you have someone review this patch who understands how we manage memory for the major webcore objects.

Adam, 

Do you mean an External could outlive its DOMWindow so we should use RefPtr<DOMWindow> here instead of DOMWindow*?

[Adam Barth]

Using RefPtr<DOMWindow> would create a reference cycle and leak memory.  I'm off for the fourth of July.  Please make sure the patch gets a high quality review before landing it.

[Yong Li]

(In reply to comment #14)
> Using RefPtr<DOMWindow> would create a reference cycle and leak memory.  I'm off for the fourth of July.  Please make sure the patch gets a high quality review before landing it.

DOMWindow::clearDOMWindowProperties() will be called when frame is destroyed or new load is committed, which will then clear the External object. So there shouldn't be reference cycle. But it would be nice if we could avoid using it.

[George Staikos]

(In reply to comment #14)
> Using RefPtr<DOMWindow> would create a reference cycle and leak memory.  I'm off for the fourth of July.  Please make sure the patch gets a high quality review before landing it.

The patch isn't really up for review yet, but I appreciate the attention.  Thanks for that.  Actually it has a massive merge error in it too.  It was written against much older baseline webkit.  I'll turn on the review flag when it's ready.  I'm definitely interested in higher-level comments now too, as I reference in my original bug description.  Is there wider desire to support this?

[Adam Barth]

Sorry for the brevity earlier.  You should make External a subclass of DOMWindowProperty and implement the appropriate virtual functions.

Note: You should email webkit-dev as described in http://www.webkit.org/coding/adding-features.html.

[George Staikos]

(In reply to comment #17)
> Sorry for the brevity earlier.  You should make External a subclass of DOMWindowProperty and implement the appropriate virtual functions.

Yes, I noticed later that this had to be changed which is one of the main reasons for not setting r? yet.  This code was developed a while ago against an older baseline and badly merged.  Enough to get something saved and figure out the build for other ports though.

> Note: You should email webkit-dev as described in http://www.webkit.org/coding/adding-features.html.

Exactly.  As per comment #0.

I'll try to post an updated patch by the weekend.

Created attachment 170740 [details]
Patch

Comment on attachment 170740 [details]
Patch

In this patch we removed the changes to the make files of other ports. It's safer if members in the other ports add the external files to their build system themselves.

External now implements DOMWindowProperty as per Adam Barth's suggestion.

We also added DRT tests.

[Adam Barth]

Comment on attachment 170740 [details]
Patch

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

Below is a technical review of your patch.  I'm not sure whether WebKit should implement this feature or not.  My understanding is that we've historically believed that embedders of WebKit should implement this feature themselves rather than implementing it in the engine directly.

> Source/WebCore/Modules/external/External.cpp:33
> +: DOMWindowProperty(frame)

Bad indent

> Source/WebCore/Modules/external/External.h:27
> +#include "PassRefPtr.h"
> +#include "RefCounted.h"

Usually we include things from wtf in the following style:

#include <wtf/RefCounted.h>

> Source/WebCore/Modules/external/External.h:31
> +namespace WTF {
> +class String;
> +}

I would just #include <wtf/text/WTFString.h>.  There isn't much to be gained from forward declaring this class since essentially every compilation unit needs to include it anyway.

> Source/WebCore/Modules/external/External.h:36
> +class DOMWindow;
> +class Frame;

By contrast, these are well worth forward declaring.  :)

> Source/WebCore/Modules/external/External.idl:26
> +        void AddSearchProvider(in DOMString engineURL) raises(DOMException);

Yuck.  These functions are in the wrong style.  :(

> Source/WebCore/page/Chrome.cpp:569
> +#if ENABLE(WINDOW_EXTERNAL)
> +void Chrome::addSearchProvider(const String& url)
> +{
> +    m_client->addSearchProvider(url);
> +}
> +
> +unsigned long Chrome::isSearchProviderInstalled(const String& url)
> +{
> +    return m_client->isSearchProviderInstalled(url);
> +}
> +#endif

There is no reason to have these functions.  Callers cal just talk to chrome()->client() directly.

> Source/WebCore/page/DOMWindow.idl:225
> +#if defined(ENABLE_WINDOW_EXTERNAL) && ENABLE_WINDOW_EXTERNAL
> +    attribute [Replaceable] External external;
> +#endif

You've half made your feature into a module (a la http://trac.webkit.org/wiki/Modules ) and half not.  You should pick one approach or the other.  In the case of this feature, I wouldn't bother making it a module since it is only one file and it is defined in the core HTML5 specification.

Rather than using #if, you can use the Conditional IDL attribute.

[Anne van Kesteren]

Note that at this point this is effectively a useless API that probably cannot be removed from Chrome/Firefox: https://html.spec.whatwg.org/multipage/obsolete.html#Window-partial. Implementing it would make WebKit-based browsers less different from other browsers.

Note that per https://github.com/whatwg/html/pull/4296 the plan is to expose window.External as well, in order to get rid of [NoInterfaceObject] in IDL long term.