New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 26 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug

Blocked on:
issue 112643

Blocking:
issue 92761

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

Defective Origins/CURRENT file disables File System API

Reported by vladi...@palant.de, Nov 4 2011

Issue description

Chrome Version       : 16.0.912.21 beta-m
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)

What steps will reproduce the problem?
1. Close Chrome
2. Go to "%LOCALAPPDATA%\Google\Chrome\User Data\Default\File System\Origins" directory and replace the file CURRENT by an empty file.
3. Run the following code from Console on any website:

window.webkitRequestFileSystem(window.PERSISTENT, 1, function() {console.log("success")}, function() {console.log("error")})

What is the expected result?

"success" should be printed to the console.

What happens instead?

"error" should be printed to the console (error code is 2 meaning SECURITY_ERR).

The current Adblock Plus development builds are using File System API to store data. Multiple people reported that data isn't being stored correctly. Upon investigation I discovered that their CURRENT file contains 16 \x00 characters instead of a valid file name. This typically happens if the computer is switched off abruptly, resulting in data loss. While data loss is understandable (possible to prevent nevertheless?), Chrome should be able to recover from the error and allow using File System API nevertheless. Unfortunately, this bug is a show-stopper for us, it means that we cannot release the next Adblock Plus version.
 

Comment 1 by vladi...@palant.de, Nov 6 2011

Additional note: so far I had two confirmed reports of this issue and a bunch more that are likely related to this issue. Given that development builds are used by roughly 5000 people that's a rather significant number. In both cases where I got the data the File API was previously used by Gmail (at least until the data got corrupted).

Comment 2 by mkwst@chromium.org, Nov 7 2011

Blocking: 92761

Comment 3 by mkwst@chromium.org, Nov 7 2011

Cc: kinuko@chromium.org michaeln@chromium.org ericu@chromium.org
Labels: -Area-Undefined Area-Internals Feature-FileSystem
Status: Untriaged
CCing some folks who might be interested in this.

Comment 4 by ericu@chromium.org, Nov 8 2011

Cc: h...@chromium.org dgro...@chromium.org
Database corruption will obviously result in data loss, but we should have a way to recover, and at least add new origins.  What's really concerning to me, though, is the high fraction of users that have seen database corruption in LevelDB.  Unless I'm using it wrong, this is quite likely to hit IndexedDB as well.

Comment 5 by vladi...@palant.de, Nov 8 2011

As I said, the kind of corruption indicates loss of disk write cache. I'm not sure whether Chrome does it already for this file but a call to base::FlushPlatformFile() might make this issue less likely.

Comment 6 by vladi...@palant.de, Nov 26 2011

A user says that the same data corruption reappeared for him after he removed the defective files. Supposedly it happened after a simple Chrome restart, without even shutting down the OS.

Comment 7 by vladi...@palant.de, Dec 5 2011

Another user suspects that this issue affects all users with non-ASCII user names (meaning non-ASCII path to the Origins directory). He says that in his experiments the issue was reproducible consistently if he changed his user name to something non-ASCII. Could somebody look into this?

Comment 8 by ericu@chromium.org, Dec 5 2011

There was a different problem with non-ASCII paths:  crbug.com/94314 .  That should be fixed in M16.  I don't believe that led to this kind of corruption, though--it probably just failed to write anything.  Kinuko, can you comment?
Cc: tzik@chromium.org
We've been hearing another requestFileSystem error report that happens only on Win 7 and has high error rates in non-US countries.  I'm not sure if all of these reports have the same root but there seem to be some remaining issues around non-ASCII profile dir.
@trev, could we get the exact repro step the user reported in #7?   When we fixed  issue 94314  we tested non-ASCII profile dir and in our simplest testing it looked ok.   Maybe restarting chrome part is the key?
The steps as I got them:

> I've experimented creating a username without accents (Andre) and download Fanboy
> predefined English list along with a custom Regional list from his site, restarted
> Chrome and the lists were there! Deleted them, inserted different ones, restarted
> computer this time, logged in same username without accents (Andre) and the newer
> lists I chose were still there.

> Same experiment but with username with accent (André) and lists weren't saved,
> either with Chrome restart or with reboot.

> This happened on the same computer, Win7 x64 with Chrome 15.0.874.121 and ABP
> experimental 1.1.4.730 (Experimental API on).

But I suspect that the key here is his Chrome version. If I understand correctly,  issue 94314  will be fixed in Chrome 16?

