Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Put an upper limit on UTF8 file sizes #9495

Closed
wants to merge 10 commits into from
Closed

Put an upper limit on UTF8 file sizes #9495

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
13d0934
add a file size governor
JeffryBooher Oct 9, 2014
f427d01
Merge branch 'master' into jeff/LimitFileSize
JeffryBooher Oct 14, 2014
2ddd7ce
rtc:remove promises, increase file limit, display more info in warning
JeffryBooher Oct 14, 2014
37f3326
code cleanup
JeffryBooher Oct 14, 2014
3785d40
more cleanup
JeffryBooher Oct 14, 2014
410f78f
remove unused modules
JeffryBooher Oct 14, 2014
2d918c2
rtc:@redmunds
JeffryBooher Oct 16, 2014
1fe5256
32=>16mb
JeffryBooher Oct 16, 2014
ae977c3
refractor machine and human data
JeffryBooher Oct 16, 2014
a33b9e6
Merge branch 'master' into jeff/LimitFileSize
JeffryBooher Oct 17, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/file/FileUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,15 @@ define(function (require, exports, module) {
StringUtils = require("utils/StringUtils");


/**
* @const {Number} Maximium file size in megabytes (32MB)
* This must be a hard-coded value since this value
* tells how low-level APIs should behave which cannot
* have a load order dependency on preferences manager
*/
var MAX_FILE_SIZE_MB = 32;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterflynn Are you ok with a limit of 32MB? FWIW, I could go down to 16MB.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@redmunds I guess I'd err toward the lower number given that we know 68MB causes a crash and afaik we haven't tested any sizes between 5 and 68, so we don't know where the threshold falls...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change this to 16MB if that seems agreeable to everyone.



/**
* Asynchronously reads a file as UTF-8 encoded text.
* @param {!File} file File to read
Expand Down Expand Up @@ -168,6 +177,9 @@ define(function (require, exports, module) {
result = Strings.CONTENTS_MODIFIED_ERR;
} else if (name === FileSystemError.UNSUPPORTED_ENCODING) {
result = Strings.UNSUPPORTED_ENCODING_ERR;
} else if (name === FileSystemError.EXCEEDS_MAX_FILE_SIZE) {
//result = Strings.EXCEEDS_MAX_FILE_SIZE;
result = StringUtils.format(Strings.EXCEEDS_MAX_FILE_SIZE, MAX_FILE_SIZE_MB);
} else {
result = StringUtils.format(Strings.GENERIC_ERROR, name);
}
Expand Down Expand Up @@ -541,4 +553,5 @@ define(function (require, exports, module) {
exports.getSmartFileExtension = getSmartFileExtension;
exports.compareFilenames = compareFilenames;
exports.comparePaths = comparePaths;
exports.MAX_FILE_SIZE_MB = MAX_FILE_SIZE_MB;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align = with the others.

});
3 changes: 2 additions & 1 deletion src/filesystem/FileSystemError.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ define(function (require, exports, module) {
TOO_MANY_ENTRIES : "TooManyEntries",
ALREADY_EXISTS : "AlreadyExists",
CONTENTS_MODIFIED : "ContentsModified",
ROOT_NOT_WATCHED : "RootNotBeingWatched"
ROOT_NOT_WATCHED : "RootNotBeingWatched",
EXCEEDS_MAX_FILE_SIZE : "ExceedsMaxFileSize"

// FUTURE: Add remote connection errors: timeout, not logged in, connection err, etc.
};
Expand Down
89 changes: 43 additions & 46 deletions src/filesystem/impls/appshell/AppshellFileSystem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,14 @@
define(function (require, exports, module) {
"use strict";

var FileUtils = require("file/FileUtils"),
FileSystemStats = require("filesystem/FileSystemStats"),
FileSystemError = require("filesystem/FileSystemError"),
NodeDomain = require("utils/NodeDomain");
var FileUtils = require("file/FileUtils"),
FileSystemStats = require("filesystem/FileSystemStats"),
FileSystemError = require("filesystem/FileSystemError"),
NodeDomain = require("utils/NodeDomain");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a space char to these 4 lines doesn't seem necessary if you want to minimize the diff.


/**
* @const
*/
var FILE_WATCHER_BATCH_TIMEOUT = 200; // 200ms - granularity of file watcher changes

/**
Expand Down Expand Up @@ -361,42 +364,35 @@ define(function (require, exports, module) {
*/
function readFile(path, options, callback) {
var encoding = options.encoding || "utf8";

// Execute the read and stat calls in parallel. Callback early if the
// read call completes first with an error; otherwise wait for both
// to finish.
var done = false, data, stat, err;

// callback to be executed when the call to stat completes
// or immediately if a stat object was passed as an argument
function doReadFile(stat) {
if (stat.size > (FileUtils.MAX_FILE_SIZE_MB * 1024 * 1024)) {
callback(FileSystemError.EXCEEDS_MAX_FILE_SIZE);
} else {
appshell.fs.readFile(path, encoding, function (_err, _data) {
if (_err) {
callback(_mapError(_err));
} else {
callback(null, _data, stat);
}
});
}
}

if (options.stat) {
done = true;
stat = options.stat;
doReadFile(options.stat);
} else {
exports.stat(path, function (_err, _stat) {
if (done) {
callback(_err, _err ? null : data, _stat);
if (_err) {
callback(_mapError(_err));
} else {
done = true;
stat = _stat;
err = _err;
doReadFile(_stat);
}
});
}

appshell.fs.readFile(path, encoding, function (_err, _data) {
if (_err) {
callback(_mapError(_err));
return;
}

if (done) {
callback(err, err ? null : _data, stat);
} else {
done = true;
data = _data;
}
});
}

/**
* Write data to the file at the given path, calling back asynchronously with
* either a FileSystemError string or the FileSystemStats object associated
Expand Down Expand Up @@ -568,23 +564,24 @@ define(function (require, exports, module) {
_nodeDomain.exec("unwatchAll")
.then(callback, callback);
}


// Export public API
exports.showOpenDialog = showOpenDialog;
exports.showSaveDialog = showSaveDialog;
exports.exists = exists;
exports.readdir = readdir;
exports.mkdir = mkdir;
exports.rename = rename;
exports.stat = stat;
exports.readFile = readFile;
exports.writeFile = writeFile;
exports.unlink = unlink;
exports.moveToTrash = moveToTrash;
exports.initWatchers = initWatchers;
exports.watchPath = watchPath;
exports.unwatchPath = unwatchPath;
exports.unwatchAll = unwatchAll;
exports.showOpenDialog = showOpenDialog;
exports.showSaveDialog = showSaveDialog;
exports.exists = exists;
exports.readdir = readdir;
exports.mkdir = mkdir;
exports.rename = rename;
exports.stat = stat;
exports.readFile = readFile;
exports.writeFile = writeFile;
exports.unlink = unlink;
exports.moveToTrash = moveToTrash;
exports.initWatchers = initWatchers;
exports.watchPath = watchPath;
exports.unwatchPath = unwatchPath;
exports.unwatchAll = unwatchAll;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing a space char to these lines doesn't seem necessary if you want to minimize the diff.


/**
* Indicates whether or not recursive watching notifications are supported
Expand Down
1 change: 1 addition & 0 deletions src/nls/root/strings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ define({
"GENERIC_ERROR" : "(error {0})",
"NOT_FOUND_ERR" : "The file could not be found.",
"NOT_READABLE_ERR" : "The file could not be read.",
"EXCEEDS_MAX_FILE_SIZE" : "Files larger than {0} MB cannot be opened in {APP_NAME}.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"MB" should be part of the {0} size parameter to make it more flexible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't "MB" need to get translated? And isn't it, in certain languages, customary to place the size before the number?

"NO_MODIFICATION_ALLOWED_ERR" : "The target directory cannot be modified.",
"NO_MODIFICATION_ALLOWED_ERR_FILE" : "The permissions do not allow you to make modifications.",
"CONTENTS_MODIFIED_ERR" : "The file has been modified outside of {APP_NAME}.",
Expand Down