This feels like a transient error to me, was this during a deploy? Higher up in the stack trace we'd already loaded Wikimedia\Parsoid\DOM\Element to check signatures (ie stack trace entry #10) so it seems unusual that it would fail further down the call stack.

Looking at the logs in the past month
https://logstash.wikimedia.org/goto/e30c32396285439876df762513d74f49

It started with the deploys that went out on May 8th

Parsoid had a fairly large deployment
https://www.mediawiki.org/wiki/Parsoid/Deployments#May_6_-_May_8_V0.21.0-a28_as_part_of_1.44.0-wmf.28
but nothing really stands out there

However, the error originates in Cite and most likely from these commits
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Cite/+/1135965
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Cite/+/1135970

Those commits are refactoring the flags that indicate whether content ends up in the dom or stashed in data-mw

Presumably the code here is trying to access an element that was part of html that got stashed and left a dangling pointer to a node

However, the error originates in Cite and most likely from these commits
...
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Cite/+/1135970

Picking a crasher /w/rest.php/v1/page/Talk%3AFour_Noble_Truths%2FArchive_3/with_html
https://logstash.wikimedia.org/app/discover#/doc/logstash-*/logstash-deploy-1-7.0.0-1-2025.05.19?id=fgf455YBfOjk-Vo1x4xr

I isolated a snippet

<ref name="thefourtruths" group="lower-alpha">asfd</ref>

{{#tag:ref|
* Bhikkhu Bodhi <ref>123</ref>
|name="thefourtruths"|group="lower-alpha"}}

and git bisect points to the culprit c7588786303f602da150712a2621362263497bf8

Change #1151063 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Cite@master] Revert (possibly) wrong "in indicator context" logic

https://gerrit.wikimedia.org/r/1151063

Change #1151063 abandoned by Thiemo Kreuz (WMDE):

[mediawiki/extensions/Cite@master] Revert (possibly) wrong "in indicator context" logic

Reason:

No, that's not the problem.

https://gerrit.wikimedia.org/r/1151063

Change #1151065 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Cite@master] Partly revert and fix untested code path for nested <ref>

https://gerrit.wikimedia.org/r/1151065

Change #1151063 restored by Thiemo Kreuz (WMDE):

[mediawiki/extensions/Cite@master] Revert (possibly) wrong "in indicator context" logic

https://gerrit.wikimedia.org/r/1151063

Change #1150732 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Arlolra):

[mediawiki/extensions/Cite@master] Revert "Remove more "embedded content" logic that's not needed"

https://gerrit.wikimedia.org/r/1150732

Sorry for the mess 🙈️ and thanks a lot for digging into the problem. Personally, I believe the only sustainable option (besides talking to each other 😅️) is to rely on test cases when we work on this code together, across teams and continents. Unfortunately it looks like neither of the two edge-cases you discovered was covered.

The revert https://gerrit.wikimedia.org/r/1150732 is sufficient and a very valid way forward, as far as I can see. Thanks a lot for constructing this patch.

However, that doesn't solve the original problem that we apparently have critical, untested code paths. I uploaded two alternative patches, https://gerrit.wikimedia.org/r/1151063 and https://gerrit.wikimedia.org/r/1151065. Both patches add new test cases that demonstrate the different behavior. I'm relatively sure these two patches cover everything and the general revert is not needed any more.

Change #1150732 abandoned by Arlolra:

[mediawiki/extensions/Cite@master] Revert "Remove more "embedded content" logic that's not needed"

Reason:

In favour of Idd3f394fef644c47575ccee449fe3d6c225ae660 and Ia465126711adfb9f798c3f1a0e4b890b597bfc3f

https://gerrit.wikimedia.org/r/1150732

In T393913#10858225, @thiemowmde wrote:

Sorry for the mess 🙈️ and thanks a lot for digging into the problem.

Don't sweat it, it happens

Personally, I believe the only sustainable option (besides talking to each other 😅️) is to rely on test cases when we work on this code together, across teams and continents. Unfortunately it looks like neither of the two edge-cases you discovered was covered.

I believe that's the Beyonce rule, and while I agree, there's also Chesterton's fence to consider. Clearly Parsoid's Cite implementation doesn't have thorough coverage so it might be helpful to solicit review from the CTT when refactoring. Tread lightly.

The revert https://gerrit.wikimedia.org/r/1150732 is sufficient and a very valid way forward, as far as I can see. Thanks a lot for constructing this patch.

However, that doesn't solve the original problem that we apparently have critical, untested code paths. I uploaded two alternative patches, https://gerrit.wikimedia.org/r/1151063 and https://gerrit.wikimedia.org/r/1151065. Both patches add new test cases that demonstrate the different behavior. I'm relatively sure these two patches cover everything and the general revert is not needed any more.

