Non-custom "callback" incorrectly sets a phantom handle without a private property |
|||||
Issue descriptionex. // http://w3c.github.io/performance-timeline/#idl-def-PerformanceObserverCallback callback PerformanceObserverCallback = void (PerformanceObserverEntryList entries, PerformanceObserver observer); produces PerformanceObserverCallback::PerformanceObserverCallback(v8::Isolate* isolate, v8::Local<v8::Function> callback) : m_callback(isolate, callback) { DCHECK(!m_callback.isEmpty()); m_callback.setPhantom(); } but doesn't stash the m_callback anywhere, which means the callback can get garbage collected randomly. It turns out that no other interface besides PerformanceObserver is using callback like this without something being custom? ex. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/IntersectionObserver.idl?type=cs&q=IntersectionObserver.idl&sq=package:chromium&l=7 does [Custom] callback, and the custom bindings do: V8PrivateProperty::getIntersectionObserverCallback(scriptState->isolate()) .set(scriptState->context(), owner, callback); but the template doesn't use V8PrivateProperty: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl?q=file:bindings+%22m_callback.setPhantom()%22+-file:gen+-file:test&sq=package:chromium&dr=C&l=14 so the PerformanceObserver callback gets garbage collected and then you get no deliveries.
,
Oct 21 2016
IDBObserver is broken in the same way: https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/modules/v8/IDBObserverCallback.cpp?dr&q=IDBObserverCallback+file:gen&sq=package:chromium&l=25 We should definitely fix this before IDBObserver ships. :)
,
Oct 21 2016
,
Oct 21 2016
IDBObserver has a custom constructor and it calls V8PrivateProperty::getIDBObserverCallback().set(). IIUC IDBObserver is not broken at this point. That said we should have a better way to manage lifetime of callbacks. I'm not sure what's the right solution for this. There are couple of options: - setWrapperReference() with [Custom=visitDOMWrapper] - private property with [CustomConstructor] or [Custom=CallEplilogue] - add a constructor to callback_function.h.tmpl which takes an owner object.
,
Oct 21 2016
Currently it's a responsibility of the observers to keep the callback function alive. And there are many ways to keep it alive (visitDOMWrapper, V8PrivateProperty etc). All the weirdness is going to be resolved once we ship traceWrapper (this will happen in Q4). In that world you just need to trace the callback function from the observer with traceWrapper().
,
Apr 10 2017
Obsolete. We use trace wrappers now. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by esprehn@chromium.org
, Oct 21 2016