[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

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

Cr-Commit-Position: refs/heads/master@{#314397}

2 files changed, 26 insertions, 15 deletions

diff --git a/net/base/upload_file_element_reader.cc b/net/base/upload_file_element_reader.cc
index 9be9117..864584d 100644
--- a/net/base/upload_file_element_reader.cc
+++ b/net/base/upload_file_element_reader.cc
@@ -174,12 +174,15 @@ void UploadFileElementReader::OnGetFileInfoCompleted(
   }
 
   // If the underlying file has been changed and the expected file modification
-  // 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.
+  // 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.
   if (!expected_modification_time_.is_null() &&
-      expected_modification_time_.ToTimeT() !=
-      file_info->last_modified.ToTimeT()) {
+      (expected_modification_time_ - file_info->last_modified)
+              .magnitude()
+              .InSeconds() != 0) {
     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 b0374ac..719ce1e 100644
--- a/net/base/upload_file_element_reader_unittest.cc
+++ b/net/base/upload_file_element_reader_unittest.cc
@@ -210,25 +210,33 @@ 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());