I think you should also revert adding $this->inReferenceList() to

public function inEmbeddedContent(): bool {
		return $this->inReferenceList() || $this->embeddedContentStack;
	}

While it's true, it's conflating the two different bookkeeping systems and is unnecessary.

I've noted that in Id64424c446b50e92c28cd0f9e3cdb2de350bbd90, you've removed a pretty import return statement.

That effectively reintroduced the crasher from Icedd6fdf718183ad5bb873a9f92421f61aa49217 / T382546.

The test case from the commit message reproduces it,

{{1x|
<ref name="ref1"/>
<ref name="ref1">some content
* <ref name="ref1">ref1</ref>
</ref>
}}

Change #1151065 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Partly revert and fix untested code path for nested <ref>

https://gerrit.wikimedia.org/r/1151065

Change #1151063 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Revert and add test for "in indicator context" logic

https://gerrit.wikimedia.org/r/1151063

Change #1153166 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Cite@master] Add missing test case for crash issue in T382546

https://gerrit.wikimedia.org/r/1153166

@ABreault-WMF

https://gerrit.wikimedia.org/r/1131771 […] removed a […] return statement. That effectively reintroduced the crasher from […] T382546.

I remember being surprised by a return in middle of a 370+ lines long function. A surprisingly large chunk of unrelated pieces of logic gets skipped, e.g. code that processes dir and follow attributes as well as conflicting and missing content. Since removing the return didn't do anything (no test failure, no test case we could think of doing anything different) we figured it must be dead code.

I tried to understand better what happened in T382546. I think the return was not the best solution but unfortunately couldn't come up with a better approach. For now I suggest to just add the return back, see https://gerrit.wikimedia.org/r/1153166.

Change #1163911 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/extensions/Cite@master] [WIP] Self-referential ref

https://gerrit.wikimedia.org/r/1163911

In T393913#10937471, @thiemowmde wrote:

@ABreault-WMF

https://gerrit.wikimedia.org/r/1131771 […] removed a […] return statement. That effectively reintroduced the crasher from […] T382546.

I remember being surprised by a return in middle of a 370+ lines long function. A surprisingly large chunk of unrelated pieces of logic gets skipped, e.g. code that processes dir and follow attributes as well as conflicting and missing content. Since removing the return didn't do anything (no test failure, no test case we could think of doing anything different) we figured it must be dead code.

In a situation like that though, it might have been helpful to use git blame to identify how the return originated. The commit message had a useful test case.

I tried to understand better what happened in T382546. I think the return was not the best solution but unfortunately couldn't come up with a better approach. For now I suggest to just add the return back, see https://gerrit.wikimedia.org/r/1153166.

I put up a patch in T393913#10949148 which will hopefully take care of it

Change #1166951 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/extensions/Cite@master] Self-referential ref

https://gerrit.wikimedia.org/r/1166951

In T393913#10949155, @ABreault-WMF wrote:

I put up a patch in T393913#10949148 which will hopefully take care of it

The patch in T393913#10981766 is a better one

Change #1166951 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Ensure self-referential refs with redefinitions emit errors, not crash

https://gerrit.wikimedia.org/r/1166951

Change #1153166 abandoned by Thiemo Kreuz (WMDE):

[mediawiki/extensions/Cite@master] Add missing test case for crash issue in T382546

https://gerrit.wikimedia.org/r/1153166

Change #1163911 abandoned by Arlolra:

[mediawiki/extensions/Cite@master] [WIP] Self-referential ref

Reason:

In favour of Id4bc98544fcd8fb2ed80b4389f228b3b9cb2b48d

https://gerrit.wikimedia.org/r/1163911

Change #1269982 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Cite@master] Add missing <references> tag to a parser test

https://gerrit.wikimedia.org/r/1269982

Change #1269982 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Add missing <references> tag to a parser test

https://gerrit.wikimedia.org/r/1269982

I think Don't mutate the DocumentFragment when comparing its HTML (1298350) · Gerrit Code Review might be related to this issue: References::checkForConflictingContent calls ParsoidExtensionAPI::domToHtml which (regrettably) has side-effects -- including removing the data-object-id when it converts to inline attributes which then causes ParsoidExtensionAPI::getContentId() to return an empty string and ParsoidExtensionAPI::getContentDOM() to fail.

Dumping the DOM in inline-attribute form instead of converting it to page bundle format (as the 1298350 patch does) ought to prevent this issue.

Change #1298350 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/extensions/Cite@master] Don't mutate the DocumentFragment when comparing its HTML

https://gerrit.wikimedia.org/r/1298350