Closed Bug 1360872 Opened 4 years ago Closed 4 years ago

Return empty strings for `null` bookmark titles

Categories

(Toolkit :: Places, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(1 file)

:Caspy7 pointed to https://www.reddit.com/r/firefox/comments/688y32/i_use_sync_when_i_log_in_to_a_clean_firefox_my/ in IRC; I can reproduce the issue.

Clearing a bookmark's title on one device caused the title to show up as "null" on another. Triggering a second sync on the other device renamed the bookmark again, from "null" back to its original name.

Unfortunately, I wasn't able to use the validator to figure out what's going on. I only have 1k bookmarks on the server (out of ~3k locally) after the mid-March node reassignment. "Server missing" drowns out all other problems, to the point where About Sync hangs just trying to render the results. :-(
I can't repro this on Nightly. What I tried:

* open a profile, "Show All Bookmarks", then clear the "name" field for a bookmark.

it auto-syncs, and the log shows:

> Sync.Engine.Bookmarks	TRACE	Outgoing: { id: mXUjdSojpJiK  index: 140  modified: undefined  ttl: undefined  payload: {"id":"mXUjdSojpJiK","type":"bookmark","parentid":"unfiled","parentName":"Other Bookmarks","dateAdded":1487151987673,"title":"","bmkUri":"https://bugzilla.mozilla.org/buglist.cgi?quicksearch=%s","loadInSidebar":false,"tags":[],"keyword":"bug"}  collection: bookmarks }

which looks correct (although note that going back into "Show All Bookmarks" list of bookmarks make it appear as though the bookmark has a title derived from the URL, but the name field does remain as an empty string when that bookmark is selected.

* start profile 2 and sync. "Show All Bookmarks" on that profile also shows an empty title (although as above, the list of bookmarks implies it has a title, but the name field remains blank)

Log shows:

> Sync.Engine.Bookmarks	TRACE	Incoming: { id: mXUjdSojpJiK  index: 140  modified: 1493604097.65  ttl: undefined  payload: {"id":"mXUjdSojpJiK","type":"bookmark","parentid":"unfiled","parentName":"Other Bookmarks","dateAdded":1487151987673,"title":"","bmkUri":"https://bugzilla.mozilla.org/buglist.cgi?quicksearch=%s","loadInSidebar":false,"tags":[],"keyword":"bug"}  collection: bookmarks }

which also looks correct.

* No matter how often I remash the "sync now" button in either profile, I can't see the above symptoms.

Kit, are you able to try again with more specific STR?
Flags: needinfo?(kit)
(In reply to Mark Hammond [:markh] from comment #1)
> which looks correct (although note that going back into "Show All Bookmarks"
> list of bookmarks make it appear as though the bookmark has a title derived
> from the URL, but the name field does remain as an empty string when that
> bookmark is selected.
> 
> * start profile 2 and sync. "Show All Bookmarks" on that profile also shows
> an empty title (although as above, the list of bookmarks implies it has a
> title, but the name field remains blank)

Aha! Yes, this is what I'm seeing. I renamed another bookmark in my toolbar, then synced. On the first device, I see:

> Sync.Engine.Bookmarks	TRACE	Outgoing: { id: 7lNrl02caACb  index: 3625  modified: undefined  ttl: undefined  payload: {"id":"7lNrl02caACb","type":"bookmark","parentid":"toolbar","parentName":"Bookmarks Toolbar","dateAdded":1488907906433,"title":"","bmkUri":"https://mozilla.slack.com/messages/general/","loadInSidebar":false,"tags":[]}  collection: bookmarks }

On the second device, I see:

> Sync.Engine.Bookmarks	TRACE	Incoming: { id: 7lNrl02caACb  index: 3625  modified: undefined  ttl: undefined  payload: {"id":"7lNrl02caACb","type":"bookmark","parentid":"toolbar","parentName":"Bookmarks Toolbar","dateAdded":1488907906433,"title":"","bmkUri":"https://mozilla.slack.com/messages/general/","loadInSidebar":false,"tags":[]}  collection: bookmarks }

And the title "null" in the toolbar. (But, if I right-click the bookmark and edit its properties, the name is empty). If I open the Library, I see a title derived from the URL, in both the list and the "Name" field underneath.

Now, if I open a new window, I see the same derived title, both in the toolbar and in properties. Can you reproduce this on the second device, Mark?

The derived title threw me off because the bookmark I renamed yesterday ("GitHub") had the same title, but the one I just renamed was different ("Slack" vs. "general | Mozilla Slack"). I'm not sure what's happening here, but you're right, it definitely doesn't look like a Sync bug.
Flags: needinfo?(kit)
Yes, I can reproduce that. Is it possible this is caused by us removing the DESCRIPTION_ANNO in PlacesSyncUtils, whereas places itself assumes this anno always exists?
(In reply to Mark Hammond [:markh] from comment #3)
> Yes, I can reproduce that. Is it possible this is caused by us removing the
> DESCRIPTION_ANNO in PlacesSyncUtils, whereas places itself assumes this anno
> always exists?

Interesting! I don't think that anno always exists, though. My profile has 3198 rows in `moz_bookmarks`, but only 917 in `moz_items_annos`.
Priority: -- → P2
I had a bit of a poke at this but failed to find the cause, so I'm hoping Mak can lend us a clue...

Mak, what happens is that we edit the "name" of a bookmark in the toolbar to be an empty string, and that change syncs. On a second device, after syncing, the title shown in the toolbar is |null|. Best we can tell, Sync is actually *removing* the annotation as it comes in on the second device (at http://searchfox.org/mozilla-central/source/toolkit/components/places/PlacesSyncUtils.jsm#1324). Most of the code I can find which gets a title seems to handle the annotation missing, but I don't seem to be able to find the path taken in this case. Everything seems to work as expected for bookmarks other than those shown in the toolbar.

Can you offer us any insights here?
Flags: needinfo?(mak77)
(In reply to Mark Hammond [:markh] from comment #5)
> Mak, what happens is that we edit the "name" of a bookmark in the toolbar to
> be an empty string, and that change syncs. On a second device, after
> syncing, the title shown in the toolbar is |null|.

> Best we can tell, Sync is
> actually *removing* the annotation as it comes in on the second device (at
> http://searchfox.org/mozilla-central/source/toolkit/components/places/
> PlacesSyncUtils.jsm#1324).

I'm not sure what an annotation has to do with the title of the bookmark. The title of a bookmark is a direct property on the bookmark, while the description annotation only controls the description of a bookmark, that is only visible when right clicking on a bookmark and choosing Properties.
Each bookmark has a title and a description, as any html page.

> Can you offer us any insights here?

First, the title fallback in views is different.
If a bookmark has an empty title, the toolbar will keep the empty title, but other views will fallback to the url. This happens because there is a habit of being able to set empty titles to save space on the toolbar.

I have some suspects.
So, what happens when the title of a bookmark changes, is that the bookmarks service sends an onItemChanged with property "title" notification. nsNavHistoryResult gets that, and forwards it to the open containers inside it, for example the toolbar forwards the message to bookmarks and folders inside it, they will forward the message to the view by calling the nodeTitleChanged notification. The view (http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/browser/components/places/content/browserPlacesViews.js#546) handles that and updates the attribute.

What happens here is that the notification gets a void string, that xpcom converts to null, the view gets null, and sets the "label" attribute to null.
I think this may be related to the move to the new bookmarking API, IIRC we don't differentiate anymore null from empty string, but that also means we could notify null to onItemChanged? it's likely, and you should probably check that.
Flags: needinfo?(mak77)
Note, I mean that you should check if Bookmarks.jsm has a bug where it notifies null as a possible new title, when the title is passed in as null, undefined or such.
Thanks Mak! So the patch below solves "null" showing up in the UI, but doesn't solve the fact that the title somehow resurrects. I'm not going to dig any more at this time, but the fact it is P2 means we should get to it fairly soon...

+++ b/toolkit/components/places/Bookmarks.jsm
@@ -555,7 +555,7 @@ var Bookmarks = Object.freeze({
         }
         if (updateInfo.hasOwnProperty("title")) {
           notify(observers, "onItemChanged", [ updatedItem._id, "title",
-                                               false, updatedItem.title,
+                                               false, updatedItem.title || "",
yes, the info object passed to updateBookmark must have a null title because we want to store  null in the DB (potentially smaller than an empty string). Any empty title, included "" is converted to null on input validation.

Then the call to mergeIntoNewObject merges title: null into updatedItem.

There is more, in insertBookmark and updateBookmark we do:
    // Don't return an empty title to the caller.
    if (updatedItem.hasOwnProperty("title") && updatedItem.title === null)
      delete updatedItem.title;

Does this make sense? should we rather convert a null title to an empty string and return an empty string?
insertBookmark has a similar "bug" in onItemAdded, where it passes item.title || null to onItemAdded, not item.title || "".

What makes more sense? I think originally I considered title null the right value for an empty title, but on a second thought it may be more likely that consumers (like Sync) expect an empty string, not null.
(In reply to Marco Bonardo [::mak] from comment #9)
> yes, the info object passed to updateBookmark must have a null title because
> we want to store  null in the DB (potentially smaller than an empty string).
> Any empty title, included "" is converted to null on input validation.

That sounds reasonable. ISTM like there's a somewhat unavoidable tension between "no title specified" vs "no title explicitly requested"...

> There is more, in insertBookmark and updateBookmark we do:
>     // Don't return an empty title to the caller.
>     if (updatedItem.hasOwnProperty("title") && updatedItem.title === null)
>       delete updatedItem.title;

That seems a little odd - from the POV of the caller, isn't that just replacing null/"" with undefined?

> What makes more sense? I think originally I considered title null the right
> value for an empty title, but on a second thought it may be more likely that
> consumers (like Sync) expect an empty string, not null.

I think it's reasonable that callers handle null, but yeah, it's tricky - null vs undefined vs "" all seem more subtle than required - but I'm still not sure where the offending consumer of my change is (almost certainly not Sync), nor who is resurrecting the title (possibly Sync).

Needs more digging...
Assignee: nobody → eoger
Assignee: eoger → nobody
Assignee: nobody → kit
TL;DR: I don't think we need the distinction between `""` and `null`.

There are two ways we can fix this, as Mak suggests in comment 9. One is to have the validator distinguish between `null` and `""` (http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/toolkit/components/places/PlacesUtils.jsm#235-236), and remove the `|| null` condition when we notify observers (http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/toolkit/components/places/Bookmarks.jsm#223,430,574).

However, I don't think we should do that. As comment 6 says, if the title is `""`, we'll hide the title in the toolbar, and fall back to the title of the history entry in other views. If the title is `null`, we'll still fall back in the other views, but show the string `null` in the toolbar...which isn't right. Additionally, Sync doesn't support `null` titles, per Richard's comments on IRC:

> •rnewman> kitcambridge: to answer yesterday's question: no, because a null title is already blocked as being invalid. https://github.com/mozilla-mobile/firefox-ios/blob/master/Sync/BookmarkPayload.swift#L222
> items cannot have a null or missing title field; it can be empty, but not null or undefined.
> (and that goes for other platforms, too)

Given that these `null` titles aren't helpful, and Sync doesn't support them, I think we should consider treating void strings (`null`) and empty strings the same way.
Comment on attachment 8875015 [details]
Bug 1360872 - Return empty strings for `null` bookmark titles.

https://reviewboard.mozilla.org/r/146362/#review150776

There are still a few failures in tests... Or is that Try run old?

Can we now change the "// Don't return an empty title to the caller." code? At this point it may be better to return an empty string, so we are coherent with the notifications.
Comment on attachment 8875015 [details]
Bug 1360872 - Return empty strings for `null` bookmark titles.

https://reviewboard.mozilla.org/r/146362/#review150780

::: toolkit/components/places/nsNavBookmarks.cpp:2026
(Diff revision 2)
>    );
>    NS_ENSURE_STATE(statement);
>    mozStorageStatementScoper scoper(statement);
>  
> +  nsAutoCString title;
> +  TruncateTitle(aTitle, title);

Is the actual problem this call using aTitle instead of title?
http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/toolkit/components/places/nsNavBookmarks.cpp#1986
Comment on attachment 8875015 [details]
Bug 1360872 - Return empty strings for `null` bookmark titles.

https://reviewboard.mozilla.org/r/146362/#review150780

> Is the actual problem this call using aTitle instead of title?
> http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/toolkit/components/places/nsNavBookmarks.cpp#1986

Actually, we already truncate in `SetItemTitle`, so this isn't necessary.
Comment on attachment 8875015 [details]
Bug 1360872 - Return empty strings for `null` bookmark titles.

https://reviewboard.mozilla.org/r/146362/#review150776

Sounds good to me, I'll make the change and fix up the xpcshell failures. I just pushed to try to see what would break.
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #16)
> Comment on attachment 8875015 [details]
> Bug 1360872 - Store empty bookmark titles as NULL; pass them to observers as
> empty strings.
> 
> https://reviewboard.mozilla.org/r/146362/#review150780
> 
> > Is the actual problem this call using aTitle instead of title?
> > http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/toolkit/components/places/nsNavBookmarks.cpp#1986
> 
> Actually, we already truncate in `SetItemTitle`, so this isn't necessary.

Yes, but the link above shows that one of the 2 paths is using aTitle instead of the truncated title.
Duplicate of this bug: 1370931
Duplicate of this bug: 1009419
Sorry if this is a naive comment, but to cope with this, why not to have a property on the toolbar to show/hide the title of the bookmarks on it? Most of the times icons are just enough.
(In reply to joaofl from comment #21)
> Sorry if this is a naive comment, but to cope with this, why not to have a
> property on the toolbar to show/hide the title of the bookmarks on it? Most
> of the times icons are just enough.

you can already do that through userChrome.css afaict. this is for the misc case where some item will have a title, some won't. Regardless, this is the backend so it should support empty titles properly.
overall it looks good, but I still see a bunch of test failures on Try?
(In reply to Marco Bonardo [::mak] from comment #24)
> overall it looks good, but I still see a bunch of test failures on Try?

Triggering a full run now, just in case I missed any other tests.
what do you think of landing this after the 56 merge? is this something Sync needs in 55 already? I'd like to give it a full nightly testing cycle.
Flags: needinfo?(kit)
(In reply to Marco Bonardo [::mak] from comment #30)
> what do you think of landing this after the 56 merge?

SGTM. This has been an issue since 53, so there's no urgency for 55. Keeping ni? so I remember to land next week.
Component: Sync → Places
Product: Firefox → Toolkit
Summary: Clearing a bookmark's title causes it to sync as `null`, then reverts → Return empty strings for `null` bookmark titles
Comment on attachment 8875015 [details]
Bug 1360872 - Return empty strings for `null` bookmark titles.

https://reviewboard.mozilla.org/r/146362/#review154012

::: toolkit/components/places/Bookmarks.jsm:1805
(Diff revision 7)
>        item[prop] = row.getResultByName(prop);
>      }
>      for (let prop of ["dateAdded", "lastModified"]) {
>        item[prop] = PlacesUtils.toDate(row.getResultByName(prop));
>      }
> -    for (let prop of ["title", "parentGuid", "url" ]) {
> +    item.title = row.getResultByName("title");

you may just add "title" to the "guid", "index", "type" loop above.
Attachment #8875015 - Flags: review?(mak77) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 4b30ba987c2a -d 798a5d133c0b: rebasing 403005:4b30ba987c2a "Bug 1360872 - Return empty strings for `null` bookmark titles. r=mak" (tip)
merging browser/components/places/PlacesUIUtils.jsm
merging toolkit/components/places/Bookmarks.jsm
merging toolkit/components/places/PlacesUtils.jsm
merging toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
warning: conflicts while merging toolkit/components/places/Bookmarks.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s b18e53a1cd27 -d a1b87937f170: rebasing 403010:b18e53a1cd27 "Bug 1360872 - Return empty strings for `null` bookmark titles. r=mak" (tip)
merging toolkit/components/places/Bookmarks.jsm
merging toolkit/components/places/PlacesUtils.jsm
warning: conflicts while merging toolkit/components/places/Bookmarks.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0fd6c0590be
Return empty strings for `null` bookmark titles. r=mak
This also triggers xpcshell failures: https://treeherder.mozilla.org/logviewer.html#?job_id=110902211&repo=autoland

> TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/bookmarks/test_405938_restore_queries.js | - false == true
Flags: needinfo?(kit)
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d7e7ced3622f
Return empty strings for `null` bookmark titles. r=mak
Backed out for failing xpcshell's test_async_transactions.js | test_creating_and_removing_a_separator, at least on OS X:

https://hg.mozilla.org/integration/autoland/rev/69c6a6754d940bb404dce835973a54f724b15420

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d7e7ced3622fd840c81106bbe4ea84e5fb9fbc18&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=111115095&repo=autoland

10:22:23     INFO -  PID 6337 | console.error:
10:22:23     INFO -  PID 6337 |   Message: Error: Invalid value for property 'title': ""
10:22:23     INFO -  PID 6337 |   Stack:
10:22:23     INFO -  PID 6337 |     validateItemProperties@resource://gre/modules/PlacesUtils.jsm:563:15
10:22:23     INFO -  PID 6337 | validateBookmarkObject@resource://gre/modules/Bookmarks.jsm:2021:10
10:22:23     INFO -  PID 6337 | insert@resource://gre/modules/Bookmarks.jsm:187:22
10:22:23     INFO -  PID 6337 | redo/promise<@resource://gre/modules/PlacesTransactions.jsm:613:19
10:22:23     INFO -  PID 6337 | async*enqueue/promise<@resource://gre/modules/PlacesTransactions.jsm:469:58
10:22:23     INFO -  PID 6337 | _do_main@/Users/cltbld/tasks/task_1498842061/build/tests/xpcshell/head.js:220:3
10:22:23     INFO -  PID 6337 | _execute_test@/Users/cltbld/tasks/task_1498842061/build/tests/xpcshell/head.js:549:5
10:22:23     INFO -  PID 6337 | @-e:1:1
10:22:23     INFO -  PID 6337 |   Can't redo a transaction, clearing redo entries.
10:22:23     INFO -  TEST-PASS | toolkit/components/places/tests/unit/test_async_transactions.js | test_creating_and_removing_a_separator - [test_creating_and_removing_a_separator : 116] true == true
10:22:23     INFO -  TEST-PASS | toolkit/components/places/tests/unit/test_async_transactions.js | test_creating_and_removing_a_separator - [test_creating_and_removing_a_separator : 131] null == null
10:22:23     INFO -  TEST-PASS | toolkit/components/places/tests/unit/test_async_transactions.js | test_creating_and_removing_a_separator - [test_creating_and_removing_a_separator : 135] null == null
10:22:23  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/unit/test_async_transactions.js | test_creating_and_removing_a_separator - [test_creating_and_removing_a_separator : 153] 0 == 1
10:22:23     INFO -  /Users/cltbld/tasks/task_1498842061/build/tests/xpcshell/tests/toolkit/components/places/tests/unit/test_async_transactions.js:ensureUndoState:153
10:22:23     INFO -  /Users/cltbld/tasks/task_1498842061/build/tests/xpcshell/tests/toolkit/components/places/tests/unit/test_async_transactions.js:test_creating_and_removing_a_separator:828
10:22:23     INFO -  exiting test
10:22:23     INFO -  Unexpected exception 2147500036
10:22:23     INFO -  undefined
10:22:23     INFO -  exiting test
10:22:23     INFO -  <<<<<<<
Flags: needinfo?(kit)
Let's see if that does the trick: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48aefe80fee5
Flags: needinfo?(kit)
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0eb8deec54bc
Return empty strings for `null` bookmark titles. r=mak
https://hg.mozilla.org/mozilla-central/rev/0eb8deec54bc
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1378554
You need to log in before you can comment on or make changes to this bug.