mirror of
https://github.com/electron/electron.git
synced 2026-04-10 03:01:51 -04:00
fix: resolve getFileHandle concurrent stalling by queuing callbacks (#50671)
fix: resolve getFileHandle concurrent stalling by queuing callbacks (#50597) Previously, concurrent calls to FileSystemAccessPermissionContext::ConfirmSensitiveEntryAccess for the same file path would silently discard the subsequent callbacks because the internal callback map used a single callback per file path and std::map::try_emplace would drop the callback if the key already existed. This caused Promises in JS (e.g., dirHandle.getFileHandle()) to stall indefinitely. This commit updates the callback map to hold a vector of callbacks, so all concurrent requesters for the same filepath are grouped together and resolved once the asynchronous blocklist check completes. Notes: Fixed an issue where concurrent `getFileHandle` requests on the same path could stall indefinitely. Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Kunal Dubey <21157775+xakep8@users.noreply.github.com>
This commit is contained in:
@@ -697,7 +697,11 @@ void FileSystemAccessPermissionContext::ConfirmSensitiveEntryAccess(
|
||||
content::GlobalRenderFrameHostId frame_id,
|
||||
base::OnceCallback<void(SensitiveEntryResult)> callback) {
|
||||
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
|
||||
callback_map_.try_emplace(path_info.path, std::move(callback));
|
||||
|
||||
auto [it, inserted] = callback_map_.try_emplace(path_info.path);
|
||||
it->second.push_back(std::move(callback));
|
||||
if (!inserted)
|
||||
return;
|
||||
|
||||
auto after_blocklist_check_callback = base::BindOnce(
|
||||
&FileSystemAccessPermissionContext::DidCheckPathAgainstBlocklist,
|
||||
@@ -769,8 +773,11 @@ void FileSystemAccessPermissionContext::PerformAfterWriteChecks(
|
||||
void FileSystemAccessPermissionContext::RunRestrictedPathCallback(
|
||||
const base::FilePath& file_path,
|
||||
SensitiveEntryResult result) {
|
||||
if (auto val = callback_map_.extract(file_path))
|
||||
std::move(val.mapped()).Run(result);
|
||||
if (auto val = callback_map_.extract(file_path)) {
|
||||
for (auto& callback : val.mapped()) {
|
||||
std::move(callback).Run(result);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
void FileSystemAccessPermissionContext::OnRestrictedPathResult(
|
||||
|
||||
@@ -196,7 +196,8 @@ class FileSystemAccessPermissionContext
|
||||
|
||||
std::map<url::Origin, base::DictValue> id_pathinfo_map_;
|
||||
|
||||
std::map<base::FilePath, base::OnceCallback<void(SensitiveEntryResult)>>
|
||||
std::map<base::FilePath,
|
||||
std::vector<base::OnceCallback<void(SensitiveEntryResult)>>>
|
||||
callback_map_;
|
||||
|
||||
std::unique_ptr<ChromeFileSystemAccessPermissionContext::BlockPathRules>
|
||||
|
||||
@@ -1063,6 +1063,64 @@ describe('chromium features', () => {
|
||||
expect(status).to.equal('granted');
|
||||
});
|
||||
|
||||
it('concurrent getFileHandle calls on the same file do not stall', (done) => {
|
||||
const writablePath = path.join(fixturesPath, 'file-system', 'test-perms.html');
|
||||
const testDir = path.join(fixturesPath, 'file-system');
|
||||
const testFile = path.join(testDir, 'test.txt');
|
||||
|
||||
const w = new BrowserWindow({
|
||||
show: false,
|
||||
webPreferences: {
|
||||
nodeIntegration: true,
|
||||
contextIsolation: false,
|
||||
sandbox: false
|
||||
}
|
||||
});
|
||||
|
||||
w.webContents.session.setPermissionRequestHandler((wc, permission, callback, details) => {
|
||||
if (permission === 'fileSystem') {
|
||||
const { href } = url.pathToFileURL(writablePath);
|
||||
expect(details).to.deep.equal({
|
||||
fileAccessType: 'readable',
|
||||
isDirectory: false,
|
||||
isMainFrame: true,
|
||||
filePath: testFile,
|
||||
requestingUrl: href
|
||||
});
|
||||
callback(true);
|
||||
} else {
|
||||
callback(false);
|
||||
}
|
||||
});
|
||||
|
||||
ipcMain.once('did-create-directory-handle', async () => {
|
||||
const result = await w.webContents.executeJavaScript(`
|
||||
new Promise(async (resolve, reject) => {
|
||||
try {
|
||||
const handles = await Promise.all([
|
||||
handle.getFileHandle('test.txt'),
|
||||
handle.getFileHandle('test.txt')
|
||||
]);
|
||||
resolve(handles.length === 2);
|
||||
} catch (err) {
|
||||
reject(err.message);
|
||||
}
|
||||
})
|
||||
`, true);
|
||||
expect(result).to.be.true();
|
||||
done();
|
||||
});
|
||||
|
||||
w.loadFile(writablePath);
|
||||
|
||||
w.webContents.once('did-finish-load', () => {
|
||||
// @ts-expect-error Undocumented testing method.
|
||||
clipboard._writeFilesForTesting([testDir]);
|
||||
w.webContents.focus();
|
||||
w.webContents.paste();
|
||||
});
|
||||
});
|
||||
|
||||
it('allows permission when trying to create a writable file handle', (done) => {
|
||||
const writablePath = path.join(fixturesPath, 'file-system', 'test-perms.html');
|
||||
const testFile = path.join(fixturesPath, 'file-system', 'test.txt');
|
||||
|
||||
Reference in New Issue
Block a user