Skip to content

Commit 092c8d7

Browse files
claucambraclaucambra
authored
Merge pull request #8080 from nextcloud/bugfix/objcpp-mem
gui/macOS: Fix memory issues in Objective-C++ code for FileProvider support
2 parents ab88bc3 + 499ca78 commit 092c8d7

11 files changed

Lines changed: 65 additions & 50 deletions

‎src/gui/macOS/fileproviderdomainmanager_mac.mm‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,6 @@ void addFileProviderDomain(const AccountState * const accountState)
275275

276276
NSFileProviderDomain * const fileProviderDomain = [[NSFileProviderDomain alloc] initWithIdentifier:domainId.toNSString()
277277
displayName:domainDisplayName.toNSString()];
278-
[fileProviderDomain retain];
279278

280279
[NSFileProviderManager addDomain:fileProviderDomain completionHandler:^(NSError * const error) {
281280
if(error) {

‎src/gui/macOS/fileproviderdomainsyncstatus_mac.mm‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ explicit MacImplementation(const QString &domainIdentifier, FileProviderDomainSy
4141
qCWarning(lcMacFileProviderDomainSyncStatus) << "Could not get manager for domain" << domainIdentifier;
4242
return;
4343
}
44+
[_manager retain];
4445

4546
if (@available(macOS 11.3, *)) {
4647
NSProgress *const downloadProgress = [_manager globalProgressForKind:NSProgressFileOperationKindDownloading];
@@ -61,6 +62,8 @@ explicit MacImplementation(const QString &domainIdentifier, FileProviderDomainSy
6162
{
6263
[_downloadProgressObserver release];
6364
[_uploadProgressObserver release];
65+
[_domain release];
66+
[_manager release];
6467
}
6568

6669
void updateDownload(NSProgress *const progress) const

‎src/gui/macOS/fileprovidereditlocallyjob_mac.mm‎

Lines changed: 26 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,7 @@
4444

4545
NSFileProviderDomain *const domain = (NSFileProviderDomain *)voidDomain;
4646
if (domain == nil) {
47-
qCWarning(lcFileProviderEditLocallyMacJob) << "Could not get domain for account:"
48-
<< userId;
47+
qCWarning(lcFileProviderEditLocallyMacJob) << "Could not get domain for account:" << userId;
4948
emit notAvailable();
5049
}
5150

@@ -56,37 +55,35 @@
5655
emit notAvailable();
5756
}
5857

59-
dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
60-
__block NSError *receivedError;
61-
__block NSURL *itemLocalUrl;
58+
[manager retain];
6259
[manager getUserVisibleURLForItemIdentifier:nsOcId
6360
completionHandler:^(NSURL *const url, NSError *const error) {
64-
[url retain];
65-
[error retain];
66-
itemLocalUrl = url;
67-
receivedError = error;
68-
dispatch_semaphore_signal(semaphore);
69-
}];
70-
dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);
7161

72-
Systray::instance()->destroyEditFileLocallyLoadingDialog();
62+
dispatch_async(dispatch_get_main_queue(), ^{
63+
Systray::instance()->destroyEditFileLocallyLoadingDialog();
64+
});
7365

74-
if (receivedError != nil) {
75-
const auto errorMessage = QString::fromNSString(receivedError.localizedDescription);
76-
qCWarning(lcFileProviderEditLocallyMacJob) << "Error getting user visible URL for item"
77-
<< ocId << ":" << errorMessage;
78-
emit notAvailable();
79-
} else if (itemLocalUrl != nil) {
80-
const auto itemLocalPath = QString::fromNSString(itemLocalUrl.path);
81-
qCDebug(lcFileProviderEditLocallyMacJob) << "Got user visible URL for item"
82-
<< ocId << ":" << itemLocalPath;
83-
[NSWorkspace.sharedWorkspace openURL:itemLocalUrl];
84-
emit finished();
85-
} else {
86-
qCWarning(lcFileProviderEditLocallyMacJob) << "Got nil user visible URL for item"
87-
<< ocId;
88-
emit notAvailable();
89-
}
66+
if (error != nil) {
67+
const auto errorMessage = QString::fromNSString(error.localizedDescription);
68+
qCWarning(lcFileProviderEditLocallyMacJob) << "Error getting user visible URL for item:" << errorMessage;
69+
dispatch_async(dispatch_get_main_queue(), ^{
70+
emit notAvailable();
71+
});
72+
} else if (url != nil) {
73+
const auto itemLocalPath = QString::fromNSString(url.path);
74+
qCDebug(lcFileProviderEditLocallyMacJob) << "Got user visible URL for item:" << itemLocalPath;
75+
[NSWorkspace.sharedWorkspace openURL:url];
76+
dispatch_async(dispatch_get_main_queue(), ^{
77+
emit finished();
78+
});
79+
} else {
80+
qCWarning(lcFileProviderEditLocallyMacJob) << "Got nil user visible URL for item" << ocId;
81+
dispatch_async(dispatch_get_main_queue(), ^{
82+
emit notAvailable();
83+
});
84+
}
85+
[manager release];
86+
}];
9087
}
9188

