Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2151243002:
Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement (Closed)
|
Created:
4 years, 5 months ago by Zhiqiang Zhang (Slow) Modified:
4 years, 5 months ago CC:
chromium-reviews, blink-reviews-html_chromium.org, feature-media-reviews_chromium.org, mlamouri+watch-blink_chromium.org, gasubic, sof, eae+blinkwatch, fs, eric.carlson_apple.com, blink-reviews-dom_chromium.org, dglazkov+blink, nessy, blink-reviews, vcarbune.chromium, rwlbuis, mlamouri (slow - plz ping) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUsing wrapWeakPersistent for VisibilityCallback in HTMLMediaElement
This is a test patch that fixes a memory leak caused by the reference between
HTMLMediaElement and ElementVisibilityObserver.
The previous patch breaks the reference cycle but seems doesn't fix the leak.
Using WeakPersistent is a workaround, but seems does not fix the leak entirely.
Keeping https://crbug.com/627539 open to keep track of the remaining leaks.
BUG=628367, 627539
Committed: https://crrev.com/df5e915f2075aa324e59087cfca9848b6ff3f58a
Cr-Commit-Position: refs/heads/master@{#405996}
Patch Set 1 #Patch Set 2 : prepare to land #Patch Set 3 : prepare to land #Messages
Total messages: 35 (24 generated)
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Description was changed from
==========
Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement
This is a test patch that reintroduce the reference cycle of
"HTMLMediaElement <-> ElementVisibilityObserver" to trigger a memory
leak. Using wrapWeakPersistent for the visibility callback seems doesn't
entirely fix the leak.
Leak log before using wrapWeakPersistent:
({"numberOfLiveActiveDOMObjects":[2,6],"numberOfLiveDocuments":[1,2],"numberOfLiveNodes":[4,61],"numberOfLiveResources":[0,4]})
Leak log after using wrapWeakPersistent (flaky, only happens occasionally):
({"numberOfLiveActiveDOMObjects":[2,3]})
The leak disappears after using wrapCrossThreadWeakPersistent.
BUG=628367
==========
to
==========
Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement
This is a test patch that reintroduce the reference cycle of
"HTMLMediaElement <-> ElementVisibilityObserver" to trigger a memory
leak. Using wrapWeakPersistent for the visibility callback seems doesn't
entirely fix the leak.
Leak log before using wrapWeakPersistent:
({"numberOfLiveActiveDOMObjects":[2,6],"numberOfLiveDocuments":[1,2],"numberOfLiveNodes":[4,61],"numberOfLiveResources":[0,4]})
Leak log after using wrapWeakPersistent (flaky, only happens occasionally):
({"numberOfLiveActiveDOMObjects":[2,3]})
BUG=628367
==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement
This is a test patch that reintroduce the reference cycle of
"HTMLMediaElement <-> ElementVisibilityObserver" to trigger a memory
leak. Using wrapWeakPersistent for the visibility callback seems doesn't
entirely fix the leak.
Leak log before using wrapWeakPersistent:
({"numberOfLiveActiveDOMObjects":[2,6],"numberOfLiveDocuments":[1,2],"numberOfLiveNodes":[4,61],"numberOfLiveResources":[0,4]})
Leak log after using wrapWeakPersistent (flaky, only happens occasionally):
({"numberOfLiveActiveDOMObjects":[2,3]})
BUG=628367
==========
to
==========
Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement
This is a test patch that fixes a memory leak caused by the reference between
HTMLMediaElement and ElementVisibilityObserver.
The previous patch breaks the reference cycle but seems doesn't fix the leak.
Using WeakPersistent is a workaround, but seems does not fix the leak entirely.
Leak log using wrapPersistent:
({"numberOfLiveActiveDOMObjects":[2,6],"numberOfLiveDocuments":[1,2],"numberOfLiveNodes":[4,61],"numberOfLiveResources":[0,4]})
Leak log using wrapWeakPersistent (flaky, only happens occasionally):
({"numberOfLiveActiveDOMObjects":[2,3]})
BUG=628367
==========
zqzhang@chromium.org changed reviewers: + haraken@chromium.org, sigbjornf@opera.com, szager@chromium.org
Hi, I created this test patch to show the fix for the ElementVisibilityObserver leak. As pointed out in the previous CL (https://codereview.chromium.org/2146383002/#msg12). It seems to fix the leak partially (flaky leaks still happen) Can you guys take a look?
This isn't what I had in mind in my comment on the other CL; I was talking about this line: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme... Maybe that can be replaced with: WeakMember<ElementVisibilityObserver>(this) ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2016/07/15 18:00:25, szager1 wrote: > This isn't what I had in mind in my comment on the other CL; I was talking about > this line: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme... > > Maybe that can be replaced with: > > WeakMember<ElementVisibilityObserver>(this) > > ? I think using wrapPersistent/wrapWeakPersistent in bind is common pattern. WTF::bind does not allow Member<> or WeakMember<>. See https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/Functional...
Is it guaranteed that m_autoplayVisibilityObserver->stop() is called in a finite time period?
On 2016/07/16 01:44:25, haraken wrote: > Is it guaranteed that m_autoplayVisibilityObserver->stop() is called in a finite > time period? No, it may never be called, so we want to ensure there's no leak in this case.
On 2016/07/16 12:38:04, Zhiqiang Zhang wrote: > On 2016/07/16 01:44:25, haraken wrote: > > Is it guaranteed that m_autoplayVisibilityObserver->stop() is called in a > finite > > time period? > > No, it may never be called, so we want to ensure there's no leak in this case. OK, then wrapWeakPersistent makes sense. But is this CL enough to make the ElementVisibilityObserver => Element reference weak? ElementVisibilityObserver::m_element is still keeping a strong reference back to the Element, isn't it?
On 2016/07/17 13:17:32, haraken wrote: > On 2016/07/16 12:38:04, Zhiqiang Zhang wrote: > > On 2016/07/16 01:44:25, haraken wrote: > > > Is it guaranteed that m_autoplayVisibilityObserver->stop() is called in a > > finite > > > time period? > > > > No, it may never be called, so we want to ensure there's no leak in this case. > > OK, then wrapWeakPersistent makes sense. > > But is this CL enough to make the ElementVisibilityObserver => Element reference > weak? ElementVisibilityObserver::m_element is still keeping a strong reference > back to the Element, isn't it? The leak does not come from Member<Element> in ElementVisibilityObserver. I think we should land this patch first and keep an eye on the remaining leaks, which I think should reside in somewhere in ElementVisibilityObserver or IntersectionObserver (have filed bug for this).
Description was changed from
==========
Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement
This is a test patch that fixes a memory leak caused by the reference between
HTMLMediaElement and ElementVisibilityObserver.
The previous patch breaks the reference cycle but seems doesn't fix the leak.
Using WeakPersistent is a workaround, but seems does not fix the leak entirely.
Leak log using wrapPersistent:
({"numberOfLiveActiveDOMObjects":[2,6],"numberOfLiveDocuments":[1,2],"numberOfLiveNodes":[4,61],"numberOfLiveResources":[0,4]})
Leak log using wrapWeakPersistent (flaky, only happens occasionally):
({"numberOfLiveActiveDOMObjects":[2,3]})
BUG=628367
==========
to
==========
Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement
This is a test patch that fixes a memory leak caused by the reference between
HTMLMediaElement and ElementVisibilityObserver.
The previous patch breaks the reference cycle but seems doesn't fix the leak.
Using WeakPersistent is a workaround, but seems does not fix the leak entirely.
Filed https://crbug.com/629044 to keep an eye on the remaining leaks.
BUG=628367
==========
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/18 11:06:17, Zhiqiang Zhang wrote: > On 2016/07/17 13:17:32, haraken wrote: > > On 2016/07/16 12:38:04, Zhiqiang Zhang wrote: > > > On 2016/07/16 01:44:25, haraken wrote: > > > > Is it guaranteed that m_autoplayVisibilityObserver->stop() is called in a > > > finite > > > > time period? > > > > > > No, it may never be called, so we want to ensure there's no leak in this > case. > > > > OK, then wrapWeakPersistent makes sense. > > > > But is this CL enough to make the ElementVisibilityObserver => Element > reference > > weak? ElementVisibilityObserver::m_element is still keeping a strong reference > > back to the Element, isn't it? > > The leak does not come from Member<Element> in ElementVisibilityObserver. > I think we should land this patch first and keep an eye on the remaining leaks, > which I think should reside in somewhere in ElementVisibilityObserver or > IntersectionObserver (have filed bug for this). This CL LGTM but I believe that this CL is not sufficient to fix the leak.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement This is a test patch that fixes a memory leak caused by the reference between HTMLMediaElement and ElementVisibilityObserver. The previous patch breaks the reference cycle but seems doesn't fix the leak. Using WeakPersistent is a workaround, but seems does not fix the leak entirely. Filed https://crbug.com/629044 to keep an eye on the remaining leaks. BUG=628367 ========== to ========== Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement This is a test patch that fixes a memory leak caused by the reference between HTMLMediaElement and ElementVisibilityObserver. The previous patch breaks the reference cycle but seems doesn't fix the leak. Using WeakPersistent is a workaround, but seems does not fix the leak entirely. Keeping https://crbug.com/629044 open to keep track of the remaining leaks. BUG=628367,627539 ==========
Description was changed from ========== Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement This is a test patch that fixes a memory leak caused by the reference between HTMLMediaElement and ElementVisibilityObserver. The previous patch breaks the reference cycle but seems doesn't fix the leak. Using WeakPersistent is a workaround, but seems does not fix the leak entirely. Keeping https://crbug.com/629044 open to keep track of the remaining leaks. BUG=628367,627539 ========== to ========== Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement This is a test patch that fixes a memory leak caused by the reference between HTMLMediaElement and ElementVisibilityObserver. The previous patch breaks the reference cycle but seems doesn't fix the leak. Using WeakPersistent is a workaround, but seems does not fix the leak entirely. Keeping https://crbug.com/627539 open to keep track of the remaining leaks. BUG=628367,627539 ==========
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2151243002/#ps80001 (title: "prepare to land")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement This is a test patch that fixes a memory leak caused by the reference between HTMLMediaElement and ElementVisibilityObserver. The previous patch breaks the reference cycle but seems doesn't fix the leak. Using WeakPersistent is a workaround, but seems does not fix the leak entirely. Keeping https://crbug.com/627539 open to keep track of the remaining leaks. BUG=628367,627539 ========== to ========== Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement This is a test patch that fixes a memory leak caused by the reference between HTMLMediaElement and ElementVisibilityObserver. The previous patch breaks the reference cycle but seems doesn't fix the leak. Using WeakPersistent is a workaround, but seems does not fix the leak entirely. Keeping https://crbug.com/627539 open to keep track of the remaining leaks. BUG=628367,627539 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement This is a test patch that fixes a memory leak caused by the reference between HTMLMediaElement and ElementVisibilityObserver. The previous patch breaks the reference cycle but seems doesn't fix the leak. Using WeakPersistent is a workaround, but seems does not fix the leak entirely. Keeping https://crbug.com/627539 open to keep track of the remaining leaks. BUG=628367,627539 ========== to ========== Using wrapWeakPersistent for VisibilityCallback in HTMLMediaElement This is a test patch that fixes a memory leak caused by the reference between HTMLMediaElement and ElementVisibilityObserver. The previous patch breaks the reference cycle but seems doesn't fix the leak. Using WeakPersistent is a workaround, but seems does not fix the leak entirely. Keeping https://crbug.com/627539 open to keep track of the remaining leaks. BUG=628367,627539 Committed: https://crrev.com/df5e915f2075aa324e59087cfca9848b6ff3f58a Cr-Commit-Position: refs/heads/master@{#405996} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/df5e915f2075aa324e59087cfca9848b6ff3f58a Cr-Commit-Position: refs/heads/master@{#405996} |