The other user (the one who has regular data corruptions) replied that his user name is proper ASCII.
Ah yes, if the user is having the problem in Chrome 15 hopefully it should go away in Chrome 16.  Also from what we store in the CURRENT file non-ascii char issue doesn't seem to have anything to do with the corruption (so I assume they are two distinct issues).
With my Fauxbar extension, I've had a couple of Windows 7 users report that the FileSystem is throwing this SECURITY_ERR with Chrome 15. (Fauxbar's site thumbnail screenshots are stored in the FileSystem)

One user found that their Chrome error log was saying something like, "CURRENT file does not end with newline"

He deleted the CURRENT file and the problem was resolved.

My error tracking for this: http://code.google.com/p/fauxbar/issues/detail?id=81

Comment 13 by ericu@chromium.org, Dec 15 2011

Cc: bbudge@chromium.org
A short update: I believe we have fixed one of the consistently reproducible bug with non-ascii username in R16, but we're still hearing some number of users seeing SECURITY_ERR in requestFileSystem after R16 (esp on Windows 7).  We keep failing to reproduce the issue but try adding more code to investigate the issue.

As for the CURRENT corruption, I've taken a look at the leveldb/chromium code for this (namely, looked into db/filename.cc and env_chromium.cc)-- basically we're doing following when we update the CURRENT file:

1. writing the string (with '\n') to a temporary file (fopen, fwrite and fclose)
2. calling ReplaceFile() to replace the temporary file and CURRENT.

This doesn't look to be a dangerous operation.  Only point which concerned me was we're calling ReplaceFile without REPLACEFILE_WRITE_THROUGH flag, which guarantees the data copied from the replaced file is flushed to disk before the function returns.  Maybe we should try passing the flag when we replace the file?

Comment 15 by vladi...@palant.de, Dec 15 2011

Just wanted to mention that I will most likely release Adblock Plus 1.2 next week despite this issue - people are getting impatient and I implemented changes to make sure that this issue doesn't result in catastrophic failure. I will also document this issue and how to resolve it (manually).

Concerning comment 14: Yes, non-ASCII directory names was a different issue, sorry. So the real issue here is how CURRENT ends up being filled with NUL characters. It seems that the second step (rename) is written to disk while the first (write file) is still in the disk cache. Loosing the disk cache at this point results in a corruption that Chrome cannot recover from.
Sorry to hear that we weren't able to deliver a fix in a timely manner.  As for the CURRENT file issue the first step (write file) consists of simple three steps-- fopen, fwrite and fclose, and in my understanding (and quick Web search says) fclose does flush any pending writes.  That's why I suspected the second step.  If any of you could think of any other options I'd really love to hear them though.
Cc: jsb...@chromium.org
Update: I've tried to add REPLACEFILE_WRITE_THROUGH flag to ReplaceFile, then considered using NTFS transactions, but brettw and cpu suggested that 1) NTFS transactions could be very slow, and 2) fclose() doesn't flush on Windows and we should NOT use fopen/close for such purpose.  Carlos (cpu) suggested that we should use FILE_FLAG_WRITE_THROUGH to manage file IO directory.  So we'll need to modify File IO related code in env_chromium.cc at least for Windows.

Comment 19 by sh...@chromium.org, Dec 22 2011

To clarify previous comment, fclose() should imply fflush(), but it only flushes to the filesystem buffers, not necessarily to disk.

Based on past experience, expect that various forced-flush tricks will only narrow the window, but will not close it.  With a big enough installed base, you can still find yourself dealing with hundreds of thousands of bad occurrences per month.  If you can get away from doing read-modify-write or write-then-rename, and replace it with append-to-a-log, it might be worthwhile.  It might not :-).

Comment 20 by noel@chromium.org, Dec 23 2011

INFO: fflush() & flushall() Do Not Write Data Directly to Disk
  http://support.microsoft.com/kb/66052

How To Force Files to Be Flushed to Disk
  http://support.microsoft.com/kb/148505

_commit() is available in leveldb, via fdatasync(int fildes).  Agree with Scott, might mitigate, mighy only narrow the window.


Comment 21 by ericu@chromium.org, Jan 18 2012

See http://code.google.com/p/leveldb/issues/detail?id=68 for the levelDB issue.
Owner: jsb...@chromium.org
Status: Assigned
Blockedon: 112643
Owner: ----
Status: Available
I believe the "fflush alone is insufficient on Windows" part of this issue was resolved by the leveldb change mentioned above:

