CLOUD: Fix CloudManager::connectStorage() memory leak

backends/cloud/box/boxstorage.cpp

@@ -56,12 +56,16 @@ BoxStorage::BoxStorage(Common::String accessToken, Common::String refreshToken):
 	_token(accessToken), _refreshToken(refreshToken) {}
 
 BoxStorage::BoxStorage(Common::String code) {
-	getAccessToken(new Common::Callback<BoxStorage, BoolResponse>(this, &BoxStorage::codeFlowComplete), code);
+	getAccessToken(
+		new Common::Callback<BoxStorage, BoolResponse>(this, &BoxStorage::codeFlowComplete),
+		new Common::Callback<BoxStorage, Networking::ErrorResponse>(this, &BoxStorage::codeFlowFailed),
+		code
+	);
 }
 
 BoxStorage::~BoxStorage() {}
 
-void BoxStorage::getAccessToken(BoolCallback callback, Common::String code) {
+void BoxStorage::getAccessToken(BoolCallback callback, Networking::ErrorCallback errorCallback, Common::String code) {
 	if (!KEY || !SECRET) loadKeyAndSecret();
 	bool codeFlow = (code != "");
 
@@ -72,7 +76,8 @@ void BoxStorage::getAccessToken(BoolCallback callback, Common::String code) {
 	}
 
 	Networking::JsonCallback innerCallback = new Common::CallbackBridge<BoxStorage, BoolResponse, Networking::JsonResponse>(this, &BoxStorage::tokenRefreshed, callback);
-	Networking::CurlJsonRequest *request = new Networking::CurlJsonRequest(innerCallback, getErrorPrintingCallback(), "https://api.box.com/oauth2/token");
+	if (errorCallback == nullptr) errorCallback = getErrorPrintingCallback();
+	Networking::CurlJsonRequest *request = new Networking::CurlJsonRequest(innerCallback, errorCallback, "https://api.box.com/oauth2/token");
 	if (codeFlow) {
 		request->addPostField("grant_type=authorization_code");
 		request->addPostField("code=" + code);
@@ -117,6 +122,7 @@ void BoxStorage::tokenRefreshed(BoolCallback callback, Networking::JsonResponse
 void BoxStorage::codeFlowComplete(BoolResponse response) {
 	if (!response.value) {
 		warning("BoxStorage: failed to get access token through code flow");
+		CloudMan.removeStorage(this);
 		return;
 	}
 
@@ -124,6 +130,12 @@ void BoxStorage::codeFlowComplete(BoolResponse response) {
 	ConfMan.flushToDisk();
 }
 
+void BoxStorage::codeFlowFailed(Networking::ErrorResponse error) {
+	debug("Box's code flow failed (%s, %ld):", (error.failed ? "failed" : "interrupted"), error.httpResponseCode);
+	debug("%s", error.response.c_str());
+	CloudMan.removeStorage(this);
+}
+
 void BoxStorage::saveConfig(Common::String keyPrefix) {
 	ConfMan.set(keyPrefix + "access_token", _token, ConfMan.kCloudDomain);
 	ConfMan.set(keyPrefix + "refresh_token", _refreshToken, ConfMan.kCloudDomain);

backends/cloud/box/boxstorage.h

@@ -42,6 +42,7 @@ class BoxStorage: public Id::IdStorage {
 
 	void tokenRefreshed(BoolCallback callback, Networking::JsonResponse response);
 	void codeFlowComplete(BoolResponse response);
+	void codeFlowFailed(Networking::ErrorResponse error);
 
 	/** Constructs StorageInfo based on JSON response from cloud. */
 	void infoInnerCallback(StorageInfoCallback outerCallback, Networking::JsonResponse json);
@@ -108,7 +109,7 @@ public:
 	 * Use "" in order to refresh token and pass a callback, so you could
 	 * continue your work when new token is available.
 	 */
-	void getAccessToken(BoolCallback callback, Common::String code = "");
+	void getAccessToken(BoolCallback callback, Networking::ErrorCallback errorCallback = nullptr, Common::String code = "");
 
 	Common::String accessToken() { return _token; }
 };

backends/cloud/cloudmanager.cpp

@@ -45,6 +45,7 @@ CloudManager::CloudManager() : _currentStorageIndex(0), _activeStorage(nullptr)
 CloudManager::~CloudManager() {
 	//TODO: do we have to save storages on manager destruction?	
 	delete _activeStorage;
+	freeStorages();
 }
 
 Common::String CloudManager::getStorageConfigName(uint32 index) const {
@@ -124,6 +125,7 @@ void CloudManager::save() {
 }
 
 void CloudManager::replaceStorage(Storage *storage, uint32 index) {
+	freeStorages();
 	if (!storage) error("CloudManager::replaceStorage: NULL storage passed");
 	if (index >= kStorageTotal) error("CloudManager::replaceStorage: invalid index passed");
 	delete _activeStorage;
@@ -138,6 +140,18 @@ void CloudManager::replaceStorage(Storage *storage, uint32 index) {
 	}
 }
 
+void CloudManager::removeStorage(Storage *storage) {
+	// can't just delete it as it's mostly likely the one who calls the method
+	// it would be freed on freeStorages() call (on next Storage connect or replace)
+	_storagesToRemove.push_back(storage);
+}
+
+void CloudManager::freeStorages() {
+	for (uint32 i = 0; i < _storagesToRemove.size(); ++i)
+		delete _storagesToRemove[i];
+	_storagesToRemove.clear();
+}
+
 Storage *CloudManager::getCurrentStorage() const {
 	return _activeStorage;
 }
@@ -207,6 +221,8 @@ void CloudManager::setStorageLastSync(uint32 index, Common::String date) {
 }
 
 void CloudManager::connectStorage(uint32 index, Common::String code) {
+	freeStorages();
+
 	Storage *storage = nullptr;
 	switch (index) {
 	case kStorageDropboxId: storage = new Dropbox::DropboxStorage(code); break;
@@ -214,7 +230,9 @@ void CloudManager::connectStorage(uint32 index, Common::String code) {
 	case kStorageGoogleDriveId: storage = new GoogleDrive::GoogleDriveStorage(code); break;
 	case kStorageBoxId: storage = new Box::BoxStorage(code); break;
 	}
-	//these would automatically request replaceStorage() when they receive the token
+	// in these constructors Storages request token using the passed code
+	// when the token is received, they call replaceStorage()
+	// or removeStorage(), if some error occurred
 }
 
 void CloudManager::printBool(Storage::BoolResponse response) const {

backends/cloud/cloudmanager.h

@@ -59,6 +59,7 @@ class CloudManager : public Common::Singleton<CloudManager> {
 	Common::Array<StorageConfig> _storages;
 	uint _currentStorageIndex;
 	Storage *_activeStorage;
+	Common::Array<Storage *> _storagesToRemove;
 
 	void printBool(Cloud::Storage::BoolResponse response) const;
 
@@ -66,6 +67,9 @@ class CloudManager : public Common::Singleton<CloudManager> {
 
 	Common::String getStorageConfigName(uint32 index) const;
 
+	/** Frees memory used by storages which failed to connect. */
+	void freeStorages();
+
 public:
 	CloudManager();
 	virtual ~CloudManager();
@@ -91,6 +95,9 @@ public:
 	 */
 	void replaceStorage(Storage *storage, uint32 index);
 
+	/** Adds storage in the list of storages to remove later. */
+	void removeStorage(Storage *storage);
+
 	/**
 	 * Returns active Storage, which could be used to interact
 	 *  with cloud storage.

backends/cloud/dropbox/dropboxstorage.cpp

@@ -62,8 +62,9 @@ DropboxStorage::~DropboxStorage() {}
 
 void DropboxStorage::getAccessToken(Common::String code) {
 	if (!KEY || !SECRET) loadKeyAndSecret();
-	Networking::JsonCallback callback = new Common::Callback<DropboxStorage, Networking::JsonResponse>(this, &DropboxStorage::codeFlowComplete);		
+	Networking::JsonCallback callback = new Common::Callback<DropboxStorage, Networking::JsonResponse>(this, &DropboxStorage::codeFlowComplete);
-	Networking::CurlJsonRequest *request = new Networking::CurlJsonRequest(callback, nullptr, "https://api.dropboxapi.com/oauth2/token");
+	Networking::ErrorCallback errorCallback = new Common::Callback<DropboxStorage, Networking::ErrorResponse>(this, &DropboxStorage::codeFlowFailed);
+	Networking::CurlJsonRequest *request = new Networking::CurlJsonRequest(callback, errorCallback, "https://api.dropboxapi.com/oauth2/token");
 	request->addPostField("code=" + code);
 	request->addPostField("grant_type=authorization_code");
 	request->addPostField("client_id=" + Common::String(KEY));
@@ -83,6 +84,7 @@ void DropboxStorage::codeFlowComplete(Networking::JsonResponse response) {
 		if (!result.contains("access_token") || !result.contains("uid")) {
 			warning("%s", json->stringify(true).c_str());
 			warning("Bad response, no token/uid passed");
+			CloudMan.removeStorage(this);
 		} else {
 			_token = result.getVal("access_token")->asString();
 			_uid = result.getVal("uid")->asString();			
@@ -94,9 +96,16 @@ void DropboxStorage::codeFlowComplete(Networking::JsonResponse response) {
 		delete json;
 	} else {
 		debug("DropboxStorage::codeFlowComplete: got NULL instead of JSON!");
+		CloudMan.removeStorage(this);
 	}
 }
 
+void DropboxStorage::codeFlowFailed(Networking::ErrorResponse error) {
+	debug("Dropbox's code flow failed (%s, %ld):", (error.failed ? "failed" : "interrupted"), error.httpResponseCode);
+	debug("%s", error.response.c_str());
+	CloudMan.removeStorage(this);
+}
+
 void DropboxStorage::saveConfig(Common::String keyPrefix) {
 	ConfMan.set(keyPrefix + "access_token", _token, ConfMan.kCloudDomain);
 	ConfMan.set(keyPrefix + "user_id", _uid, ConfMan.kCloudDomain);

backends/cloud/dropbox/dropboxstorage.h

@@ -42,6 +42,7 @@ class DropboxStorage: public Cloud::Storage {
 
 	void getAccessToken(Common::String code);
 	void codeFlowComplete(Networking::JsonResponse response);
+	void codeFlowFailed(Networking::ErrorResponse error);
 
 public:
 	/** This constructor uses OAuth code flow to get tokens. */

backends/cloud/googledrive/googledrivestorage.cpp

@@ -56,12 +56,16 @@ GoogleDriveStorage::GoogleDriveStorage(Common::String accessToken, Common::Strin
 	_token(accessToken), _refreshToken(refreshToken) {}
 
 GoogleDriveStorage::GoogleDriveStorage(Common::String code) {
-	getAccessToken(new Common::Callback<GoogleDriveStorage, BoolResponse>(this, &GoogleDriveStorage::codeFlowComplete), code);
+	getAccessToken(
+		new Common::Callback<GoogleDriveStorage, BoolResponse>(this, &GoogleDriveStorage::codeFlowComplete),
+		new Common::Callback<GoogleDriveStorage, Networking::ErrorResponse>(this, &GoogleDriveStorage::codeFlowFailed),
+		code
+	);
 }
 
 GoogleDriveStorage::~GoogleDriveStorage() {}
 
-void GoogleDriveStorage::getAccessToken(BoolCallback callback, Common::String code) {
+void GoogleDriveStorage::getAccessToken(BoolCallback callback, Networking::ErrorCallback errorCallback, Common::String code) {
 	if (!KEY || !SECRET) loadKeyAndSecret();
 	bool codeFlow = (code != "");
 
@@ -72,7 +76,8 @@ void GoogleDriveStorage::getAccessToken(BoolCallback callback, Common::String co
 	}
 
 	Networking::JsonCallback innerCallback = new Common::CallbackBridge<GoogleDriveStorage, BoolResponse, Networking::JsonResponse>(this, &GoogleDriveStorage::tokenRefreshed, callback);
-	Networking::CurlJsonRequest *request = new Networking::CurlJsonRequest(innerCallback, getErrorPrintingCallback(), "https://accounts.google.com/o/oauth2/token"); //TODO
+	if (errorCallback == nullptr) errorCallback = getErrorPrintingCallback();
+	Networking::CurlJsonRequest *request = new Networking::CurlJsonRequest(innerCallback, errorCallback, "https://accounts.google.com/o/oauth2/token"); //TODO
 	if (codeFlow) {
 		request->addPostField("code=" + code);
 		request->addPostField("grant_type=authorization_code");
@@ -118,6 +123,7 @@ void GoogleDriveStorage::tokenRefreshed(BoolCallback callback, Networking::JsonR
 void GoogleDriveStorage::codeFlowComplete(BoolResponse response) {
 	if (!response.value) {
 		warning("GoogleDriveStorage: failed to get access token through code flow");
+		CloudMan.removeStorage(this);
 		return;
 	}
 
@@ -126,6 +132,12 @@ void GoogleDriveStorage::codeFlowComplete(BoolResponse response) {
 	ConfMan.flushToDisk();
 }
 
+void GoogleDriveStorage::codeFlowFailed(Networking::ErrorResponse error) {
+	debug("Google Drive's code flow failed (%s, %ld):", (error.failed ? "failed" : "interrupted"), error.httpResponseCode);
+	debug("%s", error.response.c_str());
+	CloudMan.removeStorage(this);
+}
+
 void GoogleDriveStorage::saveConfig(Common::String keyPrefix) {	
 	ConfMan.set(keyPrefix + "access_token", _token, ConfMan.kCloudDomain);
 	ConfMan.set(keyPrefix + "refresh_token", _refreshToken, ConfMan.kCloudDomain);

backends/cloud/googledrive/googledrivestorage.h

@@ -42,6 +42,7 @@ class GoogleDriveStorage: public Id::IdStorage {
 
 	void tokenRefreshed(BoolCallback callback, Networking::JsonResponse response);
 	void codeFlowComplete(BoolResponse response);
+	void codeFlowFailed(Networking::ErrorResponse error);
 
 	/** Constructs StorageInfo based on JSON response from cloud. */
 	void infoInnerCallback(StorageInfoCallback outerCallback, Networking::JsonResponse json);
@@ -110,7 +111,7 @@ public:
 	 * Use "" in order to refresh token and pass a callback, so you could
 	 * continue your work when new token is available.
 	 */
-	void getAccessToken(BoolCallback callback, Common::String code = "");
+	void getAccessToken(BoolCallback callback, Networking::ErrorCallback errorCallback = nullptr, Common::String code = "");
 
 	Common::String accessToken() { return _token; }
 };

backends/cloud/onedrive/onedrivestorage.cpp

@@ -57,12 +57,16 @@ OneDriveStorage::OneDriveStorage(Common::String accessToken, Common::String user
 	_token(accessToken), _uid(userId), _refreshToken(refreshToken) {}
 
 OneDriveStorage::OneDriveStorage(Common::String code) {
-	getAccessToken(new Common::Callback<OneDriveStorage, BoolResponse>(this, &OneDriveStorage::codeFlowComplete), code);
+	getAccessToken(
+		new Common::Callback<OneDriveStorage, BoolResponse>(this, &OneDriveStorage::codeFlowComplete),
+		new Common::Callback<OneDriveStorage, Networking::ErrorResponse>(this, &OneDriveStorage::codeFlowFailed),
+		code
+	);
 }
 
 OneDriveStorage::~OneDriveStorage() {}
 
-void OneDriveStorage::getAccessToken(BoolCallback callback, Common::String code) {
+void OneDriveStorage::getAccessToken(BoolCallback callback, Networking::ErrorCallback errorCallback, Common::String code) {
 	if (!KEY || !SECRET) loadKeyAndSecret();
 	bool codeFlow = (code != "");
 
@@ -73,7 +77,8 @@ void OneDriveStorage::getAccessToken(BoolCallback callback, Common::String code)
 	}
 
 	Networking::JsonCallback innerCallback = new Common::CallbackBridge<OneDriveStorage, BoolResponse, Networking::JsonResponse>(this, &OneDriveStorage::tokenRefreshed, callback);
-	Networking::CurlJsonRequest *request = new Networking::CurlJsonRequest(innerCallback, getErrorPrintingCallback(), "https://login.live.com/oauth20_token.srf"); //TODO
+	if (errorCallback == nullptr) errorCallback = getErrorPrintingCallback();
+	Networking::CurlJsonRequest *request = new Networking::CurlJsonRequest(innerCallback, errorCallback, "https://login.live.com/oauth20_token.srf"); //TODO
 	if (codeFlow) {
 		request->addPostField("code=" + code);
 		request->addPostField("grant_type=authorization_code");
@@ -117,6 +122,7 @@ void OneDriveStorage::tokenRefreshed(BoolCallback callback, Networking::JsonResp
 void OneDriveStorage::codeFlowComplete(BoolResponse response) {
 	if (!response.value) {
 		warning("OneDriveStorage: failed to get access token through code flow");
+		CloudMan.removeStorage(this);
 		return;
 	}
 
@@ -125,6 +131,12 @@ void OneDriveStorage::codeFlowComplete(BoolResponse response) {
 	ConfMan.flushToDisk();
 }
 
+void OneDriveStorage::codeFlowFailed(Networking::ErrorResponse error) {
+	debug("OneDrive's code flow failed (%s, %ld):", (error.failed ? "failed" : "interrupted"), error.httpResponseCode);
+	debug("%s", error.response.c_str());
+	CloudMan.removeStorage(this);
+}
+
 void OneDriveStorage::saveConfig(Common::String keyPrefix) {
 	ConfMan.set(keyPrefix + "access_token", _token, ConfMan.kCloudDomain);
 	ConfMan.set(keyPrefix + "user_id", _uid, ConfMan.kCloudDomain);

backends/cloud/onedrive/onedrivestorage.h

@@ -41,6 +41,7 @@ class OneDriveStorage: public Cloud::Storage {
 
 	void tokenRefreshed(BoolCallback callback, Networking::JsonResponse response);
 	void codeFlowComplete(BoolResponse response);
+	void codeFlowFailed(Networking::ErrorResponse error);
 
 	/** Constructs StorageInfo based on JSON response from cloud. */
 	void infoInnerCallback(StorageInfoCallback outerCallback, Networking::JsonResponse json);
@@ -101,7 +102,7 @@ public:
 	 * Use "" in order to refresh token and pass a callback, so you could
 	 * continue your work when new token is available.
 	 */
-	void getAccessToken(BoolCallback callback, Common::String code = "");
+	void getAccessToken(BoolCallback callback, Networking::ErrorCallback errorCallback = nullptr, Common::String code = "");
 
 	Common::String accessToken() { return _token; }
 };