RESOLVED FIXED 30145
There should be a way to reuse isolated worlds
https://bugs.webkit.org/show_bug.cgi?id=30145
Summary There should be a way to reuse isolated worlds
Aaron Boodman
Reported 2009-10-06 18:11:29 PDT
There should be a way for WebKit clients to reuse isolated worlds, to execute different chunks of code in the same world.
Attachments
patchity patch (12.38 KB, patch)
2009-10-06 18:22 PDT, Aaron Boodman
abarth: review-
now with even more testing goodness (18.44 KB, patch)
2009-10-07 12:50 PDT, Aaron Boodman
abarth: review+
commit-queue: commit-queue-
Forgot to update Skipped files (20.36 KB, patch)
2009-10-07 15:41 PDT, Aaron Boodman
staikos: review+
commit-queue: commit-queue-
Aaron Boodman
Comment 1 2009-10-06 18:22:53 PDT
Created attachment 40754 [details] patchity patch
Aaron Boodman
Comment 2 2009-10-06 18:24:02 PDT
See also Chromium patch: http://codereview.chromium.org/262002
Adam Barth
Comment 3 2009-10-06 18:49:03 PDT
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
Comment 4 2009-10-07 12:50:11 PDT
Created attachment 40813 [details] now with even more testing goodness
Aaron Boodman
Comment 5 2009-10-07 12:54:11 PDT
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
Comment 6 2009-10-07 12:55:54 PDT
(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 7 2009-10-07 13:02:32 PDT
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
Comment 8 2009-10-07 13:09:38 PDT
(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 9 2009-10-07 14:05:40 PDT
Comment on attachment 40813 [details] now with even more testing goodness Rejecting patch 40813 from commit-queue. aa@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
WebKit Commit Bot
Comment 10 2009-10-07 15:29:31 PDT
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
Comment 11 2009-10-07 15:41:06 PDT
Created attachment 40827 [details] Forgot to update Skipped files
Aaron Boodman
Comment 12 2009-10-07 15:43:31 PDT
Comment on attachment 40827 [details] Forgot to update Skipped files New patch just adds the Skipped entries.
George Staikos
Comment 13 2009-10-07 15:48:37 PDT
Comment on attachment 40827 [details] Forgot to update Skipped files Assuming previous review holds, r+
Yong Li
Comment 14 2009-10-19 08:57:53 PDT
Comment on attachment 40827 [details] Forgot to update Skipped files Let commit bot land it
WebKit Commit Bot
Comment 15 2009-10-19 09:09:49 PDT
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
Comment 16 2009-10-19 10:55:31 PDT
This was already checked in.
Note You need to log in before you can comment on or make changes to this bug.