1345417 - Firefox Sync should sync POST_DATA of bookmarks

Status: NEW

(Firefox :: Sync, defect, P3)

Description

[Polzin]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20161025170357

Steps to reproduce:

1. Create a bookmark with keyword and POST_DATA entry (e.g. "test" keyword at http://www.mantisbt.org/bugs/jump_to_bug.php) at firefox A.
2. Sync this bookmark to a different firefox B.



Actual results:

"test 1" works at firefox A: "jump_to_bug.php" is opened with appropriate post data.
"test 1" does not work at firefox B, it is interpreted as a search request (e.g. to google) with "test 1".

When exporting the bookmarks, the bookmark at firefox A looks like this:

A HREF="http://www.mantisbt.org/bugs/jump_to_bug.php" ADD_DATE="1488968208" LAST_MODIFIED="1488968208" ICON_URI="http://www.mantisbt.org/bugs/images/favicon.ico" ICON="___skipped__" SHORTCUTURL="test" POST_DATA="bug_id%3D%25s" LAST_CHARSET="UTF-8"

But at firefox B this "POST_DATA" is missing.


Expected results:

The synced bookmark at firefox B should work the same as firefox A.

It used to work, I get complaints from colleges since 2016. I have a set of bookmarks using post_data. I have to manually sync them at the different locations. Adding them at one location tends to break them on the other.

I have greped the sourcecode and found a list of synced bookmark elements at
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesSyncUtils.jsm#599
, where POST_DATA is not listed. (But I am not sure if this hint is correct: ICON is also not listed, but works.)

Comment 1

[Edouard Oger [:eoger]]

At first I thought this was a duplicate of bug 1328737, which has been fixed for a while.
But, I can reproduce the bug you're describing by opening the contextual menu in the search input and selecting "Add a Keyword for this Search".

Comment 2

[Edouard Oger [:eoger]]

Oh damn, sorry about the spam, I don't know why it removed the assignment. Working on a fix right now. This is because we don't sync postData at all.

Comment 3

[Edouard Oger [:eoger]]

Attachment: Bug 1345417 - Sync bookmarks postData.

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

Comment 4

[Edouard Oger [:eoger]]

No need to change removeConflictingKeywords since we update after deleting anyway.
Might need some tests, manual testing looks OK.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8845086 [details]
Bug 1345417 - Sync bookmarks postData.

This looks OK to me on a quick glance, but I think Kit is probably a better reviewer of this. One consideration is mobile - if mobile doesn't round-trip this postdata we might have an issue, so it might be necessary to see if they do and open bugs on them if they don't. OTOH though, given desktop-desktop wasn't round-tripping it either, I doubt mobile support should be a blocker - but we should get bugs on file if necessary.

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8845086 [details]
Bug 1345417 - Sync bookmarks postData.

https://reviewboard.mozilla.org/r/118298/#review120616

I also remember PlacesUtils.keywords having a few bugs, that we manage to avoid by not syncing postData (like bug 1313188, but not only that, IIRC there are issues that have to do with the cache and DB getting out of sync in these cases, and I think I stopped digging after filing 1313188 since it seemed like nobody would get to these any time soon).

I think we'll want to be somewhat thorough about tests here as a result...

::: toolkit/components/places/PlacesSyncUtils.jsm:973
(Diff revision 1)
> -// a different keyword, etc.
>  //
>  // If we don't handle those cases by removing the conflicting keywords first,
>  // the insertion  will fail, and the keywords will either be wrong, or missing.
>  // This function handles those cases.
>  function removeConflictingKeywords(bookmarkURL, newKeyword) {

This function will now remove keywords when the URL matches even if the postData doesn't match, which is incorrect.

Comment 7

[Edouard Oger [:eoger]]

Bug not actionable until places is fixed (very unlikely since postData is rarely used).

Comment 8

[Polzin]

First I was amazed, how quickly the bug got analyzed. Now, it seems to me that it will not be fixed, because "postData" is rarely used. Am I right?

In fact, postData can will only be created using the "Add a Keyword for this Search" action. This feature is very useful, but not so popular.

Two questions:
 1) If there something that I can do to make the fix of this bug more likely?
 2) With your understanding of the bug, if there a workaround to get such a bookmark synchronized? E.g. a procedure to have multiple copies of the bookmark where each of them has a valid postData in one firefox installation? Or adding a dummy "%s" in the url, such that the missing postData still leads to the correct page, although the variables are not set? I tried converting the postData variables to GET variables, but this did not work for the sites in question.

