Return correct frame from mw.getCurrentFrame in certain edge cases

When an #invoke is passed as an argument to another #invoke,
mw.getCurrentFrame() at module scope will return the wrong frame.

On the PHP side, we need to always reset the frame when processing
an #invoke, not just when there's no frame already. I don't remember why
I82dde43e wasn't done that way, but changing it doesn't make any tests
fail and Scribunto tends to have good tests.

On the Lua side, we need to do the same. The logic wih mw.getCurrentFrame()
using a global that gets stored, modified, and reset in several places
was getting confusing, so this patch reworks the logic to inject a
globalless mw.getCurrentFrame() into each #invoke's cloned environment
instead.

Bug: T234368
Change-Id: I8cb5bc4dc14c9b448c9f267e0539daa75e72af4c

includes/engines/LuaCommon/LuaCommon.php

@@ -260,12 +260,8 @@ abstract class Scribunto_LuaEngine extends ScribuntoEngineBase {
 	 * @throws ScribuntoException
 	 */
 	public function executeModule( $chunk, $functionName, $frame ) {
-		$resetFrames = null;
-		if ( !$this->currentFrames || !isset( $this->currentFrames['current'] ) ) {
-			// Only reset frames if there isn't already current frame
 		// $resetFrames is a ScopedCallback, so it has a purpose even though it appears unused.
 		$resetFrames = $this->setupCurrentFrames( $frame );
-		}
 
 		$retval = $this->getInterpreter()->callFunction(
 			$this->mw['executeModule'], $chunk, $functionName

includes/engines/LuaCommon/lualib/mw.lua

@@ -5,7 +5,6 @@ local packageModuleFunc
 local php
 local allowEnvFuncs = false
 local logBuffer = ''
-local currentFrame
 local loadedData = {}
 local executeFunctionDepth = 0
 
@@ -453,9 +452,10 @@ end
 --
 -- @param chunk The module chunk
 -- @param name The name of the function to be returned. Nil or false causes the entire export table to be returned
+-- @param frame New frame to use; if nil, one will be created
 -- @return boolean Whether the requested value was able to be returned
 -- @return table|function|string The requested value, or if that was unable to be returned, the type of the value returned by the module
-function mw.executeModule( chunk, name )
+function mw.executeModule( chunk, name, frame )
 	local env = mw.clone( _G )
 	makePackageModule( env )
 
@@ -477,14 +477,14 @@ function mw.executeModule( chunk, name )
 	env.os.date = ttlDate
 	env.os.time = ttlTime
 
+	frame = frame or newFrame( 'current', 'parent' )
+	env.mw.getCurrentFrame = function ()
+		return frame
+	end
+
 	setfenv( chunk, env )
 
-	local oldFrame = currentFrame
-	if not currentFrame then
-		currentFrame = newFrame( 'current', 'parent' )
-	end
 	local res = chunk()
-	currentFrame = oldFrame
 
 	if not name then -- catch console whether it's evaluating its own code or a module's
 		return true, res
@@ -496,8 +496,7 @@ function mw.executeModule( chunk, name )
 end
 
 function mw.executeFunction( chunk )
-	local frame = newFrame( 'current', 'parent' )
-	local oldFrame = currentFrame
+	local frame = getfenv( chunk ).mw.getCurrentFrame()
 
 	if executeFunctionDepth == 0 then
 		-- math.random is defined as using C's rand(), and C's rand() uses 1 as
@@ -507,9 +506,7 @@ function mw.executeFunction( chunk )
 	end
 	executeFunctionDepth = executeFunctionDepth + 1
 
-	currentFrame = frame
 	local results = { chunk( frame ) }
-	currentFrame = oldFrame
 
 	local stringResults = {}
 	for i, result in ipairs( results ) do
@@ -633,13 +630,6 @@ function mw.getLogBuffer()
 	return logBuffer
 end
 
-function mw.getCurrentFrame()
-	if not currentFrame then
-		currentFrame = newFrame( 'current', 'parent' )
-	end
-	return currentFrame
-end
-
 function mw.isSubsting()
 	return php.isSubsting()
 end
@@ -750,19 +740,14 @@ function mw.loadData( module )
 		error( data, 2 )
 	end
 	if not data then
-		-- Don't allow accessing the current frame's info (bug 65687)
-		local oldFrame = currentFrame
-		currentFrame = newFrame( 'empty' )
-
 		-- The point of this is to load big data, so don't save it in package.loaded
 		-- where it will have to be copied for all future modules.
 		local l = package.loaded[module]
 		local _
 
-		_, data = mw.executeModule( function() return require( module ) end )
+		_, data = mw.executeModule( function() return require( module ) end, nil, newFrame( 'empty' ) )
 
 		package.loaded[module] = l
-		currentFrame = oldFrame
 
 		-- Validate data
 		local err

tests/phpunit/engines/LuaCommon/CommonTest.php

@@ -729,6 +729,47 @@ class Scribunto_LuaCommonTest extends Scribunto_LuaEngineTestBase {
 		}
 	}
 
+	public function testGetCurrentFrameAtModuleScopeT234368() {
+		$engine = $this->getEngine();
+		$parser = $engine->getParser();
+		$pp = $parser->getPreprocessor();
+
+		$this->extraModules['Module:Outer'] = '
+			local p = {}
+
+			function p.echo( frame )
+				return "(Outer: 1=" .. frame.args[1] .. ", 2=" .. frame.args[2] .. ")"
+			end
+
+			return p
+		';
+		$this->extraModules['Module:Inner'] = '
+			local p = {}
+
+			local f = mw.getCurrentFrame()
+			local name = f:getTitle()
+			local arg1 = f.args[1]
+
+			function p.test( frame )
+				local f2 = mw.getCurrentFrame()
+				return "(Inner: mod_name=" .. name .. ", mod_1=" .. arg1 .. ", name=" .. f2:getTitle() ..
+					", 1=".. f2.args[1] .. ")"
+			end
+
+			return p
+		';
+
+		$frame = $pp->newFrame();
+		$text = $frame->expand( $pp->preprocessToObj(
+			"{{#invoke:Outer|echo|oarg|{{#invoke:Inner|test|iarg}}}}"
+		) );
+		$text = $parser->mStripState->unstripBoth( $text );
+		$this->assertSame(
+			'(Outer: 1=oarg, 2=(Inner: mod_name=Module:Inner, mod_1=iarg, name=Module:Inner, 1=iarg))',
+			$text
+		);
+	}
+
 	public function testNonUtf8Errors() {
 		$engine = $this->getEngine();
 		$parser = $engine->getParser();