Merge pull request #20776 from hrydgard/more-fixes

Android: Fix a possible Vulkan shutdown race condition

Common/File/FileDescriptor.cpp

@@ -12,63 +12,6 @@
 
 namespace fd_util {
 
-// Slow as hell and should only be used for prototyping.
-// Reads from a socket, up to an '\n'. This means that if the line ends
-// with '\r', the '\r' will be returned.
-size_t ReadLine(int fd, char *vptr, size_t buf_size) {
-	char *buffer = vptr;
-	size_t n;
-	for (n = 1; n < buf_size; n++) {
-		char c;
-		size_t rc;
-		if ((rc = read(fd, &c, 1)) == 1) {
-			*buffer++ = c;
-			if (c == '\n')
-				break;
-		} else if (rc == 0) {
-			if (n == 1)
-				return 0;
-			else
-				break;
-		} else {
-			if (errno == EINTR)
-				continue;
-			_assert_msg_(false, "Error in Readline()");
-		}
-	}
-
-	*buffer = 0;
-	return n;
-}
-
-// Misnamed, it just writes raw data in a retry loop.
-size_t WriteLine(int fd, const char *vptr, size_t n) {
-	const char *buffer = vptr;
-	size_t nleft = n;
-
-	while (nleft > 0) {
-		int nwritten;
-		if ((nwritten = (int)write(fd, buffer, (unsigned int)nleft)) <= 0) {
-			if (errno == EINTR)
-				nwritten = 0;
-			else
-				_assert_msg_(false, "Error in Writeline()");
-		}
-		nleft  -= nwritten;
-		buffer += nwritten;
-	}
-
-	return n;
-}
-
-size_t WriteLine(int fd, const char *buffer) {
-	return WriteLine(fd, buffer, strlen(buffer));
-}
-
-size_t Write(int fd, const std::string &str) {
-	return WriteLine(fd, str.c_str(), str.size());
-}
-
 bool WaitUntilReady(int fd, double timeout, bool for_write) {
 	struct timeval tv;
 	tv.tv_sec = (long)floor(timeout);

Common/File/FileDescriptor.h

@@ -2,17 +2,10 @@
 
 #include <cstring>
 #include <string>
+#include <string_view>
 
 namespace fd_util {
 
-// Slow as hell and should only be used for prototyping.
-size_t ReadLine(int fd, char *buffer, size_t buf_size);
-
-// Decently fast.
-size_t WriteLine(int fd, const char *buffer, size_t buf_size);
-size_t WriteLine(int fd, const char *buffer);
-size_t Write(int fd, const std::string &str);
-
 // Returns true if the fd became ready, false if it didn't or
 // if there was another error.
 bool WaitUntilReady(int fd, double timeout, bool for_write = false);

Common/File/FileUtil.cpp

@@ -132,6 +132,8 @@ FILE *OpenCFile(const Path &path, const char *mode) {
 			return fdopen(descriptor, "rb");
 		} else if (!strcmp(mode, "w") || !strcmp(mode, "wb") || !strcmp(mode, "wt") || !strcmp(mode, "at") || !strcmp(mode, "a")) {
 			// Need to be able to create the file here if it doesn't exist.
+			// NOTE: The existance check is important, otherwise Android will create a numbered file by the side!
+			// This is also a terrible possible data race, ugh. Anyway...
 			// Not exactly sure which abstractions are best, let's start simple.
 			if (!File::Exists(path)) {
 				INFO_LOG(Log::IO, "OpenCFile(%s): Opening content file for write. Doesn't exist, creating empty and reopening.", path.c_str());
@@ -231,6 +233,8 @@ int OpenFD(const Path &path, OpenFlag flags) {
 		return -1;
 	}
 
+	bool knownExists = false;
+
 	if (flags & OPEN_CREATE) {
 		if (!File::Exists(path)) {
 			INFO_LOG(Log::IO, "OpenFD(%s): Creating file.", path.c_str());
@@ -241,17 +245,19 @@ int OpenFD(const Path &path, OpenFlag flags) {
 					WARN_LOG(Log::IO, "OpenFD: Failed to create file '%s' in '%s'", name.c_str(), parent.c_str());
 					return -1;
 				}
+				knownExists = true;
 			} else {
 				INFO_LOG(Log::IO, "Failed to navigate up to create file: %s", path.c_str());
 				return -1;
 			}
 		} else {
 			INFO_LOG(Log::IO, "OpenCFile(%s): Opening existing content file ('%s')", path.c_str(), OpenFlagToString(flags).c_str());
+			knownExists = true;
 		}
 	}
 
 	Android_OpenContentUriMode mode;
-	if (flags == OPEN_READ) {
+	if (flags == OPEN_READ) {  // Intentionally not a bitfield check.
 		mode = Android_OpenContentUriMode::READ;
 	} else if (flags & OPEN_WRITE) {
 		if (flags & OPEN_TRUNCATE) {
@@ -269,14 +275,16 @@ int OpenFD(const Path &path, OpenFlag flags) {
 	INFO_LOG(Log::IO, "Android_OpenContentUriFd: %s (%s)", path.c_str(), OpenFlagToString(flags).c_str());
 	int descriptor = Android_OpenContentUriFd(path.ToString(), mode);
 	if (descriptor < 0) {
-		ERROR_LOG(Log::IO, "Android_OpenContentUriFd failed: '%s'", path.c_str());
-	}
-
-	if (flags & OPEN_APPEND) {
+		// File probably just doesn't exist. No biggie.
+		if (knownExists) {
+			ERROR_LOG(Log::IO, "Android_OpenContentUriFd failed for existing file: '%s'", path.c_str());
+		} else {
+			INFO_LOG(Log::IO, "Android_OpenContentUriFd failed, probably doesn't exist: '%s'", path.c_str());
+		}
+	} else if (flags & OPEN_APPEND) {
 		// Simply seek to the end of the file to simulate append mode.
 		lseek(descriptor, 0, SEEK_END);
 	}
-
 	return descriptor;
 }
 
@@ -413,7 +421,7 @@ uint64_t ComputeRecursiveDirectorySize(const Path &path) {
 }
 
 // Returns true if file filename exists. Will return true on directories.
-bool ExistsInDir(const Path &path, const std::string &filename) {
+bool ExistsInDir(const Path &path, std::string_view filename) {
 	return Exists(path / filename);
 }
 
@@ -1307,7 +1315,7 @@ uint8_t *ReadLocalFile(const Path &filename, size_t *size) {
 	return contents;
 }
 
-bool WriteStringToFile(bool text_file, const std::string &str, const Path &filename) {
+bool WriteStringToFile(bool text_file, std::string_view str, const Path &filename) {
 	FILE *f = File::OpenCFile(filename, text_file ? "w" : "wb");
 	if (!f)
 		return false;

Common/File/FileUtil.h

@@ -66,11 +66,11 @@ std::string ResolvePath(std::string_view path);
 bool Exists(const Path &path);
 
 // Returns true if file filename exists in directory path.
-bool ExistsInDir(const Path &path, const std::string &filename);
+bool ExistsInDir(const Path &path, std::string_view filename);
 
 // Returns true if filename exists, and is a directory
 // Supports Android content URIs.
-bool IsDirectory(const Path &filename);
+bool IsDirectory(const Path &path);
 
 // Returns struct with modification date of file
 bool GetModifTime(const Path &filename, tm &return_time);
@@ -214,7 +214,7 @@ private:
 // TODO: Refactor, this was moved from the old file_util.cpp.
 
 // Whole-file reading/writing
-bool WriteStringToFile(bool textFile, const std::string &str, const Path &filename);
+bool WriteStringToFile(bool textFile, std::string_view str, const Path &filename);
 bool WriteDataToFile(bool textFile, const void* data, size_t size, const Path &filename);
 
 bool ReadFileToStringOptions(bool textFile, bool allowShort, const Path &path, std::string *str);

Common/File/PathBrowser.cpp

@@ -18,7 +18,7 @@
 #include "android/jni/app-android.h"
 #endif
 
-bool LoadRemoteFileList(const Path &url, const std::string &userAgent, bool *cancel, std::vector<File::FileInfo> &files) {
+static bool LoadRemoteFileList(const Path &url, std::string_view userAgent, bool *cancel, std::vector<File::FileInfo> &files) {
 	_dbg_assert_(url.Type() == PathType::HTTP);
 
 	http::Client http(nullptr);
@@ -263,7 +263,7 @@ void PathBrowser::NavigateUp() {
 }
 
 // TODO: Support paths like "../../hello"
-void PathBrowser::Navigate(const std::string &path) {
+void PathBrowser::Navigate(std::string_view path) {
 	if (path == ".")
 		return;
 	if (path == "..") {

Common/File/PathBrowser.h

@@ -2,7 +2,7 @@
 
 #include <condition_variable>
 #include <mutex>
-#include <string>
+#include <string_view>
 #include <cstring>
 #include <thread>
 #include <vector>
@@ -31,17 +31,17 @@ public:
 	bool CanNavigateUp();
 	void NavigateUp();
 
-	void Navigate(const std::string &subdir);
+	void Navigate(std::string_view subdir);
 
 	const Path &GetPath() const {
 		return path_;
 	}
 	std::string GetFriendlyPath() const;
 
-	void SetUserAgent(const std::string &s) {
+	void SetUserAgent(std::string_view s) {
 		userAgent_ = s;
 	}
-	void SetRootAlias(const std::string &alias, const Path &rootPath) {
+	void SetRootAlias(std::string_view alias, const Path &rootPath) {
 		aliasDisplay_ = alias;
 		aliasMatch_ = rootPath;
 	}

Common/GPU/D3D11/thin3d_d3d11.cpp

@@ -807,7 +807,7 @@ InputLayout *D3D11DrawContext::CreateInputLayout(const InputLayoutDesc &desc) {
 
 class D3D11ShaderModule : public ShaderModule {
 public:
-	D3D11ShaderModule(const std::string &tag) : tag_(tag) { }
+	D3D11ShaderModule(std::string_view tag) : tag_(tag) { }
 	~D3D11ShaderModule() {
 	}
 	ShaderStage GetStage() const override { return stage; }

Common/GPU/Vulkan/thin3d_vulkan.cpp

@@ -184,7 +184,7 @@ VkShaderStageFlagBits StageToVulkan(ShaderStage stage) {
 // invoke Compile again to recreate the shader then link them together.
 class VKShaderModule : public ShaderModule {
 public:
-	VKShaderModule(ShaderStage stage, const std::string &tag) : stage_(stage), tag_(tag) {
+	VKShaderModule(ShaderStage stage, std::string_view tag) : stage_(stage), tag_(tag) {
 		vkstage_ = StageToVulkan(stage);
 	}
 	bool Compile(VulkanContext *vulkan, const uint8_t *data, size_t size);

Common/Log/LogManager.cpp

@@ -199,7 +199,7 @@ void LogManager::SetFileLogPath(const Path &filename) {
 		fclose(fp_);
 	}
 
-	if (!filename.empty()) {
+	if (!filename.empty() && (outputs_ & LogOutput::File)) {
 		logFilename_ = Path(filename);
 		File::CreateFullPath(logFilename_.NavigateUp());
 		fp_ = File::OpenCFile(logFilename_, "at");

Common/StringUtils.cpp

@@ -196,7 +196,7 @@ std::string LineNumberString(const std::string &str) {
 	return output.str();
 }
 
-std::string IndentString(const std::string &str, const std::string &sep, bool skipFirst) {
+std::string IndentString(const std::string &str, std::string_view sep, bool skipFirst) {
 	std::stringstream input(str);
 	std::stringstream output;
 	std::string line;

Common/StringUtils.h

@@ -33,7 +33,7 @@
 
 // Useful for shaders with error messages..
 std::string LineNumberString(const std::string &str);
-std::string IndentString(const std::string &str, const std::string &sep, bool skipFirst = false);
+std::string IndentString(const std::string &str, std::string_view sep, bool skipFirst = false);
 
 // Other simple string utilities.
 

Common/System/OSD.cpp

@@ -209,14 +209,14 @@ void OnScreenDisplay::ShowChallengeIndicator(int achievementID, bool show) {
 	entries_.insert(entries_.begin(), entry);
 }
 
-void OnScreenDisplay::ShowLeaderboardTracker(int leaderboardTrackerID, const char *trackerText, bool show) {   // show=true is used both for create and update.
+void OnScreenDisplay::ShowLeaderboardTracker(int leaderboardTrackerID, std::string_view trackerText, bool show) {   // show=true is used both for create and update.
 	double now = time_now_d();
 
 	for (auto &entry : entries_) {
 		if (entry.numericID == leaderboardTrackerID && entry.type == OSDType::LEADERBOARD_TRACKER) {
 			if (show) {
 				// Just an update.
-				entry.text = trackerText ? trackerText : "";
+				entry.text = trackerText;
 				// Bump the end-time, in case it was fading out.
 				entry.endTime = now + forever_s;
 			} else {
@@ -239,17 +239,15 @@ void OnScreenDisplay::ShowLeaderboardTracker(int leaderboardTrackerID, const cha
 	entry.type = OSDType::LEADERBOARD_TRACKER;
 	entry.startTime = now;
 	entry.endTime = now + forever_s;
-	if (trackerText) {
-		entry.text = trackerText;
-	}
+	entry.text = trackerText;
 	entries_.insert(entries_.begin(), entry);
 }
 
-void OnScreenDisplay::ShowLeaderboardStartEnd(const std::string &title, const std::string &description, bool started) {
+void OnScreenDisplay::ShowLeaderboardStartEnd(std::string_view title, std::string_view description, bool started) {
 	g_OSD.Show(OSDType::LEADERBOARD_STARTED_FAILED, title, description, 3.0f);
 }
 
-void OnScreenDisplay::ShowLeaderboardSubmitted(const std::string &title, const std::string &value) {
+void OnScreenDisplay::ShowLeaderboardSubmitted(std::string_view title, std::string_view value) {
 	g_OSD.Show(OSDType::LEADERBOARD_SUBMITTED, title, value, 3.0f);
 }
 
@@ -285,7 +283,7 @@ void OnScreenDisplay::SetProgressBar(std::string_view id, std::string_view messa
 	entries_.push_back(bar);
 }
 
-void OnScreenDisplay::RemoveProgressBar(const std::string &id, bool success, float delay_s) {
+void OnScreenDisplay::RemoveProgressBar(std::string_view id, bool success, float delay_s) {
 	std::lock_guard<std::mutex> guard(mutex_);
 	for (auto &ent : entries_) {
 		if (ent.type == OSDType::PROGRESS_BAR && ent.id == id) {

Common/System/OSD.h

@@ -71,15 +71,15 @@ public:
 	void ShowAchievementUnlocked(int achievementID);
 	void ShowAchievementProgress(int achievementID, bool show);  // call with show=false to hide.  There can only be one of these. When hiding it's ok to not pass a valid achievementID.
 	void ShowChallengeIndicator(int achievementID, bool show);  // call with show=false to hide.
-	void ShowLeaderboardTracker(int leaderboardTrackerID, const char *trackerText, bool show);   // show=true is used both for create and update.
-	void ShowLeaderboardStartEnd(const std::string &title, const std::string &description, bool started);  // started = true for started, false for ended.
-	void ShowLeaderboardSubmitted(const std::string &title, const std::string &value);
+	void ShowLeaderboardTracker(int leaderboardTrackerID, std::string_view trackerText, bool show);   // show=true is used both for create and update.
+	void ShowLeaderboardStartEnd(std::string_view title, std::string_view description, bool started);  // started = true for started, false for ended.
+	void ShowLeaderboardSubmitted(std::string_view title, std::string_view value);
 
 	// Progress bar controls
 	// Set is both create and update. If you set maxValue <= minValue, you'll create an "indeterminate" progress
 	// bar that doesn't show a specific amount of progress.
 	void SetProgressBar(std::string_view id, std::string_view message, float minValue, float maxValue, float progress, float delay_s);
-	void RemoveProgressBar(const std::string &id, bool success, float delay_s);
+	void RemoveProgressBar(std::string_view id, bool success, float delay_s);
 
 	// Call every frame to keep the sidebar visible. Otherwise it'll fade out.
 	void NudgeSidebar();

Core/RetroAchievements.cpp

@@ -439,7 +439,7 @@ static void event_handler_callback(const rc_client_event_t *event, rc_client_t *
 	case RC_CLIENT_EVENT_LEADERBOARD_TRACKER_HIDE:
 		// A leaderboard_tracker has become inactive. The handler should hide the tracker text from the screen.
 		INFO_LOG(Log::Achievements, "Leaderboard tracker hide: '%s' (id %d)", event->leaderboard_tracker->display, event->leaderboard_tracker->id);
-		g_OSD.ShowLeaderboardTracker(event->leaderboard_tracker->id, nullptr, false);
+		g_OSD.ShowLeaderboardTracker(event->leaderboard_tracker->id, "", false);
 		break;
 	case RC_CLIENT_EVENT_LEADERBOARD_TRACKER_UPDATE:
 		// A leaderboard_tracker value has been updated. The handler should update the tracker text on the screen.

android/jni/app-android.cpp

@@ -340,9 +340,9 @@ static void EmuThreadJoin() {
 	INFO_LOG(Log::System, "EmuThreadJoin - joined");
 }
 
-static void PushCommand(std::string cmd, std::string param) {
+static void PushCommand(std::string_view cmd, std::string_view param) {
 	std::lock_guard<std::mutex> guard(frameCommandLock);
-	frameCommands.push(FrameCommand(std::move(cmd), std::move(param)));
+	frameCommands.emplace(std::string(cmd), std::string(param));
 }
 
 // Android implementation of callbacks to the Java part of the app
@@ -758,6 +758,7 @@ extern "C" void Java_org_ppsspp_ppsspp_NativeApp_init
 		}
 	}
 
+	// TODO: We should be able to do the Vulkan init in parallel with NativeInit.
 	NativeInit((int)args.size(), &args[0], user_data_path.c_str(), externalStorageDir.c_str(), cacheDir.c_str());
 
 	bFirstResume = true;

android/src/org/ppsspp/ppsspp/NativeActivity.java

@@ -716,9 +716,9 @@ public abstract class NativeActivity extends Activity implements SensorEventList
 			}
 
 			// If we got a surface, this starts the thread. If not, it doesn't.
-			if (mSurface == null) {
-				joinRenderLoopThread();
-			} else {
+			// NOTE: We do not try to join the thread here
+			if (mSurface != null) {
+				// applyFramerate is called in here.
 				startRenderLoopThread();
 			}
 		} else if (mSurface != null) {

android/src/org/ppsspp/ppsspp/PpssppActivity.java

@@ -284,38 +284,39 @@ public class PpssppActivity extends NativeActivity {
 	@Keep
 	@SuppressWarnings("unused")
 	public String[] listContentUriDir(String uriString) {
-		Cursor c = null;
 		try {
 			Uri uri = Uri.parse(uriString);
 			final ContentResolver resolver = getContentResolver();
 			final Uri childrenUri = DocumentsContract.buildChildDocumentsUriUsingTree(
-					uri, DocumentsContract.getDocumentId(uri));
+				uri, DocumentsContract.getDocumentId(uri));
 			final ArrayList<String> listing = new ArrayList<>();
 
-			c = resolver.query(childrenUri, columns, null, null, null);
-			if (c == null) {
-				return new String[]{ "X" };
-			}
-			while (c.moveToNext()) {
-				String str = cursorToString(c);
-				if (str != null) {
-					listing.add(str);
+			String[] projection = {
+				DocumentsContract.Document.COLUMN_DISPLAY_NAME,   // index 0
+				DocumentsContract.Document.COLUMN_SIZE,           // index 1
+				DocumentsContract.Document.COLUMN_FLAGS,          // index 2
+				DocumentsContract.Document.COLUMN_MIME_TYPE,      // index 3
+				DocumentsContract.Document.COLUMN_LAST_MODIFIED   // index 4
+			};
+
+			try (Cursor c = resolver.query(childrenUri, projection, null, null, null)) {
+				if (c == null) {
+					return new String[]{"X"};
+				}
+				while (c.moveToNext()) {
+					String str = cursorToString(c);
+					if (str != null) {
+						listing.add(str);
+					}
 				}
 			}
-			// Is ArrayList weird or what?
-			String[] strings = new String[listing.size()];
-			return listing.toArray(strings);
+
+			return listing.toArray(new String[0]);
 		} catch (IllegalArgumentException e) {
-			// Due to sloppy exception handling in resolver.query, we get this wrapping
-			// a FileNotFoundException if the directory doesn't exist.
-			return new String[]{ "X" };
+			return new String[]{"X"};
 		} catch (Exception e) {
 			Log.e(TAG, "listContentUriDir exception: " + e);
-			return new String[]{ "X" };
-		} finally {
-			if (c != null) {
-				c.close();
-			}
+			return new String[]{"X"};
 		}
 	}
 
@@ -346,6 +347,7 @@ public class PpssppActivity extends NativeActivity {
 			DocumentFile documentFile = DocumentFile.fromTreeUri(this, uri);
 			if (documentFile != null) {
 				// TODO: Check the file extension and choose MIME type appropriately.
+				// Or actually, let's not bother.
 				DocumentFile createdFile = documentFile.createFile("application/octet-stream", fileName);
 				return createdFile != null ? STORAGE_ERROR_SUCCESS : STORAGE_ERROR_UNKNOWN;
 			} else {
@@ -465,12 +467,16 @@ public class PpssppActivity extends NativeActivity {
 	@Keep
 	@SuppressWarnings("unused")
 	public String contentUriGetFileInfo(String fileName) {
-		Cursor c = null;
-		try {
-			Uri uri = Uri.parse(fileName);
-			final ContentResolver resolver = getContentResolver();
-			c = resolver.query(uri, columns, null, null, null);
-			if (c != null && c.moveToNext()) {
+		String[] projection = {
+			DocumentsContract.Document.COLUMN_DISPLAY_NAME,   // index 0
+			DocumentsContract.Document.COLUMN_SIZE,           // index 1
+			DocumentsContract.Document.COLUMN_FLAGS,          // index 2
+			DocumentsContract.Document.COLUMN_MIME_TYPE,      // index 3
+			DocumentsContract.Document.COLUMN_LAST_MODIFIED   // index 4
+		};
+
+		try (Cursor c = getContentResolver().query(Uri.parse(fileName), projection, null, null, null)) {
+			if (c != null && c.moveToFirst()) {
 				return cursorToString(c);
 			} else {
 				return null;
@@ -478,10 +484,6 @@ public class PpssppActivity extends NativeActivity {
 		} catch (Exception e) {
 			Log.e(TAG, "contentUriGetFileInfo exception: " + e);
 			return null;
-		} finally {
-			if (c != null) {
-				c.close();
-			}
 		}
 	}