Per step 3.3 of https://html.spec.whatwg.org/multipage/parsing.html#insert-a-foreign-element, the callbacks should be invoked right after node insertion, instead of scheduled in microtask. Test: https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=6197 Actually result: childNodes has two nodes. Expected result: childNodes is empty.

Per step 3.3 of https://html.spec.whatwg.org/multipage/parsing.html#insert-a-foreign-element

Comment on attachment 9007239 [details] Bug 1489437 - HTML parser should invoke custom element's callbacks before appending child nodes to it; Olli Pettay [:smaug] has approved the revision.

Pushed by echen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/07c23fd98229 HTML parser should invoke custom element's callbacks before appending child nodes to it; r=smaug

Backed out changeset 07c23fd98229 (Bug 1489437) for multiple failures in nsHtml5DocumentBuilder https://treeherder.mozilla.org/logviewer.html#?job_id=198287036&repo=autoland&lineNumber=877 02:04:18 INFO - TEST-INFO | started process 7733 (/home/cltbld/workspace/build/application/firefox/firefox -profile /tmp/tmpH7wG5Q/profile http://localhost:59379/startup_test/tspaint_test.html) 02:04:19 INFO - PID 7733 | __start_report1136__end_report 02:04:19 INFO - PID 7733 | 02:04:19 INFO - PID 7733 | __startTimestamp1536483859275__endTimestamp 02:04:19 INFO - PID 7733 | 02:04:19 INFO - PID 7733 | ###!!! [Parent][MessageChannel] Error: (msgtype=0x190080,name=PBrowser::Msg_Destroy) Closed channel: cannot send/recv 02:04:19 INFO - PID 7733 | 02:04:19 INFO - PID 7733 | 02:04:19 INFO - PID 7733 | ###!!! [Child][MessageChannel] Error: (msgtype=0x300116,name=PContent::Msg_DetachBrowsingContext) Closed channel: cannot send/recv 02:04:19 INFO - PID 7733 | 02:04:19 INFO - PID 7733 | *** UTM:SVC TimerManager:registerTimer called after profile-before-change notification. Ignoring timer registration for id: recipe-client-addon-run 02:04:20 INFO - TEST-INFO | 7733: exit 0 02:04:20 INFO - mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/I1KsZW4JRLSvMU2Co3xjNg/artifacts/public/build/target.crashreporter-symbols.zip 02:04:31 INFO - mozcrash Copy/paste: /home/cltbld/workspace/build/linux64-minidump_stackwalk /tmp/tmpH7wG5Q/profile/minidumps/6cdea4db-3446-252a-ea3e-f004ee0f335b.dmp /tmp/tmpBMCxad 02:04:37 INFO - mozcrash Saved minidump as /home/cltbld/workspace/build/blobber_upload_dir/6cdea4db-3446-252a-ea3e-f004ee0f335b.dmp 02:04:37 INFO - mozcrash Saved app info as /home/cltbld/workspace/build/blobber_upload_dir/6cdea4db-3446-252a-ea3e-f004ee0f335b.extra 02:04:37 INFO - PROCESS-CRASH | ts_paint_webext | application crashed [@ nsHtml5TreeOperation::Append(nsIContent*, nsIContent*, nsHtml5DocumentBuilder*)]

Backout by shindli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/516eec51de75 Backed out changeset 07c23fd98229 for multiple failures in nsHtml5DocumentBuilder

Hmm, we got a failure in connected-callbacks-html-fragment-parsing.html [1] with applying this fix, I suspect something incorrect in [2], investigating... [1] https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/testing/web-platform/tests/custom-elements/connected-callbacks-html-fragment-parsing.html [2] https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/parser/html/nsHtml5TreeOperation.cpp#445-507

Our implementation is correct, but the test has an incorrect expectation, filed https://github.com/web-platform-tests/wpt/issues/12947 for that.

It turns out there is a spec issue, https://github.com/whatwg/html/issues/4025.

