Changing properties of a new bookmark while adding it acts on the last bookmark in the current container

VERIFIED FIXED in Firefox 41

Status

()

Firefox
Bookmarks & History
--
blocker
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Alice0775 White, Assigned: adw)

Tracking

({dataloss, regression})

41 Branch
Firefox 44
dataloss, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 unaffected, firefox41- verified, firefox42+ verified, firefox43+ verified, firefox44 verified, relnote-firefox 41+)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
[Tracking Requested - why for this release]: Regression since Firefox41

This is one of loosing of data.
The Bookmarks system is COMPLETELY BROKEN!!!!!!!!!!!  
This should be BLOCK to release Firefox41 !!!!!!!!

Steps To Reproduce:
0. Unhide bookmarks toolbar (this step is optionally, but it makes easy to reproduce)
1. Right click on a existing bookmarks item
2. Choose "New Bookmark..." in context menu
3. Change name to "abcd" in the dialog
4. Change focus to other field such as location field

Actual Results:
You can observe that name of last bookmarks item is changed to "abcd".
And the new bookmark name is staying remain "New Bookmark"

Expected Results:
The new bookmark name should be changed properly.


Regression window:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=db8602cd98b1447b41be33cf9cf743c8ff7d1388&tochange=6dfed2a7a155


Via local build,
Last good: 31b80e27600d
First Bad: 6dfed2a7a155

Regressed by: 
6dfed2a7a155	Marco Bonardo — Bug 1194568 - Renaming live bookmark while adding it renames the wrong one. r=ttaubert


