According to the documentation[1], when updating urls in "check local lists" mode, if there is no "update" in the table version header, we should drop the existing table and start over. We're currently not doing this and always doing incremental updates. [1] http://wiki.mozilla.org/Phishing_Protection:_Server_Spec#Update_Requests

Comment on attachment 231704 [details] [diff] [review] v1: drop table if not an update >- // The line format is "[table-name #.####]" or "[table-name #.#### update]" >+ // The line format "[table-name #.####]" or "[table-name #.#### update]" >+ // The additional "update" in the header means that this is a diff. >+ // Otherwise, we should blow away the old table and start afresh. >+ PRBool isUpdate = PR_FALSE; >+ > PRInt32 spacePos = aLine.FindChar(' '); > if (spacePos == kNotFound) { > // bad table header > return NS_ERROR_FAILURE; > } > > const nsCSubstring &tableName = Substring(aLine, 1, spacePos - 1); > GetDbTableName(tableName, aDbTableName); > >+ // Check to see if it's an update >+ spacePos = aLine.FindChar(' ', spacePos + 1); Do you want to do stronger error checking on the format of the first line of the update? Like making sure it's bracketed and the number is really a number? This looks fine. >+ NS_NAMED_LITERAL_CSTRING(update, "update"); >+ if (spacePos != kNotFound && spacePos + update.Length() < aLine.Length()) { >+ const nsCSubstring &updateStr = Substring(aLine, >+ spacePos + 1, >+ update.Length()); >+ if (updateStr.Equals(update)) { >+ isUpdate = PR_TRUE; >+ } >+ }

Let's get this patch in on trunk and nominate for 1.8.1approval ASAP.

(In reply to comment #2) > Do you want to do stronger error checking on the format of the first line of > the update? Like making sure it's bracketed and the number is really a number? Sure, I'll do that. Ugh, this isn't quite right. It drops the table immediately then starts to repopulate as data comes in. That would give poor protection as the table is being repopulated. We should create a temporary table, fill that, then when we're ready to commit, do a table swap. I'll update the patch today and send out for review again.

- better validation of line header (forgot about sscanf existed) - if we're getting a new table, dump into a foo-bar-betz-new first, then swap tables at commit time

Comment on attachment 231833 [details] [diff] [review] v2: drop table if not an update, but use a temp table >+nsresult >+nsUrlClassifierDBServiceWorker::MaybeSwapTables(const nsCString& aVersionLine) >+{ >+ if (aVersionLine.Length() == 0) >+ return NS_ERROR_FAILURE; >+ >+ // Check to see if this was a full table update or not. >+ nsCString tableName; >+ PRBool isUpdate; >+ nsresult rv = ParseVersionString(aVersionLine, &tableName, &isUpdate); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ // Updates don't require any fancy logic. >+ if (isUpdate) >+ return NS_OK; >+ >+ // Not an update, so we need to swap tables by dropping the original table >+ // and copying in the values from the new table. >+ rv = MaybeDropTable(tableName); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = MaybeCreateTable(tableName); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ // Copy table. >+ // XXXtony: This can be slow for large tables, which is bad because >+ // while it's happending we can't check urls and it blocks shutdown. >+ // Rather than having a single db file, we should have a separate file >+ // per table and just rename files. >+ nsCString sql("INSERT INTO "); >+ sql.Append(tableName); >+ sql.Append(" SELECT * FROM "); >+ tableName.Append(kNEW_TABLE_SUFFIX); >+ sql.Append(tableName); >+ rv = mConnection->ExecuteSimpleSQL(sql); >+ NS_ENSURE_SUCCESS(rv, rv); Why not "RENAME new_table TO old_table"? Is it not available in this version of sql? http://dev.mysql.com/doc/refman/5.0/en/rename-table.html >+ // Drop the _new table >+ rv = MaybeDropTable(tableName); >+ // It's not so important that the final drop succeeds, so we let it go. >+ LOG(("tables swapped (%s)\n", tableName.get())); >+ >+ return NS_OK; >+}

(In reply to comment #6) > > Why not "RENAME new_table TO old_table"? Is it not available in this version > of sql? > http://dev.mysql.com/doc/refman/5.0/en/rename-table.html Oh, you're right, a bit more searching and infact, sqlite does support rename: http://www.sqlite.org/lang_altertable.html New patch coming up . . .

Comment on attachment 231860 [details] [diff] [review] v3: duh, use rename table Looks fine.

Comment on attachment 231860 [details] [diff] [review] v3: duh, use rename table >Index: toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp >+ count ++; "count ++" looks funny to me... maybe kill the space unless that's standard style elsewhere in this file. >- LOG(("Create table ok.\n")); >+ //LOG(("Create table ok.\n")); want to just delete the log statement altogether? >+ nsCString statement("DROP TABLE IF EXISTS "); >+ statement.Append(aTableName); >+ return mConnection->ExecuteSimpleSQL(statement); I think you should use nsCAutoString for statement so that you avoid two heap allocations (one for assignment and Append). >+ nsCString tableName; >+ PRBool isUpdate; >+ nsresult rv = ParseVersionString(aVersionLine, &tableName, &isUpdate); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ // Updates don't require any fancy logic. >+ if (isUpdate) >+ return NS_OK; >+ >+ // Not an update, so we need to swap tables by dropping the original table >+ // and copying in the values from the new table. >+ rv = MaybeDropTable(tableName); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCAutoString newTableName(tableName); >+ newTableName.Append(kNEW_TABLE_SUFFIX); Why not have tableName start out as a nsCAutoString? >+ // Bring over new table >+ nsCString sql("ALTER TABLE "); >+ sql.Append(newTableName); >+ sql.Append(" RENAME TO "); >+ sql.Append(tableName); sql should also be a nsCAutoString (it has a member variable that is a 64-byte buffer that it will use if the string fits) >+ const char* lineData; >+ NS_CStringGetData(aLine, &lineData); Use aLine.Data() instead. You should probably also assert that aLine is null terminated. See nsCSubstring::IsTerminated. sr=darin w/ those nits fixed

Actual version for trunk with nits fixed. Wasn't able to use aLine.Data() because it's not always terminated. Took the safer (and slower) option and just copy into a nsCAutoString.

on trunk

Comment on attachment 232211 [details] [diff] [review] version for bonecho a=drivers, please land on the branch

on branch