<link rel=stylesheet disabled> should cause the corresponding stylesheet’s ownerNode to be set to null. Note that this is separate (I think) from fully implementing what’s in https://github.com/whatwg/html/pull/4519.
Pull request: https://github.com/WebKit/WebKit/pull/17001
<rdar://problem/114736719>
Committed 269168@main (9ae24bdf27c4): <https://commits.webkit.org/269168@main> Reviewed commits have been landed. Closing PR #17001 and removing active labels.
I suspect this introduced crashes in debug on various CSS tests: ``` Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 JavaScriptCore 0x138a02244 WTFCrash + 24 (Assertions.cpp:333) 1 WebCore 0x282d078ac WTFCrashWithInfo(int, char const*, char const*, int) + 36 (Assertions.h:778) 2 WebCore 0x284134440 WebCore::HTMLLinkElement::clearSheet() + 240 (HTMLLinkElement.cpp:374) 3 WebCore 0x2841346c0 WebCore::HTMLLinkElement::removedFromAncestor(WebCore::Node::RemovalType, WebCore::ContainerNode&) + 152 (HTMLLinkElement.cpp:408) 4 WebCore 0x283aed8d4 WebCore::notifyNodeRemovedFromDocument(WebCore::ContainerNode&, WebCore::TreeScopeChange, WebCore::Node&) + 324 (ContainerNodeAlgorithms.cpp:126) 5 WebCore 0x283aed9c8 WebCore::notifyNodeRemovedFromDocument(WebCore::ContainerNode&, WebCore::TreeScopeChange, WebCore::Node&) + 568 (ContainerNodeAlgorithms.cpp:134) 6 WebCore 0x283aed9c8 WebCore::notifyNodeRemovedFromDocument(WebCore::ContainerNode&, WebCore::TreeScopeChange, WebCore::Node&) + 568 (ContainerNodeAlgorithms.cpp:134) 7 WebCore 0x283aed6f4 WebCore::notifyChildNodeRemoved(WebCore::ContainerNode&, WebCore::Node&) + 204 ```
Re-opened since this is blocked by bug 263021
For the record here: this is a compat/interop issue, because Blink and Gecko both pass these tests: - https://wpt.fyi/results/css/cssom/HTMLLinkElement-disabled-002.html - https://wpt.fyi/results/css/cssom/HTMLLinkElement-disabled-002.html …while Safari does not pass those tests.
I can’t reproduce any crashes in a local debug build with any of the fast/css tests — even running 100+ iterations at a time. Is there something additional I can be doing to try to reproduce the crashes?
(In reply to sideshowbarker from comment #7) > I can’t reproduce any crashes in a local debug build with any of the > fast/css tests — even running 100+ iterations at a time. Is there something > additional I can be doing to try to reproduce the crashes? Weird. The reason for the crash seems pretty obvious, no? In HTMLLinkElement::clearSheet() we have this assertion: ``` ASSERT(m_sheet->ownerNode() == this); ``` Because we always expect to be the owner of m_sheet. However, in your patch, you added a call to `m_sheet->clearOwnerNode();` but didn't clear m_sheet. As a result, m_sheet's owner is nullptr.
I just applied your patch then ran the layout tests like so: ``` run-webkit-tests --debug --no-build fast/css ``` fast/css/list-item-pseudo-nocrash.html crashed with: ``` Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 JavaScriptCore 0x139f15244 WTFCrash + 24 1 WebCore 0x282d1ccac WTFCrashWithInfo(int, char const*, char const*, int) + 36 (Assertions.h:778) 2 WebCore 0x284157b70 WebCore::HTMLLinkElement::clearSheet() + 240 (HTMLLinkElement.cpp:374) 3 WebCore 0x284157df0 WebCore::HTMLLinkElement::removedFromAncestor(WebCore::Node::RemovalType, WebCore::ContainerNode&) + 152 (HTMLLinkElement.cpp:408) 4 WebCore 0x283b05254 WebCore::notifyNodeRemovedFromDocument(WebCore::ContainerNode&, WebCore::TreeScopeChange, WebCore::Node&) + 324 (ContainerNodeAlgorithms.cpp:126) 5 WebCore 0x283b05348 WebCore::notifyNodeRemovedFromDocument(WebCore::ContainerNode&, WebCore::TreeScopeChange, WebCore::Node&) + 568 (ContainerNodeAlgorithms.cpp:134) 6 WebCore 0x283b05348 WebCore::notifyNodeRemovedFromDocument(WebCore::ContainerNode&, WebCore::TreeScopeChange, WebCore::Node&) + 568 (ContainerNodeAlgorithms.cpp:134) 7 WebCore 0x283b05074 WebCore::notifyChildNodeRemoved(WebCore::ContainerNode&, WebCore::Node&) + 204 (ContainerNodeAlgorithms.cpp:178) 8 WebCore 0x283afe20c WebCore::removeDetachedChildrenInContainer(WebCore::ContainerNode&) + 404 (ContainerNodeAlgorithms.cpp:198) 9 WebCore 0x283afe02c WebCore::ContainerNode::removeDetachedChildren() + 172 (ContainerNode.cpp:348) 10 WebCore 0x283b6e3f4 WebCore::Document::removedLastRef() + 580 (Document.cpp:828) 11 WebCore 0x283d58050 WebCore::Node::removedLastRef() + 160 (Node.cpp:2645) 12 WebCore 0x283279a10 WebCore::Node::deref() const + 568 (Node.h:822) 13 WebCore 0x283ae66d4 WTF::Ref<WebCore::ContainerNode, WTF::RawPtrTraits<WebCore::ContainerNode>>::~Ref() + 76 (Ref.h:61) 14 WebCore 0x283ad9700 WTF::Ref<WebCore::ContainerNode, WTF::RawPtrTraits<WebCore::ContainerNode>>::~Ref() + 32 (Ref.h:55) 15 WebCore 0x283adf5d8 WebCore::ChildNodeList::~ChildNodeList() + 96 (ChildNodeList.cpp:49) ``` I didn't check if more tests crash later on, I interrupted the run once I got one crash. If you still have trouble reproducing, try passing `--additional-env-var=__XPC_JSC_slowPathAllocsBetweenGCs=10` to run-webkit-tests too to get more aggressive garbage collection.
Pull request: https://github.com/WebKit/WebKit/pull/19331
(In reply to Chris Dumez from comment #8) > (In reply to sideshowbarker from comment #7) > > I can’t reproduce any crashes in a local debug build with any of the > > fast/css tests — even running 100+ iterations at a time. Is there something > > additional I can be doing to try to reproduce the crashes? > > Weird. The reason for the crash seems pretty obvious, no? Yeah — what’s less clear to me now is why I ever had it calling m_sheet->clearOwnerNode() directly to begin with, rather than just calling clearSheet() (which clearly seems like the right and obvious thing to do). But I still haven’t been able to reproduce the crash with fast/css/list-item-pseudo-nocrash.html locally — even after running “run-webkit-tests --debug --no-build --additional-env-var=__XPC_JSC_slowPathAllocsBetweenGCs=10 fast/css” — so what worries me is that if I couldn’t catch/reproduce the crash in my environment when I introduced the regression (and still can’t seem to reproduce it now…), I also might not be able to catch any other crash/regression another change might introduce. Anyway, I guess it’s kind of moot as far as this particular issue/PR is concerned — because the right fix for it is clear (and is what https://github.com/WebKit/WebKit/pull/19331 now does).
Committed 269753@main (123f48fb7964): <https://commits.webkit.org/269753@main> Reviewed commits have been landed. Closing PR #19331 and removing active labels.