Bug 29031 – [v8] speed up access to windows impl from various methods

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[anton muhin]

Currently we use lookupDOMWrapper to find out a proper JS wrapper which allows access to window C++ counterpart and stuff.  Let's access it immediately from holder (but that requires some duplication)

[anton muhin]

Created attachment 39183 [details]
First pass

Guys, I'd esp. ask to review security-related changes as my knowledge of this domain is limited.

And sorry if svn-preparePatch put wrong paths---I'll fix it in a next round.

[anton muhin]

Created attachment 39184 [details]
ChangeLog fixed

[Adam Barth]

Comment on attachment 39184 [details]
ChangeLog fixed

I think this is ok.  The dangerous piece is in installDOMWindow, but that should run before any evil JS has had a chance to screw up the jsWindow's prototype chain.

[Adam Barth]

Comment on attachment 39184 [details]
ChangeLog fixed

Actually, I lied.

[Adam Barth]

Can you add the following test case:

Evil window creates an iframe to honest domain.  Evil window then sets its __proto__ to the iframe's window object.  Now, try to access properties that are normally on the WindowPrototype and see if you get the wrong answers w.r.t. security.

It might be ok.  I'll have to think about it more, but we should have the test case in any event.

[Adam Barth]

I went over this this patch in detail with Anton.  Everything looks fine except for the NAMED and INDEX access checks function.  It's unclear whether |host| is always the same object as |holder|.  If the objects are different, we might be access checking one object and operating on another object.  He's going to investigate and write some tests.

[anton muhin]

Created attachment 39853 [details]
Second iteration

Another pass.  It passes newly added layout tests.

The major difference is lookupDOMWrapper is restored in accessors checks.  I believe the reason is not security, but some technical details (if you're curious: the way v8 is implemented it installs security callback on global object proxy and I don't want to put pointers to window impl on it).  I'll address those issues with a separate CL.

IMPORTANT NOTE (sorry for the case): please, do not land this patch: if it passes a review, I'd have to add a helper method to public V8 API (to lookup real named property).

[Adam Barth]

Comment on attachment 39853 [details]
Second iteration

This looks great.  Much easier to understand than the last round.  One question:

+namespace v8 {
+Handle<Value> GetRealNamedProperty(
+      Handle<Object> object,
+      Handle<String> key);
+}

Shouldn't this be in a v8 header file?  As written, it looks like it's grabbing at some random symbol in V8.

[anton muhin]

(In reply to comment #8)
> (From update of attachment 39853 [details])
> This looks great.  Much easier to understand than the last round.  One
> question:
> 
> +namespace v8 {
> +Handle<Value> GetRealNamedProperty(
> +      Handle<Object> object,
> +      Handle<String> key);
> +}
> 
> Shouldn't this be in a v8 header file?  As written, it looks like it's grabbing
> at some random symbol in V8.

You're right, Adam---I just didn't want to touch v8's header, but still have something compilable.  And exactly for same reason I asked not to land this patch as I want to add this function into public V8 API.

So I'll ask for another round when this lookup goes into v8.

Thanks a lot for review, Adam!

[anton muhin]

Created attachment 40534 [details]
Another iteration: now we have necessary method in V8 public API

Guys,

that's exactly the same patch, but now with the resolved dep on public V8 API

[Adam Barth]

Comment on attachment 40534 [details]
Another iteration: now we have necessary method in V8 public API

I just saw your committer paperwork go by, so you can either land this one yourself, or we can put it though commit queue.

[anton muhin]

(In reply to comment #11)
> (From update of attachment 40534 [details])
> I just saw your committer paperwork go by, so you can either land this one
> yourself, or we can put it though commit queue.

If you please, put it into commit queue---paper travels very slow over the ocean :)

[WebKit Commit Bot]

Comment on attachment 40534 [details]
Another iteration: now we have necessary method in V8 public API

Rejecting patch 40534 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=40534 from bug 29031 failed to download and apply.

[Eric Seidel (no email)]

Comment on attachment 39853 [details]
Second iteration

Obsoleting an older revision of this patch.

[Eric Seidel (no email)]

Comment on attachment 40534 [details]
Another iteration: now we have necessary method in V8 public API

IIRC the CodeGeneratorV8.pm diff failed to apply.

[anton muhin]

(In reply to comment #15)
> (From update of attachment 40534 [details])
> IIRC the CodeGeneratorV8.pm diff failed to apply.

Eric, anything I can help with?  I synced to 49163 and svn up'ed it all fine.

[anton muhin]

Created attachment 40796 [details]
And another attempt, for WebKit@49221

Adam, Eric,

that's just successfully compiled (on Mac) version of the same patch.

[Adam Barth]

Comment on attachment 40796 [details]
And another attempt, for WebKit@49221

Let's give it a spin.

[WebKit Commit Bot]

Comment on attachment 40796 [details]
And another attempt, for WebKit@49221

Clearing flags on attachment: 40796

Committed r49248: <http://trac.webkit.org/changeset/49248>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.