See Bug 641617. Removing local IDs from all Places URIs (we already rewrite tag URIs "the hard way") will remove some of the pain from bookmark sync. If this can get done before we rev the bookmarks engine for Sync 2.0 (Jan/Feb), that would be really nice.

Taking. The plan is to replace the existing place:folder=<id> / place:folder=TOOLBAR_FOLDER type queries with place:parent=<GUID>. We'll do migration for existing queries as well.

Mark/Kit: How should we handle Sync here? Ideally, I'd like to do a complete migration from place:folder=<id> to place:parent=<guid> on the desktop side. Obviously, not all Sync clients will necessarily be updated. Marco suggested for incoming syncs, we could translate any remaining place:folder= to place:parent=. For outgoing syncs, we could just post the place:parent= and assume older clients would be temporarily broken. Our suspicions are that there's not many people using these, though I think folder shortcuts can be accidentally created by dragging & dropping the built-in root folders. Thoughts?

(In reply to Mark Banner (:standard8) from comment #4) > Marco suggested for incoming syncs, we could translate any remaining > place:folder= to place:parent=. For outgoing syncs, we could just post the > place:parent= and assume older clients would be temporarily broken. Hmm, it's tricky to do this on the incoming side; I don't think we can know which ID the query meant to point to. Sync doesn't do anything special for `folder={id}` queries today; they're synced as-is...so a query on one client synced to another might end up pointing to a completely different folder, or, most likely, just break. Unlike tag queries, there's no `folderGuid` property in the record. With that in mind, I think it's OK if we sync `parent={guid}` queries and break older Firefoxes...they won't work until that client upgrades to 62, but they're already broken today, anyway. As you say, I think this could just be done on the outgoing side, by bumping the change counter when we rewrite the URL. If multiple clients upgrade before syncing, the first one to sync will win and upload new records, and conflict resolution logic means others won't reupload when they sync.

(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #5) > Sync doesn't do anything special for `folder={id}` queries today; they're > synced as-is...so a query on one client synced to another might end up > pointing to a completely different folder, or, most likely, just break. > Unlike tag queries, there's no `folderGuid` property in the record. Ah, I thought we were rewriting folder=id when getting the folder in question, but indeed it sounds too complex. So what are we supposed to do, just keep them as is? If by breaking they end up selecting EVERYTHING, for perf reasons it may be better to make them select nothing instead. Maybe add an excludeItems=1 to them?

(In reply to Marco Bonardo [::mak] from comment #15) > Maybe add an excludeItems=1 to them? That sounds good to me

Comment on attachment 8975851 [details] Bug 824502 - When Sync receives incoming queries with folder= in them, add excludeItems=1 to the end to avoid broken queries loading the whole database. https://reviewboard.mozilla.org/r/244048/#review249984 Looks good, thanks! ::: toolkit/components/places/SyncedBookmarksMirror.jsm:756 (Diff revision 1) > + // For any other legacy queries with a "folder" parameter, we are not able to > + // know which folder it should point to. Therefore, add excludeItems onto the > + // end, so that it doesn't return everything in the database. > + let folder = params.get("folder"); > + if (folder) { > + url.href = `${url.href}&excludeItems=1`; Do you think it's worth deleting the `folder` param, too, or should we just leave it in the query?

Comment on attachment 8975851 [details] Bug 824502 - When Sync receives incoming queries with folder= in them, add excludeItems=1 to the end to avoid broken queries loading the whole database. https://reviewboard.mozilla.org/r/244048/#review249984 > Do you think it's worth deleting the `folder` param, too, or should we just leave it in the query? I think it doesn't hurt to leave it in there - it'll be ignored, and if we ever get reports of these not working, it'll be clearer as to what has happened.

Comment on attachment 8974428 [details] Bug 824502 - Change folder=<id> to parent=<guid> in Places' bookmark queries. https://reviewboard.mozilla.org/r/242766/#review251002 ::: browser/base/content/browser-places.js:1345 (Diff revision 4) > attachPlacesView(event, node) { > // If the view is already there, bail out early. > if (node.parentNode._placesView) > return; > > - new PlacesMenu(event, "place:folder=BOOKMARKS_MENU", { > + new PlacesMenu(event, `place:parent=${PlacesUtils.bookmarks.menuGuid}`, { why the mixture, in some places you use the guid directly, here you use the property. We should probably just be consistent one side. ::: browser/components/places/content/places.js:756 (Diff revision 4) > * Folders to include when searching. > */ > _folders: [], > get folders() { > if (this._folders.length == 0) { > - this._folders.push(PlacesUtils.bookmarksMenuFolderId, > + this._folders.push(PlacesUtils.bookmarks.userContentRoots); shouldn't this use spread or =? ::: toolkit/components/places/nsNavBookmarks.h:188 (Diff revision 4) > + * GUID of the item to fetch information for. > + * @param aBookmark > + * BookmarkData to store the information. > + */ > + nsresult FetchItemInfo(const nsCString &aGUID, > + BookmarkData& _bookmark); nit: indentation ::: toolkit/components/places/nsNavBookmarks.cpp:2170 (Diff revision 4) > + // Fetch missing data. > + rv = FetchItemInfo(queryObj->Parents()[0], changeData.bookmark); > + NS_ENSURE_SUCCESS(rv, rv); > + NotifyItemChanged(changeData); > + } > + if (queryObj->Parents().Length() == 1) { I don't understand, these 2 ifs are doing the same thing? ::: toolkit/components/places/nsNavHistory.cpp (Diff revision 4) > // If this failed the shortcut is pointing to nowhere, let the error pass > // and handle it later. > if (NS_SUCCEEDED(rv)) { > // At this point the node is set up like a regular folder node. Here > // we make the necessary change to make it a folder shortcut. > - resultNode->GetAsFolder()->mTargetFolderItemId = targetFolderId; So, we are not setting anymore mTargetFolderItemId, are you removing support for it, or will this just be broken? I didn't follow all the callpoints but it appears to be used yet in results. ::: toolkit/components/places/nsNavHistoryQuery.cpp:113 (Diff revision 4) > #define QUERYKEY_MIN_VISITS "minVisits" > #define QUERYKEY_MAX_VISITS "maxVisits" > #define QUERYKEY_ONLY_BOOKMARKED "onlyBookmarked" > #define QUERYKEY_DOMAIN_IS_HOST "domainIsHost" > #define QUERYKEY_DOMAIN "domain" > #define QUERYKEY_FOLDER "folder" Is this removed in one of the next changesets?

Comment on attachment 8974429 [details] Bug 824502 - Migrate folder=<id> based queries to parent=<guid> for Places' bookmark queries. https://reviewboard.mozilla.org/r/242768/#review251018 ::: toolkit/components/places/Database.cpp:2457 (Diff revision 4) > + "WHERE url_hash BETWEEN hash('place', 'prefix_lo') AND " > + "hash('place', 'prefix_hi') " > + "AND url LIKE '%folder=%' " > + ), getter_AddRefs(stmt)); > + if (NS_FAILED(rv)) return rv; > + mozStorageStatementScoper scoper(stmt); We should not need scopers here, probably there are other misuses around. The scoper is useful when the same statement is cached and reused multiple times, that is not our case. Here it will just go out of scope and be finalized. ::: toolkit/components/places/Database.cpp:2486 (Diff revision 4) > + for (uint32_t i = 0; i < placeURLs.Length(); ++i) { > + itemId = placeURLs[i].first(); > + url = placeURLs[i].second(); > + > + rv = ConvertOldStyleQuery(url); > + if (NS_FAILED(rv)) return rv; If the query is corrupt and not parseable properly, won't this consider the whole database corrupt and throw it away? We should ignore invalid queries, not throw away the db. I'd suggest to issue a warning and continue. plus a test may be nice. ::: toolkit/components/places/Database.cpp:2497 (Diff revision 4) > + "WHERE id = :itemId " > + ), getter_AddRefs(updateStmt)); > + if (NS_FAILED(rv)) return rv; > + mozStorageStatementScoper scoper(updateStmt); > + > + rv = updateStmt->BindUTF8StringByName(NS_LITERAL_CSTRING("url"), url); We usually use URIBinder::Bind() for binding urls ::: toolkit/components/places/Database.cpp:2509 (Diff revision 4) > + > + // Update Sync fields for these queries. > + nsCOMPtr<mozIStorageStatement> syncStmt; > + rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING( > + "UPDATE moz_bookmarks SET syncChangeCounter = syncChangeCounter + 1 " > + "WHERE fk = :itemId " There's something wrong here, fk points to a place id, not an itemId You'd better just bump the counter for any place: query containing "parent=" ::: toolkit/components/places/Database.cpp:2529 (Diff revision 4) > + > +nsresult > +Database::ConvertOldStyleQuery(nsCString& aURL) > +{ > + AutoTArray<QueryKeyValuePair, 8> tokens; > + nsresult rv = TokenizeQueryString(aURL, &tokens); I suspect we could use the new GetQueryParamIterator and URLParams::Parse in TokenizeQueryString... Maybe a code cleaning follow-up. As well as URLParams::Set could be used instead of tokenizing and rebuilding from scratch. Though, I think it still doesn't support multiple params having the same name. ::: toolkit/components/places/Database.cpp:2547 (Diff revision 4) > + return NS_ERROR_OUT_OF_MEMORY; > + } > + continue; > + } > + > + nsresult rv2; doesn't look like a separate rv is really useful here... as well as shadowing rv inside the if. it's not used to loop or after the loop.

Comment on attachment 8974430 [details] Bug 824502 - Fix unit tests for the folder=<id> to parent=<guid> transition. https://reviewboard.mozilla.org/r/242770/#review251034 PlacesUtils, nsBrowserGlue and selectBookmark.js have calls to getFolderContents... are those handles elsewhere? ::: browser/components/places/tests/browser/browser_library_infoBox.js:79 (Diff revision 4) > > + const folders = [ > + PlacesUtils.bookmarks.menuGuid, > + PlacesUtils.bookmarks.unfiledGuid, > + PlacesUtils.bookmarks.toolbarGuid > + ]; can we simplify with userContentRoots here? ::: toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js:139 (Diff revision 4) > > add_task(async function check_bookmarks_query() { > - let options = PlacesUtils.history.getNewQueryOptions(); > - let query = PlacesUtils.history.getNewQuery(); > - query.setFolders([PlacesUtils.bookmarksMenuFolderId], 1); > - let result = PlacesUtils.history.executeQuery(query, options); > + var options = PlacesUtils.history.getNewQueryOptions(); > + var query = PlacesUtils.history.getNewQuery(); > + query.setParents([PlacesUtils.bookmarks.menuGuid], 1); > + var result = PlacesUtils.history.executeQuery(query, options); why the change from let to var?

Comment on attachment 8974428 [details] Bug 824502 - Change folder=<id> to parent=<guid> in Places' bookmark queries. https://reviewboard.mozilla.org/r/242766/#review251002 > why the mixture, in some places you use the guid directly, here you use the property. We should probably just be consistent one side. I was trying to avoid hitting PlacesUtils earlier than necessary on startup. I've change the xul/inc files to use PlacesUtils.bookmarks, but the one at the start of PlacesToolbarHelper I've left as it is, with a comment. > I don't understand, these 2 ifs are doing the same thing? I suspect this was a bad merge or something. > So, we are not setting anymore mTargetFolderItemId, are you removing support for it, or will this just be broken? I didn't follow all the callpoints but it appears to be used yet in results. Aha, I remember now. ResultNodeForContainer (called a few lines above), calls FetchItemInfo (which also gets the id), then calls nsNavHistoryFolderResultNode passing the Id, and that sets mTargetFolderItemId. So this extra line is actually unnecessary work. https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/toolkit/components/places/nsNavBookmarks.cpp#1512-1514 https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/toolkit/components/places/nsNavHistoryResult.cpp#3081,3083,3088

Comment on attachment 8974429 [details] Bug 824502 - Migrate folder=<id> based queries to parent=<guid> for Places' bookmark queries. https://reviewboard.mozilla.org/r/242768/#review251018 > If the query is corrupt and not parseable properly, won't this consider the whole database corrupt and throw it away? > We should ignore invalid queries, not throw away the db. > I'd suggest to issue a warning and continue. plus a test may be nice. I'm not sure we can easily test this - I think all the routes would be out-of-memory or other such errors. Unless there's a way to insert a GUID which would cause GetUTF8String to fail. I'll make sure it tries to continue though. > I suspect we could use the new > GetQueryParamIterator and URLParams::Parse in TokenizeQueryString... Maybe a code cleaning follow-up. > As well as URLParams::Set could be used instead of tokenizing and rebuilding from scratch. > Though, I think it still doesn't support multiple params having the same name. Filed bug 1463429.

(In reply to Mark Banner (:standard8) from comment #27) > > If the query is corrupt and not parseable properly, won't this consider the whole database corrupt and throw it away? > > We should ignore invalid queries, not throw away the db. > > I'd suggest to issue a warning and continue. plus a test may be nice. > > I'm not sure we can easily test this - I think all the routes would be > out-of-memory or other such errors. Unless there's a way to insert a GUID > which would cause GetUTF8String to fail. I'll make sure it tries to continue > though. The suggested test was just for migrating an invalid query, that doesn't parse at all. Do we ever fail on that?

(In reply to Marco Bonardo [::mak] from comment #28) > (In reply to Mark Banner (:standard8) from comment #27) > > > If the query is corrupt and not parseable properly, won't this consider the whole database corrupt and throw it away? > > > We should ignore invalid queries, not throw away the db. > > > I'd suggest to issue a warning and continue. plus a test may be nice. > > > > I'm not sure we can easily test this - I think all the routes would be > > out-of-memory or other such errors. Unless there's a way to insert a GUID > > which would cause GetUTF8String to fail. I'll make sure it tries to continue > > though. > > The suggested test was just for migrating an invalid query, that doesn't > parse at all. Do we ever fail on that? No, it looks like TokenizeQueryString can only fail due to out of memory: https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/toolkit/components/places/nsNavHistoryQuery.cpp#508 If the folder ID can't be parsed as an integer, then it falls back to comparing with one of the pre-defined strings, and ends up with potentially assessing it as invalid.

Comment on attachment 8974428 [details] Bug 824502 - Change folder=<id> to parent=<guid> in Places' bookmark queries. https://reviewboard.mozilla.org/r/242766/#review252552 ::: browser/base/content/browser-places.js:1010 (Diff revision 5) > * toolbar. > */ > var PlacesToolbarHelper = { > - _place: "place:folder=TOOLBAR", > + // We should use PlacesUtils.bookmarks.toolbarGuid here, but we don't to avoid > + // potentially loading PlacesUtils earlier than we need to. > + _place: "place:parent=toolbar_____", In the menu helper just below we just hardcoded the place uri in the new PlacesMenu call, couldn't we just do the same here? new PlacesToolbar(this._place); is the only consumer afaict, so we could just do the same as for new PlacesMenu and use the PU.bookmarks.toolbarGuid. Once the toolbar is visible it's very likely PU is being loaded. ::: toolkit/components/places/nsNavHistoryQuery.cpp:640 (Diff revision 5) > NS_WARNING(kvp.key.get()); > } > } > > - if (folders.Length() != 0) > - aQuery->SetFolders(folders.Elements(), folders.Length()); > + if (parents.Length() != 0) > + aQuery->Parents() = parents; Please use SetParents. I know this is tempting, but Folders() was intended as a getter, and it's const, so I prefer to go the usual path here

Comment on attachment 8974429 [details] Bug 824502 - Migrate folder=<id> based queries to parent=<guid> for Places' bookmark queries. https://reviewboard.mozilla.org/r/242768/#review252580 ::: toolkit/components/places/Database.cpp:1303 (Diff revision 5) > + if (currentSchemaVersion < 50) { > + rv = MigrateV50Up(); > + NS_ENSURE_SUCCESS(rv, rv); > + } > + > // Firefox 62 uses schema version 49. The comment should be updated ::: toolkit/components/places/Database.cpp:2520 (Diff revision 5) > + while (NS_SUCCEEDED(stmt->ExecuteStep(&hasMore)) && hasMore) { > + int64_t placeId; > + rv = stmt->GetInt64(0, &placeId); > + if (NS_FAILED(rv)) return rv; > + > + rv = stmt->GetUTF8String(1, url); nit: remove the above newline ::: toolkit/components/places/Database.cpp:2592 (Diff revision 5) > + nsAutoCString guid; > + > + for (uint32_t j = 0; j < tokens.Length(); ++j) { > + const QueryKeyValuePair& kvp = tokens[j]; > + > + // begin time copypaste leftover? ::: toolkit/components/places/Helpers.h:180 (Diff revision 5) > PRTime RoundedPRNow(); > > nsresult HashURL(const nsAString& aSpec, const nsACString& aMode, > uint64_t *_hash); > > +class QueryKeyValuePair final

Comment on attachment 8974430 [details] Bug 824502 - Fix unit tests for the folder=<id> to parent=<guid> transition. https://reviewboard.mozilla.org/r/242770/#review252816 ::: browser/components/places/tests/browser/browser_library_infoBox.js:78 (Diff revision 5) > checkAddInfoFieldsCollapsed(PO); > > // open recently bookmarked node > await PlacesUtils.bookmarks.insert({ > parentGuid: PlacesUtils.bookmarks.menuGuid, > - url: "place:folder=BOOKMARKS_MENU&folder=UNFILED_BOOKMARKS&folder=TOOLBAR" + > + url: "place:" + PlacesUtils.bookmarks.userContentRoots.map(guid => `parent=${guid}`) + How can this work if you don't join on "&"?

Comment on attachment 8974430 [details] Bug 824502 - Fix unit tests for the folder=<id> to parent=<guid> transition. https://reviewboard.mozilla.org/r/242770/#review252816 > How can this work if you don't join on "&"? It looks like it just failed to parse any folder details in the query, and hence just searched through all bookmarks, which is pretty much what this is doing anyway.

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f7f1cabf68ef Change folder=<id> to parent=<guid> in Places' bookmark queries. r=mak https://hg.mozilla.org/integration/autoland/rev/bb011366782b Migrate folder=<id> based queries to parent=<guid> for Places' bookmark queries. r=mak https://hg.mozilla.org/integration/autoland/rev/42dde688c6f2 Fix unit tests for the folder=<id> to parent=<guid> transition. r=mak https://hg.mozilla.org/integration/autoland/rev/8a4d896b435b When Sync receives incoming queries with folder= in them, add excludeItems=1 to the end to avoid broken queries loading the whole database. r=kitcambridge

https://hg.mozilla.org/mozilla-central/rev/f7f1cabf68ef https://hg.mozilla.org/mozilla-central/rev/bb011366782b https://hg.mozilla.org/mozilla-central/rev/42dde688c6f2 https://hg.mozilla.org/mozilla-central/rev/8a4d896b435b