Remove failing DCHECKs in Blob code.

Remove failing DCHECKs in Blob code.

This change removes some DCHECKs that are triggered by empty File and
Blob instances getting serialized to IndexedDB.

The DCHECKs are hit in the layout tests in the CL below.
https://codereview.chromium.org/433983002

BUG=399323

[pwnall-personal, 2014-07-31 18:21:01]

PTAL?

[cmumford, 2014-07-31 18:44:43]

lgtm

https://codereview.chromium.org/431143002/diff/1/content/browser/fileapi/blob...
File content/browser/fileapi/blob_url_request_job_unittest.cc (right):

https://codereview.chromium.org/431143002/diff/1/content/browser/fileapi/blob...
content/browser/fileapi/blob_url_request_job_unittest.cc:102:
ASSERT_EQ(static_cast<int>(0),
Why the static_cast? Isn't 0 an integer literal already?

https://codereview.chromium.org/431143002/diff/1/content/browser/fileapi/blob...
content/browser/fileapi/blob_url_request_job_unittest.cc:103:
base::WriteFile(temp_empty_file_, kTestFileData1, 0));
Nit: This certainly works, but maybe CreateTemporaryFileInDir might be a bit
cleaner.

https://codereview.chromium.org/431143002/diff/1/webkit/common/blob/blob_data.cc
File webkit/common/blob/blob_data.cc (right):

https://codereview.chromium.org/431143002/diff/1/webkit/common/blob/blob_data...
webkit/common/blob/blob_data.cc:37: DCHECK_GT(length, 0ul);
If's OK to support empty files (which makes sense) what about empty BLOB's?

[michaeln, 2014-07-31 19:31:23]

Can we identify the source of the empty elements making it this far and
eliminate them there?

https://codereview.chromium.org/431143002/diff/1/webkit/browser/blob/blob_sto...
File webkit/browser/blob/blob_storage_context.cc (left):

https://codereview.chromium.org/431143002/diff/1/webkit/browser/blob/blob_sto...
webkit/browser/blob/blob_storage_context.cc:146: DCHECK(item.length() > 0);
These DCHECK's are here because higher level logic is expected to have filtered
out empty file and data elements before getting this far. Adding zero length
element adds nothing to the byte sequence the blob represents, so callers are
expected to weed them out so things like...

for(1 to a million) blob.add("");  // its not the real api but you get my point

... don't build big collections of nothingness.

[pwnall-personal, 2014-07-31 19:34:47]

https://codereview.chromium.org/431143002/diff/1/content/browser/fileapi/blob...
File content/browser/fileapi/blob_url_request_job_unittest.cc (right):

https://codereview.chromium.org/431143002/diff/1/content/browser/fileapi/blob...
content/browser/fileapi/blob_url_request_job_unittest.cc:102:
ASSERT_EQ(static_cast<int>(0),
On 2014/07/31 18:44:43, cmumford wrote:
> Why the static_cast? Isn't 0 an integer literal already?

Done.

https://codereview.chromium.org/431143002/diff/1/content/browser/fileapi/blob...
content/browser/fileapi/blob_url_request_job_unittest.cc:103:
base::WriteFile(temp_empty_file_, kTestFileData1, 0));
On 2014/07/31 18:44:43, cmumford wrote:
> Nit: This certainly works, but maybe CreateTemporaryFileInDir might be a bit
> cleaner.

Done.

https://codereview.chromium.org/431143002/diff/1/webkit/common/blob/blob_data.cc
File webkit/common/blob/blob_data.cc (right):

https://codereview.chromium.org/431143002/diff/1/webkit/common/blob/blob_data...
webkit/common/blob/blob_data.cc:37: DCHECK_GT(length, 0ul);
On 2014/07/31 18:44:43, cmumford wrote:
> If's OK to support empty files (which makes sense) what about empty BLOB's?

I was not able to trigger this DCHECK... and I tried :/

new Blob([])
new Blob(["", "", ""])
new Blob(["", new Blob([]), ""])

I admit I haven't looked too much in the code, and I just tried wicked things.
Is there anything that you think would work?

If I can't trigger the DCHECK, I don't think I should remove it.

[pwnall-personal, 2014-07-31 19:36:54]

On 2014/07/31 19:31:23, michaeln wrote:
> Can we identify the source of the empty elements making it this far and
> eliminate them there?
> 
>
https://codereview.chromium.org/431143002/diff/1/webkit/browser/blob/blob_sto...
> File webkit/browser/blob/blob_storage_context.cc (left):
> 
>
https://codereview.chromium.org/431143002/diff/1/webkit/browser/blob/blob_sto...
> webkit/browser/blob/blob_storage_context.cc:146: DCHECK(item.length() > 0);
> These DCHECK's are here because higher level logic is expected to have
filtered
> out empty file and data elements before getting this far. Adding zero length
> element adds nothing to the byte sequence the blob represents, so callers are
> expected to weed them out so things like...
> 
> for(1 to a million) blob.add("");  // its not the real api but you get my
point
> 
> ... don't build big collections of nothingness.

I think this happens with files because blink code avoids having to
(synchronously) determine a File's length. I'll try to look into it.

[michaeln, 2014-07-31 19:46:45]

> I think this happens with files because blink code avoids having to
> (synchronously) determine a File's length. I'll try to look into it.

kunintmax means to eof

[michaeln, 2014-07-31 19:48:10]

On 2014/07/31 19:46:45, michaeln wrote:
> > I think this happens with files because blink code avoids having to
> > (synchronously) determine a File's length. I'll try to look into it.
> 
> kunintmax means to eof

sry... kuint64max

[michaeln, 2014-07-31 19:50:21]