This doesn't follow the current sepc, and Chrome & Safari don't seem to the follow the spec either. There is a spec issue filed, see https://github.com/whatwg/html/issues/4025.

(In reply to Edgar Chen [:edgar] from comment #13) > https://treeherder.mozilla.org/#/ > jobs?repo=try&tier=1&revision=485e352e22ddda1ac94a10ecade16fd7a87bacf5&group_ > state=expanded Got "ASSERTION: Want to fire DOMNodeRemoved event, but it's not safe" in wpt test, http://w3c-test.org/css/css-shadow-parts/chaining-invalid-selector.html. See https://treeherder.mozilla.org/logviewer.html#?job_id=208653119&repo=try&lineNumber=5475.

(In reply to Edgar Chen [:edgar] from comment #14) > Got "ASSERTION: Want to fire DOMNodeRemoved event, but it's not safe" in wpt > test, > http://w3c-test.org/css/css-shadow-parts/chaining-invalid-selector.html. The stack of the assertion, * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 20.1 * frame #0: 0x00000001132ca10a XUL`nsContentUtils::MaybeFireNodeRemoved(aChild=0x0000000136ff3f80, aParent=0x0000000136ff3f00) at nsContentUtils.cpp:4740 frame #1: 0x00000001134588e8 XUL`mozilla::dom::FragmentOrElement::FireNodeRemovedForChildren(this=0x0000000136ff3f00) at FragmentOrElement.cpp:2356 frame #2: 0x00000001135f73b8 XUL`nsINode::ReplaceOrInsertBefore(this=0x000000019ccd22e0, aReplace=false, aNewChild=0x0000000136ff3f00, aRefChild=0x0000000000000000, aError=0x00007ffee520a9c8) at nsINode.cpp:2298 frame #3: 0x00000001132e9fb1 XUL`nsINode::InsertBefore(this=0x000000019ccd22e0, aNode=0x0000000136ff3f00, aChild=0x0000000000000000, aError=0x00007ffee520a9c8) at nsINode.h:1798 frame #4: 0x00000001132cc0d4 XUL`nsINode::AppendChild(this=0x000000019ccd22e0, aNode=0x0000000136ff3f00, aError=0x00007ffee520a9c8) at nsINode.h:1802 frame #5: 0x0000000113a964d6 XUL`mozilla::dom::Node_Binding::appendChild(cx=0x000000010e828000, obj=Handle<JSObject *> @ 0x00007ffee520aa58, self=0x000000019ccd22e0, args=0x00007ffee520aae8) at NodeBinding.cpp:945 frame #6: 0x0000000114846510 XUL`bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(cx=0x000000010e828000, argc=1, vp=0x000000010eb49098) at BindingUtils.cpp:3314 frame #7: 0x0000000119569b44 XUL`CallJSNative(cx=0x000000010e828000, native=(XUL`bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) at BindingUtils.cpp:3283), args=0x00007ffee520d080)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) at Interpreter.cpp:461 frame #8: 0x000000011956978a XUL`js::InternalCallOrConstruct(cx=0x000000010e828000, args=0x00007ffee520d080, construct=NO_CONSTRUCT) at Interpreter.cpp:553 frame #9: 0x000000011956a061 XUL`InternalCall(cx=0x000000010e828000, args=0x00007ffee520d080) at Interpreter.cpp:607 frame #10: 0x0000000119569e5d XUL`js::CallFromStack(cx=0x000000010e828000, args=0x00007ffee520d080) at Interpreter.cpp:613 frame #11: 0x000000011955db75 XUL`Interpret(cx=0x000000010e828000, state=0x00007ffee520e1d8) at Interpreter.cpp:3451 frame #12: 0x0000000119552999 XUL`js::RunScript(cx=0x000000010e828000, state=0x00007ffee520e1d8) at Interpreter.cpp:440 frame #13: 0x0000000119569932 XUL`js::InternalCallOrConstruct(cx=0x000000010e828000, args=0x00007ffee520e480, construct=CONSTRUCT) at Interpreter.cpp:580 frame #14: 0x000000011956a63b XUL`InternalConstruct(cx=0x000000010e828000, args=0x00007ffee520e480) at Interpreter.cpp:657 frame #15: 0x000000011956a8e7 XUL`js::Construct(cx=0x000000010e828000, fval=JS::HandleValue @ 0x00007ffee520e3f0, args=0x00007ffee520e480, newTarget=JS::HandleValue @ 0x00007ffee520e3e8, objp=JS::MutableHandleObject @ 0x00007ffee520e3e0) at Interpreter.cpp:713 frame #16: 0x0000000119cbefab XUL`JS::Construct(cx=0x000000010e828000, fval=JS::HandleValue @ 0x00007ffee520e470, args=0x00007ffee520e588, objp=JS::MutableHandleObject @ 0x00007ffee520e468) at jsapi.cpp:3005 frame #17: 0x00000001133ed3ba XUL`mozilla::dom::CustomElementConstructor::Construct(this=0x00000001104e8840, aExecutionReason="Custom Element Upgrade", aRv=0x00007ffee520ead0) at CustomElementRegistry.cpp:186 frame #18: 0x00000001133f4faa XUL`mozilla::dom::(anonymous namespace)::DoUpgrade(aElement=0x00000001106cd8b0, aConstructor=0x00000001104e8840, aRv=0x00007ffee520ead0) at CustomElementRegistry.cpp:1193 frame #19: 0x00000001133f00ed XUL`mozilla::dom::CustomElementRegistry::Upgrade(aElement=0x00000001106cd8b0, aDefinition=0x0000000135d6e040, aRv=0x00007ffee520ead0) at CustomElementRegistry.cpp:1260 frame #20: 0x00000001134139fc XUL`mozilla::dom::CustomElementUpgradeReaction::Invoke(this=0x000000010df9a3c0, aElement=0x00000001106cd8b0, aRv=0x00007ffee520ead0) at CustomElementRegistry.cpp:55 frame #21: 0x00000001133f5fe3 XUL`mozilla::dom::CustomElementReactionsStack::InvokeReactions(this=0x00000001106c6980, aElementQueue=0x00000001104249d0, aGlobal=0x0000000000000000) at CustomElementRegistry.cpp:1466 frame #22: 0x00000001133f5b56 XUL`mozilla::dom::CustomElementReactionsStack::PopAndInvokeElementQueue(this=0x00000001106c6980) at CustomElementRegistry.cpp:1353 frame #23: 0x00000001127e877a XUL`mozilla::dom::CustomElementReactionsStack::LeaveCEReactions(this=0x00000001106c6980, aCx=0x0000000000000000, aWasElementQueuePushed=false) at CustomElementRegistry.h:313 frame #24: 0x00000001127e8692 XUL`mozilla::dom::AutoCEReaction::~AutoCEReaction(this=0x00007ffee520edb8) at CustomElementRegistry.h:630 frame #25: 0x00000001127e2b15 XUL`mozilla::dom::AutoCEReaction::~AutoCEReaction(this=0x00007ffee520edb8) at CustomElementRegistry.h:629 frame #26: 0x00000001127ee527 XUL`mozilla::Maybe<mozilla::dom::AutoCEReaction>::reset(this=0x00007ffee520edb8) at Maybe.h:496 frame #27: 0x00000001127ee4e5 XUL`mozilla::Maybe<mozilla::dom::AutoCEReaction>::~Maybe(this=0x00007ffee520edb8) at Maybe.h:188 frame #28: 0x00000001127e21d5 XUL`mozilla::Maybe<mozilla::dom::AutoCEReaction>::~Maybe(this=0x00007ffee520edb8) at Maybe.h:188 frame #29: 0x00000001127d82ca XUL`nsHtml5TreeOperation::Append(aNode=0x00000001106cd820, aParent=0x00000001104cd9d0, aFromParser=FROM_PARSER_NETWORK, aBuilder=0x000000010b1fbc00) at nsHtml5TreeOperation.cpp:206 frame #30: 0x00000001127dcfcf XUL`nsHtml5TreeOperation::Perform(this=0x000000011056d1e8, aBuilder=0x000000010b1fbc00, aScriptElement=0x00007ffee520f850, aInterrupted=0x00007ffee520f84f, aStreamEnded=0x00007ffee520f84e) at nsHtml5TreeOperation.cpp:831 frame #31: 0x00000001127dc94b XUL`nsHtml5TreeOpExecutor::RunFlushLoop(this=0x000000010b1fbc00) at nsHtml5TreeOpExecutor.cpp:489 frame #32: 0x00000001127e5a81 XUL`nsHtml5ExecutorReflusher::Run(this=0x0000000110423e80) at nsHtml5TreeOpExecutor.cpp:59 frame #33: 0x0000000110d8a528 XUL`mozilla::SchedulerGroup::Runnable::Run(this=0x000000011040d2e0) at SchedulerGroup.cpp:337 frame #34: 0x0000000110dc14d0 XUL`nsThread::ProcessNextEvent(this=0x000000010b107190, aMayWait=false, aResult=0x00007ffee52100b6) at nsThread.cpp:1166 frame #35: 0x0000000110dc5ed6 XUL`NS_ProcessNextEvent(aThread=0x000000010b107190, aMayWait=false) at nsThreadUtils.cpp:519 frame #36: 0x0000000111970e24 XUL`mozilla::ipc::MessagePump::Run(this=0x000000010b166e70, aDelegate=0x00007ffee52106f0) at MessagePump.cpp:97 frame #37: 0x0000000111971b16 XUL`mozilla::ipc::MessagePumpForChildProcess::Run(this=0x000000010b166e70, aDelegate=0x00007ffee52106f0) at MessagePump.cpp:301 frame #38: 0x00000001118464f5 XUL`MessageLoop::RunInternal(this=0x00007ffee52106f0) at message_loop.cc:325 frame #39: 0x0000000111846455 XUL`MessageLoop::RunHandler(this=0x00007ffee52106f0) at message_loop.cc:318 frame #40: 0x00000001118463fd XUL`MessageLoop::Run(this=0x00007ffee52106f0) at message_loop.cc:298 frame #41: 0x0000000116318203 XUL`nsBaseAppShell::Run(this=0x000000010df14ca0) at nsBaseAppShell.cpp:158 frame #42: 0x00000001163c8c43 XUL`nsAppShell::Run(this=0x000000010df14ca0) at nsAppShell.mm:742 frame #43: 0x00000001192369c0 XUL`XRE_RunAppShell() at nsEmbedFunctions.cpp:939 frame #44: 0x0000000111971972 XUL`mozilla::ipc::MessagePumpForChildProcess::Run(this=0x000000010b166e70, aDelegate=0x00007ffee52106f0) at MessagePump.cpp:269 frame #45: 0x00000001118464f5 XUL`MessageLoop::RunInternal(this=0x00007ffee52106f0) at message_loop.cc:325 frame #46: 0x0000000111846455 XUL`MessageLoop::RunHandler(this=0x00007ffee52106f0) at message_loop.cc:318 frame #47: 0x00000001118463fd XUL`MessageLoop::Run(this=0x00007ffee52106f0) at message_loop.cc:298 frame #48: 0x0000000119235f41 XUL`XRE_InitChildProcess(aArgc=16, aArgv=0x00007ffee52109b8, aChildData=0x00007ffee5210938) at nsEmbedFunctions.cpp:765 frame #49: 0x0000000119243407 XUL`mozilla::BootstrapImpl::XRE_InitChildProcess(this=0x000000010b12b190, argc=19, argv=0x00007ffee52109b8, aChildData=0x00007ffee5210938) at Bootstrap.cpp:69 frame #50: 0x000000010a9eec3c plugin-container`content_process_main(bootstrap=0x000000010b12b190, argc=19, argv=0x00007ffee52109b8) at plugin-container.cpp:50 frame #51: 0x000000010a9eecf5 plugin-container`main(argc=20, argv=0x00007ffee52109b8) at MozillaRuntimeMain.cpp:25 frame #52: 0x00007fff6d46d015 libdyld.dylib`start + 1 frame #53: 0x00007fff6d46d015 libdyld.dylib`start + 1

So that stack trace tells that script is running when it shouldn't, since mutation event might get fire. If I read the code right, we push scriptblocker on stack in nsHtml5TreeOpExecutor::RunFlushLoop(). Could we move AutoCEReaction there?

(In reply to Olli Pettay [:smaug] (high review load) from comment #17) > So that stack trace tells that script is running when it shouldn't, since > mutation event might get fire. > If I read the code right, we push scriptblocker on stack in > nsHtml5TreeOpExecutor::RunFlushLoop(). > Could we move AutoCEReaction there? Unfortunately, we could not, if we move AutoCEReaction to there, the reaction won't be invoked right after append but after whole ops execution. So we probably need to use nsHtml5AutoPauseUpdate, like https://searchfox.org/mozilla-central/rev/39cb1e96cf97713c444c5a0404d4f84627aee85d/parser/html/nsHtml5TreeOperation.cpp#414

(In reply to Edgar Chen [:edgar] from comment #19) > https://treeherder.mozilla.org/#/ > jobs?repo=try&tier=1&revision=10dc9ab482b0bf0456824b33794433818e7c292f&group_ > state=expanded There are some test failed, not sure if they are caused by my patches, investigating ...

(In reply to Edgar Chen [:edgar] from comment #20) > There are some test failed, not sure if they are caused by my patches, > investigating ... I just took a quick look, it seems nsHtml5AutoPauseUpdate changes timing of spellchecker initialization and causes some test fail ....

(In reply to Edgar Chen [:edgar] from comment #21) > I just took a quick look, it seems nsHtml5AutoPauseUpdate changes timing of > spellchecker initialization and causes some test fail .... The nsHTMLDocument::EndUpdate() may trigger editor state change and then initialize EditorSpellCheck [1]. And the patch of this bug triggers nsHTMLDocument::EndUpdate() before invoking custom-element reaction which changes the timing of EditorSpellCheck initialization and cause some spellchecker test failures. Take https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/editor/libeditor/tests/test_bug1100966.html as an example. (1) Current behaviour (without that patch), The EditorSpellCheck initialization happens before running the script of ScriptElement, and the active editing host is <script> which is not contenteditable, so mozInlineSpellChecker doesn't load dictionary, and EditorSpellCheck is created without loading dictionary. And test calls UpdateCurrentDictionary() to load ductionary. (2) With the patch: The EditorSpellCheck initialization happens when parser appends <div id="content" contenteditable> and the active editing host is <div id="content" contenteditable> which is contenteditable, so mozInlineSpellChecker tries to load dictionary, and nsIInlineSpellChecker.spellchecker returns nullptr until the dictionary is fetched. And apparently the dictionary is not ready when the test starts, so test fails at getSpellChecker().UpdateCurrentDictionary(...) which getSpellChecker() returns a nullptr. I am curious why EditorSpellCheck should be async created? And have such inconsistent behaviour? Could EditorSpellCheck be sync created (it seems to me that async created doesn't guarantee that dictionary availability, the dictionary could be async loaded after EditorSpellCheck creation)? Or maybe we should add some way to notify the EditorSpellCheck is ready to access? [1] https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/dom/html/nsHTMLDocument.cpp#2616 [2] https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/editor/spellchecker/nsIInlineSpellChecker.idl#18

(I am not sure who could shed me some light on comment #2, so just ni? to you all. Feel free to cancel or redirect)

(In reply to Edgar Chen [:edgar] from comment #23) > (I am not sure who could shed me some light on comment #2, so just ni? to > you all. Feel free to cancel or redirect) Err, comment #22.

(In reply to Edgar Chen [:edgar] from comment #22) > (In reply to Edgar Chen [:edgar] from comment #21) > > I just took a quick look, it seems nsHtml5AutoPauseUpdate changes timing of > > spellchecker initialization and causes some test fail .... > > The nsHTMLDocument::EndUpdate() may trigger editor state change and then > initialize EditorSpellCheck [1]. > And the patch of this bug triggers nsHTMLDocument::EndUpdate() before > invoking custom-element reaction which changes the timing of > EditorSpellCheck initialization and cause some spellchecker test failures. > > Take > https://searchfox.org/mozilla-central/rev/ > 17f55aee76b7c4610a974cffd3453454e0c8de7b/editor/libeditor/tests/ > test_bug1100966.html as an example. > > (1) Current behaviour (without that patch), > The EditorSpellCheck initialization happens before running the script of > ScriptElement, and the active editing host is <script> which is not > contenteditable, so mozInlineSpellChecker doesn't load dictionary, and > EditorSpellCheck is created without loading dictionary. And test calls > UpdateCurrentDictionary() to load ductionary. > > (2) With the patch: > The EditorSpellCheck initialization happens when parser appends <div > id="content" contenteditable> and the active editing host is <div > id="content" contenteditable> which is contenteditable, so > mozInlineSpellChecker tries to load dictionary, and > nsIInlineSpellChecker.spellchecker returns nullptr until the dictionary is > fetched. And apparently the dictionary is not ready when the test starts, so > test fails at getSpellChecker().UpdateCurrentDictionary(...) which > getSpellChecker() returns a nullptr. > > I am curious why EditorSpellCheck should be async created? And have such > inconsistent behaviour? Could EditorSpellCheck be sync created (it seems to > me that async created doesn't guarantee that dictionary availability, the > dictionary could be async loaded after EditorSpellCheck creation)? Or maybe > we should add some way to notify the EditorSpellCheck is ready to access? Due to performance, when initalizing spell checker is triggered by focus event on content process, we need get a infromation from chrome process, so it uses lazy initialization. And when dictionary is fetched, "inlineSpellChecker-spellCheck-ended" is observed (spellchecker tests sometimes use it). mozInlineSpellChecker::SetEnableRealTimeSpell() can create EditorSpellChecker then, mozInlineSpellChecker::GetEditorSpellCheck() can return pending EditorSpellChekcer.

(In reply to Edgar Chen [:edgar] from comment #26) > https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=6b18d4ff1852ad182c91dbf7f464c832af3b4ea0&group_state=expanded The spellcheck test failure mentioned in comment #22 could be covered by bug 1507543. But I got new test failure with the recent codebase, because of the unexpected assertions. (I didn't see this before, I think this is probably caused by recent changes) > INFO - GECKO(3154) | [Child 3261, Main Thread] ###!!! ASSERTION: The element creation should be finished!: 'mDoneCreating', file /builds/worker/workspace/build/src/dom/html/HTMLInputElement.cpp, line 4920 Take a quick look at: It seems that the reactions runs script that tries to get value from input element, but input element is not yet ready to be accessed. The value getting is from, https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/toolkit/content/widgets/datetimebox.js#21 https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/toolkit/content/widgets/datetimebox.js#54 https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/toolkit/content/widgets/datetimebox.js#648

Pushed by echen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/651fc83db5af Part 1: Make HTML parser distinguish network, document.write and fragments in append operation; r=smaug,hsivonen https://hg.mozilla.org/integration/autoland/rev/7aa96c414add Part 2: HTML parser should invoke custom element's callbacks before appending child nodes if it is not created for fragment; r=smaug

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27062 for changes under testing/web-platform/tests

Upstream PR merged by moz-wptsync-bot