I wonder that there are no automate-test..... :(
(Reporter)

Updated

3 years ago
Flags: needinfo?(mak77)
(Reporter)

Comment 1

3 years ago
This problem is affected to not only "New Bookmark..." but also "New Folder..."

Comment 2

3 years ago
Florin, would your team be able to give these STR a try and lemme know whether you hit this bug or not? And whether it impacts windows, OS X or linux? Many thanks!
Flags: needinfo?(florin.mezei)
I do hit this bug and it causes confusing data loss. When I create a new bookmark, one of my old bookmarks in the toolbar is changed instead -- renamed, but keeping the icon for the old bookmark. The behavior is very strange.
I think we should likely back out the fix in bug 1194568 and rebuild 41. Tim can you have a look? (since mak is afk right now.)
Flags: needinfo?(ttaubert)
(Reporter)

Updated

3 years ago
See Also: → bug 1206350
More on the STR.
1. Unhide bookmarks toolbar.
2. Open a new tab and go to a page. Let's say https://mozilla.org. 
3. Click "New Bookmark" in the bookmarks toolbar (the leftmost button in the toolbar)

Result: The page i'm viewing is blanked completely and so is the location bar. I don't get any sort of "new bookmark" interface.
Ah, ignore that last comment. Sorry. I was not using a fresh profile. There is no "new bookmark" button normally. I'm not sure where it came from.

Here are updated STR and a workaround.
1. Unhide bookmarks toolbar
2. Right-click on an existing bookmark in the toolbar and choose "New Bookmark" from the context menu
3. You will get a popup to create a new bookmark. add a name and url
Result: The first bookmark is partially overwritten. Its name and url change to the 2nd bookmark but its icon remains the same as the first bookmark's icon. 

Workaround: Don't right click on an existing bookmark. Right click anywhere else in the bookmarks toolbar.That works as expected to add a new bookmark without overwriting old ones. 

The workaround seems easy enough. While this is a regression we should fix I don't think it's a blocker for the release. If you hit the bug, it only affects one bookmark, not all bookmarks (which would worry me more). The bookmarks toolbar isn't on by default; the default way to add a new bookmark still works.
Comment hidden (spam)

Comment 8

3 years ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #4)
> I think we should likely back out the fix in bug 1194568 and rebuild 41. Tim
> can you have a look? (since mak is afk right now.)

Backing out the fix at this stage is *not* a risk that we should be taking. 

Again, this is a valid issue that leads to data loss, however given that doing each of these scenarios separately works ok (i.e. adding bookmarks, renaming bookmarks), I do not believe this is a ship stopper/release blocker for 41.

We should definitely fix the issue early into the Beta42 cycle given the severity.
status-firefox41: affected → wontfix
tracking-firefox41: ? → -
tracking-firefox42: ? → +
tracking-firefox43: ? → +
As with bug 1206350 (which is probably another symptom of the same underlying cause), adding a bookmark from the context menu isn't as common an action, which would lessen the severity here too.

However, this bug is worse in that it modifies existing data, which is pretty unexpected/confusing to the user. And it can also happen when attempting to add a new folder. Which I'd also expect to be somewhat infrequent (hard to tell relative popularity), but doesn't really have a more obvious/convenient primary entry.

So, I think we should either respin 41 or quickly followup with a 41.0.1. Bad timing. :(

Simply backing out bug 1194568 means taking the regressions it was fixing (for renaming Livebookmarks / RSS, which I'd expect to be even less used, and thus acceptable). Fixing this bug outright for 41 might also be an option, if the fix ends up simple and obvious.
(In reply to Justin Dolske [:Dolske] from comment #9)
> As with bug 1206350 (which is probably another symptom of the same
> underlying cause), adding a bookmark from the context menu isn't as common
> an action, which would lessen the severity here too.
> 
> However, this bug is worse in that it modifies existing data, which is
> pretty unexpected/confusing to the user. And it can also happen when
> attempting to add a new folder. Which I'd also expect to be somewhat
> infrequent (hard to tell relative popularity), but doesn't really have a
> more obvious/convenient primary entry.
> 
> So, I think we should either respin 41 or quickly followup with a 41.0.1.
> Bad timing. :(
> 
> Simply backing out bug 1194568 means taking the regressions it was fixing
> (for renaming Livebookmarks / RSS, which I'd expect to be even less used,
> and thus acceptable). Fixing this bug outright for 41 might also be an
> option, if the fix ends up simple and obvious.

Thanks Dolske for your assessment, it is extremely helpful! I would prefer to take this fix (if ready and safe) as a ride along to a dot release rather than re-spinning 41 today and delaying the 41 release as a consequence. 

Delaying the 41 release is an option for bugs that are critical with no decent workaround, which is not the case for this specific bug.
(Assignee)

Comment 11

3 years ago
The old way of getting the bookmark ID:

  this._itemId = PlacesUtils.bookmarks.getIdForItemAt(container, index);

returns a different ID from the new way:

  this._itemId = yield PlacesUtils.promiseItemId(bm.guid);

The new way seems to always return the ID of the last bookmark in the folder.  Maybe because the bookmark fetch passes PlacesUtils.bookmarks.DEFAULT_INDEX (-1) for index, which I'm guessing encodes the assumption that the new bookmark is always being added at the end, which is not the case in the STR?

Still investigating...
Assignee: nobody → adw
Status: NEW → ASSIGNED
(Assignee)

Comment 12

3 years ago
Created attachment 8663896 [details] [diff] [review]
patch

Yeah, that seems to be the problem.  This patch fixes it.  Just a typo, basically.

In summary, I'm pretty sure that this bug is fairly narrow.  It affects at most a single bookmark per folder, the last bookmark in the folder.  It happens only when a bookmark is created in the middle of a folder, not at the end, so you have to use non-primary UI like the context menu to make it happen.  Marco would certainly know more than me about it, but that's what it seems like to me.
Attachment #8663896 - Flags: review?(mak77)
(Assignee)

Comment 13

3 years ago
And actually it only happens when bookmarks are created via the bookmark properties dialog.  Bookmarks created in other ways don't trigger it.
(Assignee)

Comment 14

3 years ago
Also, does not happen when changing properties of previously created bookmarks when using the bookmark properties dialog.  Only for newly created bookmarks.
(In reply to Ritu Kothari (:ritu) from comment #2)
> Florin, would your team be able to give these STR a try and lemme know
> whether you hit this bug or not? And whether it impacts windows, OS X or
> linux? Many thanks!

Same as bug 1206350, it seems our input is no longer needed here, and we can just pick this up when it's fixed and also when it lands in Beta 42. Ritu, feel free to add back the needinfo if you still think our input is needed here.

Also, given the latest assessment, we should probably change the importance from "Blocker".
Flags: needinfo?(florin.mezei)

Updated

3 years ago
Flags: needinfo?(mak77)
Summary: Renaming new bookmark while adding it renames the other item. → Changing properties of a new bookmark while adding it acts on the last bookmark in the current container

Updated

3 years ago
Duplicate of this bug: 1206350

Updated

3 years ago
Flags: needinfo?(ttaubert)
Comment on attachment 8663896 [details] [diff] [review]
patch

Review of attachment 8663896 [details] [diff] [review]:
-----------------------------------------------------------------

yeah, it was indeed a typo (or just "lack of attention" in the code porting).
This patch is safe and trivial, so it should not be a big deal to take it in the next chemspill (but this is up to release drivers to decide). I personally don't think a backout will be safer, it will reintroduce at least 2 regressions.

Please file a follow-up bug to create a test (or modify the existing one) to ensure we are editing the right component. it's not critical to have that immediately.

::: browser/components/places/content/bookmarkProperties.js
@@ +606,5 @@
>  
>      let folderGuid = yield PlacesUtils.promiseItemGuid(container);
>      let bm = yield PlacesUtils.bookmarks.fetch({
>        parentGuid: folderGuid,
> +      index: index,

nit: just index
Attachment #8663896 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #17)
> Please file a follow-up bug to create a test (or modify the existing one) to
> ensure we are editing the right component.

s/component/bookmark/
(Assignee)

Updated

3 years ago
Depends on: 1207246
(Assignee)

Comment 20

3 years ago
Filed bug 1207246 for the test.  Moving this bug to the right component now that the problem is clear.
Component: Places → Bookmarks & History
Product: Toolkit → Firefox
https://hg.mozilla.org/mozilla-central/rev/a2ad0230946a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Comment on attachment 8663896 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1194568 
[User impact if declined]: Changing properties in the new bookmark/folder dialog changes properties of a previously existing bookmark.
[Describe test coverage new/current, TreeHerder]: Nightly
[Risks and why]: The risk is low cause the change is trivial and it's just executing the same action as the old code.
[String/UUID change made/needed]: none
Attachment #8663896 - Flags: approval-mozilla-release?
Attachment #8663896 - Flags: approval-mozilla-beta?
Attachment #8663896 - Flags: approval-mozilla-aurora?
Comment on attachment 8663896 [details] [diff] [review]
patch

Fix a regression, taking it. Should be in beta 2.
Attachment #8663896 - Flags: approval-mozilla-beta?
Attachment #8663896 - Flags: approval-mozilla-beta+
Attachment #8663896 - Flags: approval-mozilla-aurora?
Attachment #8663896 - Flags: approval-mozilla-aurora+

Updated

3 years ago
Duplicate of this bug: 1208056
Alice0775, would you be able to verify the fix works as expected on latest Nightly build (09/23 or later)? Your help is much appreciated and many thanks for finding and reporting this issue.
Flags: needinfo?(alice0775)
(Reporter)

Comment 28

3 years ago
https://hg.mozilla.org/mozilla-central/rev/bf2bc1aa78c0b72d9b6b13f7a8c6ae61c60a51dc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0 ID:20150924030231

https://hg.mozilla.org/releases/mozilla-aurora/rev/663c99a20a9b25370cdfe0acc8b70197fddd4c6d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0 ID:20150924071318

https://hg.mozilla.org/releases/mozilla-beta/rev/4d92cee6e6b9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0 ID:20150924033219

Velified.  nolonger reproduce the problem
Flags: needinfo?(alice0775)
Marking as Verified based on Alice's testing above (comment 28).
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified
status-firefox43: fixed → verified
status-firefox44: fixed → verified

Updated

3 years ago
Duplicate of this bug: 1208281

Updated

3 years ago
Duplicate of this bug: 1208524

Updated

3 years ago
Duplicate of this bug: 1199918
Comment on attachment 8663896 [details] [diff] [review]
patch

Approved for uplift to mozilla-release. Given that this is a data loss scenario impacting us from bookmarks toolbar, sidebar and menubar, I believe this meets the RC criteria. Let's land on m-r so it's included in 41 dot release.
Attachment #8663896 - Flags: approval-mozilla-release? → approval-mozilla-release+

Updated

3 years ago
status-firefox41: wontfix → affected

Updated

3 years ago
Duplicate of this bug: 1209266

Updated

3 years ago
Duplicate of this bug: 1209165
Confirming the fix on Firefox 41.0.1, build ID: 20150928151607.
The bookmark name now changes properly, following the STR from the description.

Testing was performed on Windows 7 64-bit, Windows 10 32-bit, Mac OS X 10.10.5	and Ubuntu 12.04 32-bit.
status-firefox41: fixed → verified
QA Contact: cornel.ionce

Updated

3 years ago
Duplicate of this bug: 1209799

Updated

3 years ago
Duplicate of this bug: 1209938
Added to the release notes: "Fix a regression in the benchmark creation (1206376)"
relnote-firefox: --- → 41+
Of  course: s/benchmark/bookmark/g

Updated

3 years ago
Duplicate of this bug: 1210432
You need to log in before you can comment on or make changes to this bug.