Page MenuHomePhabricator

ResourceLoader module missing styles for some vue files
Closed, ResolvedPublic

Description

Steps to reproduce (probably incomplete):

  • install Wikibase and MobileFrontend (and probably MinervaNeue)
  • set $wgWBRepoSettings['tmpMobileEditingUI'] = true;
  • in Wikibase, git review -d 1179147,3 (or otherwise download PS3 of this change)
  • load an item (possibly create it first) with ?useformat=mobile
  • make sure the item has at least one statement
  • scroll to the bottom of the statement list

Expected: The buttons are styled using flex to be next to the heading.

image.png (1,012×86 px, 7 KB)

Actual: Without styles, the buttons end up below the heading by default.

image.png (1,010×118 px, 8 KB)

For reasons I don’t understand yet, ResourceLoader isn’t loading the <style> block of the wikibase.wbui2025.propertySelector.vue file.

Versions used: MediaWiki commit dc343d28bf; Wikibase see above; PHP 8.4.11-1 on Arch Linux (inside Apache httpd 2.4.63-3)

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

I debugged this a little bit with this helper function (call it like iterator_to_array( self::getReferences( $arrayToInspect ) )):

private static function getReferences( array $array, string $path = '' ): iterable {
    foreach ( $array as $key => $value ) {
        if ( ReflectionReference::fromArrayElement( $array, $key ) !== null ) {
            yield "$path/$key";
        }
        if ( is_array( $value ) ) {
            yield from self::getReferences( $value, "$path/$key" );
        }
    }
}

And as far as I can tell, when FileModule::getScript() calls FileModule::getPackageFiles(), that method iterates over the files by reference to call readFileInfo() on them:

foreach ( $expandedPackageFiles['files'] as &$fileInfo ) {
    $this->readFileInfo( $context, $fileInfo );
}

Each files element is a reference while it’s being iterated over, and most of them stop being a reference as soon as iteration moves to the next element; but for a seemingly arbitrary yet deterministic set of files – reference.vue and propertySelector.vue – the referenceness “sticks” inside the package files. Which then in turn means that, when getScript() changes the files’ content to just the script and changes the type from script+style to script

