Bug 70421 – V8 bindings: event listener can be garbage collected, causing events loss

• Component: DOM
• Status: RESOLVED FIXED

[Eugene Nalimov]

Moved from https://bugs.webkit.org/show_bug.cgi?id=66878 because it is separate issue. For all the details and repro cases see discussion there.

Bug was in V8 binding forever. If type is subtype of Node (e.g. Node itself) then there is only weak reference from object to event listener, and event listener can be collected even when event source is alive and can send events in the future. Code generator emits special code for types that are not subtypes of Node, establishing hidden dependency.  Fix can do something similar for subtypes of Node as well. Not sure if hidden dependency is right thing (it requires extra slot, and we don't want to increase size of lot of types, when absolute majority of objects would never have event listeners attached). Maybe we should use hidden value mechanism, or object groups.

[Eugene Nalimov]

Created attachment 114561 [details]
Patch

[WebKit Review Bot]

Attachment 114561 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1

Source/WebCore/bindings/v8/DOMDataStore.h:94:  The parameter name "v8Object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 114561 [details]
Patch

Attachment 114561 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10350618

New failing tests:
media/audio-garbage-collect.html

[Adam Barth]

Comment on attachment 114561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114561&action=review

This is a nice solution.  Any idea why the test is failing?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1700
> +    # A DOMObject that is an ActiveDOMObject and also a DOMNode should be treated as an DOMNode here.

Can you explain a bit more why?  I would naively expect ActiveDOMNode here.

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:95
> -    getDOMNodeMap().set(node, wrapper);
> +    if (node->isActiveNode())

I hope this doesn't slow us down on DOM benchmarks.  Please ping the perf sheriff when landing.

[anton muhin]

Comment on attachment 114561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114561&action=review

Very nice, thanks a lot!

> Source/WebCore/bindings/v8/V8GCController.cpp:146
> +template<typename T> inline bool GCPrologueSpecialCase(T* object, v8::Persistent<v8::Object> wrapper, WrapperTypeInfo* typeInfo)

maybe just provide no definition to get link time error?

> Source/WebCore/bindings/v8/V8GCController.cpp:176
> +template<typename T> class GCPrologueVisitor : public DOMWrapperMap<T>::Visitor {

Can we avoid specialization tricks?  We can just pass a trait which does a special processing.  Something like:

struct MessagePortHandler {
  static bool process(void* object, ....) { .... }
};

struct NodeHandler {
  static bool process(....)
}

And now visitDOMWrapper(....) {
  ...
  if (Processor::process(...)) {
    ...
  }
}

WDYT?

> Source/WebCore/bindings/v8/V8GCController.cpp:411
> +template<typename T, v8::WeakReferenceCallback callback> struct GCEpilogueHelper {

ditto

[Eugene Nalimov]

Will take care of suggestions, hopefully will send new patch later today.

No idea why test is listed as failing -- it is *not* in the list of failed tests, see output http://queues.webkit.org/results/10350618

Maybe it was skipped with lot of other tests after limit of failures was reached, but I am not sure.

Regarding perf: I don't expect one added indirect call to noticeably affect perf, especially because we are adding node to the map only once after it is created. Memory allocation and search in the map are much more expensive. And perf is exactly why I added new IsActiveNode() method to node instead of getting type descriptor and calling its toActive() method -- 1 indirect call instead of 2. 

Thanks,
Eugene

[Adam Barth]

> No idea why test is listed as failing -- it is *not* in the list of failed tests, see output http://queues.webkit.org/results/10350618

Looks like it timed out:

http://queues.webkit.org/results/10350615
http://queues.webkit.org/results/10450608

> Regarding perf: I don't expect one added indirect call to noticeably affect perf, especially because we are adding node to the map only once after it is created. Memory allocation and search in the map are much more expensive. And perf is exactly why I added new IsActiveNode() method to node instead of getting type descriptor and calling its toActive() method -- 1 indirect call instead of 2. 

Sounds reasonable.  Hopefully the benchmarks will agree.  ;)

[Eugene Nalimov]

Created attachment 115043 [details]
Patch

[Eugene Nalimov]

Addressed everything:
* Test was failing just because it was too slow. On my machine when running release build it usually was just marginally able to pass in less than 6 seconds (not always). Fixed by playing not entire audio file but only its tail.
* Added comment to the generator.
* Got rid of template specialization, added traits instead. Personally I loved specialization more, but traits are Ok, too.

[WebKit Review Bot]

Attachment 115043 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1

Source/WebCore/bindings/v8/DOMDataStore.h:94:  The parameter name "v8Object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

> Fixed by playing not entire audio file but only its tail.

How long does it take now?

[Eugene Nalimov]

2.6 seconds on my Mac.

[Adam Barth]

Comment on attachment 115043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115043&action=review

> LayoutTests/media/audio-garbage-collect.html:31
> +function forceGC() {
> +    if (window.GCController)
> +        return GCController.collect();
> +
> +    // Force garbage collection
> +    for (var ndx = 0; ndx < 99000; ndx ++)
> +        var str = new String("1234");
> +}

We have a fancy new gc.js script somewhere you can use for this.

> LayoutTests/media/audio-garbage-collect.html:47
> +            a.currentTime = a.duration - 0.5;

Can we make 0.5 smaller?  Is this test racy?

[Adam Barth]

That's on the long side, easily in the 99.9th percentile.

[Adam Barth]

Comment on attachment 115043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115043&action=review

I'm happy with the code change.  I'm less excited about the long test.  Below are some extremely minor style nits.  If we can make the test faster, great.  If not, we can live with it.

> Source/WebCore/bindings/v8/V8GCController.cpp:167
> +struct SpecialCasePrologueNodeHandler {

Normally we'd use "class" for traits classes.

> Source/WebCore/bindings/v8/V8GCController.cpp:174
> +template<typename T, typename S> class GCPrologueVisitor : public DOMWrapperMap<T>::Visitor {

Usually we have a line break before "class" in these template declarations.

[Eugene Nalimov]

Will address issues and look what I can do with test tomorrow.

I was decreasing the run time till tests stopped to be the slowest one in the media tests. I probably can decrease the constant from 0.5 to something like 0.35 without affecting the test -- will look tomorrow.

[anton muhin]

Comment on attachment 115043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115043&action=review

LGTM!  And thanks again.

>> LayoutTests/media/audio-garbage-collect.html:31
>> +}
> 
> We have a fancy new gc.js script somewhere you can use for this.

I believe it should be gc(), see http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/HTMLFormElement/invalid-form-field.html#L27 for example.

[Eugene Nalimov]

Created attachment 115176 [details]
Patch

[Eugene Nalimov]

Addressed concerns:
* Replaced 'struct' by 'class', split lines between 'template' and 'class'
* Added UNUSED_PARAM() to the traits functions that not use that parameter
* Speed up test; not it runs in 1.9 seconds and is 5th longest test in LayoutTests/media, practically on par with 6th tests
* Call gc() in test instead of forcing GC by creating lot of objects.

[WebKit Review Bot]

Attachment 115176 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1

Source/WebCore/bindings/v8/DOMDataStore.h:94:  The parameter name "v8Object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 115176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115176&action=review

> LayoutTests/media/audio-garbage-collect.html:42
> +                gc();

Using the gc.js file is slightly better because then the test will run outside of the test harness.

[Eugene Nalimov]

Created attachment 115203 [details]
Patch

[Eugene Nalimov]

Addressed concern: using g() from LayoutTests/Resources/gc.js.

[WebKit Review Bot]

Attachment 115203 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1

Source/WebCore/bindings/v8/DOMDataStore.h:94:  The parameter name "v8Object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 115203 [details]
Patch

Clearing flags on attachment: 115203

Committed r100307: <http://trac.webkit.org/changeset/100307>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Peter Kasting]

Could this have cause a crash on the Mac 10.6 dbg bot when running fast/dom/image-object.html?  See http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fdom%2Fimage-object.html .  The crash stack looks like some kind of memory error:

Process:         DumpRenderTree [8957]
Path:            /b/build/slave/Webkit_Mac10_6__CG__dbg_/build/src/xcodebuild/Debug/DumpRenderTree.app/Contents/MacOS/DumpRenderTree
Identifier:      DumpRenderTree
Version:         ??? (???)
Code Type:       X86 (Native)
Parent Process:  Python [7453]

Date/Time:       2011-11-10 21:52:50.153 -0800
OS Version:      Mac OS X Server 10.6.8 (10K549)
Report Version:  6

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_PROTECTION_FAILURE at 0x00000000bec00010
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   com.apple.testnetscapeplugin  	0x13aa8eb9 NPP_SetWindow + 297
1   DumpRenderTree                	0x3c3b0783 webkit::npapi::PluginInstance::NPP_SetWindow(_NPWindow*) + 515
2   DumpRenderTree                	0x3c3eb8ad webkit::npapi::WebPluginDelegateImpl::WindowlessSetWindow() + 349
3   DumpRenderTree                	0x3c3ed750 webkit::npapi::WebPluginDelegateImpl::WindowlessUpdateGeometry(gfx::Rect const&, gfx::Rect const&) + 656
4   DumpRenderTree                	0x3c3e891b webkit::npapi::WebPluginDelegateImpl::UpdateGeometry(gfx::Rect const&, gfx::Rect const&) + 139
5   DumpRenderTree                	0x3c3f5669 webkit::npapi::WebPluginImpl::updateGeometry(WebKit::WebRect const&, WebKit::WebRect const&, WebKit::WebVector<WebKit::WebRect> const&, bool) + 857
6   DumpRenderTree                	0x3c3f591a non-virtual thunk to webkit::npapi::WebPluginImpl::updateGeometry(WebKit::WebRect const&, WebKit::WebRect const&, WebKit::WebVector<WebKit::WebRect> const&, bool) + 90
7   DumpRenderTree                	0x38f81a75 WebKit::WebPluginContainerImpl::reportGeometry() + 341
8   DumpRenderTree                	0x38f80038 WebKit::WebPluginContainerImpl::setFrameRect(WebCore::IntRect const&) + 88
9   DumpRenderTree                	0x3ba40031 WebCore::RenderWidget::setWidgetGeometry(WebCore::IntRect const&) + 321
10  DumpRenderTree                	0x3ba402b6 WebCore::RenderWidget::updateWidgetGeometry() + 470
11  DumpRenderTree                	0x3ba41404 WebCore::RenderWidget::updateWidgetPosition() + 100
12  DumpRenderTree                	0x3ba2a5d5 WebCore::RenderView::updateWidgetPositions() + 117
13  DumpRenderTree                	0x3b5013e1 WebCore::FrameView::performPostLayoutTasks() + 465
14  DumpRenderTree                	0x3b500f70 WebCore::FrameView::layout(bool) + 4528
15  DumpRenderTree                	0x3a7aca47 WebCore::Document::updateLayout() + 311
16  DumpRenderTree                	0x3a7acbf3 WebCore::Document::updateLayoutIgnorePendingStylesheets() + 243
17  DumpRenderTree                	0x3a82dcf8 WebCore::Element::scrollHeight() + 56
18  DumpRenderTree                	0x38f3c917 WebKit::WebFrameImpl::documentElementScrollHeight() const + 135
19  DumpRenderTree                	0x38e64528 WebViewHost::didUpdateLayout(WebKit::WebFrame*) + 136
20  DumpRenderTree                	0x38e64589 non-virtual thunk to WebViewHost::didUpdateLayout(WebKit::WebFrame*) + 41
21  DumpRenderTree                	0x38e93e07 WebKit::ChromeClientImpl::layoutUpdated(WebCore::Frame*) const + 103
22  DumpRenderTree                	0x3b501157 WebCore::FrameView::layout(bool) + 5015
23  DumpRenderTree                	0x3a7ac483 WebCore::Document::implicitClose() + 995
24  DumpRenderTree                	0x3b3ac9c2 WebCore::FrameLoader::checkCallImplicitClose() + 178
25  DumpRenderTree                	0x3b3ac6ca WebCore::FrameLoader::checkCompleted() + 330
26  DumpRenderTree                	0x3b3ab13e WebCore::FrameLoader::finishedParsing() + 190
27  DumpRenderTree                	0x3a7b93b2 WebCore::Document::finishedParsing() + 626
28  DumpRenderTree                	0x3aaafd6a WebCore::HTMLTreeBuilder::finished() + 170
29  DumpRenderTree                	0x3aa7730e WebCore::HTMLDocumentParser::end() + 254
30  DumpRenderTree                	0x3aa7610c WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd() + 300
31  DumpRenderTree                	0x3aa75e7c WebCore::HTMLDocumentParser::prepareToStopParsing() + 300
32  DumpRenderTree                	0x3aa77385 WebCore::HTMLDocumentParser::attemptToEnd() + 85
33  DumpRenderTree                	0x3aa77409 WebCore::HTMLDocumentParser::finish() + 89
34  DumpRenderTree                	0x3b39ed9a WebCore::DocumentWriter::endIfNotLoadingMainResource() + 282
35  DumpRenderTree                	0x3b39e2d3 WebCore::DocumentWriter::end() + 67
36  DumpRenderTree                	0x3b383f42 WebCore::DocumentLoader::finishedLoading() + 114
37  DumpRenderTree                	0x3b3b6665 WebCore::FrameLoader::finishedLoading() + 101
38  DumpRenderTree                	0x3b3d4672 WebCore::MainResourceLoader::didFinishLoading(double) + 434
39  DumpRenderTree                	0x3b3f4198 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) + 200
40  DumpRenderTree                	0x38ef5da9 WebCore::ResourceHandleInternal::didFinishLoading(WebKit::WebURLLoader*, double) + 265
41  DumpRenderTree                	0x3c456480 webkit_glue::WebURLLoaderImpl::Context::OnCompletedRequest(net::URLRequestStatus const&, std::string const&, base::Time const&) + 992
... (remainder trimmed)

[Adam Barth]

> Date/Time:       2011-11-10 21:52:50.153 -0800

This crash stack is several days old.

[Peter Kasting]

(In reply to comment #28)
> > Date/Time:       2011-11-10 21:52:50.153 -0800
> 
> This crash stack is several days old.

Sigh... sorry.  I was misled by the flakiness dashboard whose results at this point seem to be almost always wrong.

Never mind.  Looks like the real problem here is a different bug.