Bug 260642 – Setting a link@rel=stylesheet to disabled should fully clear the stylesheet (set ownerNode=null)

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[sideshowbarker]

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

[sideshowbarker]

Pull request: https://github.com/WebKit/WebKit/pull/17001

[Radar WebKit Bug Importer]

<rdar://problem/114736719>

[EWS]

Committed 269168@main (9ae24bdf27c4): <https://commits.webkit.org/269168@main>

Reviewed commits have been landed. Closing PR #17001 and removing active labels.

[Chris Dumez]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 263021

[sideshowbarker]

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.

[sideshowbarker]

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?

[Chris Dumez]

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

[Chris Dumez]

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.

[sideshowbarker]

Pull request: https://github.com/WebKit/WebKit/pull/19331

[sideshowbarker]

(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).

[EWS]

Committed 269753@main (123f48fb7964): <https://commits.webkit.org/269753@main>

Reviewed commits have been landed. Closing PR #19331 and removing active labels.