1420811 - Rename bookmark tag to empty will cause issue

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[steely.wing]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171112125346

Steps to reproduce:

!!! Don't test in your firefox, will cause all bookmark tag corrupt !!!

- Bookmark URL with tag
- Open bookmark library (Ctrl+Shift+B)
- Select the tag in the sidebar
- In the name field, rename to empty



Actual results:

Now you can't add tag to any bookmark, all tag is corrupt, if you sync with server, it will remove all tag in server, restore bookmark from backup will not fix, you need to delete the whole profile.


Expected results:

Normal

Comment 1

[steely.wing]

Step 3: Select the tag in the sidebar (not the bookmark, is the tag name under "Tags")

Comment 2

[Marco Bonardo [:mak]]

We should investigate this for sure.

Comment 3

[Mark Banner (:standard8)]

I tried this on 57/58/59 without syncing it. In all cases I was still able to add and remove tags existing bookmarks via the properties section of the library window.

I was able to restore a previously saved json file as well (although the "empty" tag still remained). Additionally, I couldn't delete the empty titled tag, but I was still able to add and delete others.

I wouldn't be surprised if having sync involved might get things a little more confused/broken.

Comment 4

[steely.wing]

(In reply to Mark Banner (:standard8) from comment #3)
> I tried this on 57/58/59 without syncing it. In all cases I was still able
> to add and remove tags existing bookmarks via the properties section of the
> library window.
> 
> I was able to restore a previously saved json file as well (although the
> "empty" tag still remained). Additionally, I couldn't delete the empty
> titled tag, but I was still able to add and delete others.
> 
> I wouldn't be surprised if having sync involved might get things a little
> more confused/broken.

Sorry, you are right, we can still edit tag of other bookmark, only can't edit the bookmark that with empty tag (and can't delete the bookmark), but after sync, other bookmark tags may be removed too ( I don't want to confirm this using my account >.< ).

Comment 5

[Lina Butler [:lina]]

Ah, I didn't realize the UI allowed empty tags. I think Sync does the wrong thing in https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/toolkit/components/places/PlacesSyncUtils.jsm#1715-1716, where it filters them out.

Comment 6

[Lina Butler [:lina]]

ni? myself to look at this tomorrow.

Comment 7

[Mark Banner (:standard8)]

I think allowing tags with empty names is probably a bad idea - there's nothing in the current UI that makes it easy. However, I do think we should be managing this broken case correctly.

Comment 8

[Marco Bonardo [:mak]]

The API should discard empty tags, things like "sometag,,someothertag" should only add 2. and if there's just an empty tag it should throw as well.
I suspect here the problem is that renaming a tag ends up renaming a folder, and folders CAN have an empty name. Another case where tags as folders is an horrible idea.
We should disallow renaming to empty in the UI, but this won't remove the fact previous versions could allow that and Sync must cope with the problem :(

Comment 9

[Marco Bonardo [:mak]]

We actually already have a PlacesDBUtils task to rename empty named tags to "(notitle)" (D.12), though, the Sync trigger doesn't update synChangeCounter if a bookmark title changes.
I can fix the UI glitch that allows this renaming, and eventually also patch the bookmarks code to throw.

Kit, what should we do for Sync?

Comment 10

[Marco Bonardo [:mak]]

Attachment: Bug 1420811 - Rename bookmark tag to empty will cause issue.

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

Comment 11

[Marco Bonardo [:mak]]

not adding a test for the Bookmarks.jsm changes, since tags are likely to change soon.
Still needs to figure out the Sync changes, eventually

Comment 12

[Marco Bonardo [:mak]]

Ah, Kit is on PTO, I wonder if Thom can help, I'd like to fix this in 59.

Comment 13

[Marco Bonardo [:mak]]

The trigger is this one
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/toolkit/components/places/PlacesDBUtils.jsm#746
I honestly don't recall why we don't bump on a title change

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8941856 [details]
Bug 1420811 - Rename bookmark tag to empty will cause issue.

https://reviewboard.mozilla.org/r/212076/#review218136

That works nicely. I think there's just a couple of small tests that could be added to ensure we maintain the Bookmarks.jsm changes.

::: toolkit/components/places/Bookmarks.jsm:211
(Diff revision 1)
>        let parent = await fetchBookmark({ guid: insertInfo.parentGuid });
>        if (!parent)
>          throw new Error("parentGuid must be valid");
> +      let isTagging = parent._parentId == PlacesUtils.tagsFolderId;
> +      if (isTagging && info.hasOwnProperty("title") && !info.title) {
> +        throw new Error("Cannot set an empty tag title");

Please can you add an Assert.rejects test for this in test_bookmarks_insert?

::: toolkit/components/places/Bookmarks.jsm:1253
(Diff revision 1)
>        tuples.set("dateAdded", { value: PlacesUtils.toPRTime(info.dateAdded) });
>  
>      await db.executeTransaction(async function() {
>        let isTagging = item._grandParentId == PlacesUtils.tagsFolderId;
> +      if (isTagging && info.hasOwnProperty("title") && !info.title) {
> +        throw new Error("Cannot set an empty tag title");

Please can you add an Assert.rejects test for this in test_bookmarks_update.js?

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8941856 [details]
Bug 1420811 - Rename bookmark tag to empty will cause issue.

https://reviewboard.mozilla.org/r/212076/#review218166

Sorry, I'd missed comment 11 about not adding tests at this stage (due to future refactoring)

Comment 16

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

As Kit indicated, currently Sync will already filter out empty tags so I don't think it will make things worse here, hopefully. https://searchfox.org/mozilla-central/source/toolkit/components/places/PlacesSyncUtils.jsm#1718

> I honestly don't recall why we don't bump on a title change

I think this is because we'd need to bump the change counter for all the tagged items, and not for the tag itself. It seems like a bug that we don't do this currently, though. I'm a bit fuzzy on how tags are represented in places, so it's possible I'm confused here.

(FWIW I think that any other reason for changing a title (or any other synced property) in a maintenance task should bump the change counter)

> and eventually also patch the bookmarks code to throw

This concerns me a little since we'd probably fail to clean up the dummy URI we add for tagging things https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/toolkit/components/places/PlacesSyncUtils.jsm#1727. That wouldn't be the end of the world, but is certainly not ideal.

I think probably it should be in a try/finally anyway though, but maybe there's a reason this isn't a problem (kit would know).

Comment 17

[Marco Bonardo [:mak]]

well, we can avoid throwing and just fix the ui glitch for now... the tags rewrite should ideally get rid of all of this mess regardless...

Comment 18

[Marco Bonardo [:mak]]

(In reply to Thom Chiovoloni [:tcsc] from comment #16)
> I think this is because we'd need to bump the change counter for all the
> tagged items, and not for the tag itself. It seems like a bug that we don't
> do this currently, though. I'm a bit fuzzy on how tags are represented in
> places, so it's possible I'm confused here.

We all are, it doesn't make sense :) Hopefully we can fix it this year.

> > and eventually also patch the bookmarks code to throw
> 
> This concerns me a little since we'd probably fail to clean up the dummy URI
> we add for tagging things

To avoid any problem I will remove the Bookmarks.jsm changes and just fix the UI bug.

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 8941856 [details]
Bug 1420811 - Rename bookmark tag to empty will cause issue.

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

Comment 20

[Marco Bonardo [:mak]]

Moved the Sync issue to bug 1430573.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1024fb2fa343
Rename bookmark tag to empty will cause issue. r=standard8

Comment 22

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/1024fb2fa343