Commit 8a9d71c8 authored by ericu@chromium.org's avatar ericu@chromium.org

Check File snapshots and Blob byte counts when writing to IDB.

This also fixes a few bugs in the IDB/Blob stuff:
* Removed a DCHECK that could hit depending on test teardown order.
* Made ChainedBlobWriterImpl truly asynchronous, fixing a crash if a File write failed before the first Blob write was attempted.
* Fix a crash where we'd try to free a scoped_ptr twice in the event of an abort.

Note that most of the time we won't actually receive File metadata, as Files often aren't snapshotted; I've got a blink change in progress [https://codereview.chromium.org/325383002/] that will always send it.

BUG=

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@278509 0039d316-1c4b-4281-b951-d872f2087c98
parent 18bd2389
...@@ -18,7 +18,6 @@ IndexedDBActiveBlobRegistry::IndexedDBActiveBlobRegistry( ...@@ -18,7 +18,6 @@ IndexedDBActiveBlobRegistry::IndexedDBActiveBlobRegistry(
: backing_store_(backing_store), weak_factory_(this) {} : backing_store_(backing_store), weak_factory_(this) {}
IndexedDBActiveBlobRegistry::~IndexedDBActiveBlobRegistry() { IndexedDBActiveBlobRegistry::~IndexedDBActiveBlobRegistry() {
DCHECK(use_tracker_.empty());
} }
void IndexedDBActiveBlobRegistry::AddBlobRef(int64 database_id, void IndexedDBActiveBlobRegistry::AddBlobRef(int64 database_id,
......
...@@ -2160,17 +2160,18 @@ class IndexedDBBackingStore::Transaction::ChainedBlobWriterImpl ...@@ -2160,17 +2160,18 @@ class IndexedDBBackingStore::Transaction::ChainedBlobWriterImpl
WriteDescriptorVec; WriteDescriptorVec;
ChainedBlobWriterImpl( ChainedBlobWriterImpl(
int64 database_id, int64 database_id,
IndexedDBBackingStore* backingStore, IndexedDBBackingStore* backing_store,
WriteDescriptorVec& blobs, WriteDescriptorVec& blobs,
scoped_refptr<IndexedDBBackingStore::BlobWriteCallback> callback) scoped_refptr<IndexedDBBackingStore::BlobWriteCallback> callback)
: waiting_for_callback_(false), : waiting_for_callback_(false),
database_id_(database_id), database_id_(database_id),
backing_store_(backingStore), backing_store_(backing_store),
callback_(callback), callback_(callback),
aborted_(false) { aborted_(false) {
blobs_.swap(blobs); blobs_.swap(blobs);
iter_ = blobs_.begin(); iter_ = blobs_.begin();
WriteNextFile(); backing_store->task_runner()->PostTask(
FROM_HERE, base::Bind(&ChainedBlobWriterImpl::WriteNextFile, this));
} }
virtual void set_delegate(scoped_ptr<FileWriterDelegate> delegate) OVERRIDE { virtual void set_delegate(scoped_ptr<FileWriterDelegate> delegate) OVERRIDE {
...@@ -2179,7 +2180,6 @@ class IndexedDBBackingStore::Transaction::ChainedBlobWriterImpl ...@@ -2179,7 +2180,6 @@ class IndexedDBBackingStore::Transaction::ChainedBlobWriterImpl
virtual void ReportWriteCompletion(bool succeeded, virtual void ReportWriteCompletion(bool succeeded,
int64 bytes_written) OVERRIDE { int64 bytes_written) OVERRIDE {
// TODO(ericu): Check bytes_written against the blob's snapshot value.
DCHECK(waiting_for_callback_); DCHECK(waiting_for_callback_);
DCHECK(!succeeded || bytes_written >= 0); DCHECK(!succeeded || bytes_written >= 0);
waiting_for_callback_ = false; waiting_for_callback_ = false;
...@@ -2190,10 +2190,14 @@ class IndexedDBBackingStore::Transaction::ChainedBlobWriterImpl ...@@ -2190,10 +2190,14 @@ class IndexedDBBackingStore::Transaction::ChainedBlobWriterImpl
self_ref_ = NULL; self_ref_ = NULL;
return; return;
} }
if (succeeded) if (iter_->size() != -1 && iter_->size() != bytes_written)
succeeded = false;
if (succeeded) {
++iter_;
WriteNextFile(); WriteNextFile();
else } else {
callback_->Run(false); callback_->Run(false);
}
} }
virtual void Abort() OVERRIDE { virtual void Abort() OVERRIDE {
...@@ -2219,7 +2223,6 @@ class IndexedDBBackingStore::Transaction::ChainedBlobWriterImpl ...@@ -2219,7 +2223,6 @@ class IndexedDBBackingStore::Transaction::ChainedBlobWriterImpl
return; return;
} }
waiting_for_callback_ = true; waiting_for_callback_ = true;
++iter_;
} }
} }
...@@ -2244,17 +2247,17 @@ class LocalWriteClosure : public FileWriterDelegate::DelegateWriteCallback, ...@@ -2244,17 +2247,17 @@ class LocalWriteClosure : public FileWriterDelegate::DelegateWriteCallback,
base::TaskRunner* task_runner) base::TaskRunner* task_runner)
: chained_blob_writer_(chained_blob_writer), : chained_blob_writer_(chained_blob_writer),
task_runner_(task_runner), task_runner_(task_runner),
bytes_written_(-1) {} bytes_written_(0) {}
void Run(base::File::Error rv, void Run(base::File::Error rv,
int64 bytes, int64 bytes,
FileWriterDelegate::WriteProgressStatus write_status) { FileWriterDelegate::WriteProgressStatus write_status) {
DCHECK_GE(bytes, 0);
bytes_written_ += bytes;
if (write_status == FileWriterDelegate::SUCCESS_IO_PENDING) if (write_status == FileWriterDelegate::SUCCESS_IO_PENDING)
return; // We don't care about progress events. return; // We don't care about progress events.
if (rv == base::File::FILE_OK) { if (rv == base::File::FILE_OK) {
DCHECK_GE(bytes, 0);
DCHECK_EQ(write_status, FileWriterDelegate::SUCCESS_COMPLETED); DCHECK_EQ(write_status, FileWriterDelegate::SUCCESS_COMPLETED);
bytes_written_ = bytes;
} else { } else {
DCHECK(write_status == FileWriterDelegate::ERROR_WRITE_STARTED || DCHECK(write_status == FileWriterDelegate::ERROR_WRITE_STARTED ||
write_status == FileWriterDelegate::ERROR_WRITE_NOT_STARTED); write_status == FileWriterDelegate::ERROR_WRITE_NOT_STARTED);
...@@ -2320,8 +2323,15 @@ bool IndexedDBBackingStore::WriteBlobFile( ...@@ -2320,8 +2323,15 @@ bool IndexedDBBackingStore::WriteBlobFile(
base::File::Info info; base::File::Info info;
if (base::GetFileInfo(descriptor.file_path(), &info)) { if (base::GetFileInfo(descriptor.file_path(), &info)) {
// TODO(ericu): Validate the snapshot date here. Expand WriteDescriptor if (descriptor.size() != -1) {
// to include snapshot date and file size, and check both. if (descriptor.size() != info.size)
return false;
// The round-trip can be lossy; round to nearest millisecond.
int64 delta = (descriptor.last_modified() -
info.last_modified).InMilliseconds();
if (std::abs(delta) > 1)
return false;
}
if (!base::TouchFile(path, info.last_accessed, info.last_modified)) { if (!base::TouchFile(path, info.last_accessed, info.last_modified)) {
// TODO(ericu): Complain quietly; timestamp's probably not vital. // TODO(ericu): Complain quietly; timestamp's probably not vital.
} }
...@@ -3876,10 +3886,15 @@ leveldb::Status IndexedDBBackingStore::Transaction::HandleBlobPreTransaction( ...@@ -3876,10 +3886,15 @@ leveldb::Status IndexedDBBackingStore::Transaction::HandleBlobPreTransaction(
journal.push_back(journal_entry); journal.push_back(journal_entry);
if (info_iter->is_file()) { if (info_iter->is_file()) {
new_files_to_write->push_back( new_files_to_write->push_back(
WriteDescriptor(info_iter->file_path(), next_blob_key)); WriteDescriptor(info_iter->file_path(),
next_blob_key,
info_iter->size(),
info_iter->last_modified()));
} else { } else {
new_files_to_write->push_back(WriteDescriptor( new_files_to_write->push_back(
getURLFromUUID(info_iter->uuid()), next_blob_key)); WriteDescriptor(getURLFromUUID(info_iter->uuid()),
next_blob_key,
info_iter->size()));
} }
info_iter->set_key(next_blob_key); info_iter->set_key(next_blob_key);
new_blob_keys.push_back(&*info_iter); new_blob_keys.push_back(&*info_iter);
...@@ -4066,7 +4081,8 @@ class IndexedDBBackingStore::Transaction::BlobWriteCallbackWrapper ...@@ -4066,7 +4081,8 @@ class IndexedDBBackingStore::Transaction::BlobWriteCallbackWrapper
: transaction_(transaction), callback_(callback) {} : transaction_(transaction), callback_(callback) {}
virtual void Run(bool succeeded) OVERRIDE { virtual void Run(bool succeeded) OVERRIDE {
callback_->Run(succeeded); callback_->Run(succeeded);
transaction_->chained_blob_writer_ = NULL; if (succeeded) // Else it's already been deleted during rollback.
transaction_->chained_blob_writer_ = NULL;
} }
private: private:
...@@ -4213,12 +4229,21 @@ void IndexedDBBackingStore::Transaction::PutBlobInfo( ...@@ -4213,12 +4229,21 @@ void IndexedDBBackingStore::Transaction::PutBlobInfo(
IndexedDBBackingStore::Transaction::WriteDescriptor::WriteDescriptor( IndexedDBBackingStore::Transaction::WriteDescriptor::WriteDescriptor(
const GURL& url, const GURL& url,
int64_t key) int64_t key,
: is_file_(false), url_(url), key_(key) {} int64_t size)
: is_file_(false), url_(url), key_(key), size_(size) {
}
IndexedDBBackingStore::Transaction::WriteDescriptor::WriteDescriptor( IndexedDBBackingStore::Transaction::WriteDescriptor::WriteDescriptor(
const FilePath& file_path, const FilePath& file_path,
int64_t key) int64_t key,
: is_file_(true), file_path_(file_path), key_(key) {} int64_t size,
base::Time last_modified)
: is_file_(true),
file_path_(file_path),
key_(key),
size_(size),
last_modified_(last_modified) {
}
} // namespace content } // namespace content
...@@ -16,6 +16,7 @@ ...@@ -16,6 +16,7 @@
#include "base/memory/ref_counted.h" #include "base/memory/ref_counted.h"
#include "base/memory/scoped_ptr.h" #include "base/memory/scoped_ptr.h"
#include "base/strings/string_piece.h" #include "base/strings/string_piece.h"
#include "base/time/time.h"
#include "base/timer/timer.h" #include "base/timer/timer.h"
#include "content/browser/indexed_db/indexed_db.h" #include "content/browser/indexed_db/indexed_db.h"
#include "content/browser/indexed_db/indexed_db_active_blob_registry.h" #include "content/browser/indexed_db/indexed_db_active_blob_registry.h"
...@@ -435,8 +436,11 @@ class CONTENT_EXPORT IndexedDBBackingStore ...@@ -435,8 +436,11 @@ class CONTENT_EXPORT IndexedDBBackingStore
class WriteDescriptor { class WriteDescriptor {
public: public:
WriteDescriptor(const GURL& url, int64_t key); WriteDescriptor(const GURL& url, int64_t key, int64_t size);
WriteDescriptor(const base::FilePath& path, int64_t key); WriteDescriptor(const base::FilePath& path,
int64_t key,
int64_t size,
base::Time last_modified);
bool is_file() const { return is_file_; } bool is_file() const { return is_file_; }
const GURL& url() const { const GURL& url() const {
...@@ -448,12 +452,16 @@ class CONTENT_EXPORT IndexedDBBackingStore ...@@ -448,12 +452,16 @@ class CONTENT_EXPORT IndexedDBBackingStore
return file_path_; return file_path_;
} }
int64_t key() const { return key_; } int64_t key() const { return key_; }
int64_t size() const { return size_; }
base::Time last_modified() const { return last_modified_; }
private: private:
bool is_file_; bool is_file_;
GURL url_; GURL url_;
base::FilePath file_path_; base::FilePath file_path_;
int64_t key_; int64_t key_;
int64_t size_;
base::Time last_modified_;
}; };
class ChainedBlobWriter : public base::RefCounted<ChainedBlobWriter> { class ChainedBlobWriter : public base::RefCounted<ChainedBlobWriter> {
......
...@@ -650,6 +650,11 @@ void IndexedDBDispatcherHost::DatabaseDispatcherHost::OnPut( ...@@ -650,6 +650,11 @@ void IndexedDBDispatcherHost::DatabaseDispatcherHost::OnPut(
} }
blob_info[i] = blob_info[i] =
IndexedDBBlobInfo(info.uuid, path, info.file_name, info.mime_type); IndexedDBBlobInfo(info.uuid, path, info.file_name, info.mime_type);
if (info.size != static_cast<uint64_t>(-1)) {
blob_info[i].set_last_modified(
base::Time::FromDoubleT(info.last_modified));
blob_info[i].set_size(info.size);
}
} else { } else {
blob_info[i] = IndexedDBBlobInfo(info.uuid, info.mime_type, info.size); blob_info[i] = IndexedDBBlobInfo(info.uuid, info.mime_type, info.size);
} }
......
...@@ -374,9 +374,9 @@ void IndexedDBDispatcher::RequestIDBDatabasePut( ...@@ -374,9 +374,9 @@ void IndexedDBDispatcher::RequestIDBDatabasePut(
if (info.isFile()) { if (info.isFile()) {
blob_or_file_info.file_path = info.filePath(); blob_or_file_info.file_path = info.filePath();
blob_or_file_info.file_name = info.fileName(); blob_or_file_info.file_name = info.fileName();
} else { blob_or_file_info.last_modified = info.lastModified();
blob_or_file_info.size = info.size();
} }
blob_or_file_info.size = info.size();
blob_or_file_info.uuid = info.uuid().latin1(); blob_or_file_info.uuid = info.uuid().latin1();
DCHECK(blob_or_file_info.uuid.size()); DCHECK(blob_or_file_info.uuid.size());
blob_or_file_info.mime_type = info.type(); blob_or_file_info.mime_type = info.type();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment