Bug 135178 – JSDOMWindowShell leaks on pages with media elements

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Joseph Pecoraro]

With MEDIA_CONTROLS_SCRIPT reloading a page with a <video> creates JSDOMWindowShell/JSGlobalObject objects that never get destroyed.

The window shell is created under HTMLMediaElement::ensureMediaControlsInjectedScript, but never destroyed.

(lldb) bt
  * frame #0: WebCore::ScriptController::createWindowShell(this=0x0000620000100240, world=0x0000600000103ba0) + 23 at ScriptController.cpp:112
    frame #1: WebCore::ScriptController::initScript(this=0x0000620000100240, world=0x0000600000103ba0) + 138 at ScriptController.cpp:229
    frame #2: WebCore::ScriptController::windowShell(this=0x0000620000100240, world=0x0000600000103ba0) + 199 at ScriptController.h:89
    frame #3: WebCore::ScriptController::globalObject(this=0x0000620000100240, world=0x0000600000103ba0) + 29 at ScriptController.h:98
    frame #4: WebCore::HTMLMediaElement::ensureMediaControlsInjectedScript(this=0x00000001130ad800) + 247 at HTMLMediaElement.cpp:5795
    frame #5: WebCore::HTMLMediaElement::updateCaptionContainer(this=0x00000001130ad800) + 124 at HTMLMediaElement.cpp:3717
    frame #6: WebCore::HTMLMediaElement::configureTextTrackGroup(this=0x00000001130ad800, group=0x00007fff5fbfcfd0) + 2505 at HTMLMediaElement.cpp:3676
    frame #7: WebCore::HTMLMediaElement::configureTextTracks(this=0x00000001130ad800) + 1309 at HTMLMediaElement.cpp:3848
    frame #8: WebCore::HTMLMediaElement::loadTimerFired(this=0x00000001130ad800, (null)=0x00000001130ad8c0) + 99 at HTMLMediaElement.cpp:796

Andreas pointed out that we should be calling clearWrappers on the DOMWrapperWorld. Doing so does clear the excess worlds.

[Joseph Pecoraro]

<rdar://problem/17713033>

[Joseph Pecoraro]

Created attachment 235317 [details]
[PATCH] Proposed Fix

I am somewhat concerned that other iterations over m_windowShells could trigger a GC that modifies m_windowShells. However they are far more straightforward than WebCore::ScriptController::clearWindowShell.

It also seems dangerous that ScriptCachedFrameData reaches directly in and iterates ScriptController.m_windowShells.

[WebKit Commit Bot]

Attachment 235317 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScriptController.cpp:189:  Declaration has space between type name and * in JSDOMWindowShell *windowShell  [whitespace/declaration] [3]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 235317 [details]
[PATCH] Proposed Fix

Attachment 235317 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6048076619841536

New failing tests:
media/track/add-and-remove-track.html

[Build Bot]

Created attachment 235322 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Joseph Pecoraro]

I am unable to reproduce the crash the bots saw, but it appear stop be in the TestRunner InjectedBundle overflowing a Vector bounds:

    Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
    Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef

    Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
    0   com.apple.JavaScriptCore      	0x00000001090eb49e WTFCrash + 62 (Assertions.cpp:329)
    1   WebKitTestRunnerInjectedBundle	0x000000010cf53703 WTF::CrashOnOverflow::overflowed() + 9
    2   WebKitTestRunnerInjectedBundle	0x000000010cf562e4 WTR::InjectedBundle::page() const + 24 (InjectedBundle.cpp:134)
    3   WebKitTestRunnerInjectedBundle	0x000000010cf498a1 WTR::TestRunner::removeAllWebNotificationPermissions() + 27 (InjectedBundlePage.h:41)
    4   WebKitTestRunnerInjectedBundle	0x000000010cf56054 WTR::InjectedBundle::didReceiveMessage(OpaqueWKString const*, void const*) + 398 (Vector.h:610)
    5   com.apple.WebKit              	0x00000001080ca6d8 WebKit::InjectedBundleClient::didReceiveMessage(WebKit::InjectedBundle*, WTF::String const&, API::Object*) + 120 (WKSharedAPICast.h:130)
    6   com.apple.WebKit              	0x0000000108225bc5 WebKit::WebProcess::postInjectedBundleMessage(IPC::DataReference const&) + 135 (RefPtr.h:55)
    7   com.apple.WebKit              	0x000000010822f258 WebKit::WebProcess::didReceiveWebProcessMessage(IPC::Connection*, IPC::MessageDecoder&) + 2356 (HandleMessage.h:16)
    ...

Perhaps this is flakey on the bots or related to other tests running simultaneously.

[Joseph Pecoraro]

Created attachment 235328 [details]
[PATCH] Proposed Fix

[Oliver Hunt]

Comment on attachment 235328 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=235328&action=review

> Source/WebCore/bindings/js/ScriptController.cpp:189
> +    Vector<JSC::Strong<JSDOMWindowShell>> windowShells;
> +    copyValuesToVector(m_windowShells, windowShells);
> +
> +    for (size_t i = 0; i < windowShells.size(); ++i) {
> +        JSDOMWindowShell* windowShell = windowShells[i].get();

We must take this, file it as a separate bug as this code is otherwise sadly unsafe :(

> Source/WebCore/html/HTMLMediaElement.cpp:421
> +    if (m_isolatedWorld)
> +        m_isolatedWorld->clearWrappers();

This is wrong - it means if we ever destroy an htmlmediaelement we'll nuke all the dom wrappers

[Andreas Kling]

(In reply to comment #8)
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:421
> > +    if (m_isolatedWorld)
> > +        m_isolatedWorld->clearWrappers();
> 
> This is wrong - it means if we ever destroy an htmlmediaelement we'll nuke all the dom wrappers

Why is this wrong? It will only nuke the DOM wrappers inside the isolated world *created by* this media element.

[Oliver Hunt]

(In reply to comment #9)
> (In reply to comment #8)
> > 
> > > Source/WebCore/html/HTMLMediaElement.cpp:421
> > > +    if (m_isolatedWorld)
> > > +        m_isolatedWorld->clearWrappers();
> > 
> > This is wrong - it means if we ever destroy an htmlmediaelement we'll nuke all the dom wrappers
> 
> Why is this wrong? It will only nuke the DOM wrappers inside the isolated world *created by* this media element.

Wait each media element has its own entirely self container world?!?!?

[Oliver Hunt]

Comment on attachment 235328 [details]
[PATCH] Proposed Fix

Talked to Jer, this is in fact correct sorry :)

Minor concern - is it possible for an object in a media element's world to point to something that refs the element? That would still be a cycle that needs to be broken

[WebKit Commit Bot]

Comment on attachment 235328 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 235328

Committed r171481: <http://trac.webkit.org/changeset/171481>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Man, we really need to improve the isolated worlds API / default memory management.

[Joseph Pecoraro]

I think there is another case of iterating m_windowShells when it could be mutated in ScriptController::updateDocument:
<http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r171485%20(5831)/fast/html/details-nested-1-crash-log.txt>

I'll file a follow-up.

[Joseph Pecoraro]

<https://webkit.org/b/135211> ScriptController::updateDocument ASSERT mutating map while iterating map