Closed Bug 1420811 Opened 7 years ago Closed 6 years ago

Rename bookmark tag to empty will cause issue

Categories

(Toolkit :: Places, defect, P1)

Product:
Toolkit
Component:
Places
Version:
57 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: steely.wing, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: dataloss, Whiteboard: [fxsearch])

Attachments

(1 file)

Bug 1420811 - Rename bookmark tag to empty will cause issue.
59 bytes, text/x-review-board-request
standard8
: review+
Details 
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
Step 3: Select the tag in the sidebar (not the bookmark, is the tag name under "Tags")
Component: Untriaged → Places
Product: Firefox → Toolkit
We should investigate this for sure.
Blocks: 1410877
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: dataloss
Priority: -- → P1
Whiteboard: [fxsearch]
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.
(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 >.< ).
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.
ni? myself to look at this tomorrow.
Flags: needinfo?(kit)
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.
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 :(
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?
Assignee: nobody → mak77
Status: NEW → ASSIGNED
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
Ah, Kit is on PTO, I wonder if Thom can help, I'd like to fix this in 59.
Flags: needinfo?(tchiovoloni)
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?
Attachment #8941856 - Flags: review?(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)
Attachment #8941856 - Flags: review+
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).
Flags: needinfo?(tchiovoloni)
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...
(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.
Moved the Sync issue to bug 1430573.
Flags: needinfo?(kit)
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/1024fb2fa343
Rename bookmark tag to empty will cause issue. r=standard8
https://hg.mozilla.org/mozilla-central/rev/1024fb2fa343
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: