Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(705)

Issue 7366: Split window support from V8. ... (Closed)

Created:
12 years, 2 months ago by Feng Qian
Modified:
9 years, 7 months ago
CC:
v8-dev
Visibility:
Public.

Description

Split window support from V8. Here is a description of the background and design of split window in Chrome and V8: https://docs.google.com/a/google.com/Doc?id=chhjkpg_47fwddxbfr This change list splits the window object into two parts: 1) an inner window object used as the global object of contexts; 2) an outer window object exposed to JavaScript and accessible by the name 'window'. Firefox did it awhile ago, here are some discussions: https://wiki.mozilla.org/Gecko:SplitWindow. One additional benefit of splitting window in Chrome is that accessing global variables don't need security checks anymore, it can improve applications that use many global variables. V8 support of split window: There are a small number of changes on V8 api to support split window: Security context is removed from V8, so does related API functions; A global object can be detached from its context and reused by a new context; Access checks on an object template can be turned on/off by default; An object can turn on its access checks later; V8 has a new object type, ApiGlobalObject, which is the outer window object type. The existing JSGlobalObject becomes the inner window object type. Security checks are moved from JSGlobalObject to ApiGlobalObject. ApiGlobalObject is the one exposed to JavaScript, it is accessible through Context::Global(). ApiGlobalObject's prototype is set to JSGlobalObject so that property lookups are forwarded to JSGlobalObject. ApiGlobalObject forwards all other property access requests to JSGlobalObject, such as SetProperty, DeleteProperty, etc. Security token is moved to a global context, and ApiGlobalObject has a reference to its global context. JSGlobalObject has a reference to its global context as well. When accessing properties on a global object in JavaScript, the domain security check is performed by comparing the security token of the lexical context (Top::global_context()) to the token of global object's context. The check is only needed when the receiver is a window object, such as 'window.document'. Accessing global variables, such as 'var foo = 3; foo' does not need checks because the receiver is the inner window object. When an outer window is detached from its global context (when a frame navigates away from a page), it is completely detached from the inner window. A new context is created for the new page, and the outer global object is reused. At this point, the access check on the DOMWindow wrapper of the old context is turned on. The code in old context is still able to access DOMWindow properties, but it has to go through domain security checks. It is debatable on how to implement the outer window object. Currently each property access function has to check if the receiver is ApiGlobalObject type. This approach might be error-prone that one may forget to check the receiver when adding new functions. It is unlikely a performance issue because accessing global variables are more common than 'window.foo' style coding. I am still working on the ARM port, and I'd like to hear comments and suggestions on the best way to support it in V8. Committed: http://code.google.com/p/v8/source/detail?r=540

Patch Set 1 #

