Bug 83322 - [Qt] Fix WebKit1 build with V8
Summary: [Qt] Fix WebKit1 build with V8
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 76778
  Show dependency treegraph
 
Reported: 2012-04-05 15:41 PDT by Balazs Kelemen
Modified: 2012-04-12 15:57 PDT (History)
12 users (show)

See Also:


Attachments
Patch (22.98 KB, patch)
2012-04-06 07:12 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (23.08 KB, patch)
2012-04-06 07:23 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (24.87 KB, patch)
2012-04-06 09:28 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (22.34 KB, patch)
2012-04-12 08:27 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Speculative Windows build fix (22.31 KB, patch)
2012-04-12 10:14 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2012-04-05 15:41:14 PDT
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.
Comment 1 Balazs Kelemen 2012-04-06 07:12:42 PDT
Created attachment 136018 [details]
Patch
Comment 2 Balazs Kelemen 2012-04-06 07:23:24 PDT
Created attachment 136020 [details]
Patch
Comment 3 Balazs Kelemen 2012-04-06 08:49:26 PDT
Comment on attachment 136020 [details]
Patch

It doesn't build without explicitly setting --no-webkit2. I'm working on the fix.
Comment 4 Balazs Kelemen 2012-04-06 09:28:24 PDT
Created attachment 136034 [details]
Patch
Comment 5 Adam Barth 2012-04-06 11:16:28 PDT
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.
Comment 6 Balazs Kelemen 2012-04-06 14:25:54 PDT
(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?
Comment 7 Adam Barth 2012-04-06 14:53:49 PDT
> 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.
Comment 8 Adam Barth 2012-04-06 14:56:41 PDT
> 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.
Comment 9 Simon Hausmann 2012-04-09 23:59:25 PDT
(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.
Comment 10 Balazs Kelemen 2012-04-10 07:38:57 PDT
(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?
Comment 11 Andras Becsi 2012-04-10 08:00:35 PDT
(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.
Comment 12 Simon Hausmann 2012-04-11 05:54:18 PDT
(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 :)
Comment 13 Balazs Kelemen 2012-04-12 08:27:17 PDT
Created attachment 136915 [details]
Patch
Comment 14 Balazs Kelemen 2012-04-12 08:30:17 PDT
(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
Comment 15 Adam Barth 2012-04-12 08:35:00 PDT
Comment on attachment 136915 [details]
Patch

Yay!  Thanks.
Comment 16 Build Bot 2012-04-12 08:49:15 PDT
Comment on attachment 136915 [details]
Patch

Attachment 136915 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12392444
Comment 17 Balazs Kelemen 2012-04-12 10:14:49 PDT
Created attachment 136924 [details]
Speculative Windows build fix
Comment 18 Adam Barth 2012-04-12 10:54:09 PDT
Comment on attachment 136924 [details]
Speculative Windows build fix

Looks like your windows build fix succeeded.
Comment 19 Balazs Kelemen 2012-04-12 15:57:10 PDT
Comment on attachment 136924 [details]
Speculative Windows build fix

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