Bug 40079 – [Qt] Support evaluateScriptInIsolatedWorld()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Robert Hogan]

This initial implementation passes 25 of the 27 tests in
http/tests/isolatedworld as well as four others that use the
call in LayoutTests.

Further work/investigation is required to support the two
remaining tests.

[Robert Hogan]

Created attachment 57690 [details]
Patch

[Adam Barth]

Comment on attachment 57690 [details]
Patch

WebKit/qt/Api/qwebframe.cpp:1380
 +          JSC::JSValue v = d->frame->script()->executeScriptInWorld(scriptWorld->world(), scriptSource, true).jsValue();
Why not use the |proxy| local variable here?

WebKit/qt/Api/qwebframe.cpp:1381
 +          int distance = 0;
Move this below the if?

WebKit/qt/Api/qwebscriptworld.cpp:35
 +    d = new QWebScriptWorldPrivate(ScriptController::createWorld());
Bad indent

WebKit/qt/Api/qwebscriptworld.h:45
 +  private:
Why the second private declaration?

WebKit/qt/Api/qwebscriptworld_p.h:35
 +      }
Maybe add a blank line after this { } and the next?  Not sure what the style here is supposed to be.

[Robert Hogan]

Committed r61478: <http://trac.webkit.org/changeset/61478>

[Simon Hausmann]

