260642 2023-08-23 18:46:27 -0700 Setting a link@rel=stylesheet to disabled should fully clear the stylesheet (set ownerNode=null) 2023-10-25 02:20:25 -0700 1 1 1 Unclassified WebKit DOM WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=212515 BrowserCompat, InRadar P2 Normal --- 263021 1 mike mike ahmad.saleem792 annevk cdumez commit-queue karlcow webkit-bug-importer oldest_to_newest 1973169 0 mike 2023-08-23 18:46:27 -0700 <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. 1973170 1 mike 2023-08-23 18:52:15 -0700 Pull request: https://github.com/WebKit/WebKit/pull/17001 1974600 2 webkit-bug-importer 2023-08-30 18:47:19 -0700 <rdar://problem/114736719> 1984121 3 ews-feeder 2023-10-10 14:12:10 -0700 Committed 269168@main (9ae24bdf27c4): <https://commits.webkit.org/269168@main> Reviewed commits have been landed. Closing PR #17001 and removing active labels. 1984316 4 cdumez 2023-10-11 09:46:21 -0700 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 ``` 1984320 5 commit-queue 2023-10-11 10:02:51 -0700 Re-opened since this is blocked by bug 263021 1985305 6 mike 2023-10-16 06:07:03 -0700 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. 1986007 7 mike 2023-10-18 16:47:55 -0700 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? 1986156 8 cdumez 2023-10-19 08:32:56 -0700 (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. 1986158 9 cdumez 2023-10-19 08:45:33 -0700 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. 1986360 10 mike 2023-10-19 21:37:56 -0700 Pull request: https://github.com/WebKit/WebKit/pull/19331 1986363 11 mike 2023-10-19 21:51:17 -0700 (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). 1987342 12 ews-feeder 2023-10-25 02:20:23 -0700 Committed 269753@main (123f48fb7964): <https://commits.webkit.org/269753@main> Reviewed commits have been landed. Closing PR #19331 and removing active labels.