Bug 132890 – Script worlds are hard to use correctly and easy to leak.

• Status: NEW
• Component: WebCore JavaScript

[Andreas Kling]

<rdar://problem/16886955>

[Andreas Kling]

Created attachment 231414 [details]
Patch

[Geoffrey Garen]

Comment on attachment 231414 [details]
Patch

r=me

I wonder if this should be built into WebCore::DOMWrapperWorld.

[Andreas Kling]

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.

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Andreas Kling]

Comment on attachment 231437 [details]
Patch II

Ownership model isn't quite right here. This needs more work.

[Andreas Kling]

Created attachment 231495 [details]
Idea for EWS

[Andreas Kling]

Created attachment 231538 [details]
Idea for EWS with crash fix

[WebKit Commit Bot]

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.

[Andreas Kling]

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.

[Andreas Kling]

I think it might also make sense to call this something like "ScriptWorld" instead of DOMWrapperWorld.

[Andreas Kling]

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.

[Csaba Osztrogonác]

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.