Bug 96717

Created attachment 164026 [details] Refactoring

I am a little concerned about Empty class size will not be zero on release build. see http://www.geeksforgeeks.org/archives/14606 or type "Why is the size of an empty class not zero?" on google.

(In reply to comment #2) > I am a little concerned about Empty class size will not be zero on release build. see http://www.geeksforgeeks.org/archives/14606 or type "Why is the size of an empty class not zero?" on google. The size of AssertNoEventDispatch will certainly be non-zero. Nonetheless, it should not affect Release builds since the compiler should never instantiate this object in Release builds as constructor and destructor are empty in Release builds. Even if the object was actually alloacted, all it does is to bump up the number we add to esp (4 or 8), and it will not have a significant effect on the performace.

Comment on attachment 164026 [details] Refactoring Rejecting attachment 164026 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ucceeded at 4105 (offset -1 lines). patching file Source/WebCore/dom/EventDispatcher.cpp patching file Source/WebCore/dom/EventTarget.cpp patching file Source/WebCore/dom/EventTarget.h patching file Source/WebCore/dom/Node.cpp patching file Source/WebCore/dom/WebKitNamedFlow.cpp patching file Source/WebCore/html/HTMLMediaElement.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Abhishek A..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/13844490

Comment on attachment 164026 [details] Refactoring Rejecting attachment 164026 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ucceeded at 4105 (offset -1 lines). patching file Source/WebCore/dom/EventDispatcher.cpp patching file Source/WebCore/dom/EventTarget.cpp patching file Source/WebCore/dom/EventTarget.h patching file Source/WebCore/dom/Node.cpp patching file Source/WebCore/dom/WebKitNamedFlow.cpp patching file Source/WebCore/html/HTMLMediaElement.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Abhishek A..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/13847432

Created attachment 164233 [details] Patch for landing

Comment on attachment 164233 [details] Patch for landing Clearing flags on attachment: 164233 Committed r128673: <http://trac.webkit.org/changeset/128673>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 96847

Crash from the linux (dbg) bot - looks very similar to the crashes from https://bugs.webkit.org/show_bug.cgi?id=96706 crash log for DumpRenderTree (pid 21566): STDOUT: <empty> STDERR: [21566:21566:15134271848845:ERROR:process_util_posix.cc(143)] Received signal 11 STDERR: base::debug::StackTrace::StackTrace() [0x7f5557aa45fa] STDERR: base::(anonymous namespace)::StackDumpSignalHandler() [0x7f5557b0bbcd] STDERR: 0x7f5550dc9af0 STDERR: WebCore::Document::frame() [0x7f5559ecf6b8] STDERR: WebKit::WebPluginContainerImpl::clearScriptObjects() [0x7f5559f9750f] STDERR: webkit::npapi::WebPluginImpl::TearDownPluginInstance() [0x7f555587398e] STDERR: webkit::npapi::WebPluginImpl::SetContainer() [0x7f55558729ce] STDERR: webkit::npapi::WebPluginImpl::destroy() [0x7f555586dce9] STDERR: WebKit::WebPluginContainerImpl::~WebPluginContainerImpl() [0x7f5559f989bf] STDERR: WTF::RefCounted<>::deref() [0x7f5559eb4478] STDERR: WTF::derefIfNotNull<>() [0x7f555aa8a024] STDERR: WTF::RefPtr<>::~RefPtr() [0x7f555aaf6eeb] STDERR: WTF::KeyValuePair<>::~KeyValuePair() [0x7f555b767032] STDERR: WTF::HashTable<>::deallocateTable() [0x7f555b76707d] STDERR: WTF::HashTable<>::~HashTable() [0x7f555b76686c] STDERR: WTF::HashMap<>::~HashMap() [0x7f555b7665a4] STDERR: WebCore::RenderWidget::resumeWidgetHierarchyUpdates() [0x7f555b764694] STDERR: WebCore::ContainerNode::removeChildren() [0x7f555a7cbcc2] STDERR: WebCore::Document::implicitOpen() [0x7f555a7f2ab6] STDERR: WebCore::Document::open() [0x7f555a7f296e] STDERR: WebCore::V8HTMLDocument::openCallback() [0x7f555afe001b] STDERR: v8::internal::HandleApiCallHelper<>() [0x7f5557f205e2] STDERR: v8::internal::Builtin_Impl_HandleApiCall() [0x7f5557f1b241] STDERR: v8::internal::Builtin_HandleApiCall() [0x7f5557f1b212] STDERR: 0x3cf9ae80618e

Sigh... I have no idea what the hell is going on there.

The same problem. Unassigning myself from the bug.

Committed r130077: <http://trac.webkit.org/changeset/130077>

Comment on attachment 164026 [details] Refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=164026&action=review > Source/WebCore/dom/ContainerNode.h:44 > +class AssertNoEventDispatch { We’d like class names to be nouns, not verbs. It can be confusing to have an object with a name that’s a verb. I would have named this NoEventDispatchAssertion or something like that.

(In reply to comment #14) > (From update of attachment 164026 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164026&action=review > > > Source/WebCore/dom/ContainerNode.h:44 > > +class AssertNoEventDispatch { > > We’d like class names to be nouns, not verbs. It can be confusing to have an object with a name that’s a verb. > > I would have named this NoEventDispatchAssertion or something like that. I can certainly rename it.

This hits an assert in http/tests/websocket/tests/hybi/workers/receive-blob.html. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fworkers%2Freceive-blob.html

(In reply to comment #16) > This hits an assert in http/tests/websocket/tests/hybi/workers/receive-blob.html. > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fworkers%2Freceive-blob.html It's a really careless bug. I somehow assumed that these objects are only instantiated in the main thread but it can certainly be instantiated in worker threads. Fixed in http://trac.webkit.org/changeset/130093.

I think this may have caused a performance regression. http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_domcoreattr/report.html?history=150&rev=-1&graph=dom_attr_element_expando___value http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibtraversejquery/report.html?history=150&rev=-1&graph=jslib_traverse_jquery_jQuery___parents_x10 http://trac.webkit.org/log/?verbose=on&rev=130076&stop_rev=130069

(In reply to comment #18) > I think this may have caused a performance regression. > > http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_domcoreattr/report.html?history=150&rev=-1&graph=dom_attr_element_expando___value > http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibtraversejquery/report.html?history=150&rev=-1&graph=jslib_traverse_jquery_jQuery___parents_x10 > http://trac.webkit.org/log/?verbose=on&rev=130076&stop_rev=130069 I think the code added here http://trac.webkit.org/changeset/130093/trunk/Source/WebCore/dom/ContainerNode.h if (!isMainThread()) return; needs to be in #ifndef NDEBUG

(In reply to comment #19) > (In reply to comment #18) > > I think this may have caused a performance regression. > > > > http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_domcoreattr/report.html?history=150&rev=-1&graph=dom_attr_element_expando___value > > http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibtraversejquery/report.html?history=150&rev=-1&graph=jslib_traverse_jquery_jQuery___parents_x10 > > http://trac.webkit.org/log/?verbose=on&rev=130076&stop_rev=130069 > > > I think the code added here > http://trac.webkit.org/changeset/130093/trunk/Source/WebCore/dom/ContainerNode.h > > if (!isMainThread()) > return; > > needs to be in #ifndef NDEBUG Actually, I think we're fine. It looks like it was rolling in a new version of V8 that caused the regression.

Actually, it looks like there might still be a regression from the followup patch: http://trac.webkit.org/changeset/130093/ http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dom_perf/report.html?history=150&rev=-1&graph=CloneNodes

(In reply to comment #21) > Actually, it looks like there might still be a regression from the followup patch: > http://trac.webkit.org/changeset/130093/ > > http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dom_perf/report.html?history=150&rev=-1&graph=CloneNodes Yeah, probably due to what Abhishek said. I'll put isMainThread() back into NDEBUG.

Seeing crashes in release builds: https://bugs.webkit.org/show_bug.cgi?id=79013 So moving the ifndef line (http://trac.webkit.org/changeset/130206) won't fix those.