From: Timo Tijhof <krinklemail@gmail.com>
Date: Wed, 22 Feb 2017 21:54:40 +0000 (-0800)
Subject: resourceloader: Avoid endless module_deps write for the same value
X-Git-Tag: 1.31.0-rc.0~4019^2
X-Git-Url: http://git.cyclocoop.org/?a=commitdiff_plain;h=e2c4c40c96b678742ebad913727893c8340ca4c5;p=lhc%2Fweb%2Fwiklou.git

resourceloader: Avoid endless module_deps write for the same value

Follows-up 047b60b96d (ref T111481).

The if-condition compared the expanded paths, not the relative paths.
This meant there were two conditions under which the code will perform
a useless write that inserts *literally* the exact same JSON value.

1. The base directory ($IP) changes after a branch upgrade.
2. Paths contain '../', '//' or other unnormalized paths.

The latter caused various Echo and ULS methods to keep writing the
same value because one of their images is referenced in CSS using
'../'. When inserted in the database as relative path and then
expanded again at run-time and compared to the input value, they
don't match ("$IP/foo/../bar.png" != "$IP/bar.png") and cause a write.

Bug: T158813
Change-Id: I223c232d3a8c4337d09ecf7ec6e5cd7cf7effbff
---

diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php
index 71e5939c2f..6d2bc0d030 100644
--- a/includes/resourceloader/ResourceLoaderModule.php
+++ b/includes/resourceloader/ResourceLoaderModule.php
@@ -461,13 +461,28 @@ abstract class ResourceLoaderModule implements LoggerAwareInterface {
 	 * @param array $localFileRefs List of files
 	 */
 	protected function saveFileDependencies( ResourceLoaderContext $context, $localFileRefs ) {
-		// Normalise array
-		$localFileRefs = array_values( array_unique( $localFileRefs ) );
-		sort( $localFileRefs );
 
 		try {
+			// Related bugs and performance considerations:
+			// 1. Don't needlessly change the database value with the same list in a
+			//    different order or with duplicates.
+			// 2. Use relative paths to avoid ghost entries when $IP changes. (T111481)
+			// 3. Don't needlessly replace the database with the same value
+			//    just because $IP changed (e.g. when upgrading a wiki).
+			// 4. Don't create an endless replace loop on every request for this
+			//    module when '../' is used anywhere. Even though both are expanded
+			//    (one expanded by getFileDependencies from the DB, the other is
+			//    still raw as originally read by RL), the latter has not
+			//    been normalized yet.
+
+			// Normalise
+			$localFileRefs = array_values( array_unique( $localFileRefs ) );
+			sort( $localFileRefs );
+			$localPaths = self::getRelativePaths( $localFileRefs );
+
+			$storedPaths = self::getRelativePaths( $this->getFileDependencies( $context ) );
 			// If the list has been modified since last time we cached it, update the cache
-			if ( $localFileRefs !== $this->getFileDependencies( $context ) ) {
+			if ( $localPaths !== $storedPaths ) {
 				$vary = $context->getSkin() . '|' . $context->getLanguage();
 				$cache = ObjectCache::getLocalClusterInstance();
 				$key = $cache->makeKey( __METHOD__, $this->getName(), $vary );
@@ -476,8 +491,7 @@ abstract class ResourceLoaderModule implements LoggerAwareInterface {
 					return; // T124649; avoid write slams
 				}
 
-				// Use relative paths to avoid ghost entries when $IP changes (T111481)
-				$deps = FormatJson::encode( self::getRelativePaths( $localFileRefs ) );
+				$deps = FormatJson::encode( $localPaths );
 				$dbw = wfGetDB( DB_MASTER );
 				$dbw->upsert( 'module_deps',
 					[