Bug 96717

Summary: Turn forbidEventDispatch and allowEventDispatch into a RAII object
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, darin, eric.carlson, eric, feature-media-reviews, inferno, jamesr, morrita, ojan, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 96847    
Bug Blocks: 96702, 98191    
Attachments:
Description Flags
Refactoring
none
Patch for landing none

Description Ryosuke Niwa 2012-09-13 18:43:37 PDT
Calling forbidEventDispatch and allowEventDispatch pair is error prone. We should turn it into a RAII object.
Comment 1 Ryosuke Niwa 2012-09-13 18:47:00 PDT
Created attachment 164026 [details]
Refactoring
Comment 2 Abhishek Arya 2012-09-13 19:02:25 PDT
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.
Comment 3 Ryosuke Niwa 2012-09-13 19:10:18 PDT
(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 4 WebKit Review Bot 2012-09-13 21:03:38 PDT
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 5 WebKit Review Bot 2012-09-13 21:36:29 PDT
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
Comment 6 Ryosuke Niwa 2012-09-14 15:08:08 PDT
Created attachment 164233 [details]
Patch for landing
Comment 7 WebKit Review Bot 2012-09-14 17:04:37 PDT
Comment on attachment 164233 [details]
Patch for landing

Clearing flags on attachment: 164233

Committed r128673: <http://trac.webkit.org/changeset/128673>
Comment 8 WebKit Review Bot 2012-09-14 17:04:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2012-09-14 18:35:57 PDT
Re-opened since this is blocked by 96847
Comment 10 James Robinson 2012-09-14 18:37:35 PDT
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
Comment 11 Ryosuke Niwa 2012-09-14 18:38:50 PDT
Sigh... I have no idea what the hell is going on there.
Comment 12 Ryosuke Niwa 2012-09-14 18:45:16 PDT
The same problem. Unassigning myself from the bug.
Comment 13 Ryosuke Niwa 2012-10-01 14:20:26 PDT
Committed r130077: <http://trac.webkit.org/changeset/130077>
Comment 14 Darin Adler 2012-10-01 14:34:25 PDT
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.
Comment 15 Ryosuke Niwa 2012-10-01 14:35:10 PDT
(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.
Comment 16 Ojan Vafai 2012-10-01 15:48:09 PDT
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
Comment 17 Ryosuke Niwa 2012-10-01 16:12:15 PDT
(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.
Comment 20 Ojan Vafai 2012-10-02 11:33:17 PDT
(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.
Comment 21 Ojan Vafai 2012-10-02 12:28:47 PDT
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
Comment 22 Ryosuke Niwa 2012-10-02 12:51:08 PDT
(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.
Comment 23 Ojan Vafai 2012-10-02 14:27:03 PDT
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.