Bug 162029

Summary: ASSERTION FAILED: m_items.isEmpty() in CustomElementReactionQueue destructor
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: New BugsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, commit-queue, dbates, esprehn+autocc, kangil.han, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 164814    
Bug Blocks: 154907    
Attachments:
Description Flags
Fixes the bug
none
Fixed a regression
none
Removed a duplicate change log entry cdumez: review+

Ryan Haddad
Reported 2016-09-15 12:51:32 PDT
Encountered on iOS 10 WK2 with LayoutTest fast/custom-elements/attribute-changed-callback.html https://build.webkit.org/results/Apple%20iOS%2010%20Simulator%20Debug%20WK2%20(Tests)/r205989%20(50)/results.html ASSERTION FAILED: m_items.isEmpty() /Volumes/Data/slave/ios-simulator-10-debug/build/Source/WebCore/dom/CustomElementReactionQueue.cpp(114) : WebCore::CustomElementReactionQueue::~CustomElementReactionQueue() 1 0x10d74896d WTFCrash 2 0x10fb2eef9 WebCore::CustomElementReactionQueue::~CustomElementReactionQueue() 3 0x10fb2ef85 WebCore::CustomElementReactionQueue::~CustomElementReactionQueue() 4 0x10fb2fe05 WebCore::CustomElementReactionStack::processQueue() 5 0x1105a4985 WebCore::CustomElementReactionStack::~CustomElementReactionStack() 6 0x1105a3c55 WebCore::CustomElementReactionStack::~CustomElementReactionStack() 7 0x11071cb6e WebCore::jsElementPrototypeFunctionSetAttribute(JSC::ExecState*) 8 0x4bb236de028 9 0x10d44dbc5 llint_entry 10 0x10d44e00d llint_entry 11 0x10d44db4b llint_entry 12 0x10d44dbc5 llint_entry 13 0x10d446b9e vmEntryToJavaScript 14 0x10d254339 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) 15 0x10d1fc794 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) 16 0x10cc9367d JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) 17 0x10cc9384e JSC::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) 18 0x11152a32b WebCore::JSMainThreadExecState::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) 19 0x11152a128 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*) 20 0x11152a40d WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ExceptionDetails*) 21 0x111538c4b WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) 22 0x111537ae9 WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) 23 0x1101d78d1 WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) 24 0x1101d76ea WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) 25 0x110102988 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() 26 0x110102e03 WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode, bool, WebCore::PumpSession&) 27 0x110101ac8 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) 28 0x11010161b WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) 29 0x1101040d6 WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl>&&) 30 0x10fba3972 WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter&, char const*, unsigned long) 31 0x10fcd92e9 WebCore::DocumentWriter::addData(char const*, unsigned long) LEAK: 1 WebProcessPool
Attachments
Fixes the bug (7.24 KB, patch)
2016-12-06 21:46 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Fixed a regression (10.89 KB, patch)
2016-12-07 14:10 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Removed a duplicate change log entry (10.34 KB, patch)
2016-12-07 16:56 PST, Ryosuke Niwa 		cdumez: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2016-09-15 21:28:26 PDT
<rdar://problem/28332657>
Ryosuke Niwa
Comment 2 2016-10-23 18:59:36 PDT
I haven't been able to reproduce this but this should be fixed in https://trac.webkit.org/changeset/207710 because we clear the queue for sure now.
Ryosuke Niwa
Comment 3 2016-12-06 20:56:17 PST
We’re still seeing this.
Ryosuke Niwa
Comment 4 2016-12-06 21:46:11 PST
Created attachment 296373 [details] Fixes the bug
Ryosuke Niwa
Comment 5 2016-12-07 14:10:36 PST
Created attachment 296420 [details] Fixed a regression
Ryosuke Niwa
Comment 6 2016-12-07 14:40:37 PST
<rdar://problem/28945851>
Chris Dumez
Comment 7 2016-12-07 16:14:41 PST
Comment on attachment 296420 [details] Fixed a regression View in context: https://bugs.webkit.org/attachment.cgi?id=296420&action=review > Source/WebCore/dom/CustomElementReactionQueue.cpp:153 > + if (element.document().refCount() <= 0) I really don't like that we're checking the refCount. Could we instead check if the document's frame is null? A document's frame should always be null after calling prepareForDestruction() on it. > LayoutTests/ChangeLog:18 > +2016-12-07 Ryosuke Niwa <rniwa@webkit.org> Duplicate ChangeLog.
Ryosuke Niwa
Comment 8 2016-12-07 16:56:57 PST
Created attachment 296441 [details] Removed a duplicate change log entry
Ryosuke Niwa
Comment 9 2016-12-07 17:23:22 PST
(In reply to comment #7) > Comment on attachment 296420 [details] > Fixed a regression > > View in context: > https://bugs.webkit.org/attachment.cgi?id=296420&action=review > > > Source/WebCore/dom/CustomElementReactionQueue.cpp:153 > > + if (element.document().refCount() <= 0) > > I really don't like that we're checking the refCount. Could we instead check > if the document's frame is null? A document's frame should always be null > after calling prepareForDestruction() on it. I did that (checking active DOM objects are stopped instead) in my previous patch, and that broke the test case I’m adding (disconnected-callback-in-detached-iframe.html). Because custom elements are coming from a different realm (iframe), we still need to enqueue disconnectedCallback in such a case. An alternative approach is to add a flag on Document which indicates that we’re in the middle of Document tear down but I’m not sure that’s better than checking refcount.
Ryosuke Niwa
Comment 10 2016-12-08 16:54:01 PST
Committed r209582: <http://trac.webkit.org/changeset/209582>
Note You need to log in before you can comment on or make changes to this bug.