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

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Alice0775 White]

[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..... :(

Comment 1

[Alice0775 White]

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

Comment 2

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

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.)

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

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 7

[Alice0775 White]

ha ha ha

My workaround is that DO NOT USE Firefox.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(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.

Comment 9

[Justin Dolske [:Dolske]]

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.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(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.

Comment 11

[Drew Willcoxon :adw]

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...

Comment 12

[Drew Willcoxon :adw]

Attachment: 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.

Comment 13

[Drew Willcoxon :adw]

And actually it only happens when bookmarks are created via the bookmark properties dialog.  Bookmarks created in other ways don't trigger it.

Comment 14

[Drew Willcoxon :adw]

Also, does not happen when changing properties of previously created bookmarks when using the bookmark properties dialog.  Only for newly created bookmarks.

Comment 15

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(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".

Comment 17

[Marco Bonardo [:mak]]

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

Comment 18

[Marco Bonardo [:mak]]

(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/

Comment 19

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/a2ad0230946a

Comment 20

[Drew Willcoxon :adw]

Filed bug 1207246 for the test.  Moving this bug to the right component now that the problem is clear.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a2ad0230946a

Comment 22

[Marco Bonardo [:mak]]

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

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8663896 [details] [diff] [review]
patch

Fix a regression, taking it. Should be in beta 2.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/be47489093d1

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a363a3510295

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 28

[Alice0775 White]

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

Comment 29

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Marking as Verified based on Alice's testing above (comment 28).

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-release/rev/109eaf2e7802

Comment 37

[Cornel Ionce [:noni] [Hubs QA]]

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.

Comment 40

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes: "Fix a regression in the benchmark creation (1206376)"

Comment 41

[Sylvestre Ledru [:Sylvestre]]

Of  course: s/benchmark/bookmark/g