Chromium Code Reviews Chromium Code Reviews
Issue 2725873002:
Media: fix memory leak because of the document holding on an EventListener. (Closed)
| Index: third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp |
| diff --git a/third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp b/third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp |
| new file mode 100644 |
| index 0000000000000000000000000000000000000000..a6b3704fc131498a615a12af7fdf02b2b378f77a |
| --- /dev/null |
| +++ b/third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp |
| @@ -0,0 +1,78 @@ |
| +// Copyright 2017 The Chromium Authors. All rights reserved. |
| +// Use of this source code is governed by a BSD-style license that can be |
| +// found in the LICENSE file. |
| + |
| +#include "core/html/shadow/MediaControls.h" |
| + |
| +#include "core/dom/Document.h" |
| +#include "core/html/HTMLVideoElement.h" |
| +#include "core/testing/DummyPageHolder.h" |
| +#include "platform/geometry/IntSize.h" |
| +#include "platform/heap/ThreadState.h" |
| +#include "testing/gtest/include/gtest/gtest.h" |
| + |
| +namespace blink { |
| + |
| +class MediaControlsLeakTest : public ::testing::Test { |
| + protected: |
| + virtual void SetUp() { |
| + m_pageHolder = DummyPageHolder::create(IntSize(800, 600)); |
| + Document& document = this->document(); |
| + |
| + document.write("<body><video controls></video></body>"); |
| + m_video = toHTMLVideoElement(document.querySelector("video")); |
| + m_controls = m_video->mediaControls(); |
| + } |
| + |
| + Document& document() { return m_pageHolder->document(); } |
| + HTMLVideoElement* video() { return m_video; } |
| + MediaControls* controls() { return m_controls; } |
| + |
| + private: |
| + std::unique_ptr<DummyPageHolder> m_pageHolder; |
| + WeakPersistent<HTMLVideoElement> m_video; |
| + WeakPersistent<MediaControls> m_controls; |
| +}; |
| + |
| +TEST_F(MediaControlsLeakTest, RemovingFromDocumentCollectsAll) { |
| + ASSERT_NE(video(), nullptr); |
| + EXPECT_TRUE(video()->hasEventListeners()); |
| + EXPECT_NE(controls(), nullptr); |
| + EXPECT_TRUE(document().hasEventListeners()); |
| + |
| + document().body()->setInnerHTML(""); |
| + |
| + // When removed from the document, the event listeners should have been |
| + // dropped. |
| + EXPECT_FALSE(document().hasEventListeners()); |
| + // The video element should still have some event listeners. |
| + EXPECT_TRUE(video()->hasEventListeners()); |
| + |
| + ThreadState::current()->collectAllGarbage(); |
| + |
| + // They have been GC'd. |
| + EXPECT_EQ(video(), nullptr); |
| + EXPECT_EQ(controls(), nullptr); |
| +} |
| + |
| +TEST_F(MediaControlsLeakTest, ReInsertingInDocumentCollectsControls) { |
| + ASSERT_NE(video(), nullptr); |
| + EXPECT_TRUE(video()->hasEventListeners()); |
| + EXPECT_NE(controls(), nullptr); |
| + EXPECT_TRUE(document().hasEventListeners()); |
| + |
| + // This should be a no-op. |
| + document().body()->removeChild(video()); |
| + document().body()->appendChild(video()); |
|
Zhiqiang Zhang (Slow)
2017/03/01 21:20:52
Can GC happen between these two lines? Maybe keep
mlamouri (slow - plz ping)
2017/03/01 21:44:01
I don't think it is expected to happen in the midd
sof
2017/03/01 21:45:35
Conservative GCs can happen at any allocation.
|
| + |
| + EXPECT_TRUE(document().hasEventListeners()); |
| + EXPECT_TRUE(video()->hasEventListeners()); |
| + |
| + ThreadState::current()->collectAllGarbage(); |
| + |
| + ASSERT_NE(video(), nullptr); |
| + EXPECT_NE(controls(), nullptr); |
| + EXPECT_EQ(controls(), video()->mediaControls()); |
| +} |
| + |
| +} // namespace blink |