(In reply to comment #3)
> Committed r61478: <http://trac.webkit.org/changeset/61478>

This commit adds new API to QtWebKit that uses WebCore types. Is that intentional Robert?

Was it meant to be part of DumpRenderTreeSupport?

[Jocelyn Turcotte]

This patch broke the trunk build on MSVC2008:

qwebframe.cpp
c:\dev\build-qt-47\include\qtcore\../../../qt/src/corelib/tools/qshareddata.h(159) : error C2027: use of undefined type 'QWebScriptWorldPrivate'
        c:\dev\webkit\webkit\qt\api\qwebscriptworld.h(32) : see declaration of 'QWebScriptWorldPrivate'
        c:\dev\build-qt-47\include\qtcore\../../../qt/src/corelib/tools/qshareddata.h(159) : while compiling class template member function 'QExplicitlySharedDataPointer<T>::~QExplicitlySharedDataPointer(void)'
        with
        [
            T=QWebScriptWorldPrivate
        ]
        c:\dev\webkit\webkit\qt\api\qwebscriptworld.h(43) : see reference to class template instantiation 'QExplicitlySharedDataPointer<T>' being compiled
        with
        [
            T=QWebScriptWorldPrivate
        ]
c:\dev\build-qt-47\include\qtcore\../../../qt/src/corelib/tools/qshareddata.h(159) : error C2227: left of '->ref' must point to class/struct/union/generic type
c:\dev\build-qt-47\include\qtcore\../../../qt/src/corelib/tools/qshareddata.h(159) : error C2228: left of '.deref' must have class/struct/union


I'm not sure if it is possible to use a QExplicitlySharedDataPointer without having the full QWebScriptWorldPrivate type included. The fix is not trivial so it would be nice if you can have a look at it.

[Robert Hogan]

(In reply to comment #4)
> (In reply to comment #3)
> > Committed r61478: <http://trac.webkit.org/changeset/61478>
> 
> This commit adds new API to QtWebKit that uses WebCore types. Is that intentional Robert?
> 
> Was it meant to be part of DumpRenderTreeSupport?

Ultimately I just wanted to pass the tests, but when I looked at the implementation of other ports I noticed they all supported it through public API. (windows: stringByEvaluatingJavaScriptInScriptWorld(), chromium: executeScriptInIsolatedWorld()) so I did the same.

Seemed a waste to just support through DRTSupport..

On reflection though - should have sought review from Qt people before committing for an API change.

[Robert Hogan]

(In reply to comment #5)
> This patch broke the trunk build on MSVC2008:
> 
> qwebframe.cpp
> c:\dev\build-qt-47\include\qtcore\../../../qt/src/corelib/tools/qshareddata.h(159) : error C2027: use of undefined type 'QWebScriptWorldPrivate'
>         c:\dev\webkit\webkit\qt\api\qwebscriptworld.h(32) : see declaration of 'QWebScriptWorldPrivate'
>         c:\dev\build-qt-47\include\qtcore\../../../qt/src/corelib/tools/qshareddata.h(159) : while compiling class template member function 'QExplicitlySharedDataPointer<T>::~QExplicitlySharedDataPointer(void)'
>         with
>         [
>             T=QWebScriptWorldPrivate
>         ]
>         c:\dev\webkit\webkit\qt\api\qwebscriptworld.h(43) : see reference to class template instantiation 'QExplicitlySharedDataPointer<T>' being compiled
>         with
>         [
>             T=QWebScriptWorldPrivate
>         ]
> c:\dev\build-qt-47\include\qtcore\../../../qt/src/corelib/tools/qshareddata.h(159) : error C2227: left of '->ref' must point to class/struct/union/generic type
> c:\dev\build-qt-47\include\qtcore\../../../qt/src/corelib/tools/qshareddata.h(159) : error C2228: left of '.deref' must have class/struct/union
> 
> 
> I'm not sure if it is possible to use a QExplicitlySharedDataPointer without having the full QWebScriptWorldPrivate type included. The fix is not trivial so it would be nice if you can have a look at it.

As far as I can tell, my implementation is the same as QWebSecurityOriginPrivate. 

Does it mean I need to include DOMWrapperWorld.h in qwebscriptworld_p.h ?

[Laszlo Gombos]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Committed r61478: <http://trac.webkit.org/changeset/61478>
> > 
> > This commit adds new API to QtWebKit that uses WebCore types. Is that intentional Robert?
> > 
> > Was it meant to be part of DumpRenderTreeSupport?
> 
> Ultimately I just wanted to pass the tests, but when I looked at the implementation of other ports I noticed they all supported it through public API. (windows: stringByEvaluatingJavaScriptInScriptWorld(), chromium: executeScriptInIsolatedWorld()) so I did the same.
> 
> Seemed a waste to just support through DRTSupport..
> 
> On reflection though - should have sought review from Qt people before committing for an API change.

Yes, we need a proper API review for this CCing Simon and Kenneth.

One issue I notices is that this exposes WebCore internal structures (WebCore::DOMWrapperWorld) publicly which is a bad idea. 

REOPEN the bug to address the WebCore exposure and to make sure this gets an API discussion.

[Robert Hogan]

(In reply to comment #8)
> 
> One issue I notices is that this exposes WebCore internal structures (WebCore::DOMWrapperWorld) publicly which is a bad idea. 
> 

Ah, OK. So just have QWebFrame::evaluateScriptInIsolatedWorld(script) and don't export qwebscriptworld.h?

[Jocelyn Turcotte]

(In reply to comment #9)
> Ah, OK. So just have QWebFrame::evaluateScriptInIsolatedWorld(script) and don't export qwebscriptworld.h?

If I understood correctly, this functionnality in WebCore serves to run user-provided javascripts more securely (as with Chrome extensions).

I think that it would be nice to know real world use cases for apps using QtWebKit for this function, to try solving an actual problem and not just try exposing a mechanism.

This would help prevent cumbersome APIs that we might have to step over if we want to further extend QtWebKit scripting possibilities in the future.

[Robert Hogan]

Created attachment 59684 [details]
Patch

[Robert Hogan]

(In reply to comment #10)
> (In reply to comment #9)
> > Ah, OK. So just have QWebFrame::evaluateScriptInIsolatedWorld(script) and don't export qwebscriptworld.h?
> 
> If I understood correctly, this functionnality in WebCore serves to run user-provided javascripts more securely (as with Chrome extensions).
> 
> I think that it would be nice to know real world use cases for apps using QtWebKit for this function, to try solving an actual problem and not just try exposing a mechanism.
> 
> This would help prevent cumbersome APIs that we might have to step over if we want to further extend QtWebKit scripting possibilities in the future.

Agreed that this was the discussion that needed to happen before adding API. So this patch moves the functionality back into DRT where it appears to belong for now!

[Robert Hogan]

Committed r61879: <http://trac.webkit.org/changeset/61879>

[Yaohan Chen]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Ah, OK. So just have QWebFrame::evaluateScriptInIsolatedWorld(script) and don't export qwebscriptworld.h?
> > 
> > If I understood correctly, this functionnality in WebCore serves to run user-provided javascripts more securely (as with Chrome extensions).
> > 
> > I think that it would be nice to know real world use cases for apps using QtWebKit for this function, to try solving an actual problem and not just try exposing a mechanism.
> > 
> > This would help prevent cumbersome APIs that we might have to step over if we want to further extend QtWebKit scripting possibilities in the future.
> 
> Agreed that this was the discussion that needed to happen before adding API. So this patch moves the functionality back into DRT where it appears to belong for now!

Thanks for working to support isolated world in QtWebkit, but I am disappointed that the decision seems to be to not expose the API now. I am still confused about the plans for this feature: obviously without the API applications using QtWebkit would not be using the feature, so what would be needed before the API can be added?

It seems to me a typical use case would be that a browser can provide extra API similar to GreaseMonkey's GM_XMLHttpRequest or Chrome's chrome.extension.* to userscripts or javascript extensions, but not to website scripts, using isolated world. addToJavaScriptWindowObject already makes it possible to provide such features, but am I correct that it would also expose them to website scripts, and this is where isolated worlds would be useful? This thread between a Uzbl (a Webkit-GTK browser) developer and Webkit developers <https://lists.webkit.org/pipermail/webkit-dev/2010-January/011122.html> seems to suggest so. If that is correct, could there be an API in QtWebkit to allow the application to expose a Qt Object to an isolated world and evaluate javascript in it?