Bug 96717 – Turn forbidEventDispatch and allowEventDispatch into a RAII object

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Calling forbidEventDispatch and allowEventDispatch pair is error prone. We should turn it into a RAII object.

[Ryosuke Niwa]

Created attachment 164026 [details]
Refactoring

[Abhishek Arya]

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.

[Ryosuke Niwa]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Ryosuke Niwa]

Created attachment 164233 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 164233 [details]
Patch for landing

Clearing flags on attachment: 164233

Committed r128673: <http://trac.webkit.org/changeset/128673>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 96847

[James Robinson]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

The same problem. Unassigning myself from the bug.

[Ryosuke Niwa]

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

[Darin Adler]

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.

[Ryosuke Niwa]

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

[Ojan Vafai]

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

[Ryosuke Niwa]

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

[Ojan Vafai]

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

[Abhishek Arya]

(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

[Ojan Vafai]

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

[Ojan Vafai]

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

[Ryosuke Niwa]

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

[Ojan Vafai]

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.