Comment 9

[Lina Butler [:lina]]

Comment on attachment 8845086 [details]
Bug 1345417 - Sync bookmarks postData.

https://reviewboard.mozilla.org/r/118298/#review125028

This is a fun one. Minusing because I think we'll need to consider a different approach in fixing this.

Bug 1125113, comment 1 suggests that there should be a 1:1 mapping between keywords and `(url, postData)` tuples. IOW, different URLs (with or without `postData`) can't have the same keyword, a URL without `postData` can only have 1 keyword, and a URL with N unique `postData` can have N keywords. This matches the comment in the old `setKeywordForBookmark` implementation (http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/toolkit/components/places/nsNavBookmarks.cpp#2839-2840), but I don't see that we actually enforce this.

The new `PlacesUtils.keywords` implementation and its tests seem to contradict this, too. There's no UNIQUE constraint on `place_id` or `post_data`, and `Keywords.insert` lets you set multiple 
keywords for the same URL. By default, `Keywords.fetch({ url })` will return the first one, but it's possible to fetch all of them by passing a function as the second argument. To make matters more interesting, the synchronous `getKeywordForBookmark` will return the most recent (in database insertion order) keyword.

However, the edit bookmark UI (http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/browser/components/places/content/editBookmarkOverlay.xul#149-159) only shows 1 keyword per bookmark, and `PlacesRemoveItemTransaction` still uses `getKeywordForBookmark` (http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/toolkit/components/places/PlacesUtils.jsm#3168-3172). So, it looks like deleting a bookmark will remove all its keywords (http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/toolkit/components/places/nsNavBookmarks.cpp#2867-2882), undoing a delete will only restore the most recent one, and changing a keyword in the UI (http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/toolkit/components/places/PlacesTransactions.jsm#1337-1369) will not remove other keywords associated with that URL.

So, that's Places on Desktop. But Sync introduces another level of indirection because it associates keywords with bookmarks, not URLs. Since it's possible to have multiple bookmarks with the same URL, and we can receive those bookmarks in any order, I suspect every bookmark will need to carry all its keywords and `postData` values. With that in mind, perhaps the "right" change is to replace the `keyword` field with a `keywords` array that contains all possible `{ keyword, postData }` tuples.

Another complication: Android's schema matches the Sync format (http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java#114-144), and associates keywords with bookmarks. So we can't turn `keyword` into an array without a migration plan. Should we think about adding the notion of a "primary" keyword that's stored in the "keyword" field (and shown in the Desktop and Android UI), and "alternate" keywords that are synced and used only on Desktop?

TL;DR:

* Desktop associates keywords with URLs, and a URL can have many keywords, regardless of `postData`.
* Sync and Android associate keywords with bookmarks, and assume 1 keyword per bookmark.
* `getKeywordForBookmark` and `Keywords.fetch` return the last inserted keyword, but `fetch` can also pass all keywords matching a URL to a callback.
* `setKeywordForBookmark` and `Keywords.insert` let you set multiple keywords for the same URL.
* Multiple bookmarks can have the same URL (and, by extension, the same keywords and `postData`), and Sync can see them in any order.

To comment 8's point, there are several layers to this, and untangling them will take some more thought. I can understand your frustration, and I wish I had a better answer (or workarounds). We certainly want to fix this, but the fix is complicated, and we're a very small team for a project this size. :-) There are lots of high-priority bugs needing attention, and the bar for spending time on any bug is high.