Bug 107253 - [V8] Create a persistent wrapper for Window.prototype and innerGlobalObject
Summary: [V8] Create a persistent wrapper for Window.prototype and innerGlobalObject
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-18 03:12 PST by Kentaro Hara
Modified: 2013-09-01 10:32 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.49 KB, patch)
2013-01-18 03:13 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for landing (5.10 KB, patch)
2013-01-20 22:22 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for landing (5.10 KB, patch)
2013-01-20 22:26 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for landing (2.74 KB, patch)
2013-01-21 01:30 PST, Kentaro Hara
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2013-01-18 03:12:09 PST
This is one of steps to avoid hitting an ASSERT() that will be added in https://bugs.webkit.org/show_bug.cgi?id=107137 . We should have a persistent wrapper for all DOM objects and set a class id on the wrapper.
Comment 1 Kentaro Hara 2013-01-18 03:13:32 PST
Created attachment 183419 [details]
Patch
Comment 2 Kentaro Hara 2013-01-18 03:23:16 PST
Please r- the patch if it's wrong.

Actually I don't fully understand hidden prototype objects around DOMWindow. My theory is that all DOM objects (1) should have persistent wrappers, (2) should set class ids on the wrappers, and (3) should set native information on their internal fields. isDOMWrapper() that will be added in https://bugs.webkit.org/show_bug.cgi?id=107137 is going to check that.

So the patch is going to create a persistent wrapper for Window.prototype and innerGlobalObject and set class ids on them. I'm not sure if it's a right thing to do.
Comment 3 Adam Barth 2013-01-20 01:07:41 PST
Comment on attachment 183419 [details]
Patch

This patch looks harmless.  Setting the class ID should be fine given that we set native info on these wrappers.  Storing these objects in the DOMDataStore should be harmless, but also pretty useless.  No one should be looking for them in the DOMDataStore because we have a custom implementation of toV8 for DOMWindow.

The only weird thing about storing them in the DOMDataStore is that we'll get a collision because we'll have three all stored in the HashMap under the key |window|.  There's nothing really wrong with that (especially since the last one to be written will basically kick the other ones out), but it's just a weird case.  I guess we're trading one weird case for another.

I don't think this patch will cause any trouble, but I'm not sure it's really making things better either.
Comment 4 Kentaro Hara 2013-01-20 02:28:34 PST
Comment on attachment 183419 [details]
Patch

Thanks! Makes sense.
Comment 5 WebKit Review Bot 2013-01-20 02:36:28 PST
Comment on attachment 183419 [details]
Patch

Clearing flags on attachment: 183419

Committed r140270: <http://trac.webkit.org/changeset/140270>
Comment 6 WebKit Review Bot 2013-01-20 02:36:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Kentaro Hara 2013-01-20 18:31:26 PST
Reverted r140270 for reason:

Hit asserts in a debug build

Committed r140288: <http://trac.webkit.org/changeset/140288>
Comment 8 Kentaro Hara 2013-01-20 22:22:44 PST
Created attachment 183711 [details]
patch for landing
Comment 9 Kentaro Hara 2013-01-20 22:24:47 PST
(In reply to comment #3)
> The only weird thing about storing them in the DOMDataStore is that we'll get a collision because we'll have three all stored in the HashMap under the key |window|.  There's nothing really wrong with that (especially since the last one to be written will basically kick the other ones out), but it's just a weird case.  I guess we're trading one weird case for another.

This was a problem. Storing the same window twice hits an ASSERT(m_map.contains(key)) in DOMWrapperMap.h. I changed the code so that it doesn't store the same window twice.
Comment 10 WebKit Review Bot 2013-01-20 22:24:57 PST
Comment on attachment 183711 [details]
patch for landing

Rejecting attachment 183711 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/15968911
Comment 11 Kentaro Hara 2013-01-20 22:26:23 PST
Created attachment 183714 [details]
patch for landing
Comment 12 Adam Barth 2013-01-20 22:58:14 PST
I'm glad we have an ASSERT for that case.  This new patch is probably better.
Comment 13 WebKit Review Bot 2013-01-20 23:35:33 PST
Comment on attachment 183714 [details]
patch for landing

Rejecting attachment 183714 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 1.
patching file Source/WebCore/bindings/v8/V8DOMWindowShell.cpp
patching file Source/WebCore/bindings/v8/V8DOMWrapper.h
Hunk #1 FAILED at 64.
Hunk #2 FAILED at 72.
Hunk #3 succeeded at 103 (offset 8 lines).
2 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/bindings/v8/V8DOMWrapper.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/15977905
Comment 14 Kentaro Hara 2013-01-21 01:30:41 PST
Created attachment 183733 [details]
patch for landing
Comment 15 WebKit Review Bot 2013-01-22 17:09:09 PST
Comment on attachment 183733 [details]
patch for landing

Rejecting attachment 183733 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
force --reset --delete_unversioned_trees' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 103.
Re-trying 'depot_tools/gclient sync --force --reset --delete_unversioned_trees'
Died at /mnt/git/webkit-commit-queue/Tools/Scripts/webkitdirs.pm line 2553.

Failed to run "['Tools/Scripts/build-webkit', '--release', '--chromium', '--update-chromium']" exit_code: 1
-reset --delete_unversioned_trees'
Died at /mnt/git/webkit-commit-queue/Tools/Scripts/webkitdirs.pm line 2553.

Full output: http://queues.webkit.org/results/16040896
Comment 16 WebKit Review Bot 2013-01-24 02:13:38 PST
Comment on attachment 183733 [details]
patch for landing

Rejecting attachment 183733 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 183733, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
patch from 1 bug.
Processing patch 183733 from bug 107253.
Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Parsed 2 diffs from patch file(s).
patch: **** Can't create file /tmp/pp8GrKkx : No space left on device
patch: **** Can't create file /tmp/ppIdcTzA : No space left on device

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16077648
Comment 17 WebKit Review Bot 2013-02-07 12:41:01 PST
Comment on attachment 183733 [details]
patch for landing

Rejecting attachment 183733 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
l.cpp -o obj/Source/WebCore/bindings/v8/webcore_remaining.V8DOMWindowShell.o
../../Source/WebCore/bindings/v8/V8DOMWindowShell.cpp: In member function 'bool WebCore::V8DOMWindowShell::installDOMWindow()':
../../Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:323: error: 'setWrapperClass' is not a member of 'WebCore::V8DOMWrapper'
../../Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:341: error: 'setWrapperClass' is not a member of 'WebCore::V8DOMWrapper'
ninja: build stopped: subcommand failed.

Full output: http://queues.webkit.org/results/16434309
Comment 18 Anders Carlsson 2013-09-01 10:32:52 PDT
V8 is gone.