<rdar://problem/16886955>
Created attachment 231414 [details] Patch
Comment on attachment 231414 [details] Patch r=me I wonder if this should be built into WebCore::DOMWrapperWorld.
Created attachment 231437 [details] Patch II InjectedBundleScriptWorld should only clearWrappers() for DOMWrapperWorlds that it "owns". The previous patch broke the case where the WTR bundle was using InjectedBundleScriptWorld to wrap already existing DOMWrapperWorlds.
Comment on attachment 231437 [details] Patch II Attachment 231437 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5779318680584192 New failing tests: http/tests/security/isolatedWorld/window-setTimeout-string.html http/tests/security/isolatedWorld/dispatchEvent.html
Created attachment 231445 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 231437 [details] Patch II View in context: https://bugs.webkit.org/attachment.cgi?id=231437&action=review Do you know why these two tests fail? http/tests/security/isolatedWorld/dispatchEvent.html [ Failure ] http/tests/security/isolatedWorld/window-setTimeout-string.html [ Failure ] > Source/WebKit2/Shared/API/c/WKDeprecatedFunctions.cpp:83 > +void WKBundleScriptWorldClearWrappers(WKBundleScriptWorldRef) > +{ > +} Without any comment it’s not obvious why we are keeping this around. I presume the reason is specifically to keep old versions of Safari working, but perhaps there are other reasons? I don’t think that “once nobody uses them” is precise enough. We’d really like something here that could make it easy for a future contributor to be able to make the right decision about when to delete this.
Comment on attachment 231437 [details] Patch II Ownership model isn't quite right here. This needs more work.
Created attachment 231495 [details] Idea for EWS
Created attachment 231538 [details] Idea for EWS with crash fix
Attachment 231538 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/ScriptController.cpp:98: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
My plan here is to let DOMWrapperWorld directly own the JSDOMWindowShells. JSDOMWindowShell gets a DOMWrapperWorld& instead of a Ref<DOMWrapperWorld> since it's now strongly owned from the world. The main difference is that once a DOMWrapperWorld's refcount goes to zero, the JSDOMWindowShell will be up for GC. (Do note that JSEventListener refs the DOMWrapperWorld, for example.) Previously, DOMWrapperWorld would have a +1 ref from being in ScriptController's ShellMap. Calling clearWrappers() was the only way to bust out of that map before ScriptController destruction. With these changes, that's no longer necessary, just let your RefPtr<DOMWrapperWorld> go out of scope and it'll take care of the rest.
I think it might also make sense to call this something like "ScriptWorld" instead of DOMWrapperWorld.
Created attachment 231595 [details] For my future self I'm not going to work on this right now, so I'm uploading the patch as far as I got it.
Comment on attachment 231414 [details] Patch Cleared Geoffrey Garen's review+ from obsolete attachment 231414 [details] so that this bug does not appear in http://webkit.org/pending-commit.