When we switched to wal we forced a bunch of async fsyncs to be sure nothing could go really wrong. I have not really seen any corruptions reports explosion, plus we have a good number of bookmarks backups, so I think it's a good time to try to remove those. This is the interesting query: http://mxr.mozilla.org/mozilla-central/search?string=ForceWALCheckpoint We should still keep the single forced fsync after schema creation, since failing there would create greater problems than we can handle. The problem is that bookmarks roots creation is part of this hot path, so the plan is to: - move bookmarks roots creation to Database.cpp, as part of the schema creation - remove all ForceWALCheckpoint calls but the one after schema creation From that point on, the only fsyncs will happen when the journal reaches 512KB.

This patch removes superfluous WAL checkpoints, but adds a checkpoint after bookmark roots initialization if the database is freshly created.

This is an alternate patch that explicitly initializes the bookmark roots in Database.cpp and then does a WAL checkpoint

Try run for b405cd34b6b1 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=b405cd34b6b1 Results (out of 13 total builds): exception: 13 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-b405cd34b6b1

Try run for 578de5b1aa60 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=578de5b1aa60 Results (out of 189 total builds): success: 159 warnings: 29 failure: 1 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-578de5b1aa60

Comment on attachment 576864 [details] [diff] [review] Removed WAL checkpoints, longer solution Review of attachment 576864 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/nsNavBookmarks.cpp @@ +277,5 @@ > > PRUint16 dbStatus = mDB->GetDatabaseStatus(); > + bool forceInit = ((dbStatus != nsINavHistoryService::DATABASE_STATUS_OK) && > + (dbStatus != nsINavHistoryService::DATABASE_STATUS_CREATE)); > + nsresult rv = InitRoots(forceInit); Why do you have to do this? Once Database.cpp takes care of roots creation, there should be no need for Bookmarks.cpp to keep doing that. It should just read the values (so never force init) and the creation code should be removed.

> Why do you have to do this? Once Database.cpp takes care of roots creation, > there should be no need for Bookmarks.cpp to keep doing that. It should just > read the values (so never force init) and the creation code should be > removed. I added that as extra protection against bugs causing inconsistencies between Database and nsNavBookmarks: - If the database is migrated, then Database::InitBookmarkRoots won't be called. If the migration function misses anything, then forcing initialization here would fix it: e.g. if a root title changes in the new version of StringBundle and the migration function neglects this, then InitRoots->CreateRoot would apply the new title 2) If Database::InitBookmarkRoots ever has a bug causing corruption, this would ensure the bookmark roots are consistent. I am probably being too careful -- I can take it out.

