New issue
Advanced search Search tips

Issue 658030 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 658086

Blocking:
issue 635596



Sign in to add a comment

Long task Performance Entries not delivered consistently

Project Member Reported by panicker@google.com, Oct 20 2016

Issue description

Sometimes the callback member inside the generated bindings for PerformanceObserverCallback is empty.
https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/PerformanceObserverCallback.cpp?l=43

This prevents delivery of the entries to JS.
 
Cc: haraken@chromium.org esprehn@chromium.org
Owner: ----
Haraken, could you weigh in on this?

Just discussed with Elliott and this appears to be an error in bindings generator 
From:
https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/PerformanceObserverCallback.cpp

Nothing is holding onto m_callback here, so it could get garbage collected: 
PerformanceObserverCallback::PerformanceObserverCallback(v8::Isolate* isolate, v8::Local<v8::Function> callback)
    : m_callback(isolate, callback)
{
    DCHECK(!m_callback.isEmpty());
    m_callback.setPhantom();
}
Blockedon: 658086

Comment 3 by panicker@google.com, Oct 21 2016

Cc: caseq@chromium.org
+caseq (you were on the right track with garbage collection :))
Cc: yukishiino@chromium.org
Owner: bashi@chromium.org
You need to use visitDOMWrapper to add a strong reference from the wrapper to m_callback.

(In the near future, traceWrapper will be available, which will make the lifetime maintenance a lot easier.)

Could you provide more specific guidance for PerformanceObserver:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/timing/PerformanceObserver.idl

OR provide an example?

Looking around the pattern I found is adding [ .. Custom=VisitDOMWrapper ] in the idl interface.
Eg. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/IntersectionObserver.idl?l=13
And then implement something like this:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/custom/V8IntersectionObserverCustom.cpp?l=62

But not sure how the latter translates to PerformanceObserverCallback.

Comment 6 by bashi@chromium.org, Oct 21 2016

I'm preparing a fix (not the right fix, workaround until trace wrapper is fully implemented).
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cb168398aeefebfb5bf8cb39aa9e82b1d47c7b01

commit cb168398aeefebfb5bf8cb39aa9e82b1d47c7b01
Author: bashi <bashi@chromium.org>
Date: Fri Oct 21 08:45:49 2016

bindings: Set strong reference from PerformanceObserver to callback

Otherwise the callback can be garbage-collected. This is a tentative
solution until TraceWrapper is fully implemented.

BUG= 658030 

Review-Url: https://chromiumcodereview.appspot.com/2435263002
Cr-Commit-Position: refs/heads/master@{#426753}

[add] https://crrev.com/cb168398aeefebfb5bf8cb39aa9e82b1d47c7b01/third_party/WebKit/LayoutTests/fast/performance/performance-observer-callback-after-gc.html
[modify] https://crrev.com/cb168398aeefebfb5bf8cb39aa9e82b1d47c7b01/third_party/WebKit/Source/bindings/core/v8/custom/V8PerformanceObserverCustom.cpp

Comment 8 by bashi@chromium.org, Oct 27 2016

Status: Fixed (was: Assigned)

Sign in to add a comment