Bug 30145 – There should be a way to reuse isolated worlds

• Component: Platform
• Status: RESOLVED FIXED

[Aaron Boodman]

There should be a way for WebKit clients to reuse isolated worlds, to execute different chunks of code in the same world.

[Aaron Boodman]

Created attachment 40754 [details]
patchity patch

[Aaron Boodman]

See also Chromium patch:
http://codereview.chromium.org/262002

[Adam Barth]

Comment on attachment 40754 [details]
patchity patch

+ To destroy it, call deleteSoon().

This method doesn't seem to exist.  Also, delete() isn't the right name.  detach() is probably better.

+ V8Proxy::~V8Proxy()

I wish we had a small scoped object to do this work instead of doing it explicitly in the destructor.

You seem to only clear the worldID map during destruction of V8Proxy, but doesn't V8Proxy have Frame-lifetime?  You need to clean up the map on every navigation.  You probably want to do that work during |clearForNavigation|.

This design also doesn't account for the PageCache, but we don't support PageCache anyway...

Tests???  Feel free to modify layoutTestController to make this stuff testable.  You can see examples in LayoutTests/http/tests/security/isolatedWorld

[Aaron Boodman]

Created attachment 40813 [details]
now with even more testing goodness

[Aaron Boodman]

I renamed the method destroy(). I think this is accurate as, it basically queues the object to be deleted as soon as possible. Since the destructor is private, I don't think there's any chance for confusion. But if you feel strongly about 'weak' or 'release' or whatever, just say so.

Regarding a scoped wrapper for V8Proxy. That sounds fine, but using smart pointers with collections always sketches me out a bit because it's not clear what the behavior will be when the collection is resized. Let me know if there is a webkity way to do this I can look to.

Regarding clearing the map of worlds. I added a call to clear in navigation and close. It seems to work ,but I am not that familiar with the details of V8Proxy. Please double-check.

[Aaron Boodman]

(In reply to comment #4)
> Created an attachment (id=40813) [details]
> now with even more testing goodness

The 'even more' here is in reference to the testing I already added for this downstream :)

[Adam Barth]

Comment on attachment 40813 [details]
now with even more testing goodness

Great.  Thanks Aaron.

I woud probably have picked a different name than |destroy|, but that's not essential.  Also, you don't need to mark the HTML files as executable.

[Adam Barth]

(In reply to comment #5)
> If you feel
> strongly about 'weak' or 'release' or whatever, just say so.

I don't feel strongly about it.

> Regarding a scoped wrapper for V8Proxy. That sounds fine, but using smart
> pointers with collections always sketches me out a bit because it's not clear
> what the behavior will be when the collection is resized. Let me know if there
> is a webkity way to do this I can look to.

If we see the pattern recurring it might be worth investing in machinery to make it easier.  We'll have to keep an eye out.

> Regarding clearing the map of worlds. I added a call to clear in navigation and
> close. It seems to work ,but I am not that familiar with the details of
> V8Proxy. Please double-check.

I believe that is correct for the non-PageCache case, which we don't support.

[WebKit Commit Bot]

Comment on attachment 40813 [details]
now with even more testing goodness

Rejecting patch 40813 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

[WebKit Commit Bot]

Comment on attachment 40813 [details]
now with even more testing goodness

Rejecting patch 40813 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11405 test cases.
http/tests/security/isolatedWorld/iframe.html -> new (results generated in /Users/eseidel/Projects/CommitQueue/LayoutTests/platform/mac/http/tests/security/isolatedWorld)

Exiting early after 1 failures. 8829 tests run.
248.44s total testing time

8828 test cases (99%) succeeded
1 test case (<1%) was new
5 test cases (<1%) had stderr output

[Aaron Boodman]

Created attachment 40827 [details]
Forgot to update Skipped files

[Aaron Boodman]

Comment on attachment 40827 [details]
Forgot to update Skipped files

New patch just adds the Skipped entries.

[George Staikos]

Comment on attachment 40827 [details]
Forgot to update Skipped files

Assuming previous review holds, r+

[Yong Li]

Comment on attachment 40827 [details]
Forgot to update Skipped files

Let commit bot land it

[WebKit Commit Bot]

Comment on attachment 40827 [details]
Forgot to update Skipped files

Rejecting patch 40827 from commit-queue.

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

[Aaron Boodman]

This was already checked in.