* The levelDB change (r57, bug linked above) adds WriteStringToFileSync (now used for updating CURRENT) which calls file->Sync()
* env_chromium's implementation of Sync calls fflush_unlocked(file) and fdatasync(fileno(file))
* on Windows, fflush_unlocked calls fflush(file)
* on Windows, ffdatasync calls _commit(fileds)

This doesn't address shess's concerns, though.
jsbell@, do you know in which chromium version the LevelDB flush change will be rolled in?  Thanks,
Another question: do you know if LevelDB tries to recover the CURRENT file if it finds it's corrupted?  Or once it's been corrupted it keeps returning a corruption error?
kinuko@ - we rolled it into r120426 (about a month ago)

If the CURRENT file is corrupt, leveldb fails the open. leveldb itself offers a RepairDB that attempts to recover the data but does not guarantee the integrity. Clients must call it manually.

For IndexedDB we're planning to call DestroyDB and retry the Open in case of corruption. See https://bugs.webkit.org/show_bug.cgi?id=79413 - we expose LevelDBDatabase::destroy() in the WebCore/platform wrapper for leveldb. This is blocked on a leveldb bug (just identified today); we expect a fix soon, and will roll it into Chromium ASAP.

@jsbell thanks for the info!!  We'll probably need to do the same.  Taiju can you take a look at this one (after the blocking leveldb issue is resolved)?

Comment 29 by tzik@chromium.org, Mar 9 2012

Owner: tzik@chromium.org
Status: Assigned

Comment 30 by tzik@chromium.org, Mar 23 2012

Labels: -Pri-2 Pri-1 Mstone-20
Project Member

Comment 31 by bugdroid1@chromium.org, Mar 29 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=129631

------------------------------------------------------------------------
r129631 | tzik@chromium.org | Thu Mar 29 09:45:48 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_origin_database.cc?r1=129631&r2=129630&pathrev=129631
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_util.h?r1=129631&r2=129630&pathrev=129631
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_util.cc?r1=129631&r2=129630&pathrev=129631
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_origin_database_unittest.cc?r1=129631&r2=129630&pathrev=129631
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_origin_database.h?r1=129631&r2=129630&pathrev=129631
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_directory_database.cc?r1=129631&r2=129630&pathrev=129631
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/obfuscated_file_util.cc?r1=129631&r2=129630&pathrev=129631
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_directory_database_unittest.cc?r1=129631&r2=129630&pathrev=129631
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_directory_database.h?r1=129631&r2=129630&pathrev=129631

Add database recovery for FileSystemOriginDatabase

BUG= 103018 ,116615
TEST=FileSystemOriginDatabase.DatabaseRecoveryTest


Review URL: http://codereview.chromium.org/9663021
------------------------------------------------------------------------
Project Member

Comment 32 by bugdroid1@chromium.org, Apr 2 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=130211

------------------------------------------------------------------------
r130211 | tzik@chromium.org | Mon Apr 02 14:31:43 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_origin_database.cc?r1=130211&r2=130210&pathrev=130211
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_directory_database.cc?r1=130211&r2=130210&pathrev=130211

Refine UMA stats for file system databases.


BUG= 103018 ,116615
TEST=

Review URL: http://codereview.chromium.org/9956059
------------------------------------------------------------------------

Comment 33 by tzik@chromium.org, Apr 5 2012

Cc: lafo...@chromium.org
Labels: -Mstone-20 Mstone-19 Merge-Requested
Requesting to merge http://crrev.com/129631 and http://crrev.com/130211 to R19.

This database recovering is critical for FileSystem API, and UMA stats from Canary Chrome says it works well for a week.
Could you let me merge it to next Beta?

Comment 34 by laforge@google.com, Apr 10 2012

Labels: -Merge-Requested Merge-Approved
I'm not going to be approving additional patches for this area of code.  If this causes any issues, we will need to revert it and wait for 20.

Comment 35 by tzik@chromium.org, Apr 11 2012

Labels: -Merge-Approved Merge-Merged
Thank you! I merged it to 19.
I believe it works well and no more fix for it.
Project Member

Comment 36 by bugdroid1@chromium.org, Apr 11 2012

Labels: merge-merged-1084
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=131716

