Bug 83322 – [Qt] Fix WebKit1 build with V8

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Balazs Kelemen]

It's broken currently. The main issue is that the bindings are expecting a more up-to-date v8 version than what we have in qtjsbackend.

[Balazs Kelemen]

Created attachment 136018 [details]
Patch

[Balazs Kelemen]

Created attachment 136020 [details]
Patch

[Balazs Kelemen]

Comment on attachment 136020 [details]
Patch

It doesn't build without explicitly setting --no-webkit2. I'm working on the fix.

[Balazs Kelemen]

Created attachment 136034 [details]
Patch

[Adam Barth]

Comment on attachment 136034 [details]
Patch

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

> Source/WebCore/bindings/v8/ScriptState.cpp:138
> +#if PLATFORM(QT)
> +    // The v8 version in qtjsbackend does not have this API yet.
> +    return true;
> +#else

Can we update the V8 version in qtjsbackend?  This code is necessary for security.  You need to have a version of V8 that supports this feature in order when using a version of WebKit that contains this code.

[Balazs Kelemen]

(In reply to comment #5)
> (From update of attachment 136034 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136034&action=review
> 
> > Source/WebCore/bindings/v8/ScriptState.cpp:138
> > +#if PLATFORM(QT)
> > +    // The v8 version in qtjsbackend does not have this API yet.
> > +    return true;
> > +#else
> 
> Can we update the V8 version in qtjsbackend?  This code is necessary for security.  You need to have a version of V8 that supports this feature in order when using a version of WebKit that contains this code.

The update is not trivial because the Qt's Qml module need to be adjusted to the new version. It won't be a big issue if Qml did not depend on a patched v8 but in fact it does. Notable efforts have been done to upstream the changes but it did not get positive feedback from the v8 team. However, Peter Varga (stampho) is continuosly working on the update so hopefully we will get an up-to-date version soon. Anyway, I don't think that this security issue should stop us from setting up Qt+v8 without actually making it the default configuration.
Btw, thanks for warn us about the issue! Could you describe the problem? Could you recommend some workaround until we don't have an up-to-date v8?

[Adam Barth]

> Could you describe the problem?

Content-Security-Policy implement most of, but not all of, it's content restrictions.  In particular, eval isn't blocked when it should be.

> Could you recommend some workaround until we don't have an up-to-date v8?

There isn't really a workaround (other than to disable Content-Security-Policy entirely), which is why we added this API to V8.

[Adam Barth]

> Anyway, I don't think that this security issue should stop us from setting up Qt+v8 without actually making it the default configuration.

We're going to continue to add code to the V8 bindings in WebKit that depend on having the most recent version of V8.  If you're not able to freely update your version of V8, you're going to keep running into this problem.

There's a bunch of stuff in your patch that makes sense to land, but I don't think we should be disabling security features because you're not able to update V8.  IMHO, you'll probably need to unfork your copy of V8 before you'll be able to use the V8 bindings anyway.

[Simon Hausmann]

(In reply to comment #8)
> > Anyway, I don't think that this security issue should stop us from setting up Qt+v8 without actually making it the default configuration.
> 
> We're going to continue to add code to the V8 bindings in WebKit that depend on having the most recent version of V8.  If you're not able to freely update your version of V8, you're going to keep running into this problem.
> 
> There's a bunch of stuff in your patch that makes sense to land, but I don't think we should be disabling security features because you're not able to update V8.  IMHO, you'll probably need to unfork your copy of V8 before you'll be able to use the V8 bindings anyway.

I agree with Adam here. If there is anything missing in Qt's copy of V8, then we should include it instead of working around it here, especially when it comes to small API additions/changes required for security purposes. Usually these changes apply easily. (put me or Kent into the reviewer field in Gerrit)

That said, unforking is not an option right now due to the terms of the contribution agreement required by Google for V8.

[Balazs Kelemen]

(In reply to comment #9)
> (In reply to comment #8)
> > > Anyway, I don't think that this security issue should stop us from setting up Qt+v8 without actually making it the default configuration.
> > 
> > We're going to continue to add code to the V8 bindings in WebKit that depend on having the most recent version of V8.  If you're not able to freely update your version of V8, you're going to keep running into this problem.
> > 
> > There's a bunch of stuff in your patch that makes sense to land, but I don't think we should be disabling security features because you're not able to update V8.  IMHO, you'll probably need to unfork your copy of V8 before you'll be able to use the V8 bindings anyway.
> 
> I agree with Adam here. If there is anything missing in Qt's copy of V8, then we should include it instead of working around it here, especially when it comes to small API additions/changes required for security purposes. Usually these changes apply easily. (put me or Kent into the reviewer field in Gerrit)
> 
> That said, unforking is not an option right now due to the terms of the contribution agreement required by Google for V8.

I'm not sure cherry picking these API additions is worthwhile. Peter has already uploaded some patches to gerrit (CC'ed you and Kent) for the update and his plan is to reach the present of v8 in qtjsbackend ASAP. We are planning to set up a new submodule repository and keep v8 always up-to-date there. So, until we have a well-defined update process we could support building v8+qtwebkit with this repository. What do you think?

[Andras Becsi]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > > Anyway, I don't think that this security issue should stop us from setting up Qt+v8 without actually making it the default configuration.
> > > 
> > > We're going to continue to add code to the V8 bindings in WebKit that depend on having the most recent version of V8.  If you're not able to freely update your version of V8, you're going to keep running into this problem.
> > > 
> > > There's a bunch of stuff in your patch that makes sense to land, but I don't think we should be disabling security features because you're not able to update V8.  IMHO, you'll probably need to unfork your copy of V8 before you'll be able to use the V8 bindings anyway.
> > 
> > I agree with Adam here. If there is anything missing in Qt's copy of V8, then we should include it instead of working around it here, especially when it comes to small API additions/changes required for security purposes. Usually these changes apply easily. (put me or Kent into the reviewer field in Gerrit)
> > 
> > That said, unforking is not an option right now due to the terms of the contribution agreement required by Google for V8.
> 
> I'm not sure cherry picking these API additions is worthwhile. Peter has already uploaded some patches to gerrit (CC'ed you and Kent) for the update and his plan is to reach the present of v8 in qtjsbackend ASAP. We are planning to set up a new submodule repository and keep v8 always up-to-date there. So, until we have a well-defined update process we could support building v8+qtwebkit with this repository. What do you think?

I tend to agree that if it is possible, we should allow to build QtWebKit + v8 with an upstream copy of v8, at least until we have a proper update process for the v8 copy in Qt5.

But there were some issues with QtScript and this was the cause why we decided to use the Qt5 v8 copy. Not sure whether these would be still valid now.

Cherry picking changes would in my opinion only escalate these issues since AFAIK now there are only a few additional patches (~10) on top of the v8 copy in Qt5.
And there is apparently a continuous effort from Peter's side on shrinking this number.

[Simon Hausmann]

(In reply to comment #11)
[...]
> Cherry picking changes would in my opinion only escalate these issues since AFAIK now there are only a few additional patches (~10) on top of the v8 copy in Qt5.

I think cherry-picking a small upstream change that will automatically disappear with the next rebase comes at a significantly lower cost than adding a change that implements new behaviour and requires continued maintenance across rebases.

> And there is apparently a continuous effort from Peter's side on shrinking this number.

Which is great, btw :)

[Balazs Kelemen]

Created attachment 136915 [details]
Patch

[Balazs Kelemen]

(In reply to comment #13)
> Created an attachment (id=136915) [details]
> Patch

No ifdef's, no disabled features :)
To build with --v8 you need to use qtjsbackend from https://qt.gitorious.org/~stampho/qt/stamphos-qtjsbackend

[Adam Barth]

Comment on attachment 136915 [details]
Patch

Yay!  Thanks.

[Build Bot]

Comment on attachment 136915 [details]
Patch

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

[Balazs Kelemen]

Created attachment 136924 [details]
Speculative Windows build fix

[Adam Barth]

Comment on attachment 136924 [details]
Speculative Windows build fix

Looks like your windows build fix succeeded.

[Balazs Kelemen]

Comment on attachment 136924 [details]
Speculative Windows build fix

Landed in http://trac.webkit.org/changeset/114042