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

Ryosuke Niwa
Reported 2012-09-13 16:33:55 PDT
suspendWidgetHierarchyUpdates and resumeWidgetHierarchyUpdates pair should be turned into a RAII object.
Attachments
Refactoring (20.00 KB, patch)
2012-09-13 16:42 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Reverted irrelevant xcodeproj change (15.55 KB, patch)
2012-09-13 16:45 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Speculative fix (22.39 KB, patch)
2012-09-13 21:26 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2012-09-13 16:40:30 PDT
This could coudl be duped with bug 96702, one way or the other.
Ryosuke Niwa
Comment 2 2012-09-13 16:42:00 PDT
Created attachment 164000 [details] Refactoring
Ryosuke Niwa
Comment 3 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.
Ryosuke Niwa
Comment 4 2012-09-13 16:45:24 PDT
Created attachment 164001 [details] Reverted irrelevant xcodeproj change
kov's GTK+ EWS bot
Comment 5 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
Ryosuke Niwa
Comment 6 2012-09-13 16:56:01 PDT
Will land after merging http://trac.webkit.org/changeset/128524.
Build Bot
Comment 7 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
Gyuyoung Kim
Comment 8 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
Ryosuke Niwa
Comment 9 2012-09-13 17:19:31 PDT
Committed r128531: <http://trac.webkit.org/changeset/128531>
Kent Tamura
Comment 10 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
WebKit Review Bot
Comment 12 2012-09-13 19:24:06 PDT
Re-opened since this is blocked by 96719
Ryosuke Niwa
Comment 13 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.
Ryosuke Niwa
Comment 14 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.
Ryosuke Niwa
Comment 15 2012-09-13 21:26:58 PDT
Created attachment 164043 [details] Speculative fix
Simon Fraser (smfr)
Comment 16 2012-09-13 21:53:09 PDT
Comment on attachment 164043 [details] Speculative fix What's the difference between this and the previous patch?
Ryosuke Niwa
Comment 17 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.
Ryosuke Niwa
Comment 18 2012-09-13 21:59:18 PDT
I'm landing this speculative fix for now.
Ryosuke Niwa
Comment 19 2012-09-13 21:59:42 PDT
Committed r128552: <http://trac.webkit.org/changeset/128552>
WebKit Review Bot
Comment 20 2012-09-13 23:11:47 PDT
Re-opened since this is blocked by 96729
Ryosuke Niwa
Comment 21 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.
Ryosuke Niwa
Comment 22 2012-09-14 08:43:01 PDT
I don't intend to work on this bug.
Ryosuke Niwa
Comment 23 2012-09-24 13:34:07 PDT
Committed r129406: <http://trac.webkit.org/changeset/129406>
Ryosuke Niwa
Comment 24 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!
Note You need to log in before you can comment on or make changes to this bug.