public function getScript( Context $context ) {
	$packageFiles = $this->getPackageFiles( $context );
	if ( $packageFiles !== null ) {
		foreach ( $packageFiles['files'] as &$file ) {
			if ( $file['type'] === 'script+style' ) {
				$file['content'] = $file['content']['script'];
				$file['type'] = 'script';
			}
		}
		return $packageFiles;
	}

– it does this not only on its copy of the file, but also by-reference on the original file that the module caches in its fullyExpandedPackageFiles; and this means that getStyles(), called afterwards, will see those files as script-only files instead of script+style, and so there’s no styles for them.

This can be worked around by making getScript() not use references:

diff --git i/includes/ResourceLoader/FileModule.php w/includes/ResourceLoader/FileModule.php
index b030e4810a..854618dcbc 100644
--- i/includes/ResourceLoader/FileModule.php
+++ w/includes/ResourceLoader/FileModule.php
@@ -320,13 +320,16 @@ public static function extractBasePaths(
 	public function getScript( Context $context ) {
 		$packageFiles = $this->getPackageFiles( $context );
 		if ( $packageFiles !== null ) {
-			foreach ( $packageFiles['files'] as &$file ) {
-				if ( $file['type'] === 'script+style' ) {
-					$file['content'] = $file['content']['script'];
-					$file['type'] = 'script';
-				}
-			}
-			return $packageFiles;
+			return [
+				...$packageFiles,
+				'files' => array_map( function ( array $file ): array {
+					if ( $file['type'] === 'script+style' ) {
+						$file['content'] = $file['content']['script'];
+						$file['type'] = 'script';
+					}
+					return $file;
+				}, $packageFiles['files'] ),
+			];
 		}
 
 		$files = $this->getScriptFiles( $context );

But so far I have no idea where this reference confusion comes from in the first place – it feels like a bug in PHP…

ToDo: Attempt to reproduce to confirm how far reaching this bug is

This is what I see when I pull the code:

2025-08-20-103947_446x107_scrot.png (446×107 px, 5 KB)

I managed to reduce it somewhat (though for now you still need to have Wikibase with the change checked out): P81584

I’ll see if I can reduce it further without spending too much time on it. I still feel like it must be a bug in PHP.

I reduced it much further – you still need to be in a MediaWiki checkout, but Wikibase shouldn’t be necessary anymore: P81585

Especially noteworthy is what I’ve done to the parseVueContent() method:

	protected function parseVueContent( string $content ): array {
		$this->vueComponentParser ??= new VueComponentParser();
		$this->vueComponentParser->parse( $content, [ 'minifyTemplate' => false ] );
		$parsedComponent = [
			'script' => '',
			'template' => '<span @click=""></span>',
			'style' => '.wikibase-wbui2025-property-selector { display: flex; }',
			'styleLang' => 'less',
		];

Note that the result of the VueComponentParser isn’t actually used, it’s instead hard-coded afterwards. And yet, commenting out this ->parse() call will change the behavior of the code, eliminating the bug! I’m now even more convinced it’s a bug in PHP (probably core or ext-dom, unless RemexHtml includes a C extension which I don’t think it does?).

The bug also goes away if, in the template contents (line 22), you remove the @ (or change it to v-on:). So the presence of this character in an attribute name somehow confuses PHP so badly that it loses track of references in arrays…

I reduced it a bit further and it now no longer requires MediaWiki core, just RemexHtml. So I put the reproducer into a separate repository: https://gitlab.wikimedia.org/lucaswerkmeister-wmde/repro-T402278

This is biting me on https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ReaderExperiments/+/1178959

  • on PatchDemo, everything works fine
  • on my Mac, with PHP 8.4.11 installed from Homebrew, one Vue component gets styles and the others don't, reliably
  • if I patch FileModule.php as per the patch above, all styles work as expected on the Mac

After some poking around at repro: disabling xdebug resolves the error on my homebrew-given PHP 8.4.11 install on macOS.

After some poking around at repro: disabling xdebug resolves the error on my homebrew-given PHP 8.4.11 install on macOS.

Can confirm, disabling zend_extension=xdebug makes the styles show up on my wiki. Amazing.

And if I install xdebug in a php:8.4.11 Docker container, using pecl install xdebug + docker-php-ext-enable xdebug, then I can reproduce the bug inside that container too. It also seems to happen in php:8.3.24 – I’ll try to bisect it a bit more later.

After some poking around at repro: disabling xdebug resolves the error on my homebrew-given PHP 8.4.11 install on macOS.

Can confirm, disabling zend_extension=xdebug makes the styles show up on my wiki. Amazing.

xdebug breaks destructor semantics in some cases, see https://bugs.xdebug.org/view.php?id=2222.

I tested locally and confirmed that not loading xdebug or setting -dxdebug.mode=debug (to get rid of the default develop,debug value) makes the error go away.

Judging by the comment history, upstream seems to be treating this as a feature, not a bug.

xdebug breaks destructor semantics in some cases, see https://bugs.xdebug.org/view.php?id=2222.

Interesting… could that also be the cause of T366986? (T366986#10553331: “looks like PHP is getting confused about the refcount of the $magicScopeVariable in Parser::preprocess() and not calling its destructor”)

Edit: looks like it, I’ll leave a comment over there

Okay, now it’s starting to make sense.

  • Parsing the Vue template with an @click handler causes ext-dom to throw a DOMException, “Invalid Character Error”.
  • RemexHtml’s DOMBuilder::maybeCoerce() dutifully catches this exception and (in rethrowIfNotDomException()) throws it away. However, prior to that…
  • xdebug grabs a reference to this exception and keeps it alive. This includes the exception’s entire stack trace and every stack frame in it, including local variables. According to https://bugs.xdebug.org/view.php?id=2222, this is a feature to provide more information about the last 8 exceptions.
  • Thus, when we return back into ResourceLoader, some parts of the $this->fullyExpandedPackageFiles are still references, being shared between $this->fullyExpandedPackageFiles and some local variables in those stack frames.
  • Therefore, when getScript() iterates over what it thinks is a copy of the package files, and modifies some of the package files by-reference, it ends up modifying part of the original $this->fullyExpandedPackageFiles as well, and this modification leaks into getStyles().
  • Finally, getStyles() sees files where the type is “script”, rather than the original “script+style” (because getScript() rewrote them this way), and emits no styles for these files.

Change #1180866 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/core@master] ResourceLoader: Use fewer references

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

Change #1180866 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Avoid references in FileModule::getScript

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

Lucas_Werkmeister_WMDE claimed this task.

Alright, I think we can close this particular task.

Here’s an even shorter way to reproduce what I think is still the same issue – xdebug changing the behavior of a program when references are involved:

<?php

$a = [ 'a' => [] ];

function f( array &$a ): void {
	try {
		throw new Exception();
	} catch ( Exception ) {
	}
}

f( $a['a'] );

$b = $a;

$a['a'][] = 'a';

echo count( $a['a'] ) . '/' . count( $b['a'] ) . PHP_EOL;
$ XDEBUG_MODE= php T402278.php
1/0
$ XDEBUG_MODE=develop php T402278.php
1/1

(And if you change the syntax slightly to make it more backwards-compatible, you can see on 3v4l that, without xdebug, this code has behaved the same ever since PHP 5.0 introduced exceptions. PHP references may be confusing, but their behavior in this regard has at least been consistent, as far as I can tell.)