User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/603.1.23 (KHTML, like Gecko) Version/10.1 Safari/603.1.23 Steps to reproduce: • Craft a WebExtension which, upon loading, invokes browser.bookmarks.getRecent() with a parameter of "3", meaning that it should return the 3 most recently-bookmarked bookmarks. The API https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/bookmarks/getRecent is clear that this function "retrieves a specified number of the most recently added **bookmarks**" [emphasis added]. • Run Firefox 51, or Firefox 54 Nightly 2017-02-10. • Add three new bookmarks. • Add a tag to the last new bookmark that you just added. • Load the extension. • Look at the console and see what was retrieved by getRecent(). Actual results: An array of 3 objects is returned as expected, but only two are the newest bookmarks that you added. The third is an object with no URL whose title is the name of the tag you added – it's the tag! Expected results: It should have got all *three* new bookmarks with no mention of any tags. * * * Apparently, this issue was realized during the development of Bug 1251269, because in toolkit/components/places/Bookmarks.jsm, in function fetchRecentBookmarks(numberOfItems), line 1198, the query contains this clause: WHERE p.parent <> :tags_folder which is obviously intended to exclude tags from the result. But it's not working for some reason.

I have been able to reproduce this, and I've looked at the code and I'm not sure why it's not working as expected. Marco, could you please take a look at the code for `fetchRecentBookmarks` in Bookmarks.jsm [1] to see if you can tell why it is including tags, when I have attempted to code it to exclude them, using similar logic to `fetchBookmarksByURL` [2], for example. [1] http://searchfox.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#1186 [2] http://searchfox.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#1162

off-hand looks like the query is also returning folders and separators. I don't know if that's intended. the tags that are being returned seems to be the tag folders? I don't know if the API is intended to return folders, if not it should AND type = BOOKMARK, otherwise it should _also_ AND b.parent <> :tags_folder

Hello, Marco. Regarding folders and separators: The API documentation https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/bookmarks/getRecent specifies that this function shall return "most recently added bookmarks". It would be nice if that documentation were more explicit, but I'd say that, in this context, that means it should *not* include folders, nor separators. Regarding "tag folders": Yes, tags are in fact represented in moz_bookmark as folders, that is, rows whose `type` column is TYPE_FOLDER=2. And this is indeed what I see being returned. Regarding the WHERE clause in the query: I tried adding the type = BOOKMARK clause you suggested. That is, I changed line 1198 to WHERE p.parent <> :tags_folder AND b.type = :type_bookmark and defined type_bookmark by changing line 1201 to `, { tags_folder: PlacesUtils.tagsFolderId, type_bookmark: Bookmarks.TYPE_BOOKMARK, numberOfItems }); With these changes, the result no longer contains those "tag folders". Good! However, the getRecent() result still contains the hidden smart folders which descend from that "All Bookmarks" folder. As I recall from previous work, a query which will exclude all of this garbage requires also checking parent id against some of these.

Thanks for the bug report, Jerry, and your additional observations. I have finished a patch which I believe fixes the issue entirely. It is now attached to this bug. If you want to give it a try and see if it fixes all of the issues you are seeing that would be great.

Comment on attachment 8842986 [details] Bug 1341541 - WebExtensions bookmarks.getRecent() results include raw tag objects https://reviewboard.mozilla.org/r/116716/#review118470 Test changes look reasonable, but I am not very familiar with the places db stuff so expect other review for that.

Thank you, Bob. I have tested your patch and indeed you have found and fixed this bug – no more tags. However the six garbage hidden smart folders, or whatever they are, still appear in getRecent() results. These six items apparently serve some internal purpose in Firefox, do not appear in the user interface, and therefore should not appear in getRecent() results. I tested using a small set of bookmarks – the default bookmarks in a new Firefox profile, plus one bookmark that I added with two tags. My places.sqlite dumps something like this: ? id guid title type fk parent position - -- ------------ ------------------------- ---- ---- ------ -------- z 1 root________ 2 NULL 0 0 z 2 menu________ Bookmarks Menu 2 NULL 1 0 z 3 toolbar_____ Bookmarks Toolbar 2 NULL 1 1 z 4 tags________ Tags 2 NULL 1 2 z 5 unfiled_____ Other Bookmarks 2 NULL 1 3 z 6 mobile______ mobile 2 NULL 1 4 + 7 Hj322gIlkrsS Get Involved 1 1 3 1 z 8 QwNSEkp1Cm2o Firefox Nightly Resources 2 NULL 2 2 + 9 3GkHrh41MXtd Firefox Nightly blog 1 2 8 0 + 10 MHz792dMnaZf Mozilla Bug Tracker 1 3 8 1 + 14 4NIi92Z5p5KW Discuss Nightly on IRC 1 7 8 2 + 16 qKZbOcbVSaox Most Visited 1 9 3 0 + 17 59-dsKmX0Oom Recent Tags 1 10 2 0 z 18 5M86We-KwYo9 NULL 3 NULL 2 1 z 19 o3tTQpjze_7L 2 NULL 1 5 - 20 eLqTj_9cgyNd History 1 11 19 0 - 21 LGeGmVaqsQOy Downloads 1 12 19 1 - 22 czH7f87fqVlU Tags 1 13 19 2 z 23 VKM92jydd9Fr All Bookmarks 2 NULL 19 3 - 24 6-D-HYpyJGBx NULL 1 14 23 0 - 25 Gc-5kOU4jyPs NULL 1 15 23 1 - 26 JniVnF2ObIAy NULL 1 16 23 2 + 27 XN_fxhuJXu4_ My Bookmark 1 18 5 0 z 40 wM9iVfdd9ALL MyTag1 2 NULL 4 0 z 41 sFNiptGY0IFf NULL 1 18 40 0 z 42 Fu73YHel9Esk MyTag2 2 NULL 4 1 z 43 QAxAieGQM-eJ NULL 1 18 42 0 where I added the first "?" column manually. Here is what that column means: z = should not and does not appear in getRecent() results. Good. + = should and does appear in getRecent()results. Good. - = should not but does appear in getRecent() results. Bad. I was able to get rid of the six undesired "-" items by adding two more lines to Bookmarks.jsm: 1200 AND b.parent <> :all_bookmarks_folder 1201 AND b.parent <> :shadow_root However I don't know the constants (if there even are any) for those "All Bookmarks" and (what I call the) "shadow root" items. You can see them at rows 23 and 19 in my places.sqlite dump. To do a proof of concept, I cheated and hard-coded their id, so my line 1205 is now this: `, { tags_folder: PlacesUtils.tagsFolderId, all_bookmarks_folder:23, shadow_root:19, type: Bookmarks.TYPE_BOOKMARK, numberOfItems }); and voila now getRecent() works all as expected. Do you want to deal with that in this bug, or should I copy and paste the above information into a new bug?

