FileSystem: Prevent iterator from being deleted after database.

FileSystem: Prevent iterator from being deleted after database.

leveldb::Iterator's must be deleted before leveldb::DB. No known
crashes for these two functions have been observed.

Committed: https://crrev.com/1665eaf43626e138d805ddc4df79190b734a18c0
Cr-Commit-Position: refs/heads/master@{#371814}

[cmumford, 2016-01-26 23:40:52]

cmumford@chromium.org changed reviewers:
+ nhiroki@chromium.org

[nhiroki, 2016-01-27 00:48:00]

LGTM. Thank you for fixing this!

Are you making a similar patch for ServiceWorkerDatabase? If not, I'll work on
it.

https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
File storage/browser/fileapi/sandbox_directory_database.cc (right):

https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
storage/browser/fileapi/sandbox_directory_database.cc:848:
scoped_ptr<leveldb::Iterator> iter(
Can you add a comment about why we need to wrap these lines with {}?

https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
storage/browser/fileapi/sandbox_directory_database.cc:868:
HandleError(FROM_HERE, status);
Can you add a comment in the header file like this?

// Close the database. Before this, all iterators associated with the database
must be deleted.
void HandleError(const tracked_objects::Location& from_here,
                 const leveldb::Status& status);

https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
File storage/browser/fileapi/sandbox_origin_database.cc (right):

https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
storage/browser/fileapi/sandbox_origin_database.cc:335:
scoped_ptr<leveldb::Iterator> iter(
ditto.

[cmumford, 2016-01-27 03:08:16]

On 2016/01/27 00:48:00, nhiroki wrote:
> LGTM. Thank you for fixing this!
> 
> Are you making a similar patch for ServiceWorkerDatabase? If not, I'll work on
> it.
> 
>
https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
> File storage/browser/fileapi/sandbox_directory_database.cc (right):
> 
>
https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
> storage/browser/fileapi/sandbox_directory_database.cc:848:
> scoped_ptr<leveldb::Iterator> iter(
> Can you add a comment about why we need to wrap these lines with {}?
> 
>
https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
> storage/browser/fileapi/sandbox_directory_database.cc:868:
> HandleError(FROM_HERE, status);
> Can you add a comment in the header file like this?
> 
> // Close the database. Before this, all iterators associated with the database
> must be deleted.
> void HandleError(const tracked_objects::Location& from_here,
>                  const leveldb::Status& status);
> 
>
https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
> File storage/browser/fileapi/sandbox_origin_database.cc (right):
> 
>
https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
> storage/browser/fileapi/sandbox_origin_database.cc:335:
> scoped_ptr<leveldb::Iterator> iter(
> ditto.

Yes: https://codereview.chromium.org/1635223002/

[cmumford, 2016-01-27 03:08:57]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2016-01-27 03:09:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1639973002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1639973002/1

[cmumford, 2016-01-27 03:18:06]

The CQ bit was unchecked by cmumford@chromium.org

[nhiroki, 2016-01-27 03:41:11]

On 2016/01/27 03:08:16, cmumford wrote:
> On 2016/01/27 00:48:00, nhiroki wrote:
> > LGTM. Thank you for fixing this!
> > 
> > Are you making a similar patch for ServiceWorkerDatabase? If not, I'll work
on
> > it.
> > 
> >
>
https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
> > File storage/browser/fileapi/sandbox_directory_database.cc (right):
> > 
> >
>
https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
> > storage/browser/fileapi/sandbox_directory_database.cc:848:
> > scoped_ptr<leveldb::Iterator> iter(
> > Can you add a comment about why we need to wrap these lines with {}?
> > 
> >
>
https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
> > storage/browser/fileapi/sandbox_directory_database.cc:868:
> > HandleError(FROM_HERE, status);
> > Can you add a comment in the header file like this?
> > 
> > // Close the database. Before this, all iterators associated with the
database
> > must be deleted.
> > void HandleError(const tracked_objects::Location& from_here,
> >                  const leveldb::Status& status);
> > 
> >
>
https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
> > File storage/browser/fileapi/sandbox_origin_database.cc (right):
> > 
> >
>
https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
> > storage/browser/fileapi/sandbox_origin_database.cc:335:
> > scoped_ptr<leveldb::Iterator> iter(
> > ditto.
> 
> Yes: https://codereview.chromium.org/1635223002/

Great! Thank you :)

[cmumford, 2016-01-27 16:33:56]

Comments added - submitting now.

https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
File storage/browser/fileapi/sandbox_directory_database.cc (right):

https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
storage/browser/fileapi/sandbox_directory_database.cc:848:
scoped_ptr<leveldb::Iterator> iter(
On 2016/01/27 00:48:00, nhiroki wrote:
> Can you add a comment about why we need to wrap these lines with {}?

Done.

https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
storage/browser/fileapi/sandbox_directory_database.cc:868:
HandleError(FROM_HERE, status);
On 2016/01/27 00:48:00, nhiroki wrote:
> Can you add a comment in the header file like this?
> 
> // Close the database. Before this, all iterators associated with the database
> must be deleted.
> void HandleError(const tracked_objects::Location& from_here,
>                  const leveldb::Status& status);

Done.

https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
File storage/browser/fileapi/sandbox_origin_database.cc (right):

https://codereview.chromium.org/1639973002/diff/1/storage/browser/fileapi/san...
storage/browser/fileapi/sandbox_origin_database.cc:335:
scoped_ptr<leveldb::Iterator> iter(
On 2016/01/27 00:48:00, nhiroki wrote:
> ditto.

Done.

[cmumford, 2016-01-27 16:34:01]

The CQ bit was checked by cmumford@chromium.org

[cmumford, 2016-01-27 16:34:01]

The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/1639973002/#ps20001
(title: "Added comments.")

[commit-bot: I haz the power, 2016-01-27 16:34:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1639973002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1639973002/20001

[commit-bot: I haz the power, 2016-01-27 17:36:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-27 17:37:50]

Description was changed from

==========
FileSystem: Prevent iterator from being deleted after database.

leveldb::Iterator's must be deleted before leveldb::DB. No known
crashes for these two functions have been observed.
==========

to

==========
FileSystem: Prevent iterator from being deleted after database.

leveldb::Iterator's must be deleted before leveldb::DB. No known
crashes for these two functions have been observed.

Committed: https://crrev.com/1665eaf43626e138d805ddc4df79190b734a18c0
Cr-Commit-Position: refs/heads/master@{#371814}
==========

[commit-bot: I haz the power, 2016-01-27 17:37:51]

Patchset 2 (id:??) landed as
https://crrev.com/1665eaf43626e138d805ddc4df79190b734a18c0
Cr-Commit-Position: refs/heads/master@{#371814}