Bug 107253 – [V8] Create a persistent wrapper for Window.prototype and innerGlobalObject

• Component: WebCore JavaScript
• Status: RESOLVED INVALID

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 183419 [details]
Patch

[Kentaro Hara]

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.

[Adam Barth]

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.

[Kentaro Hara]

Comment on attachment 183419 [details]
Patch

Thanks! Makes sense.

[WebKit Review Bot]

Comment on attachment 183419 [details]
Patch

Clearing flags on attachment: 183419

Committed r140270: <http://trac.webkit.org/changeset/140270>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Reverted r140270 for reason:

Hit asserts in a debug build

Committed r140288: <http://trac.webkit.org/changeset/140288>

[Kentaro Hara]

Created attachment 183711 [details]
patch for landing

[Kentaro Hara]

(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.

[WebKit Review Bot]

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

[Kentaro Hara]

Created attachment 183714 [details]
patch for landing

[Adam Barth]

I'm glad we have an ASSERT for that case.  This new patch is probably better.

[WebKit Review Bot]

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

[Kentaro Hara]

Created attachment 183733 [details]
patch for landing

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Anders Carlsson]

V8 is gone.