Bug 70421

Created attachment 114561 [details] Patch

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.

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

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.

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

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

> 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. ;)

Created attachment 115043 [details] Patch

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.

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.

> Fixed by playing not entire audio file but only its tail. How long does it take now?

2.6 seconds on my Mac.

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?

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

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.

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.

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.

Created attachment 115176 [details] Patch

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.

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.

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.

Created attachment 115203 [details] Patch

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

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.

Comment on attachment 115203 [details] Patch Clearing flags on attachment: 115203 Committed r100307: <http://trac.webkit.org/changeset/100307>

All reviewed patches have been landed. Closing bug.

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)

> Date/Time: 2011-11-10 21:52:50.153 -0800 This crash stack is several days old.

(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.