From 7ccf4fab5d4473e431f1a289056033eea0b3dd43 Mon Sep 17 00:00:00 2001 From: noel Date: Tue, 3 Feb 2015 16:48:13 -0800 Subject: [PATCH] Revert of [net] Subtract timestamps to determine if uploading file changed. (patchset #2 id:20001 of https://codereview.chromium.org/868253012/) Reason for revert: Seems this made http/tests/local/fileapi/send-sliced-dragged-file.html fail the blink bots. http://test-results.appspot.com/dashboards/flakiness_dashboard.html#master=ChromiumWebkit&tests=http/tests/local/fileapi/send-sliced-dragged-file.html Original issue's description: > [net] Subtract timestamps to determine if uploading file changed. > > UploadFileElementReader relies on checking the modified time of files > being uploaded to determine if a sliced file was modified during upload. > Clients of the net stack (in particular Blink) currently pass around the > expected modified time in a manner which cause the timestamp to lose > precision (E.g. converting to and from a double time_t). > > As a result the expected timestamp and the current timestamp as returned > by GetFileInfo() will not match exactly. Current code attempted to > compensate for this by converting both timestamps to (integer) time_t. > However, since the conversion rounds down, this check would only succeed > if the loss of precision of the expected timestamp also caused it to > round down. This is not always the case. (E.g. (epoch + 10.999999) will > become 10 when converted to time_t, but the expected timestamp may have > rounded up to (epoch + 11.0) in the meantime.) > > This patch compares the timestamps by checking if the magnitude of the > difference is less than one second. > > BUG=426465 > R=mmenke > > Committed: https://crrev.com/b77c8ffae588001875fb50ead987a147ca882bdb > Cr-Commit-Position: refs/heads/master@{#314397} TBR=mmenke@chromium.org,asanka@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=426465 Review URL: https://codereview.chromium.org/893323003 Cr-Commit-Position: refs/heads/master@{#314451} --- net/base/upload_file_element_reader.cc | 13 +++++------- net/base/upload_file_element_reader_unittest.cc | 28 +++++++++---------------- 2 files changed, 15 insertions(+), 26 deletions(-) diff --git a/net/base/upload_file_element_reader.cc b/net/base/upload_file_element_reader.cc index 864584d849d4..9be9117c2ab5 100644 --- a/net/base/upload_file_element_reader.cc +++ b/net/base/upload_file_element_reader.cc @@ -174,15 +174,12 @@ void UploadFileElementReader::OnGetFileInfoCompleted( } // If the underlying file has been changed and the expected file modification - // time is set, treat it as error. Note that |expected_modification_time_| may - // have gone through multiple conversion steps involving loss of precision - // (including conversion to time_t). Therefore the check below only verifies - // that the timestamps are within one second of each other. This check is used - // for sliced files. + // time is set, treat it as error. Note that the expected modification time + // from WebKit is based on time_t precision. So we have to convert both to + // time_t to compare. This check is used for sliced files. if (!expected_modification_time_.is_null() && - (expected_modification_time_ - file_info->last_modified) - .magnitude() - .InSeconds() != 0) { + expected_modification_time_.ToTimeT() != + file_info->last_modified.ToTimeT()) { callback.Run(ERR_UPLOAD_FILE_CHANGED); return; } diff --git a/net/base/upload_file_element_reader_unittest.cc b/net/base/upload_file_element_reader_unittest.cc index 719ce1e71c54..b0374acd966b 100644 --- a/net/base/upload_file_element_reader_unittest.cc +++ b/net/base/upload_file_element_reader_unittest.cc @@ -210,33 +210,25 @@ TEST_F(UploadFileElementReaderTest, FileChanged) { // Expect one second before the actual modification time to simulate change. const base::Time expected_modification_time = info.last_modified - base::TimeDelta::FromSeconds(1); - reader_.reset(new UploadFileElementReader( - base::MessageLoopProxy::current().get(), temp_file_path_, 0, kuint64max, - expected_modification_time)); + reader_.reset( + new UploadFileElementReader(base::MessageLoopProxy::current().get(), + temp_file_path_, + 0, + kuint64max, + expected_modification_time)); TestCompletionCallback init_callback; ASSERT_EQ(ERR_IO_PENDING, reader_->Init(init_callback.callback())); EXPECT_EQ(ERR_UPLOAD_FILE_CHANGED, init_callback.WaitForResult()); } -TEST_F(UploadFileElementReaderTest, InexactExpectedTimeStamp) { - base::File::Info info; - ASSERT_TRUE(base::GetFileInfo(temp_file_path_, &info)); - - const base::Time expected_modification_time = - info.last_modified - base::TimeDelta::FromMilliseconds(900); - reader_.reset(new UploadFileElementReader( - base::MessageLoopProxy::current().get(), temp_file_path_, 0, kuint64max, - expected_modification_time)); - TestCompletionCallback init_callback; - ASSERT_EQ(ERR_IO_PENDING, reader_->Init(init_callback.callback())); - EXPECT_EQ(OK, init_callback.WaitForResult()); -} - TEST_F(UploadFileElementReaderTest, WrongPath) { const base::FilePath wrong_path(FILE_PATH_LITERAL("wrong_path")); reader_.reset( new UploadFileElementReader(base::MessageLoopProxy::current().get(), - wrong_path, 0, kuint64max, base::Time())); + wrong_path, + 0, + kuint64max, + base::Time())); TestCompletionCallback init_callback; ASSERT_EQ(ERR_IO_PENDING, reader_->Init(init_callback.callback())); EXPECT_EQ(ERR_FILE_NOT_FOUND, init_callback.WaitForResult()); -- 2.11.4.GIT