Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2146383002:
ElementVisibilityObserver: get rid of reference cycle. (Closed)
|
Created:
4 years, 5 months ago by szager1 Modified:
4 years, 5 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionElementVisibilityObserver: get rid of reference cycle.
BUG=627539
R=mlamouri@chromium.org,zqzhang@chromium.org
Committed: https://crrev.com/0adb9749b2374898b245206356ab73cd9224913e
Cr-Commit-Position: refs/heads/master@{#405619}
Patch Set 1 #
Messages
Total messages: 22 (5 generated)
This is fast. I've tried your patch locally and it works! lgtm \o/
lgtm, though it would be great if there was a test :)
On 2016/07/14 19:16:11, Zhiqiang Zhang wrote: > This is fast. I've tried your patch locally and it works! > lgtm \o/ BTW, I'm still curious about why the reference cycle causes the leak. Do you have any ideas?
On 2016/07/14 19:23:48, Mounir Lamouri wrote: > lgtm, though it would be great if there was a test :) I think the test is the leak detector that flagged the problem.
On 2016/07/14 19:24:06, Zhiqiang Zhang wrote: > On 2016/07/14 19:16:11, Zhiqiang Zhang wrote: > > This is fast. I've tried your patch locally and it works! > > lgtm \o/ > > BTW, I'm still curious about why the reference cycle causes the leak. Do you > have any ideas? It's definitely a reference cycle; the question is: why didn't the garbage collector detect it? That's something for the oilpan folks to explain.
On 2016/07/14 at 19:31:34, szager wrote: > On 2016/07/14 19:24:06, Zhiqiang Zhang wrote: > > On 2016/07/14 19:16:11, Zhiqiang Zhang wrote: > > > This is fast. I've tried your patch locally and it works! > > > lgtm \o/ > > > > BTW, I'm still curious about why the reference cycle causes the leak. Do you > > have any ideas? > > It's definitely a reference cycle; the question is: why didn't the garbage collector detect it? That's something for the oilpan folks to explain. According to oilpan's doc, it's fine to have reference cycles: https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...
Description was changed from ========== ElementVisibilityObserver: get rid of reference cycle. BUG=627539 R=mlamouri@chromium.org,zqzhang@chromium.org ========== to ========== ElementVisibilityObserver: get rid of reference cycle. BUG=627539 R=mlamouri@chromium.org,zqzhang@chromium.org ==========
szager@chromium.org changed reviewers: + skobes@chromium.org
+skobes for OWNERS.
On 2016/07/14 19:31:34, szager1 wrote: > On 2016/07/14 19:24:06, Zhiqiang Zhang wrote: > > On 2016/07/14 19:16:11, Zhiqiang Zhang wrote: > > > This is fast. I've tried your patch locally and it works! > > > lgtm \o/ > > > > BTW, I'm still curious about why the reference cycle causes the leak. Do you > > have any ideas? > > It's definitely a reference cycle; the question is: why didn't the garbage > collector detect it? That's something for the oilpan folks to explain. HTMLMediaElement creates a VisibilityCallback holding a Persistent<> to the HTMLMediaElement when creating an ElementVisibilityObserver. That creates a cycle by way of a Persistent<>. That's the bug to fix here.
On 2016/07/14 19:45:24, sof wrote: > On 2016/07/14 19:31:34, szager1 wrote: > > On 2016/07/14 19:24:06, Zhiqiang Zhang wrote: > > > On 2016/07/14 19:16:11, Zhiqiang Zhang wrote: > > > > This is fast. I've tried your patch locally and it works! > > > > lgtm \o/ > > > > > > BTW, I'm still curious about why the reference cycle causes the leak. Do you > > > have any ideas? > > > > It's definitely a reference cycle; the question is: why didn't the garbage > > collector detect it? That's something for the oilpan folks to explain. > > HTMLMediaElement creates a VisibilityCallback holding a Persistent<> to the > HTMLMediaElement when creating an ElementVisibilityObserver. That creates a > cycle by way of a Persistent<>. > > That's the bug to fix here. Aha! So, mlamouri, why does ElementVisibilityObserver::start() use wrapWeakPersistent? The ElementVisibilityObserver is not stack-allocated, so that seems wrong.
I still think this is a good patch, BTW, but sigbjorn is right about the persistent reference.
Filed a new about the persistent reference: https://bugs.chromium.org/p/chromium/issues/detail?id=628367
lgtm
The CQ bit was checked by szager@chromium.org
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 ========== ElementVisibilityObserver: get rid of reference cycle. BUG=627539 R=mlamouri@chromium.org,zqzhang@chromium.org ========== to ========== ElementVisibilityObserver: get rid of reference cycle. BUG=627539 R=mlamouri@chromium.org,zqzhang@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== ElementVisibilityObserver: get rid of reference cycle. BUG=627539 R=mlamouri@chromium.org,zqzhang@chromium.org ========== to ========== ElementVisibilityObserver: get rid of reference cycle. BUG=627539 R=mlamouri@chromium.org,zqzhang@chromium.org Committed: https://crrev.com/0adb9749b2374898b245206356ab73cd9224913e Cr-Commit-Position: refs/heads/master@{#405619} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/0adb9749b2374898b245206356ab73cd9224913e Cr-Commit-Position: refs/heads/master@{#405619} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
