Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2303893003:
Pass ResizeObserver as 'this' to callback (Closed)
DescriptionPass ResizeObserver as 'this' to callback
I had to implement a custom callback dispatch handler.
Used MutationObserver code as a template.
Added a test case.
BUG=641353
Committed: https://crrev.com/e4ae0871bcb1c269d81b006eb9522c306a2bdf37
Cr-Commit-Position: refs/heads/master@{#417464}
Patch Set 1 #
Total comments: 3
Patch Set 2 : CR fixes #
Total comments: 5
Patch Set 3 : CR fix, merge master #
Messages
Total messages: 35 (17 generated)
Description was changed from ========== Pass ResizeObserver as 'this' to callback BUG=641353 ========== to ========== Pass ResizeObserver as 'this' to callback I had to implement a custom callback dispatch handler. Used MutationObserver code as a template. BUG=641353 ==========
atotic@google.com changed reviewers: + eae@chromium.org, esprehn@chromium.org
ResizeObserver callback was not setting 'this' in callback to ResizeObserver. Fixed by implementing custom callback dispatch code. Modeled on MutationObserver.
Description was changed from ========== Pass ResizeObserver as 'this' to callback I had to implement a custom callback dispatch handler. Used MutationObserver code as a template. BUG=641353 ========== to ========== Pass ResizeObserver as 'this' to callback I had to implement a custom callback dispatch handler. Used MutationObserver code as a template. Added a test case. BUG=641353 ==========
haraken@chromium.org changed reviewers: + haraken@chromium.org
In terms of bindings, LGTM with comments. bashi@, lkawai@: The custom bindings should be removed once you implement callback functions in the IDL compiler. FYI. https://codereview.chromium.org/2303893003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8ResizeObserverCallback.cpp (left): https://codereview.chromium.org/2303893003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8ResizeObserverCallback.cpp:41: if (m_callback.isEmpty()) You need this check. Basically please just copy & paste V8MutationObserverCallback the way it is. https://codereview.chromium.org/2303893003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8ResizeObserverCallback.cpp (right): https://codereview.chromium.org/2303893003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8ResizeObserverCallback.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2303893003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8ResizeObserverCallback.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. Can you move this file to bindings/core/v8/custom/V8ResizeObserverCallbackCustom.cpp?
Patchset #2 (id:20001) has been deleted
On 2016/09/01 at 21:28:24, haraken wrote: > Basically please just copy & paste V8MutationObserverCallback the way it is. Done. > 2016 Done. > https://codereview.chromium.org/2303893003/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/core/v8/V8ResizeObserverCallback.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. > > Can you move this file to bindings/core/v8/custom/V8ResizeObserverCallbackCustom.cpp? Done. Please review.
The CQ bit was checked by atotic@google.com 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...
LGTM from the binding perspective. I want to have eae@ take another look from the feature perspective.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2303893003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/resize-observer/observe.html (right): https://codereview.chromium.org/2303893003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/resize-observer/observe.html:177: ro = null; // must null every reference to RO to prevent leaks Is this documented anywhere?
https://codereview.chromium.org/2303893003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/resize-observer/observe.html (right): https://codereview.chromium.org/2303893003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/resize-observer/observe.html:177: ro = null; // must null every reference to RO to prevent leaks On 2016/09/05 at 20:52:48, eae wrote: > Is this documented anywhere? Yeah that doesn't seem right, you shouldn't need to null anything. What's leaking here? https://codereview.chromium.org/2303893003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/resize-observer/observe.html:177: ro = null; // must null every reference to RO to prevent leaks On 2016/09/05 at 20:52:48, eae wrote: > Is this documented anywhere? Yeah that doesn't seem right, you shouldn't need to null anything. What's leaking here?
The answer to leaks.... https://codereview.chromium.org/2303893003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/resize-observer/observe.html (right): https://codereview.chromium.org/2303893003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/resize-observer/observe.html:177: ro = null; // must null every reference to RO to prevent leaks On 2016/09/06 at 01:35:31, esprehn wrote: > On 2016/09/05 at 20:52:48, eae wrote: > Is this documented anywhere? This is only required to pass run-webkit-tests --enable-leak-detection > What's leaking here? I have no proof, since leak detector does not report what has leaked. This is what I believe is happening: - leak-detection runs on harnessTest.done(). - leak-detection reports a leak if ResizeObserver is alive when it runs. - all the references from JS to ResizeObserver must be cleared for ResizeObserver to be GCd - therefore, all references from JS to ResizeObserver must be set to null before test is done. My other tests also null ResizeObserver before calling done, but this is done by test framework in ResizeTestHelper._done.
https://codereview.chromium.org/2303893003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/resize-observer/observe.html (right): https://codereview.chromium.org/2303893003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/resize-observer/observe.html:177: ro = null; // must null every reference to RO to prevent leaks On 2016/09/06 17:22:37, atotic1 wrote: > On 2016/09/06 at 01:35:31, esprehn wrote: > > On 2016/09/05 at 20:52:48, eae wrote: > > Is this documented anywhere? > > This is only required to pass run-webkit-tests --enable-leak-detection > > > What's leaking here? > > I have no proof, since leak detector does not report what has leaked. This > is what I believe is happening: > - leak-detection runs on harnessTest.done(). > - leak-detection reports a leak if ResizeObserver is alive when it runs. > - all the references from JS to ResizeObserver must be cleared for > ResizeObserver to be GCd > - therefore, all references from JS to ResizeObserver must be set to null before > test is done. > > My other tests also null ResizeObserver before calling done, but this is done by > test framework in ResizeTestHelper._done. Ah, this makes sense. It's totally valid a thing that the underlying DOM object is kept alive while the JS wrapper is alive. This is not a leak. Reword the comment at line 177.
That still doesn't make sense though, all refs should be dropped at the end of the test, and we do major oilpan and JS GCs before running the leak detector. Why are the nodes alive? I worry this is a real bug. -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
That still doesn't make sense though, all refs should be dropped at the end of the test, and we do major oilpan and JS GCs before running the leak detector. Why are the nodes alive? I worry this is a real bug. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/09/07 02:01:51, esprehn wrote: > That still doesn't make sense though, all refs should be dropped at the end > of the test, and we do major oilpan and JS GCs before running the leak > detector. Why are the nodes alive? > > I worry this is a real bug. For example, if you set window.foo = new HTMLDivElement(), that's still alive at the end of the test. We run multiple V8 and Oilpan GCs but they cannot collect the object because there's a strong reference from the window object. observe.html is doing the same thing, because harnesstest.done() is called before the scope of the |ro| object exits.
The CQ bit was checked by atotic@google.com 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/09/07 at 02:05:10, haraken wrote:
> On 2016/09/07 02:01:51, esprehn wrote:
> > That still doesn't make sense though, all refs should be dropped at the end
> > of the test, and we do major oilpan and JS GCs before running the leak
> > detector. Why are the nodes alive?
> >
> > I worry this is a real bug.
>
> For example, if you set window.foo = new HTMLDivElement(), that's still alive
at the end of the test. We run multiple V8 and Oilpan GCs but they cannot
collect the object because there's a strong reference from the window object.
>
> observe.html is doing the same thing, because harnesstest.done() is called
before the scope of the |ro| object exits.
This caused a lot of confusion, so I've added a long comment:
// every reference to RO must be null before test completes
// to avoid triggering test leak-detection
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by atotic@google.com
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by atotic@google.com
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.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Pass ResizeObserver as 'this' to callback I had to implement a custom callback dispatch handler. Used MutationObserver code as a template. Added a test case. BUG=641353 ========== to ========== Pass ResizeObserver as 'this' to callback I had to implement a custom callback dispatch handler. Used MutationObserver code as a template. Added a test case. BUG=641353 Committed: https://crrev.com/e4ae0871bcb1c269d81b006eb9522c306a2bdf37 Cr-Commit-Position: refs/heads/master@{#417464} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e4ae0871bcb1c269d81b006eb9522c306a2bdf37 Cr-Commit-Position: refs/heads/master@{#417464} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
