Bug 96706

Summary: suspend/resumeWidgetHierarchyUpdates should be a RAII object
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cmarcelo, eric, gtk-ews, gustavo, pdr, simon.fraser, tkent, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 96719, 96729    
Bug Blocks: 96702    
Attachments:
Description Flags
Refactoring
none
Reverted irrelevant xcodeproj change
none
Speculative fix none

Description Ryosuke Niwa 2012-09-13 16:33:55 PDT 
suspendWidgetHierarchyUpdates and resumeWidgetHierarchyUpdates pair should be turned into a RAII object.
Comment 1 Eric Seidel (no email) 2012-09-13 16:40:30 PDT 
This could coudl be duped with bug 96702, one way or the other.
Comment 2 Ryosuke Niwa 2012-09-13 16:42:00 PDT 
Created attachment 164000 [details]
Refactoring
Comment 3 Ryosuke Niwa 2012-09-13 16:45:19 PDT 
(In reply to comment #1)
> This could coudl be duped with bug 96702, one way or the other.

I think the bug 96702 is a more generic bug. Once we've turned all these paired methods, we can consider creating a object that encompasses multiple RAII objects.
Comment 4 Ryosuke Niwa 2012-09-13 16:45:24 PDT 
Created attachment 164001 [details]
Reverted irrelevant xcodeproj change
Comment 5 kov's GTK+ EWS bot 2012-09-13 16:52:41 PDT 
Comment on attachment 164001 [details]
Reverted irrelevant xcodeproj change

Attachment 164001 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13849359
Comment 6 Ryosuke Niwa 2012-09-13 16:56:01 PDT 
Will land after merging http://trac.webkit.org/changeset/128524.
Comment 7 Build Bot 2012-09-13 17:05:45 PDT 
Comment on attachment 164001 [details]
Reverted irrelevant xcodeproj change

Attachment 164001 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13851308
Comment 8 Gyuyoung Kim 2012-09-13 17:07:28 PDT 
Comment on attachment 164001 [details]
Reverted irrelevant xcodeproj change

Attachment 164001 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13841546
Comment 9 Ryosuke Niwa 2012-09-13 17:19:31 PDT 
Committed r128531: <http://trac.webkit.org/changeset/128531>
Comment 10 Kent Tamura 2012-09-13 19:21:40 PDT 
(In reply to comment #9)
> Committed r128531: <http://trac.webkit.org/changeset/128531>

The patch made plugins/document-open.html crash on Chromium-Linux debug. I'll roll it out.

crash log for DumpRenderTree (pid 4685):
STDOUT: <empty>
STDERR: [4685:4685:15051224664231:ERROR:process_util_posix.cc(143)] Received signal 11
STDERR: 	base::debug::StackTrace::StackTrace() [0x7f1dfb72213a]
STDERR: 	base::(anonymous namespace)::StackDumpSignalHandler() [0x7f1dfb78970d]
STDERR: 	0x7f1df4a5baf0
STDERR: 	WebCore::Document::frame() [0x7f1dfdb4cf38]
STDERR: 	WebKit::WebPluginContainerImpl::clearScriptObjects() [0x7f1dfdc14c27]
STDERR: 	webkit::npapi::WebPluginImpl::TearDownPluginInstance() [0x7f1df95057be]
STDERR: 	webkit::npapi::WebPluginImpl::SetContainer() [0x7f1df95047fe]
STDERR: 	webkit::npapi::WebPluginImpl::destroy() [0x7f1df94ffb19]
STDERR: 	WebKit::WebPluginContainerImpl::~WebPluginContainerImpl() [0x7f1dfdc160d7]
STDERR: 	WTF::RefCounted<>::deref() [0x7f1dfdb31cf8]
STDERR: 	WTF::derefIfNotNull<>() [0x7f1dfe703e68]
STDERR: 	WTF::RefPtr<>::~RefPtr() [0x7f1dfe770e2b]
STDERR: 	WTF::KeyValuePair<>::~KeyValuePair() [0x7f1dff3d8ed8]
STDERR: 	WTF::HashTable<>::deallocateTable() [0x7f1dff3d8f23]
STDERR: 	WTF::HashTable<>::~HashTable() [0x7f1dff3d84b8]
STDERR: 	WTF::HashMap<>::~HashMap() [0x7f1dff3d816e]
STDERR: 	WebCore::WidgetHierarchyUpdatesSuspensionScope::moveWidgets() [0x7f1dff3d61fd]
STDERR: 	WebCore::WidgetHierarchyUpdatesSuspensionScope::~WidgetHierarchyUpdatesSuspensionScope() [0x7f1dfe44b61a]
STDERR: 	WebCore::ContainerNode::removeChildren() [0x7f1dfe448f56]
STDERR: 	WebCore::Document::implicitOpen() [0x7f1dfe46ec0c]
STDERR: 	WebCore::Document::open() [0x7f1dfe46eac4]
STDERR: 	WebCore::V8HTMLDocument::openCallback() [0x7f1dfec5638b]
STDERR: 	v8::internal::HandleApiCallHelper<>() [0x7f1dfbb9e54e]
STDERR: 	v8::internal::Builtin_Impl_HandleApiCall() [0x7f1dfbb991ad]
STDERR: 	v8::internal::Builtin_HandleApiCall() [0x7f1dfbb9917e]
STDERR: 	0x31cc0730618e
Comment 12 WebKit Review Bot 2012-09-13 19:24:06 PDT 
Re-opened since this is blocked by 96719
Comment 13 Ryosuke Niwa 2012-09-13 19:29:39 PDT 
Strange. I ran all the tests in plugins prior to landing this patch and it didn't crash.
Comment 14 Ryosuke Niwa 2012-09-13 21:26:44 PDT 
I can't reproduce the failure on my machine. I'll do a speculative fix and see what EWS has to say.
Comment 15 Ryosuke Niwa 2012-09-13 21:26:58 PDT 
Created attachment 164043 [details]
Speculative fix
Comment 16 Simon Fraser (smfr) 2012-09-13 21:53:09 PDT 
Comment on attachment 164043 [details]
Speculative fix

What's the difference between this and the previous patch?
Comment 17 Ryosuke Niwa 2012-09-13 21:56:10 PDT 
(In reply to comment #16)
> (From update of attachment 164043 [details])
> What's the difference between this and the previous patch?

In Element::attach, resumePostAttachCallbacks is called before suspendWidgetHierarchyUpdates is destructed.
Comment 18 Ryosuke Niwa 2012-09-13 21:59:18 PDT 
I'm landing this speculative fix for now.
Comment 19 Ryosuke Niwa 2012-09-13 21:59:42 PDT 
Committed r128552: <http://trac.webkit.org/changeset/128552>
Comment 20 WebKit Review Bot 2012-09-13 23:11:47 PDT 
Re-opened since this is blocked by 96729
Comment 21 Ryosuke Niwa 2012-09-13 23:14:29 PDT 
At this point, I don't know what to do with this bug. I can't fix it.
Comment 22 Ryosuke Niwa 2012-09-14 08:43:01 PDT 
I don't intend to work on this bug.
Comment 23 Ryosuke Niwa 2012-09-24 13:34:07 PDT 
Committed r129406: <http://trac.webkit.org/changeset/129406>
Comment 24 Ryosuke Niwa 2012-09-24 13:35:50 PDT 
pdr ran the tests on cr-linux with a fix (keep removedChildren outside of the scope in ContainerNode::removeChildren) for me. Thanks pdr!