DeprecationHelper.php on line 214
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Lucas_Werkmeister_WMDE
	Apr 25 2024, 3:52 PM

Description

I’ve apparently run into some pretty cursed interaction between Parser, ScopedCallback, DeprecationHelper, and destructors.

Steps to reproduce:

Have at least MediaWiki core (as of 7edec968c3) and Scribunto (as of d5bd9a2532) installed. It’s possible that more extensions that I happen to have installed are relevant as well.
Have ini_set( 'display_errors', 1 ); in LocalSettings.php.
Start editing a Wikitext page. I suggest using ?action=submit to bypass VisualEditor.
Enter wikitext like {{#invoke:Abc|def}}. (Module:Abc need not exist.)
Preview.

Instead of steps 3-5, you can also load api.php?action=parse&text={{%23invoke%3AAbc|def}}&contentmodel=wikitext&prop=text.

Actual result:
The output ends with a warning similar to this:

( ! ) Warning: Undefined property: MediaWiki\Parser\Parser::$dynamicPropertiesAccessDeprecated in /srv/http/wiki1/includes/debug/DeprecationHelper.php on line 214
Call Stack
1 1.0133 65243256 Wikimedia\ScopedCallback->__destruct( ) .../ScopedCallback.php:0
2 1.0133 65243256 call_user_func_array:{/srv/http/wiki1/vendor/wikimedia/scoped-callback/src/ScopedCallback.php:102}( $callback = class Closure { public $this = class MediaWiki\Parser\Parser { /* many other uninitialized variables */ private $dynamicPropertiesAccessDeprecated = *uninitialized* } }, $args = [] ) .../ScopedCallback.php:102
3 1.0133 65243456 MediaWiki\Parser\Parser->MediaWiki\Parser\{closure:/srv/http/wiki1/includes/parser/Parser.php:6402-6404}( ) .../ScopedCallback.php:102
4 1.0133 65243568 MediaWiki\Parser\Parser->__set( $name = 'mInParse', $value = FALSE ) .../Parser.php:6403
5 1.0133 65244016 MediaWiki\Parser\Parser->__get( $name = 'dynamicPropertiesAccessDeprecated' )

If you’re using VisualEditor, the issue is still reproducible but you’ll only see “Invalid response from server.” in the user interface, because the warning message breaks the API’s JSON response.

Expected result:

No such warning.

What happens:

Good question! As far as I’ve been able to figure out:

The ScopedCallback returned by Parser::lock() is destructed, and tries to set $this->mInParse = false. ($this refers to the Parser.)
Apparently the Parser as a whole, and possibly other objects as well, are also being destructed at this time. (Note that the issue isn’t reproducible with simple wikitext like Abc; I suspect that Scribunto holds a reference to the Parser and somehow causes it to be destructed later than it otherwise would be. Also, for the record, I originally noticed this with Wikibase’s {{#statements:…}} parser function, which probably does a similar thing; I’m reporting it with Scribunto here because that’s more likely to be installed.)
DeprecationHelper::__set() is called (on the $parser – DeprecationHelper is a trait) with $name = 'mInParse' and $value = false. As far as I understand, this should already not happen, because mInParse is a regular and accessible property. I’m guessing it’s now inaccessible due to the ongoing destruction.
DeprecationHelper::__set() wants to check if dynamic property access is deprecated or not, and accesses $this->dynamicPropertiesAccessDeprecated.
DeprecationHelper::__get() is called with $name = 'dynamicPropertiesAccessDeprecated. Again, as far as I understand this isn’t supposed to happen.
DeprecationHelper::__get() doesn’t find an $ownerClass, so it falls through to the next check.
DeprecationHelper::__get() checks property_exists( $this, $name ). This returns true, even though at this point get_object_vars( $this ) is [] (empty).
DeprecationHelper::__get() tries to return $this->$name, but despite the property_exists() check, this produces the warning seen above.

Environment:

PHP 8.3.6, Apache2 using libphp, Arch Linux.

Details

	Subject	Repo	Branch	Lines +/-
	DeprecationHelper: Don’t crash during destruction	mediawiki/core	master	+15 -0

Customize query in gerrit

Related Objects

Mentioned Here: rELUAd5bd9a25325e: Localisation updates from https://translatewiki.net.
rMW7edec968c33f: Merge "Tests: enable non-integration tests to use TitleFactory service."

Event Timeline

Lucas_Werkmeister_WMDE created this task.Apr 25 2024, 3:52 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 25 2024, 3:52 PM

Lucas_Werkmeister_WMDE renamed this task from Warning: Undefined property: MediaWiki\Parser\Parser::$dynamicPropertiesAccessDeprecated in /srv/http/wiki1/includes/debug/DeprecationHelper.php on line 214 to Warning: Undefined property: MediaWiki\Parser\Parser::$dynamicPropertiesAccessDeprecated in includes/debug/DeprecationHelper.php on line 214.Apr 25 2024, 3:53 PM

I doubt it’s the “correct” solution, but FWIW, this seems to work to avoid the error:

diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php
index a4fe096e15..b528870cea 100644
--- a/includes/parser/Parser.php
+++ b/includes/parser/Parser.php
@@ -6400,6 +6400,10 @@ protected function lock() {
 		$this->mInParse = $e->getTraceAsString();
 
 		$recursiveCheck = new ScopedCallback( function () {
+			if ( get_object_vars( $this ) === [] ) {
+				// Parser is being destructed, do nothing (T363492)
+				return;
+			}
 			$this->mInParse = false;
 		} );

Is there any way for DeprecationHelper to determine that the object is being destructed, and suppress its warning in that case?

Also work double-checking that no one is explicitly calling unset($parser->mInParse) since that's a pattern I had to fix in a number of places. Well-meaning folks were thinking "I'll reduce memory overhead a little bit" (even though that's completely pointless, as the hash table allocated for dynamic properties by PHP was allocated when the dynamic property was set and doesn't go away, it just sits there empty instead) and unsetting their dynamic properties "when they were done with them" which then makes them dynamic and subject to a complaint from DeprecationHelper.

It doesn't seem right for object destruction to unset the properties of $parser which it is still live, and it is still live as long as the ScopedCallback hasn't been called and is holding a reference to $this.

cscott added projects: Content-Transform-Team-WIP, Content-Transform-Team.Apr 29 2024, 3:10 PM

In T363492#9745545, @cscott wrote:

Also work double-checking that no one is explicitly calling unset($parser->mInParse) …

I don’t think anything is unsetting mInParse in the code I have installed:

$ grep -rlF mInParse
HISTORY
includes/parser/Parser.php

In T363492#9745519, @cscott wrote:

Is there any way for DeprecationHelper to determine that the object is being destructed, and suppress its warning in that case?

I think checking get_object_vars( $this ) === [] should work – even if the class has no other properties, the expected result would be at least [ 'dynamicPropertiesAccessDeprecated' => false ]. Either of these patches seems to work for me:

diff --git a/includes/debug/DeprecationHelper.php b/includes/debug/DeprecationHelper.php
index 55efe457db..c422f3e053 100644
--- a/includes/debug/DeprecationHelper.php
+++ b/includes/debug/DeprecationHelper.php
@@ -220,6 +220,13 @@ public function __get( $name ) {
 	}
 
 	public function __set( $name, $value ) {
+		if ( get_object_vars( $this ) === [] ) {
+			// Object is being destructed, all bets are off (T363492).
+			// Just set the property and hope for the best?
+			$this->$name = $value;
+			return;
+		}
+
 		if ( isset( self::$deprecatedPublicProperties[$name] ) ) {
 			[ $version, $class, $component, , $setter ] = self::$deprecatedPublicProperties[$name];
 			$qualifiedName = $class . '::$' . $name;

diff --git a/includes/debug/DeprecationHelper.php b/includes/debug/DeprecationHelper.php
index 55efe457db..52cc1a61b0 100644
--- a/includes/debug/DeprecationHelper.php
+++ b/includes/debug/DeprecationHelper.php
@@ -240,7 +240,7 @@ public function __set( $name, $value ) {
 			// Someone tried to access a normal non-public property. Try to behave like PHP would.
 			trigger_error( "Cannot access non-public property $qualifiedName", E_USER_ERROR );
 		} else {
-			if ( $this->dynamicPropertiesAccessDeprecated ) {
+			if ( get_object_vars( $this ) !== [] && $this->dynamicPropertiesAccessDeprecated ) {
 				[ $version, $class, $component ] = $this->dynamicPropertiesAccessDeprecated;
 				$qualifiedName = $class . '::$' . $name;
 				wfDeprecated( $qualifiedName, $version, $component, 2 );

((array)$this instead of get_object_vars( $this ) also seems to work.)

MSantos added a project: Essential-Work.Mon, May 13, 3:06 PM

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

[mediawiki/core@master] DeprecationHelper: Don’t crash during destruction

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

gerritbot added a project: Patch-For-Review.Mon, May 13, 3:13 PM

Change #1031002 merged by jenkins-bot:

[mediawiki/core@master] DeprecationHelper: Don’t crash during destruction

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

Maintenance_bot removed a project: Patch-For-Review.Mon, May 13, 10:31 PM

ReleaseTaggerBot added a project: MW-1.43-notes (1.43.0-wmf.5; 2024-05-14).Mon, May 13, 11:00 PM

Lucas_Werkmeister_WMDE closed this task as Resolved.Tue, May 14, 8:01 AM