9289
} // namespace OCC::Mac

‎src/gui/macOS/fileprovideritemmetadata_mac.mm‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,9 +120,9 @@ QString nsNameComponentsToLocalisedQString(NSPersonNameComponents *const nameCom
120120
}
121121

122122
__block QString returnPath = QObject::tr("Unknown");
123-
NSFileProviderManager *manager = FileProviderUtils::managerForDomainIdentifier(domainId);
123+
NSFileProviderManager *const manager = FileProviderUtils::managerForDomainIdentifier(domainId);
124124

125-
if (manager == nil) {
125+
if (manager == nil) {
126126
qCWarning(lcMacImplFileProviderItemMetadata) << "Null manager, cannot get item path";
127127
return returnPath;
128128
}
@@ -132,6 +132,7 @@ QString nsNameComponentsToLocalisedQString(NSPersonNameComponents *const nameCom
132132

133133
// getUserVisibleUrl is async, so wait here
134134

135+
[manager retain];
135136
[manager getUserVisibleURLForItemIdentifier:nsItemIdentifier
136137
completionHandler:^(NSURL *const userVisibleFile, NSError *const error) {
137138

@@ -141,6 +142,7 @@ QString nsNameComponentsToLocalisedQString(NSPersonNameComponents *const nameCom
141142
returnPath = QString::fromNSString(userVisibleFile.path);
142143
}
143144

145+
[manager release];
144146
dispatch_semaphore_signal(semaphore);
145147
}];
146148

‎src/gui/macOS/fileprovidermaterialiseditemsmodel_mac.mm‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030

3131
void FileProviderMaterialisedItemsModel::evictItem(const QString &identifier, const QString &domainIdentifier)
3232
{
33-
NSFileProviderManager * const manager = FileProviderUtils::managerForDomainIdentifier(domainIdentifier);
33+
NSFileProviderManager *const manager = FileProviderUtils::managerForDomainIdentifier(domainIdentifier);
3434
if (manager == nil) {
3535
qCWarning(lcMacImplFileProviderMaterialisedItemsModelMac) << "Received null manager for domain"
3636
<< domainIdentifier
@@ -42,7 +42,8 @@
4242
return;
4343
}
4444

45-
__block BOOL successfullyDeleted = YES;
45+
__block BOOL successfullyDeleted = NO;
46+
dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
4647

4748
[manager evictItemWithIdentifier:identifier.toNSString() completionHandler:^(NSError *error) {
4849
if (error != nil) {
@@ -51,10 +52,15 @@
5152
Systray::instance()->showMessage(tr("Error"),
5253
tr("An error occurred while trying to delete the local copy of this item: %1").arg(errorDesc),
5354
QSystemTrayIcon::Warning);
54-
successfullyDeleted = NO;
55+
} else {
56+
successfullyDeleted = YES;
5557
}
58+
dispatch_semaphore_signal(semaphore);
5659
}];
5760

61+
dispatch_semaphore_wait(semaphore, dispatch_time(DISPATCH_TIME_NOW, 3 * NSEC_PER_SEC));
62+
[manager release];
63+
5864
if (successfullyDeleted == NO) {
5965
return;
6066
}

‎src/gui/macOS/fileprovidersettingscontroller_mac.mm‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,7 @@ void signalFileProviderDomain(const QString &userIdAtHost) const
167167
qCInfo(lcFileProviderSettingsController) << "Signalling file provider domain" << userIdAtHost;
168168
NSFileProviderDomain * const domain = FileProviderUtils::domainForIdentifier(userIdAtHost);
169169
NSFileProviderManager * const manager = [NSFileProviderManager managerForDomain:domain];
170+
[domain release];
170171
[manager signalEnumeratorForContainerItemIdentifier:NSFileProviderRootContainerItemIdentifier
171172
completionHandler:^(NSError *const error) {
172173
if (error != nil) {
@@ -186,6 +187,7 @@ void signalFileProviderDomain(const QString &userIdAtHost) const
186187
}
187188

188189
public slots:
190+
// NOTE: This method will release the provided args so make sure to retain them beforehand
189191
void enumerateMaterialisedFilesForDomainManager(NSFileProviderManager * const managerForDomain,
190192
NSFileProviderDomain * const domain)
191193
{
@@ -194,7 +196,6 @@ void enumerateMaterialisedFilesForDomainManager(NSFileProviderManager * const ma
194196
[enumerator retain];
195197

196198
FileProviderStorageUseEnumerationObserver *const storageUseObserver = [[FileProviderStorageUseEnumerationObserver alloc] init];
197-
[storageUseObserver retain];
198199
storageUseObserver.enumerationFinishedHandler = ^(NSError *const error) {
199200
qCInfo(lcFileProviderSettingsController) << "Enumeration finished for" << domain.identifier;
200201
if (error != nil) {
@@ -229,6 +230,9 @@ void enumerateMaterialisedFilesForDomainManager(NSFileProviderManager * const ma
229230

230231
[storageUseObserver release];
231232
[enumerator release];
233+
234+
[managerForDomain release];
235+
[domain release];
232236
};
233237
[enumerator enumerateItemsForObserver:storageUseObserver startingAtPage:NSFileProviderInitialPageSortedByName];
234238
}
@@ -284,7 +288,8 @@ void fetchMaterialisedFilesStorageUsage()
284288
<< ", returning early.";
285289
return;
286290
}
287-
291+
[managerForDomain retain];
292+
[domain retain];
288293
enumerateMaterialisedFilesForDomainManager(managerForDomain, domain);
289294
}
290295
}];

‎src/gui/macOS/fileproviderutils.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,10 @@ class QString;
2929
*
3030
* You should threfore try to avoid using this in C++ code wherever possible
3131
* and only use this in *_mac.mm implementation files.
32+
*
33+
* IMPORTANT: All Objective-C objects returned here need to be released!
34+
* They have been internally retained due to the async nature of the
35+
* FileProvider API.
3236
*/
3337

3438
namespace OCC {

‎src/gui/macOS/fileproviderxpc_mac.mm‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,8 @@
148148
} else {
149149
qCWarning(lcFileProviderXPC) << "Could not open debug log file" << filename;
150150
}
151+
152+
[rcvdDebugLogString release];
151153
}
152154

153155
bool FileProviderXPC::fileProviderExtReachable(const QString &extensionAccountId, const bool retry, const bool reconfigureOnFail)

‎src/gui/macOS/fileproviderxpc_mac_utils.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,6 @@ NSArray<NSXPCConnection *> *connectToFileProviderServices(NSArray<NSDictionary<N
2929
void configureFileProviderConnection(NSXPCConnection *connection);
3030
NSObject *getRemoteServiceObject(NSXPCConnection *connection, Protocol *protocol);
3131
NSString *getExtensionAccountId(NSObject<ClientCommunicationProtocol> *clientCommService);
32-
QHash<QString, void*> processClientCommunicationConnections(NSArray *connections);
32+
QHash<QString, void*> processClientCommunicationConnections(NSArray<NSXPCConnection *> *connections);
3333

3434
}

‎src/gui/macOS/fileproviderxpc_mac_utils.mm‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
NSArray<NSFileProviderManager *> *getDomainManagers()
3131
{
3232
dispatch_group_t group = dispatch_group_create();
33-
__block NSMutableArray<NSFileProviderManager *> *managers = NSMutableArray.array;
33+
__block NSMutableArray<NSFileProviderManager *> *const managers = NSMutableArray.array;
3434

3535
dispatch_group_enter(group);
3636

@@ -45,8 +45,11 @@
4545
for (NSFileProviderDomain *const domain in domains) {
4646
qCInfo(lcFileProviderXPCUtils) << "Got domain" << domain.identifier;
4747
NSFileProviderManager *const manager = [NSFileProviderManager managerForDomain:domain];
48-
[manager retain];
49-
[managers addObject:manager];
48+
if (manager) {
49+
[managers addObject:manager];
50+
} else {
51+
qCWarning(lcFileProviderXPCUtils) << "Could not get manager for domain" << domain.identifier;
52+
}
5053
}
5154

5255
dispatch_group_leave(group);
@@ -78,7 +81,6 @@
7881
} else if (service == nil) {
7982
qCWarning(lcFileProviderXPCUtils) << "Service is nil";
8083
} else {
81-
[service retain];
8284
[fpServices addObject:@{service.name: service}];
8385
}
8486
dispatch_group_leave(group);
@@ -95,7 +97,7 @@
9597
NSArray<NSURL *> *getDomainUrlsForManagers(NSArray<NSFileProviderManager *> *managers)
9698
{
9799
dispatch_group_t group = dispatch_group_create();
98-
__block NSMutableArray<NSURL *> *urls = NSMutableArray.array;
100+
__block NSMutableArray<NSURL *> *const urls = NSMutableArray.array;
99101

100102
for (NSFileProviderManager *const manager in managers) {
101103

@@ -191,7 +193,6 @@
191193
return;
192194
}
193195

194-
[connection retain];
195196
[connections addObject:connection];
196197
dispatch_group_leave(group);
197198
}];
@@ -243,15 +244,14 @@ void configureFileProviderConnection(NSXPCConnection *const connection)
243244
dispatch_group_leave(group);
244245
return;
245246
}
246-
extensionNcAccount = [NSString stringWithString:extensionAccountId];
247-
[extensionNcAccount retain];
247+
extensionNcAccount = [[NSString alloc] initWithString:extensionAccountId];
248248
dispatch_group_leave(group);
249249
}];
250250
dispatch_group_wait(group, DISPATCH_TIME_FOREVER);
251251
return extensionNcAccount;
252252
}
253253

254-
QHash<QString, void*> processClientCommunicationConnections(NSArray *const connections)
254+
QHash<QString, void*> processClientCommunicationConnections(NSArray<NSXPCConnection *> *const connections)
255255
{
256256
QHash<QString, void*> clientCommServices;
257257

0 commit comments

Comments
 (0)