Skip to content

Commit

Permalink
NaCl: Fix NaClBrowserTestPnaclNonSfi.IrtManifestFile
Browse files Browse the repository at this point in the history 
We have switched from fgets to read, but the change did not
modify code which converts CRLF to LF.

https://codereview.chromium.org/133033002

Due to this reason, we were running strlen for the buffer
filled by read, which is not always NULL terminated.

This change simply removes the CRLF-to-LF conversion. This is
safe because we do not have any newlines in the test file.

BUG=392768
TEST=./out/Debug/browser_tests --gtest_filter='*NonSfi*IrtMani*'
TEST=trybots

Review URL: https://codereview.chromium.org/383063002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@282838 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
hamaji@chromium.org committed Jul 12, 2014
1 parent fa8f32b commit 670621d
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 93 deletions.
30 changes: 1 addition & 29 deletions chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,36 +49,8 @@ std::string LoadManifestSuccess(TYPE_nacl_irt_query *query_func) {
char buffer[4096];
int len;
while ((len = read(desc, buffer, sizeof buffer - 1)) > 0) {
// NB: fgets does not discard the newline nor any carriage return
// character before that.
//
// Note that CR LF is the default end-of-line style for Windows.
// Furthermore, when the test_file (input data, which happens to
// be the nmf file) is initially created in a change list, the
// patch is sent to our try bots as text. This means that when
// the file arrives, it has CR LF endings instead of the original
// LF line endings. Since the expected or golden data is
// (manually) encoded in the HTML file's JavaScript, there will be
// a mismatch. After submission, the svn property svn:eol-style
// will be set to LF, so a clean check out should have LF and not
// CR LF endings, and the tests will pass without CR removal.
// However -- and there's always a however in long discourses --
// if the nmf file is edited, say, because the test is being
// modified, and the modification is being done on a Windows
// machine, then it is likely that the editor used by the
// programmer will convert the file to CR LF endings. Which,
// unfortunatly, implies that the test will mysteriously fail
// again.
//
// To defend against such nonsense, we weaken the test slighty,
// and just strip the CR if it is present.
int len = strlen(buffer);
if (len >= 2 && buffer[len-1] == '\n' && buffer[len-2] == '\r') {
buffer[len-2] = '\n';
buffer[len-1] = '\0';
}
// Null terminate.
buffer[len] = 0;
buffer[len] = '\0';
str += buffer;
}

Expand Down
29 changes: 1 addition & 28 deletions chrome/test/data/nacl/manifest_file/pm_manifest_file_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -428,35 +428,8 @@ void Worker::ManifestOpenTest(nacl::StringBuffer *sb) {
char buffer[4096];
int len;
while ((len = read(desc, buffer, sizeof buffer - 1)) > 0) {
// NB: fgets does not discard the newline nor any carriage return
// character before that.
//
// Note that CR LF is the default end-of-line style for Windows.
// Furthermore, when the test_file (input data, which happens to
// be the nmf file) is initially created in a change list, the
// patch is sent to our try bots as text. This means that when
// the file arrives, it has CR LF endings instead of the original
// LF line endings. Since the expected or golden data is
// (manually) encoded in the HTML file's JavaScript, there will be
// a mismatch. After submission, the svn property svn:eol-style
// will be set to LF, so a clean check out should have LF and not
// CR LF endings, and the tests will pass without CR removal.
// However -- and there's always a however in long discourses --
// if the nmf file is edited, say, because the test is being
// modified, and the modification is being done on a Windows
// machine, then it is likely that the editor used by the
// programmer will convert the file to CR LF endings. Which,
// unfortunatly, implies that the test will mysteriously fail
// again.
//
// To defend against such nonsense, we weaken the test slighty,
// and just strip the CR if it is present.
if (len >= 2 && buffer[len-1] == '\n' && buffer[len-2] == '\r') {
buffer[len-2] = '\n';
buffer[len-1] = '\0';
}
// Null terminate.
buffer[len] = 0;
buffer[len] = '\0';
sb->Printf("%s", buffer);
}
NaClSrpcDtor(&manifest_channel);
Expand Down
30 changes: 1 addition & 29 deletions chrome/test/data/nacl/manifest_file/pm_pre_init_manifest_file_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,36 +115,8 @@ void TestManifestContents() {
char buffer[4096];
int len;
while ((len = read(desc, buffer, sizeof buffer - 1)) > 0) {
// NB: fgets does not discard the newline nor any carriage return
// character before that.
//
// Note that CR LF is the default end-of-line style for Windows.
// Furthermore, when the test_file (input data, which happens to
// be the nmf file) is initially created in a change list, the
// patch is sent to our try bots as text. This means that when
// the file arrives, it has CR LF endings instead of the original
// LF line endings. Since the expected or golden data is
// (manually) encoded in the HTML file's JavaScript, there will be
// a mismatch. After submission, the svn property svn:eol-style
// will be set to LF, so a clean check out should have LF and not
// CR LF endings, and the tests will pass without CR removal.
// However -- and there's always a however in long discourses --
// if the nmf file is edited, say, because the test is being
// modified, and the modification is being done on a Windows
// machine, then it is likely that the editor used by the
// programmer will convert the file to CR LF endings. Which,
// unfortunatly, implies that the test will mysteriously fail
// again.
//
// To defend against such nonsense, we weaken the test slighty,
// and just strip the CR if it is present.
int len = strlen(buffer);
if (len >= 2 && buffer[len-1] == '\n' && buffer[len-2] == '\r') {
buffer[len-2] = '\n';
buffer[len-1] = '\0';
}
// Null terminate.
buffer[len] = 0;
buffer[len] = '\0';
sb.Printf("%s", buffer);
}

Expand Down
8 changes: 1 addition & 7 deletions chrome/test/nacl/nacl_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,14 +144,8 @@ IN_PROC_BROWSER_TEST_F(NaClBrowserTestGLibc,
IN_PROC_BROWSER_TEST_F(NaClBrowserTestNewlib, IrtManifestFile) {
RunNaClIntegrationTest(FILE_PATH_LITERAL("irt_manifest_file_test.html"));
}
// crbug.com/392768
#if defined(OS_LINUX)
# define MAYBE_IrtManifestFile DISABLED_IrtManifestFile
#else
# define MAYBE_IrtManifestFile MAYBE_PNACL_NONSFI(IrtManifestFile)
#endif
IN_PROC_BROWSER_TEST_F(NaClBrowserTestPnaclNonSfi,
MAYBE_IrtManifestFile) {
MAYBE_PNACL_NONSFI(IrtManifestFile)) {
RunNaClIntegrationTest(FILE_PATH_LITERAL("irt_manifest_file_test.html"));
}

Expand Down

0 comments on commit 670621d

Please sign in to comment.