Patch Set 2 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+810 lines, -415 lines) Patch
M include/v8.h View 1 5 chunks +22 lines, -7 lines 0 comments Download
M src/api.h View 1 7 chunks +4 lines, -24 lines 0 comments Download
M src/api.cc View 1 11 chunks +88 lines, -45 lines 0 comments Download
M src/bootstrapper.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M src/bootstrapper.cc View 1 11 chunks +167 lines, -73 lines 0 comments Download
M src/builtins.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M src/builtins-ia32.cc View 1 2 chunks +4 lines, -2 lines 0 comments Download
M src/codegen-ia32.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M src/codegen-ia32.cc View 1 6 chunks +19 lines, -9 lines 0 comments Download
M src/contexts.h View 1 3 chunks +9 lines, -1 line 0 comments Download
M src/contexts.cc View 1 1 chunk +9 lines, -0 lines 0 comments Download
M src/debug.h View 1 1 chunk +0 lines, -1 line 0 comments Download
M src/debug.cc View 1 1 chunk +0 lines, -1 line 0 comments Download
M src/factory.h View 1 1 chunk +9 lines, -2 lines 0 comments Download
M src/factory.cc View 1 6 chunks +23 lines, -13 lines 0 comments Download
M src/handles.h View 1 2 chunks +3 lines, -4 lines 0 comments Download
M src/handles.cc View 1 4 chunks +6 lines, -10 lines 0 comments Download
M src/heap.h View 1 1 chunk +4 lines, -4 lines 0 comments Download
M src/heap.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M src/ic.cc View 1 3 chunks +8 lines, -0 lines 0 comments Download
M src/ic-ia32.cc View 1 4 chunks +4 lines, -4 lines 0 comments Download
M src/macro-assembler-ia32.h View 1 1 chunk +3 lines, -1 line 0 comments Download
M src/macro-assembler-ia32.cc View 1 4 chunks +66 lines, -22 lines 0 comments Download
M src/messages.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M src/mirror-delay.js View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/objects.h View 1 12 chunks +50 lines, -14 lines 0 comments Download
M src/objects.cc View 1 14 chunks +74 lines, -8 lines 0 comments Download
M src/objects-debug.cc View 1 3 chunks +28 lines, -0 lines 0 comments Download
M src/objects-inl.h View 1 6 chunks +21 lines, -15 lines 0 comments Download
M src/runtime.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M src/runtime.cc View 1 5 chunks +10 lines, -4 lines 0 comments Download
M src/serialize.cc View 1 2 chunks +0 lines, -18 lines 0 comments Download
M src/string-stream.cc View 1 1 chunk +4 lines, -13 lines 0 comments Download
M src/stub-cache-ia32.cc View 1 3 chunks +9 lines, -9 lines 0 comments Download
M src/top.h View 1 6 chunks +13 lines, -30 lines 0 comments Download
M src/top.cc View 1 6 chunks +39 lines, -27 lines 0 comments Download
M src/v8natives.js View 1 1 chunk +1 line, -1 line 0 comments Download
M test/cctest/test-api.cc View 1 15 chunks +85 lines, -36 lines 0 comments Download
M test/cctest/test-debug.cc View 1 3 chunks +14 lines, -8 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
Feng Qian
12 years, 2 months ago (2008-10-15 21:16:41 UTC) #1
Mads Ager (chromium)
Feng, I'm not done yet, but I thought I would give you my initial comments ...
12 years, 2 months ago (2008-10-17 11:11:18 UTC) #2
Feng Qian
Mads, I did a comparison of interceptor and hand-coded forwarding, the differences are significant. On ...
12 years, 2 months ago (2008-10-20 23:12:07 UTC) #3
Feng Qian
Sunspider suffers from interceptors too, I got 1250ms vs 1100ms. One test case from Dramaeo ...
12 years, 2 months ago (2008-10-21 00:00:39 UTC) #4
Kasper Lund
Overall, this change LGTM. I'm not as scared by the change as I thought I ...
12 years, 2 months ago (2008-10-21 10:50:02 UTC) #5
Feng Qian
12 years, 2 months ago (2008-10-21 16:30:50 UTC) #6
Feng Qian
12 years, 2 months ago (2008-10-21 18:19:43 UTC) #7
Hi Thanks,

Fixed most of your comments and renamed ApiGlobalObject to JSGlobalProxy.

Filed 3 bugs for TODO's, and reverted two unrelated changes.

http://codereview.chromium.org/7366/diff/1/5
File include/v8.h (right):