(In reply to Vladan Djeric (:vladan) from comment #6) > If the migration function misses anything, then forcing > initialization here would fix it Would be a bug we should have a test for, rather than a workaround. > if a root title changes in the new > version of StringBundle and the migration function neglects this, then > InitRoots->CreateRoot would apply the new title This is a valid point, I forgot on migration we try to fix titles, but you could handle that in Database. > 2) If Database::InitBookmarkRoots ever has a bug causing corruption, this > would ensure the bookmark roots are consistent. This would be a bug that tests would likely catch, while shadowing it they wouldn't So, I think it's worth to remove the code from bookmarks and handle the "update bookmarks roots titles on schema migration" in Database.

Applied mak's review

Comment on attachment 577804 [details] [diff] [review] Remove WAL checkpoints + initialize & migrate bookmark roots in Database.cpp Review of attachment 577804 [details] [diff] [review]: ----------------------------------------------------------------- Still some things to adjust ::: toolkit/components/places/Database.cpp @@ +633,5 @@ > // Firefox 8 uses schema version 12. > > // Schema Upgrades must add migration code here. > + > + // Update titles of bookmark roots global-nit: end comments with a period. Btw, in this case the comment is useless, since it's just repeating what the method name says already. @@ +635,5 @@ > // Schema Upgrades must add migration code here. > + > + // Update titles of bookmark roots > + rv = UpdateBookmarkRootTitles(); > + if (NS_FAILED(rv)) return rv; We don't want to return an error here, otherwise a broken localization may cause us to think the database is corrupt and we'd replace it without a reason. Instead just MOZ_ASSERT(NS_SUCCEEDED(rv)); and comment on what I said. @@ +716,5 @@ > rv = mMainConn->ExecuteSimpleSQL(CREATE_IDX_MOZ_ITEMSANNOS_PLACEATTRIBUTE); > NS_ENSURE_SUCCESS(rv, rv); > + > + // Initialize the bookmark roots in the new DB > + // -- NavBookmarksService will take care of roots in the migration case the second part is now false and should be removed. @@ +717,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > + > + // Initialize the bookmark roots in the new DB > + // -- NavBookmarksService will take care of roots in the migration case > + rv = InitBookmarkRoots(); Let's rename this to CreateBookmarkRoots @@ +744,5 @@ > + PRInt64 aParentId, > + PRInt32 aItemPosition, > + nsIStringBundle* aBundle, > + const PRUnichar* aTitleStringId, > + PRInt64 aTimestamp) let's rename to CreateRoot, if you pass into aDBConn you may even move this to an anonymous namespaced helper, it can largely be simplified up to that point. See below. @@ +746,5 @@ > + nsIStringBundle* aBundle, > + const PRUnichar* aTitleStringId, > + PRInt64 aTimestamp) > +{ > + // Next row ID to use for inserts into the moz_bookmarks table MOZ_ASSERT(NS_IsMainThread()); @@ +748,5 @@ > + PRInt64 aTimestamp) > +{ > + // Next row ID to use for inserts into the moz_bookmarks table > + static PRInt64 rowId = 0; > + ++rowId; id is an auto increment field, you should not need this at all. Instead you may do this for position, by having a static inited to 0, and incrementing it (at the end of the method) only if aRootName is not the main root. @@ +756,5 @@ > + nsXPIDLString title; > + if (aTitleStringId) { > + rv = aBundle->GetStringFromName(aTitleStringId, getter_Copies(title)); > + NS_ENSURE_SUCCESS(rv, rv); > + } Instead of passing into the bundle and conditioning here, wouldn't be simpler to directly pass in the title for sub-roots and EmptyCString() for the main root? @@ +759,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + } > + > + // Create a new bookmark folder for the root > + nsCOMPtr<mozIStorageStatement> stmt = GetStatement( Don't use GetStatement, or it will be cached. We just actually need this at startup and the cache doesn't have the concept of dropping cached statements after a certain time they are not used (that would be a nice improvement to file!)... just do mMainConn->CreateStatement for now. @@ +761,5 @@ > + > + // Create a new bookmark folder for the root > + nsCOMPtr<mozIStorageStatement> stmt = GetStatement( > + "INSERT INTO moz_bookmarks " > + "(id, type, parent, position, title, dateAdded, lastModified, guid) " don't insert an id, it will be auto-generated. @@ +765,5 @@ > + "(id, type, parent, position, title, dateAdded, lastModified, guid) " > + "VALUES (:item_id, :item_type, :parent, :item_index, :item_title," > + ":date_added, :last_modified, GENERATE_GUID())"); > + NS_ENSURE_STATE(stmt); > + mozStorageStatementScoper newFolderScoper(stmt); and if you use CreateStatement, no reason for the scoper. @@ +768,5 @@ > + NS_ENSURE_STATE(stmt); > + mozStorageStatementScoper newFolderScoper(stmt); > + > + rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), rowId); > + NS_ENSURE_SUCCESS(rv, rv); killme! @@ +786,5 @@ > + rv = stmt->Execute(); > + NS_ENSURE_SUCCESS(rv, rv); > + > + // Create an entry in moz_bookmarks_roots to link the folder to the root > + nsCOMPtr<mozIStorageStatement> newRootStmt = GetStatement( ditto on using createStatement @@ +790,5 @@ > + nsCOMPtr<mozIStorageStatement> newRootStmt = GetStatement( > + "INSERT INTO moz_bookmarks_roots (root_name, folder_id) " > + "VALUES (:root_name, :item_id)"); > + NS_ENSURE_STATE(newRootStmt); > + mozStorageStatementScoper newRootScoper(newRootStmt); and if you use CreateStatement, no reason for the scoper. @@ +795,5 @@ > + > + rv = newRootStmt->BindUTF8StringByName(NS_LITERAL_CSTRING("root_name"), > + aRootName); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = newRootStmt->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), rowId); rather than using rowId, use a subselect like: VALUES (:root_name, (SELECT id FROM moz_bookmarks WHERE parent = :root_parent AND position = :root_position))"); @@ +805,5 @@ > +} > + > +nsresult > +Database::InitBookmarkRoots() > +{ MOZ_ASSERT(NS_IsMainThread()); @@ +808,5 @@ > +Database::InitBookmarkRoots() > +{ > + nsCOMPtr<nsIStringBundleService> bundleService = > + services::GetStringBundleService(); > + NS_ENSURE_TRUE(bundleService, NS_ERROR_UNEXPECTED); NS_ENSURE_STATE(bundleService); @@ +811,5 @@ > + services::GetStringBundleService(); > + NS_ENSURE_TRUE(bundleService, NS_ERROR_UNEXPECTED); > + nsCOMPtr<nsIStringBundle> bundle; > + nsresult rv = bundleService->CreateBundle( > + "chrome://places/locale/places.properties", move this to a #define PLACES_BUNDLE please, then you can probably oneline this call @@ +813,5 @@ > + nsCOMPtr<nsIStringBundle> bundle; > + nsresult rv = bundleService->CreateBundle( > + "chrome://places/locale/places.properties", > + getter_AddRefs(bundle)); > + NS_ENSURE_SUCCESS(rv, nsnull); why nsnull? @@ +817,5 @@ > + NS_ENSURE_SUCCESS(rv, nsnull); > + > + // Use a single creation timestamp for all roots so that the parent folder's > + // last modification time isn't earlier than its childrens' creation time > + PRTime now = PR_Now(); Why would that be a problem? I'd probably not care about this... unless you consider this a small perf optimization? @@ +820,5 @@ > + // last modification time isn't earlier than its childrens' creation time > + PRTime now = PR_Now(); > + const PRInt64 rootId = 1; > + rv = InitRoot(NS_LITERAL_CSTRING("places"), 0, 0, nsnull, nsnull, now); > + NS_ENSURE_SUCCESS(rv, rv); Add an #ifdef DEBUG section that MOZ_ASSERTs the root has really been created at id 1.. Otherwise, there is an alternative where you could use a subselect like IFNULL((SELECT id FROM moz_bookmarks WHERE parent = 0), 0) as the INSERT parent value, and you would not care at all which id has the parent. So, finally, your initRoot method would just get the root name and the title, you wouldn't need anything else. @@ +842,5 @@ > + rv = InitRoot(NS_LITERAL_CSTRING("unfiled"), rootId, folderPosition, bundle, > + NS_LITERAL_STRING("UnsortedBookmarksFolderTitle").get(), now); > + NS_ENSURE_SUCCESS(rv, rv); > + > + return NS_OK; Before returning, add an #ifdef DEBUG section that MOZ_ASSERTs that the number of entries in moz_bookmarks equals the number of entries in moz_bookmarks_roots, should be easy as a SELECT (SELECT count(*) ...) = (SELECT count(*) ...) @@ +886,5 @@ > + NS_ENSURE_TRUE(bundleService, NS_ERROR_UNEXPECTED); > + > + nsCOMPtr<nsIStringBundle> bundle; > + nsresult rv = bundleService->CreateBundle( > + "chrome://places/locale/places.properties", ditto on the #define @@ +896,5 @@ > + "UPDATE moz_bookmarks SET title = :new_title WHERE id = " > + "(SELECT folder_id FROM moz_bookmarks_roots WHERE root_name = :root_name)" > + ), getter_AddRefs(stmt)); > + if (NS_FAILED(rv)) return rv; > + mozStorageStatementScoper newTitleScoper(stmt); the scoper is in the wrong place, you should put it at the beginning of the for loop, so that it automatically resets for you at each loop and you don't have to invoke Reset(). here it is useless. Btw, As I say below, I think we may use an async statement and a BindingParamsArray here. @@ +904,5 @@ > + "BookmarksMenuFolderTitle", "BookmarksToolbarFolderTitle", > + "TagsFolderTitle", "UnsortedBookmarksFolderTitle" > + }; > + > + for (int i = 0; i < sizeof(rootNames)/sizeof(const char *); i++) { Use ArrayLength(rootNames), it's new and shiny. @@ +916,5 @@ > + if (NS_FAILED(rv)) return rv; > + rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("root_name"), > + nsDependentCString(rootNames[i])); > + if (NS_FAILED(rv)) return rv; > + rv = stmt->Execute(); You could probably make this a mozIAsyncStorageStatement and use a BindingParamsArray to bind multiple values in one pass. Plus we don't need this to be synchronous on startup. @@ +918,5 @@ > + nsDependentCString(rootNames[i])); > + if (NS_FAILED(rv)) return rv; > + rv = stmt->Execute(); > + if (NS_FAILED(rv)) return rv; > + stmt->Reset(); missing error check ::: toolkit/components/places/nsNavBookmarks.cpp @@ +275,5 @@ > (void)os->AddObserver(this, TOPIC_PLACES_CONNECTION_CLOSED, true); > } > > PRUint16 dbStatus = mDB->GetDatabaseStatus(); > + nsresult rv = InitRoots(); the call to GetDatabaseStatus() can be removed @@ +336,5 @@ > > + if (NS_FAILED(rv)) return rv; > + > + if (!mRoot || !mMenuRoot || !mToolbarRoot || !mTagsRoot || !mUnfiledRoot) > + return NS_ERROR_FAILURE; I think this check is enough, so please remove the previous changes to rv = and if (NS_FAILED(rv)) return rv;, just keep this final check. ::: toolkit/components/places/nsNavBookmarks.h @@ +267,2 @@ > */ > + nsresult InitRoots(); Let's rename this to ReadRoots() to clarify it doesn't write anything anymore.

Applied mak's review above + results of IRC discussion with mak

Comment on attachment 579808 [details] [diff] [review] Remove WAL checkpoints + initialize & migrate bookmark roots in Database.cpp, v2 Review of attachment 579808 [details] [diff] [review]: ----------------------------------------------------------------- I couldn't have done better! Just in case, please get a Try run after addressing these comments, so we check the asserts you added on all platforms. ::: toolkit/components/places/Database.cpp @@ +304,5 @@ > + // Create an entry in moz_bookmarks_roots to link the folder to the root. > + nsCOMPtr<mozIStorageStatement> newRootStmt; > + rv = aDBConn->CreateStatement(NS_LITERAL_CSTRING( > + "INSERT INTO moz_bookmarks_roots (root_name, folder_id) " > + "VALUES (:root_name, (SELECT MAX(id) from moz_bookmarks))" While this will work in this specific case, I still prefer if you select by parent and position. May look like a crazy request, but using MAX(id) is not thread-safe (ideally another thread may insert between the insert and the select) and I don't want to give false assumptions to people looking at our code for examples. @@ +834,5 @@ > + services::GetStringBundleService(); > + NS_ENSURE_STATE(bundleService); > + nsCOMPtr<nsIStringBundle> bundle; > + nsresult rv; > + rv = bundleService->CreateBundle(PLACES_BUNDLE, getter_AddRefs(bundle)); declare rv on first use please @@ +838,5 @@ > + rv = bundleService->CreateBundle(PLACES_BUNDLE, getter_AddRefs(bundle)); > + if (NS_FAILED(rv)) return rv; > + > + // This is the next bookmark's position within its folder. > + static PRInt32 itemPosition = 0; Looks like you forgot to remove this? It's already declared in CreateRoot @@ +842,5 @@ > + static PRInt32 itemPosition = 0; > + > + nsXPIDLString rootTitle; > + rv = CreateRoot(mMainConn, NS_LITERAL_CSTRING("places"), rootTitle); > + if (NS_FAILED(rv)) return rv; for clarity, I prefer if you pass EmptyCString() and declare rootTitle later. It's the same but doesn't look like you forgot to init it, so I think may be less error-prone. @@ +844,5 @@ > + nsXPIDLString rootTitle; > + rv = CreateRoot(mMainConn, NS_LITERAL_CSTRING("places"), rootTitle); > + if (NS_FAILED(rv)) return rv; > + > + // Fetch the internationalized folder name fetched from the string bundle. s/fetched// since the comment starts with "Fetch" already @@ +869,5 @@ > + if (NS_FAILED(rv)) return rv; > + rv = CreateRoot(mMainConn, NS_LITERAL_CSTRING("unfiled"), rootTitle); > + if (NS_FAILED(rv)) return rv; > + > +#if DEBUG I was just looking into the fact we don't have any tests for moz_bookmarks_roots! So this debug test is really welcome! bravo! Another interesting test you may add here is getting SUM(position) and checking it's 6, so we also check positions are correct with a really simple equality check. @@ +933,5 @@ > + nsCOMPtr<nsIStringBundleService> bundleService = > + services::GetStringBundleService(); > + NS_ENSURE_STATE(bundleService); > + > + nsresult rv; please declare on first use @@ +941,5 @@ > + > + nsCOMPtr<mozIStorageAsyncStatement> stmt; > + rv = mMainConn->CreateAsyncStatement(NS_LITERAL_CSTRING( > + "UPDATE moz_bookmarks SET title = :new_title WHERE id = " > + "(SELECT folder_id FROM moz_bookmarks_roots WHERE root_name = :root_name)" please indent this once more @@ +955,5 @@ > + "BookmarksMenuFolderTitle", "BookmarksToolbarFolderTitle", > + "TagsFolderTitle", "UnsortedBookmarksFolderTitle" > + }; > + > + for (PRUint32 i = 0; i < ArrayLength(rootNames); i++) { nit: I prefer ++i, most compilers will do the right optimization, but just in case... ::: toolkit/components/places/nsNavBookmarks.cpp @@ +304,5 @@ > ), getter_AddRefs(stmt)); > NS_ENSURE_SUCCESS(rv, rv); > > bool hasResult; > + while (NS_SUCCEEDED(rv = stmt->ExecuteStep(&hasResult)) && hasResult) { this change is not needed and just a leftover (please remove "rv = ")

> @@ +941,5 @@ > > + > > + nsCOMPtr<mozIStorageAsyncStatement> stmt; > > + rv = mMainConn->CreateAsyncStatement(NS_LITERAL_CSTRING( > > + "UPDATE moz_bookmarks SET title = :new_title WHERE id = " > > + "(SELECT folder_id FROM moz_bookmarks_roots WHERE root_name = :root_name)" > > please indent this once more I mean only the subquery here, not the UPDATE

> > + nsXPIDLString rootTitle; > > + rv = CreateRoot(mMainConn, NS_LITERAL_CSTRING("places"), rootTitle); > > + if (NS_FAILED(rv)) return rv; > > for clarity, I prefer if you pass EmptyCString() and declare rootTitle > later. It's the same but doesn't look like you forgot to init it, so I think > may be less error-prone. EmptyCString is not equivalent to XPIDLString and XPIDLStrings only take another XPIDLString in their constructor :( I added a comment explaining the intent.

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

Try run for 388dba74a0f9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=388dba74a0f9 Results (out of 207 total builds): exception: 3 success: 160 warnings: 39 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-388dba74a0f9

(In reply to Vladan Djeric (:vladan) from comment #13) > EmptyCString is not equivalent to XPIDLString and XPIDLStrings only take > another XPIDLString in their constructor :( I added a comment explaining the > intent. Ah, :( OK!

The test failure you got in test_analyze.js makes sense, previously we were expecting an empty table, while not bookmarks contain something. Just update the comment in the top of the file specifying the database contains bookmarks roots and flip the do_check_false to do_check_true

Comment on attachment 580590 [details] [diff] [review] Remove WAL checkpoints + initialize & migrate bookmark roots in Database.cpp, v3 Review of attachment 580590 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/Database.cpp @@ +308,5 @@ > + "INSERT INTO moz_bookmarks_roots (root_name, folder_id) " > + "VALUES (:root_name, " > + "(SELECT MAX(id) from moz_bookmarks WHERE " > + " position = :item_position AND " > + " parent IN (0, (SELECT MIN(id) FROM moz_bookmarks))))" hm, this query looks strange. why do you need MAX(id) there is only one id at a certain parent and position. And the parent query may be the same as in the above query: IFNULL((SELECT id FROM moz_bookmarks WHERE parent = 0), 0)) So to me looks like it may be (SELECT id FROM moz_bookmarks WHERE position = : item_position AND parent = IFNULL((SELECT id FROM moz_bookmarks WHERE parent = 0), 0) )

(In reply to Marco Bonardo [:mak] from comment #33) > hm, this query looks strange. > why do you need MAX(id) there is only one id at a certain parent and > position. Both the root (id=1,parent=0,pos=0) and the first "real" folder (id=2,parent=1,pos=0) are in position 0. The "parent IN (...)" clause does not distinguish between the two, so that's where the MAX comes in. > And the parent query may be the same as in the above query: IFNULL((SELECT > id FROM moz_bookmarks WHERE parent = 0), 0)) > > So to me looks like it may be > (SELECT id FROM moz_bookmarks > WHERE position = : item_position > AND parent = IFNULL((SELECT id FROM moz_bookmarks WHERE parent = 0), 0) > ) This actually wouldn't work. When we execute this statement the very first time, there actually is one row in moz_bookmarks (id=1,parent=0,pos=0) so when this clause is executed it would return NULL because there is no record with position=0 and parent=1. If it's mainly a question of aesthetics, I could do the following instead: > "INSERT INTO moz_bookmarks_roots (root_name, folder_id) " > "VALUES (:root_name, " > "(SELECT id from moz_bookmarks WHERE " > " position = :item_position AND " > " parent = IFNULL((SELECT MIN(folder_id) FROM moz_bookmarks_roots), 0)))"

(In reply to Vladan Djeric (:vladan) from comment #34) > This actually wouldn't work. When we execute this statement the very first > time, there actually is one row in moz_bookmarks (id=1,parent=0,pos=0) so > when this clause is executed it would return NULL because there is no record > with position=0 and parent=1. hrm, right! > If it's mainly a question of aesthetics, I could do the following instead: > > "INSERT INTO moz_bookmarks_roots (root_name, folder_id) " > > "VALUES (:root_name, " > > "(SELECT id from moz_bookmarks WHERE " > > " position = :item_position AND " > > " parent = IFNULL((SELECT MIN(folder_id) FROM moz_bookmarks_roots), 0)))" Well, it's not an aesthetic problem, the original query using all of MAX, IN and MIN looks fragile, I would like to reduce those. I don't have better ideas atm, so let's use your last suggestion.

Update SQL query + failing test case

Comment on attachment 581314 [details] [diff] [review] Remove WAL checkpoints + initialize & migrate bookmark roots in Database.cpp, v4 I only reviewed the diff, looks good.