Bug 96706 – suspend/resumeWidgetHierarchyUpdates should be a RAII object

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

suspendWidgetHierarchyUpdates and resumeWidgetHierarchyUpdates pair should be turned into a RAII object.

[Eric Seidel (no email)]

This could coudl be duped with bug 96702, one way or the other.

[Ryosuke Niwa]

Created attachment 164000 [details]
Refactoring

[Ryosuke Niwa]

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

Created attachment 164001 [details]
Reverted irrelevant xcodeproj change

[kov's GTK+ EWS bot]

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]

Will land after merging http://trac.webkit.org/changeset/128524.

[Build Bot]

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 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]

Committed r128531: <http://trac.webkit.org/changeset/128531>

[Kent Tamura]

(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

[Kent Tamura]

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&showLargeExpectations=true&tests=document-open.html

[WebKit Review Bot]

Re-opened since this is blocked by 96719

[Ryosuke Niwa]

Strange. I ran all the tests in plugins prior to landing this patch and it didn't crash.

[Ryosuke Niwa]

I can't reproduce the failure on my machine. I'll do a speculative fix and see what EWS has to say.

[Ryosuke Niwa]

Created attachment 164043 [details]
Speculative fix

[Simon Fraser (smfr)]

Comment on attachment 164043 [details]
Speculative fix

What's the difference between this and the previous patch?

[Ryosuke Niwa]

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

I'm landing this speculative fix for now.

[Ryosuke Niwa]

Committed r128552: <http://trac.webkit.org/changeset/128552>

[WebKit Review Bot]

Re-opened since this is blocked by 96729

[Ryosuke Niwa]

At this point, I don't know what to do with this bug. I can't fix it.

[Ryosuke Niwa]

I don't intend to work on this bug.

[Ryosuke Niwa]

Committed r129406: <http://trac.webkit.org/changeset/129406>

[Ryosuke Niwa]

pdr ran the tests on cr-linux with a fix (keep removedChildren outside of the scope in ContainerNode::removeChildren) for me. Thanks pdr!