Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make shell.moveItemToTrash return false on Windows when move is unsuccessful #25171

Merged
merged 1 commit into from Aug 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion shell/common/platform_util_win.cc
Expand Up @@ -387,10 +387,14 @@ bool MoveItemToTrash(const base::FilePath& path, bool delete_on_fail) {
if (!delete_sink)
return false;

BOOL pfAnyOperationsAborted;

// Processes the queued command DeleteItem. This will trigger
// the DeleteFileProgressSink to check for Recycle Bin.
return SUCCEEDED(pfo->DeleteItem(delete_item.Get(), delete_sink.Get())) &&
SUCCEEDED(pfo->PerformOperations());
SUCCEEDED(pfo->PerformOperations()) &&
SUCCEEDED(pfo->GetAnyOperationsAborted(&pfAnyOperationsAborted)) &&
!pfAnyOperationsAborted;
}

bool GetFolderPath(int key, base::FilePath* result) {
Expand Down
35 changes: 35 additions & 0 deletions spec-main/api-shell-spec.ts
Expand Up @@ -7,6 +7,8 @@ import * as fs from 'fs-extra';
import * as path from 'path';
import { AddressInfo } from 'net';
import { expect } from 'chai';
import { ifit } from './spec-helpers';
import { execSync } from 'child_process';

describe('shell module', () => {
describe('shell.openExternal()', () => {
Expand Down Expand Up @@ -76,5 +78,38 @@ describe('shell module', () => {
const result = shell.moveItemToTrash(filename);
expect(result).to.be.false();
});

ifit(process.platform === 'darwin')('returns false when file has immutable flag', async () => {
const dir = await fs.mkdtemp(path.resolve(app.getPath('temp'), 'electron-shell-spec-'));
const tempPath = path.join(dir, 'locked-file');
await fs.writeFile(tempPath, 'delete me if you can');

// https://ss64.com/osx/chflags.html
execSync(`chflags uchg ${tempPath}`);
expect(shell.moveItemToTrash(tempPath)).to.be.false();
expect(await fs.pathExists(tempPath)).to.be.true();

execSync(`chflags nouchg ${tempPath}`);
expect(shell.moveItemToTrash(tempPath)).to.be.true();
expect(await fs.pathExists(tempPath)).to.be.false();
});

ifit(process.platform === 'win32')('returns false when path is in use', async () => {
const tempPath = await fs.mkdtemp(path.resolve(app.getPath('temp'), 'electron-shell-spec-'));
const cwd = process.cwd();
try {
// A process working directory is automatically locked on Windows.
// This is a workaround to avoid pulling in fs-extras flock method.
process.chdir(tempPath);

expect(shell.moveItemToTrash(tempPath)).to.be.false();
expect(await fs.pathExists(tempPath)).to.be.true();
} finally {
process.chdir(cwd);
}

expect(shell.moveItemToTrash(tempPath)).to.be.true();
expect(await fs.pathExists(tempPath)).to.be.false();
});
});
});