------------------------------------------------------------------------
r131716 | tzik@chromium.org | Tue Apr 10 22:12:15 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/1084/src/webkit/fileapi/file_system_util.cc?r1=131716&r2=131715&pathrev=131716
 M http://src.chromium.org/viewvc/chrome/branches/1084/src/webkit/fileapi/file_system_origin_database_unittest.cc?r1=131716&r2=131715&pathrev=131716
 M http://src.chromium.org/viewvc/chrome/branches/1084/src/webkit/fileapi/file_system_directory_database_unittest.cc?r1=131716&r2=131715&pathrev=131716
 M http://src.chromium.org/viewvc/chrome/branches/1084/src/webkit/fileapi/file_system_origin_database.h?r1=131716&r2=131715&pathrev=131716
 M http://src.chromium.org/viewvc/chrome/branches/1084/src/webkit/fileapi/file_system_directory_database.h?r1=131716&r2=131715&pathrev=131716
 M http://src.chromium.org/viewvc/chrome/branches/1084/src/webkit/fileapi/file_system_directory_database.cc?r1=131716&r2=131715&pathrev=131716
 M http://src.chromium.org/viewvc/chrome/branches/1084/src/webkit/fileapi/obfuscated_file_util.cc?r1=131716&r2=131715&pathrev=131716
 M http://src.chromium.org/viewvc/chrome/branches/1084/src/webkit/fileapi/file_system_origin_database.cc?r1=131716&r2=131715&pathrev=131716
 M http://src.chromium.org/viewvc/chrome/branches/1084/src/webkit/fileapi/file_system_util.h?r1=131716&r2=131715&pathrev=131716

Merge 129631 - Add database recovery for FileSystemOriginDatabase

BUG= 103018 ,116615
TEST=FileSystemOriginDatabase.DatabaseRecoveryTest


Review URL: http://codereview.chromium.org/9663021

TBR=tzik@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10051002
------------------------------------------------------------------------
Project Member

Comment 37 by bugdroid1@chromium.org, Apr 16 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=132369

------------------------------------------------------------------------
r132369 | tzik@chromium.org | Sun Apr 15 21:15:20 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_usage_cache.cc?r1=132369&r2=132368&pathrev=132369
 A http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_database_test_helper.cc?r1=132369&r2=132368&pathrev=132369
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/tools/test_shell/test_shell.gypi?r1=132369&r2=132368&pathrev=132369
 A http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_database_test_helper.h?r1=132369&r2=132368&pathrev=132369
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_usage_cache_unittest.cc?r1=132369&r2=132368&pathrev=132369
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_origin_database_unittest.cc?r1=132369&r2=132368&pathrev=132369
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/sandbox_mount_point_provider.cc?r1=132369&r2=132368&pathrev=132369
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_usage_cache.h?r1=132369&r2=132368&pathrev=132369
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_directory_database.cc?r1=132369&r2=132368&pathrev=132369
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_directory_database_unittest.cc?r1=132369&r2=132368&pathrev=132369
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_directory_database.h?r1=132369&r2=132368&pathrev=132369

Add database recovery for FileSystemDirectoryDatabase.

BUG= 103018 ,116615
TEST='FileSystemDirectoryDatabaseTest.*'

Review URL: https://chromiumcodereview.appspot.com/9910005
------------------------------------------------------------------------
Project Member

Comment 38 by bugdroid1@chromium.org, May 7 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=135632

------------------------------------------------------------------------
r135632 | tzik@chromium.org | Mon May 07 05:18:50 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_origin_database.cc?r1=135632&r2=135631&pathrev=135632
 M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_directory_database.cc?r1=135632&r2=135631&pathrev=135632

Add non corruption error handling to FileSystem databases.


BUG= 103018 
TEST=existing tests'



Review URL: http://codereview.chromium.org/10386006
------------------------------------------------------------------------
Blockedon: -chromium:112643 chromium:112643
Taiju, can we mark this FIXED?

Comment 41 by tzik@chromium.org, Jul 26 2012

Status: Fixed
Yes. I believe it's fix. Thanks for noticing me.
Project Member

Comment 42 by bugdroid1@chromium.org, Oct 13 2012

Blocking: -chromium:92761 chromium:92761
Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member

Comment 43 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Area-Internals -Mstone-19 M-19 Cr-Internals
Project Member

Comment 44 by bugdroid1@chromium.org, Mar 14 2013

Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue

Sign in to add a comment