On 2014/07/31 19:48:10, michaeln wrote:
> On 2014/07/31 19:46:45, michaeln wrote:
> > > I think this happens with files because blink code avoids having to
> > > (synchronously) determine a File's length. I'll try to look into it.
> > 
> > kunintmax means to eof
> 
> sry... kuint64max

in blinkspeak... BlobDataItem::toEndOfFile

[jsbell, 2014-08-26 19:10:05]

In content/browser/indexed_db/indexed_db_callbacks.cc, CreateBlobData, we could
simply skip the blob_data->AppendFile() call if blob_info.size() is 0.

So far as I can tell from a skim of the code, the difference in behavior we'd no
longer place DataElement entry in the BlobData recording the
expected_last_modified time, so if something somehow touched the file in the IDB
blob backing store we wouldn't detect this and invalidate reads. Since data in
the IDB blob backing store is immutable, this shouldn't be a concern.

What am I missing?

[michaeln, 2014-08-26 21:06:43]

thnx for digging up where the zero-length inputs are coming from. looks like it
might be ok to not add the emtpy file, but what does the
AddFinalReleaseCallback(blob_info.release_callback()) callback do?

> In content/browser/indexed_db/indexed_db_callbacks.cc, CreateBlobData, we
could
> simply skip the blob_data->AppendFile() call if blob_info.size() is 0.
> 
> So far as I can tell from a skim of the code, the difference in behavior we'd
no
> longer place DataElement entry in the BlobData recording the
> expected_last_modified time, so if something somehow touched the file in the
IDB
> blob backing store we wouldn't detect this and invalidate reads. Since data in
> the IDB blob backing store is immutable, this shouldn't be a concern.
> 
> What am I missing?

[michaeln, 2014-08-26 21:16:42]

On 2014/08/26 21:06:43, michaeln wrote:
> thnx for digging up where the zero-length inputs are coming from. looks like
it
> might be ok to not add the emtpy file, but what does the
> AddFinalReleaseCallback(blob_info.release_callback()) callback do?

looks like it's using that mechanism to know when the blobs it vends are no
longer referenced

[michaeln, 2014-08-26 22:37:58]

> looks like it's using that mechanism to know when the blobs it vends are no
> longer referenced

i'm not familiar with what idb does with notification that a blob it's produced
is no longer needed but i see it get into ReportBlobUnused(db_id, blob_key)
which has more todo with idb internal book keeping that it does file deletion.

seems like the idb system is using the ShareableFileRef class to indirectly get
a notification about a blob its vended. it's more interested in the blob than
the file. if thats right, maybe the blob system could provide a direct
notifcation about a blob_uuid being removed from the central blob_map_ instead
of relying on the fileref indirection (which is kind of obscure)?

it'd be easier to relax the dcheck for FILE item types but its kinda sloppy
feeling

thoughts?

[michaeln, 2014-08-26 23:24:30]

> thoughts?

i thought of something else.

blobs can be composed of other blobs (or slices of them). the composed blobs
incorporate the raw elements of any blob items they're constructed with.
shareablefilerefs will be shared by all blobs that incorporate a shareablefile.
so the idb system won't get notificed of until the *file* really is no longer
needed. that's different than when the original blob bearing that file is
dropped (bah).

here's an assertion, idb shouldn't write zero-length files on disk to start with

[jsbell, 2014-12-10 01:18:23]

On 2014/08/26 23:24:30, michaeln wrote:
> here's an assertion, idb shouldn't write zero-length files on disk to start
with

I like that idea in theory... although I recently learned that we don't persist
the lastModified
time of File objects in the database; we actually use the timestamp of the file
on disk. So if we actually had a zero-length File and it wasn't on disk we'd
lose that data. Bleah.

[cmumford, 2015-02-21 01:57:46]

On 2014/12/10 01:18:23, jsbell wrote:
> On 2014/08/26 23:24:30, michaeln wrote:
> > here's an assertion, idb shouldn't write zero-length files on disk to start
> with
> 
> I like that idea in theory... although I recently learned that we don't
persist
> the lastModified
> time of File objects in the database; we actually use the timestamp of the
file
> on disk. So if we actually had a zero-length File and it wasn't on disk we'd
> lose that data. Bleah.

Yes, I agree, but I think that we should write a zero length file to disk - just
like any other OS (which we are) when asked to create an empty file.

[michaeln, 2015-02-23 19:29:12]

On 2015/02/21 01:57:46, cmumford wrote:
> On 2014/12/10 01:18:23, jsbell wrote:
> > On 2014/08/26 23:24:30, michaeln wrote:
> > > here's an assertion, idb shouldn't write zero-length files on disk to
start
> > with
> > 
> > I like that idea in theory... although I recently learned that we don't
> persist
> > the lastModified
> > time of File objects in the database; we actually use the timestamp of the
> file
> > on disk. So if we actually had a zero-length File and it wasn't on disk we'd
> > lose that data. Bleah.
> 
> Yes, I agree, but I think that we should write a zero length file to disk -
just
> like any other OS (which we are) when asked to create an empty file.

The question is more a matter of whether to use the OS filesystem to store some
of the meta data for 'files' written into idb values. I'm not sure if this is a
complete listing of the meta data consist we have to handle? I've put notes
about where i think those pieces are currently stored, but i'm not 100% sure
about that.

- filename (currently stored in leveldb)
- filesize (current stored in leveldb and in the OS filesystem)
- contenttype (current stored in leveldb)
- datetimes (current stored in the OS filesystem)

Its makes some sense to store all of the metadata we care to store in leveldb
for consistency, and to only use the filesystem for the bag of unstructured
bits.