Bug 96706 - suspend/resumeWidgetHierarchyUpdates should be a RAII object
Summary: suspend/resumeWidgetHierarchyUpdates should be a RAII object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 96719 96729
Blocks: 96702
  Show dependency tree / graph
 
Reported: 2012-09-13 16:33 PDT by Ryosuke Niwa
Modified: 2012-09-24 13:35 PDT (History)
10 users (show)

See Also:

Attachments
Refactoring (20.00 KB, patch)
2012-09-13 16:42 PDT, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
Reverted irrelevant xcodeproj change (15.55 KB, patch)
2012-09-13 16:45 PDT, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
Speculative fix (22.39 KB, patch)
2012-09-13 21:26 PDT, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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!