Skip to content

Commit

Permalink
Revert "Make sure kFile BlobDataItems have a expected_modification_ti…
Browse files Browse the repository at this point in the history
…me."

This reverts commit 9b568e6.

Reason for revert: this breaks XHR downloading to blobs, since the file that writes to might not be closed/updated until after the blob is created.

Original change's description:
> Make sure kFile BlobDataItems have a expected_modification_time.
> 
> Having expected_modification_time set guards us against files changing
> out from under us, and accidentally exposing those changes to the web.
> Without this change any files selected in <input> elements would be
> perpetually readable with whatever the file on disk changed to.
> 
> Bug: 710190
> Change-Id: I4b05ccfbe2d3c42116769d442bcec853973ff67c
> Reviewed-on: https://chromium-review.googlesource.com/920783
> Commit-Queue: Daniel Murphy <dmurph@chromium.org>
> Reviewed-by: Daniel Murphy <dmurph@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#538331}

TBR=dmurph@chromium.org,mek@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 710190, 826178
Change-Id: I7f742f6d63d39a546c441203e5815abf5d0ba4b2
Reviewed-on: https://chromium-review.googlesource.com/981502
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546278}
  • Loading branch information
mkruisselbrink authored and Commit Bot committed Mar 27, 2018
1 parent 9126ccb commit 724fb40
Show file tree
Hide file tree
Showing 7 changed files with 4 additions and 158 deletions.
10 changes: 0 additions & 10 deletions storage/browser/blob/blob_data_item.cc
Original file line number Diff line number Diff line change
Expand Up @@ -167,16 +167,6 @@ void BlobDataItem::GrowFile(uint64_t new_length) {
length_ = new_length;
}

// static
void BlobDataItem::SetFileModificationTimes(
std::vector<scoped_refptr<BlobDataItem>> items,
std::vector<base::Time> times) {
DCHECK_EQ(items.size(), times.size());
for (size_t i = 0; i < items.size(); ++i) {
items[i]->expected_modification_time_ = times[i];
}
}

void PrintTo(const BlobDataItem& x, ::std::ostream* os) {
const uint64_t kMaxDataPrintLength = 40;
DCHECK(os);
Expand Down
4 changes: 0 additions & 4 deletions storage/browser/blob/blob_data_item.h
Original file line number Diff line number Diff line change
Expand Up @@ -167,10 +167,6 @@ class STORAGE_EXPORT BlobDataItem : public base::RefCounted<BlobDataItem> {
expected_modification_time_ = time;
}

static void SetFileModificationTimes(
std::vector<scoped_refptr<BlobDataItem>> items,
std::vector<base::Time> times);

Type type_;
uint64_t offset_;
uint64_t length_;
Expand Down
40 changes: 0 additions & 40 deletions storage/browser/blob/blob_storage_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include "base/bind.h"
#include "base/callback.h"
#include "base/callback_helpers.h"
#include "base/files/file_util.h"
#include "base/location.h"
#include "base/logging.h"
#include "base/memory/ptr_util.h"
Expand All @@ -24,7 +23,6 @@
#include "base/numerics/safe_math.h"
#include "base/strings/stringprintf.h"
#include "base/task_runner.h"
#include "base/task_scheduler/post_task.h"
#include "base/threading/thread_task_runner_handle.h"
#include "base/trace_event/memory_dump_manager.h"
#include "base/trace_event/trace_event.h"
Expand All @@ -38,21 +36,6 @@ namespace storage {
namespace {
using ItemCopyEntry = BlobEntry::ItemCopyEntry;
using QuotaAllocationTask = BlobMemoryController::QuotaAllocationTask;

std::vector<base::Time> GetModificationTimes(
std::vector<base::FilePath> paths) {
std::vector<base::Time> result;
result.reserve(paths.size());
for (const auto& path : paths) {
base::File::Info info;
if (!base::GetFileInfo(path, &info)) {
result.emplace_back();
continue;
}
result.emplace_back(info.last_modified);
}
return result;
}
} // namespace

BlobStorageContext::BlobStorageContext()
Expand Down Expand Up @@ -245,29 +228,6 @@ std::unique_ptr<BlobDataHandle> BlobStorageContext::BuildBlobInternal(
UMA_HISTOGRAM_COUNTS_1M("Storage.Blob.TotalUnsharedSize",
total_memory_needed / 1024);

std::vector<scoped_refptr<BlobDataItem>> items_needing_timestamp;
std::vector<base::FilePath> file_paths_needing_timestamp;
for (auto& item : entry->items_) {
if (item->item()->type() == BlobDataItem::Type::kFile &&
!item->item()->IsFutureFileItem() &&
item->item()->expected_modification_time().is_null()) {
items_needing_timestamp.push_back(item->item());
file_paths_needing_timestamp.push_back(item->item()->path());
}
}
if (!items_needing_timestamp.empty()) {
// Blob construction isn't blocked on getting these timestamps. The created
// blob will be fully functional whether or not timestamps are set. When
// the timestamp isn't set the blob just won't be able to detect the file
// on disk changing after the blob is created.
base::PostTaskWithTraitsAndReplyWithResult(
FROM_HERE, {base::MayBlock()},
base::BindOnce(&GetModificationTimes,
std::move(file_paths_needing_timestamp)),
base::BindOnce(&BlobDataItem::SetFileModificationTimes,
std::move(items_needing_timestamp)));
}

size_t num_building_dependent_blobs = 0;
std::vector<std::unique_ptr<BlobDataHandle>> dependent_blobs;
// We hold a handle to all blobs we're using. This is important, as our memory
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,9 @@ writer.write(blob);
// Verify the contents.
assert(writer.length == 2 * testData.length + extensionOffset);
assert(writer.position == writer.length);
var file = entry.file();
var reader = new FileReaderSync();
var contents = reader.readAsBinaryString(entry.file());
var contents = reader.readAsBinaryString(file);
var i;
for (i = 0; i < testData.length + extensionOffset; ++i)
assert(contents.charCodeAt(i) == testData.charCodeAt(i));
Expand All @@ -55,7 +56,7 @@ assert(writer.length == testData.length + extensionOffset);
assert(writer.position == writer.length);

// Verify the contents.
contents = reader.readAsBinaryString(entry.file());
contents = reader.readAsBinaryString(file);
for (i = 0; i < extensionOffset; ++i)
assert(contents.charCodeAt(i) == testData.charCodeAt(i));
for (j = 0; i < writer.length; ++i, ++j)
Expand All @@ -79,7 +80,7 @@ assert(writer.length == 2 * testData.length);
assert(writer.position == testData.length + extensionOffset);

// Verify the contents.
contents = reader.readAsBinaryString(entry.file());
contents = reader.readAsBinaryString(file);
for (i = 0; i < extensionOffset; ++i)
assert(contents.charCodeAt(i) == testData.charCodeAt(i));
for (j = 0; i < testData.length + extensionOffset; ++i, ++j)
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

0 comments on commit 724fb40

Please sign in to comment.