Chromium Code Reviews Chromium Code Reviews
Issue 2725873002:
Media: fix memory leak because of the document holding on an EventListener. (Closed)
| OLD | NEW |
|---|---|
| (Empty) | |
| 1 // Copyright 2017 The Chromium Authors. All rights reserved. | |
| 2 // Use of this source code is governed by a BSD-style license that can be | |
| 3 // found in the LICENSE file. | |
| 4 | |
| 5 #include "core/html/shadow/MediaControls.h" | |
| 6 | |
| 7 #include "core/dom/Document.h" | |
| 8 #include "core/html/HTMLVideoElement.h" | |
| 9 #include "core/testing/DummyPageHolder.h" | |
| 10 #include "platform/geometry/IntSize.h" | |
| 11 #include "platform/heap/ThreadState.h" | |
| 12 #include "testing/gtest/include/gtest/gtest.h" | |
| 13 | |
| 14 namespace blink { | |
| 15 | |
| 16 class MediaControlsLeakTest : public ::testing::Test { | |
| 17 protected: | |
| 18 virtual void SetUp() { | |
| 19 m_pageHolder = DummyPageHolder::create(IntSize(800, 600)); | |
| 20 Document& document = this->document(); | |
| 21 | |
| 22 document.write("<body><video controls></video></body>"); | |
| 23 m_video = toHTMLVideoElement(document.querySelector("video")); | |
| 24 m_controls = m_video->mediaControls(); | |
| 25 } | |
| 26 | |
| 27 Document& document() { return m_pageHolder->document(); } | |
| 28 HTMLVideoElement* video() { return m_video; } | |
| 29 MediaControls* controls() { return m_controls; } | |
| 30 | |
| 31 private: | |
| 32 std::unique_ptr<DummyPageHolder> m_pageHolder; | |
| 33 WeakPersistent<HTMLVideoElement> m_video; | |
| 34 WeakPersistent<MediaControls> m_controls; | |
| 35 }; | |
| 36 | |
| 37 TEST_F(MediaControlsLeakTest, RemovingFromDocumentCollectsAll) { | |
| 38 ASSERT_NE(video(), nullptr); | |
| 39 EXPECT_TRUE(video()->hasEventListeners()); | |
| 40 EXPECT_NE(controls(), nullptr); | |
| 41 EXPECT_TRUE(document().hasEventListeners()); | |
| 42 | |
| 43 document().body()->setInnerHTML(""); | |
| 44 | |
| 45 // When removed from the document, the event listeners should have been | |
| 46 // dropped. | |
| 47 EXPECT_FALSE(document().hasEventListeners()); | |
| 48 // The video element should still have some event listeners. | |
| 49 EXPECT_TRUE(video()->hasEventListeners()); | |
| 50 | |
| 51 ThreadState::current()->collectAllGarbage(); | |
| 52 | |
| 53 // They have been GC'd. | |
| 54 EXPECT_EQ(video(), nullptr); | |
| 55 EXPECT_EQ(controls(), nullptr); | |
| 56 } | |
| 57 | |
| 58 TEST_F(MediaControlsLeakTest, ReInsertingInDocumentCollectsControls) { | |
| 59 ASSERT_NE(video(), nullptr); | |
| 60 EXPECT_TRUE(video()->hasEventListeners()); | |
| 61 EXPECT_NE(controls(), nullptr); | |
| 62 EXPECT_TRUE(document().hasEventListeners()); | |
| 63 | |
| 64 // This should be a no-op. | |
| 65 document().body()->removeChild(video()); | |
| 66 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.
| |
| 67 | |
| 68 EXPECT_TRUE(document().hasEventListeners()); | |
| 69 EXPECT_TRUE(video()->hasEventListeners()); | |
| 70 | |
| 71 ThreadState::current()->collectAllGarbage(); | |
| 72 | |
| 73 ASSERT_NE(video(), nullptr); | |
| 74 EXPECT_NE(controls(), nullptr); | |
| 75 EXPECT_EQ(controls(), video()->mediaControls()); | |
| 76 } | |
| 77 | |
| 78 } // namespace blink | |
| OLD | NEW |
