<rdar://problem/15600186>
Created attachment 232457 [details] Patch
Comment on attachment 232457 [details] Patch /me slaps forehead
Comment on attachment 232457 [details] Patch Clearing flags on attachment: 232457 Committed r169587: <http://trac.webkit.org/changeset/169587>
All reviewed patches have been landed. Closing bug.
(In reply to comment #3) > (From update of attachment 232457 [details]) > Clearing flags on attachment: 232457 > > Committed r169587: <http://trac.webkit.org/changeset/169587> FYI: It broke many tests on Apple Mac WK2 bots and killed Mac WK2 EWS too.
And so EWS is now 24 patches behind. Finally rolling out.
Re-opened since this is blocked by bug 133552
This looks like the best crash trace from the bots: http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r169614%20(17957)/fast/workers/storage/open-database-while-transaction-in-progress-crash-log.txt
Created attachment 273467 [details] Fix
Comment on attachment 273467 [details] Fix Attachment 273467 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/950114 New failing tests: js/function-apply.html
Created attachment 273482 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 273523 [details] Updated
Attachment 273523 [details] did not pass style-queue: ERROR: Source/WebCore/platform/sql/SQLiteFileSystem.cpp:53: Use nullptr instead of NULL. [readability/null] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 273523 [details] Updated View in context: https://bugs.webkit.org/attachment.cgi?id=273523&action=review > Source/WebCore/platform/sql/SQLiteDatabase.cpp:123 > LOG_ERROR("journal_mode of database should be 'wal', but is '%s'", mode.utf8().data()); I think this should say 'WAL', not 'wal'. > Source/WebCore/platform/sql/SQLiteFileSystem.cpp:53 > + return sqlite3_open_v2(fileSystemRepresentation(filename).data(), database, SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_AUTOPROXY, NULL); I agree with the style checker, we should use nullptr instead of NULL.
Transmitting file data ... Committed revision 197922.
I think that this broke several WebSQL tests on iOS: https://build.webkit.org/results/Apple%20iOS%209%20Simulator%20Release%20WK2%20(Tests)/r197922%20(3855)/results.html
Re-opened since this is blocked by bug 155340
Created attachment 276111 [details] With testing fix
Attachment 276111 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1173: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1175: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1176: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1172: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WebCore/platform/sql/SQLiteFileSystem.cpp:53: Use nullptr instead of NULL. [readability/null] [5] Total errors found: 5 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 276112 [details] With style fix
Attachment 276112 [details] did not pass style-queue: ERROR: Source/WebCore/platform/sql/SQLiteFileSystem.cpp:53: Use nullptr instead of NULL. [readability/null] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 276114 [details] With style fix
Comment on attachment 276114 [details] With style fix View in context: https://bugs.webkit.org/attachment.cgi?id=276114&action=review > Source/WebCore/ChangeLog:33 > + - should et SQLITE_OPEN_AUTOPROXY flag when opening database. missing "s" in "set" > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:824 > +// This method is only intended for use by DumpRenderTree / WebKitTestRunner. > +// Actually deleting the databases is necessary to reset to a known state before running > +// each test case, but may be unsafe in deployment use cases (where multiple applications > +// may be accessing the same databases concurrently). This comment should be more closely associated with the IOSDeletionMode::Delete usage. That’s what needs the comment. In fact, I assume this is the *only* place using that flag. > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:825 > void DatabaseTracker::deleteAllDatabases() This should be renamed so it’s clear it’s only appropriate to use it for testing. That’s likely to be more effective than the comment. > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:831 > + deleteOrigin(origin.get(), IOSDeletionMode::Delete); Since this is an enum, not an enum class, there’s no need to use the IOSDeletionMode prefix here. Or we could use "enum class". > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1183 > #if !PLATFORM(IOS) > - return SQLiteFileSystem::deleteDatabaseFile(fullPath); > + UNUSED_PARAM(iOSDeletionMode); > #else > - // On the phone, other background processes may still be accessing this database. Deleting the database directly > - // would nuke the POSIX file locks, potentially causing Safari/WebApp to corrupt the new db if it's running in the background. > - // We'll instead truncate the database file to 0 bytes. If another process is operating on this same database file after > - // the truncation, it should get an error since the database file is no longer valid. When Safari is launched > - // next time, it'll go through the database files and clean up any zero-bytes ones. > - SQLiteDatabase database; > - if (database.open(fullPath)) > - return SQLiteFileSystem::truncateDatabaseFile(database.sqlite3Handle()); > - return false; > + if (iOSDeletionMode == IOSDeletionMode::Truncate) { > + // On the phone, other background processes may still be accessing this database. Deleting the database directly > + // would nuke the POSIX file locks, potentially causing Safari/WebApp to corrupt the new db if it's running in the background. > + // We'll instead truncate the database file to 0 bytes. If another process is operating on this same database file after > + // the truncation, it should get an error since the database file is no longer valid. When Safari is launched > + // next time, it'll go through the database files and clean up any zero-bytes ones. > + SQLiteDatabase database; > + if (database.open(fullPath)) > + return SQLiteFileSystem::truncateDatabaseFile(database.sqlite3Handle()); > + return false; > + } > #endif Does this code really need to be platform-specific? Why isn’t it needed on any platform other than iOS? Why, for example, does it not affect the Mac? I also think the code would read better with early return when open fails, rather than nesting the call to truncateDatabaseFile in an if statement body. > Source/WebCore/Modules/webdatabase/DatabaseTracker.h:101 > + enum IOSDeletionMode { > + Delete, > + Truncate > + }; If we need an enum, then I think we can come up with a better name for this than iOS deletion mode. Maybe something like enum FileLockPreservation { PreserveFileLocks, BlowAwayFileLocks }? Also, I’d put this all on a single line. Vertical formatting doesn’t seem helpful. > Source/WebCore/Modules/webdatabase/DatabaseTracker.h:105 > + WEBCORE_EXPORT bool deleteOrigin(SecurityOrigin*, IOSDeletionMode = IOSDeletionMode::Truncate); Since this is an enum, not an enum class, there’s no need to repeat the name of the enum; "IOSDeletionMode = Truncate" would work. Seems a shame to expose this functionality in a public function. Can we leave the public one without the deletion mode and add a private overload? That way the enum could be private too. > Source/WebCore/platform/sql/SQLiteDatabase.cpp:123 > LOG_ERROR("journal_mode of database should be 'wal', but is '%s'", mode.utf8().data()); Log message should say WAL rather than 'wal', I think.
Created attachment 276132 [details] With review fixes
(In reply to comment #23) Most review comments addressed. Will consider renames for deleteAllDatabases & the new enum. > Does this code really need to be platform-specific? Why isn’t it needed on > any platform other than iOS? Why, for example, does it not affect the Mac? I think that the current handling both for iOS and other platforms seems potentially problematic - this should be looked at further. Of the two mechanisms currently implemented, immediate deletion is most appropriate for testing needs on all platforms. For other purposes, things are less clear cut.
Okay, so I get the problem with the names Delete/Truncate – it's a little too specific about what we're doing, and not why. However I think PreserveFileLocks/BlowAwayFileLocks are equally problematic, for the opposite reason – they speak too much to our intended outcome, and hide important specifics of the implementation that may have unintended consequences (i.e. that deleteDatabaseFile may not actually delete the file!). I'm currently leaning towards the terminology of Soon/Immediately. 'Immediately' may also be a descriptive postfix for deleteAllDatabases to call out its different behavior from other methods.
Created attachment 276161 [details] Patch with review comment fixes for EWS
Created attachment 276163 [details] With review fixes for EWS
Created attachment 276164 [details] Patch for EWS
Committed revision 199309.
Broke storage/websql/alter-to-info-table.html on iOS. Disabled in r199381.
Created attachment 276676 [details] Enable on iOS
Comment on attachment 276676 [details] Enable on iOS View in context: https://bugs.webkit.org/attachment.cgi?id=276676&action=review > LayoutTests/platform/ios-simulator/TestExpectations:2528 > +storage/websql/alter-to-info-table.html [ Failure ] Could you please add a bugzilla/radar reference here?
Committed revision 199697.