1341541 - WebExtensions bookmarks.getRecent() results include raw tag objects

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Jerry Krinock]

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.

Comment 1

[Bob Silverberg [:bsilverberg]]

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

Comment 2

[Marco Bonardo [:mak]]

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

Comment 3

[Jerry Krinock]

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.

Comment 4

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1341541 - WebExtensions bookmarks.getRecent() results include raw tag objects

Review commit: https://reviewboard.mozilla.org/r/116716/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116716/

Comment 5

[Bob Silverberg [:bsilverberg]]

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 6

[Shane Caraveo (:mixedpuppy)]

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.

Comment 7

[Jerry Krinock]

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?

Comment 8

[Bob Silverberg [:bsilverberg]]

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.

Comment 9

[Marco Bonardo [:mak]]

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.

Comment 10

[Marco Bonardo [:mak]]

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)

Comment 11

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8842986 [details]
Bug 1341541 - WebExtensions bookmarks.getRecent() results include raw tag objects

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116716/diff/1-2/

Comment 12

[Bob Silverberg [:bsilverberg]]

Thanks Marco. This is ready for another review.

Comment 13

[Marco Bonardo [:mak]]

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.

Comment 14

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8842986 [details]
Bug 1341541 - WebExtensions bookmarks.getRecent() results include raw tag objects

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116716/diff/2-3/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/353df4a8b88c
WebExtensions bookmarks.getRecent() results include raw tag objects, r=mak,mixedpuppy

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/353df4a8b88c