Thanks for the info, Jerry. I think we can keep this in this bug, and just treat this bug as trying to get getRecent() working properly. Marco, can you please take a look at Jerry's comments in comment #7 and let me know what you think? I'm still not that familiar with the places database, so I'm not sure what these extraneous items are, or whether Jerry's approach to eliminating them from the results makes sense.

Those are "smart" queries, things like "place:folder=XYZ" or such. I agree we may not want these. To exclude those you should add to the query: AND url_hash NOT BETWEEN hash("place", "prefix_lo") AND hash("place", "prefix_hi") Let me clarify what this does. It would potentially be enough to check that url doesn't begin with "place:", but for space reasons we dropped the url index and instead added an url hash. The problem is that an index on the url hash doesn't allow to quickly match schemas, and for some queries we needed that. So the hash has been built as [ 16bit schema hash + 32bit url hash). The hash("place", "prefix_lo") builds a [ 16bit "place" hash + 0x00] while hash("place", "prefix_hi") builds [ 16bit "place" hash + 0xFF]. The between thus allows to select or exclude hashes for urls like place:XXX.

Please, update the javadoc to clarify the API excludes folders, separators and queries, and add a test for those to toolkit/components/places/tests/bookmarks (where there's already the existing getRecent test)

Thanks Marco. This is ready for another review.

Comment on attachment 8842986 [details] Bug 1341541 - WebExtensions bookmarks.getRecent() results include raw tag objects https://reviewboard.mozilla.org/r/116716/#review119240 ::: toolkit/components/places/Bookmarks.jsm:1200 (Diff revision 2) > NULL AS _syncStatus > FROM moz_bookmarks b > LEFT JOIN moz_bookmarks p ON p.id = b.parent > LEFT JOIN moz_places h ON h.id = b.fk > WHERE p.parent <> :tags_folder > + AND b.parent <> :tags_folder Checking b.parent is no more needed if you already check type = BOOKMARK, since :tags_folder can only contain folders. ::: toolkit/components/places/Bookmarks.jsm:1203 (Diff revision 2) > LEFT JOIN moz_places h ON h.id = b.fk > WHERE p.parent <> :tags_folder > + AND b.parent <> :tags_folder > + AND b.type = :type > + AND url_hash NOT BETWEEN hash("place", "prefix_lo") > + AND hash("place", "prefix_hi") nit: align the 2 "hash" calls, adding more indentation before AND ::: toolkit/components/places/Bookmarks.jsm:1206 (Diff revision 2) > + AND b.type = :type > + AND url_hash NOT BETWEEN hash("place", "prefix_lo") > + AND hash("place", "prefix_hi") > ORDER BY b.dateAdded DESC, b.ROWID DESC > LIMIT :numberOfItems > - `, { tags_folder: PlacesUtils.tagsFolderId, numberOfItems }); > + `, { tags_folder: PlacesUtils.tagsFolderId, type: Bookmarks.TYPE_BOOKMARK, numberOfItems }); nit: this should go multi-line for the 80-chars soft limit of the coding style. ::: toolkit/components/places/tests/bookmarks/test_bookmarks_getRecent.js:42 (Diff revision 2) > + // Add a query bookmark. > + let queryURL = `place:folder=${PlacesUtils.bookmarksMenuFolderId}&queryType=1`; > + let bm5 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, > + url: queryURL, > + title: "a test query" }); > + checkBookmarkObject(bm5); I'd also add a separator to the test, just in case.

Pushed by bsilverberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/353df4a8b88c WebExtensions bookmarks.getRecent() results include raw tag objects, r=mak,mixedpuppy