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.
<rdar://problem/17713033>
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.
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.
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
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
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.
Created attachment 235328 [details] [PATCH] Proposed Fix
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
(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.
(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?!?!?
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
Comment on attachment 235328 [details] [PATCH] Proposed Fix Clearing flags on attachment: 235328 Committed r171481: <http://trac.webkit.org/changeset/171481>
All reviewed patches have been landed. Closing bug.
Man, we really need to improve the isolated worlds API / default memory management.
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.
<https://webkit.org/b/135211> ScriptController::updateDocument ASSERT mutating map while iterating map