http://codereview.chromium.org/7366/diff/1/5#newcode2061
Line 2061: /** Detaches the global object from its context before
On 2008/10/21 10:50:02, Kasper Lund wrote:
> Move the comment (starting with Detaches) to the next line to better match the
> other multi-line doc comments.

Done.

http://codereview.chromium.org/7366/diff/1/8
File src/api.cc (right):

http://codereview.chromium.org/7366/diff/1/8#newcode1940
Line 1940: new_map->set_needs_access_check();
Names must have confused you.
FunctionTemplate::needs_access_check(bool) takes a boolean argument, and
Map::needs_access_check() does not take arguments. I agree that naming is
confusing, so I am going to rename Map::needs_access_check() to
Map::is_access_check_needed().

Unfortunately, FunctionTemplate uses a macro DECL_BOOLEAN_ACCESSORS which can
only take a bool value, and there are several other accessors declared in the
same way, so I think a bool argument is fine after renaming
Map::needs_access_check().

On 2008/10/21 10:50:02, Kasper Lund wrote:
> Do we really want a boolean default value for the set_needs_access_check
method?
> I would much rather be more explicit and maybe turn the true/false values into
> an enum with proper names for the values like DONT_CHECK_ON_ACCESS and
> CHECK_ON_ACCESS.

http://codereview.chromium.org/7366/diff/1/7
File src/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/7366/diff/1/7#newcode570
Line 570: push(holder_reg);
I agree. Another approach is to profile how often this case is hit in real word
(cross-frame accesses in the same domain). The most common case, in-frame
accesses won't hit this case.

I am going to add a TODO there.

On 2008/10/21 10:50:02, Kasper Lund wrote:
> We should definitely try to get rid of this push/pop either by using another
> scratch register or by finding a way of providing more direct access to the
> security token.

http://codereview.chromium.org/7366/diff/1/7#newcode570
Line 570: push(holder_reg);
I filed a bug and added a TODO(119)
On 2008/10/21 10:50:02, Kasper Lund wrote:
> We should definitely try to get rid of this push/pop either by using another
> scratch register or by finding a way of providing more direct access to the
> security token.

http://codereview.chromium.org/7366/diff/1/38
File src/objects-inl.h (right):

http://codereview.chromium.org/7366/diff/1/38#newcode357
Line 357: #ifdef DEBUG
On 2008/10/21 10:50:02, Kasper Lund wrote:
> How about rewriting this to something ala:
> bool result = <tag and type check>;
> ASSERT(!result || IsAccessCheckNeeded());

Done.

http://codereview.chromium.org/7366/diff/1/38#newcode371
Line 371: return IsHeapObject() &&
On 2008/10/21 10:50:02, Kasper Lund wrote:
> I think we should cache the instance type in a local variable here. Unrelated
to
> your change of course.

Done.

http://codereview.chromium.org/7366/diff/1/16
File src/objects.cc (right):

http://codereview.chromium.org/7366/diff/1/16#newcode1255
Line 1255: 
On 2008/10/21 10:50:02, Kasper Lund wrote:
> Whitespace? Why?

Done.

http://codereview.chromium.org/7366/diff/1/28
File src/objects.h (right):

http://codereview.chromium.org/7366/diff/1/28#newcode2821
Line 2821: 
On 2008/10/21 10:50:02, Kasper Lund wrote:
> Do we need whitespace here?

Done.

http://codereview.chromium.org/7366/diff/1/24
File src/runtime.js (right):

http://codereview.chromium.org/7366/diff/1/24#newcode77
Line 77: } else if (x == null || IS_UNDEFINED(x)) {
I'd better not include this change in this change list.
I am going to revert it and re-do it in a separate CB.
This is related to issue 102.

On 2008/10/21 10:50:02, Kasper Lund wrote:
> I don't think this makes sense at all because undefined is equal (==) to null.
> Please remove this.

http://codereview.chromium.org/7366/diff/1/17
File src/top.cc (right):

http://codereview.chromium.org/7366/diff/1/17#newcode586
Line 586: GetProperty(Top::builtins(), key));
On 2008/10/21 10:50:02, Kasper Lund wrote:
> Maybe this fits on one line now?

Done.

http://codereview.chromium.org/7366/diff/1/4
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/7366/diff/1/4#newcode4831
Line 4831: // TODO (fqian): when running "cctest test-api", the initial count is
2,
On 2008/10/21 10:50:02, Kasper Lund wrote:
> File a bug report and add the issue number here instead of your username -
like
> TODO(121): When running...

Done.

Powered by Google App Engine
This is Rietveld 408576698