1194568 - Renaming live bookmark while adding it renames the wrong one

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Alistair Buxton]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150811134815

Steps to reproduce:

1. Delete all bookmarks from the bookmarks toolbar.
2. Navigate to an RSS feed eg http://feeds.bbci.co.uk/news/rss.xml?edition=uk
3. Click "Subscribe" - a dialog appears.
4. Try to edit the live bookmark name in the dialog. This does not work.
5. Add the bookmark.
6. Repeat steps 2-4. This time you can edit the bookmark name.
7. Add the bookmark.


Actual results:

When adding the first bookmark it's name cannot be modified.
When adding the second bookmark the first bookmark is renamed instead.


Expected results:

Renaming a live bookmark while adding it should rename the live bookmark you are adding, not a different one.

Always reproducible including in a non-e10s window.

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: Data loss

1. Delete all bookmarks from the bookmarks toolbar.
2. Navigate to an RSS feed eg http://feeds.bbci.co.uk/news/rss.xml?edition=uk
3. Click "Subscribe" - a dialog appears.
4. Try to edit the live bookmark name in the dialog. and click [Subscribe]

Actual results:
A existing bookmark is renamed. --- this is critical bug



Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=1a0302982302&tochange=6126a2cc97c6

Regressed by:
6126a2cc97c6	Marco Bonardo — Bug 1094900 - Livemarks service should use the new Bookmarks.jsm. r=ttaubert

Comment 2

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

Tracked for 41+ as it's a regression.

Comment 3

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

I gave this a try in the current Nightly, build 20150820030202 and could not get a response from clicking "Subscribe" at all.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

This is too late for 40 but we should take a patch in 41.
Tim, do you know who could help with this bug?

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

taking, but no promises on an ETA cause I'm overloaded at the moment, if anyone wants to contribute a patch, feel free to.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #3)
> I gave this a try in the current Nightly, build 20150820030202 and could not
> get a response from clicking "Subscribe" at all.

RSS Feed UI is broken in e10s mode (bug 1109714)

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1

The issue is due to the fact addLivemark now is no more fake-async, it is really async (the folder is not created synchronously when the method is invoked). We were arriving there sooner or later.

To properly patch this we'd need PlacesTransactions.jsm, since we don't yet have it, I'm hacking around the existing transaction.

Will push to Try with the other patches in queue soon.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8655506 [details] [diff] [review]
patch v1

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

::: browser/components/places/tests/browser/browser_bookmarkProperties_addLivemark.js
@@ +8,5 @@
> +    tree.selectItems([itemId]);
> +
> +    yield withBookmarksDialog(
> +      true,
> +      function openDialog() { 

Nit: trailing space

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6dfed2a7a155

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8655506 [details] [diff] [review]
patch v1

Approval Request Comment
[Feature/regressing bug #]: bug 1094900
[User impact if declined]: editing a livemark title will edit a previously added bookmark title
[Describe test coverage new/current, TreeHerder]: unit-test
[Risks and why]: nothing specific we can predict
[String/UUID change made/needed]: none

Comment 12

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

Comment on attachment 8655506 [details] [diff] [review]
patch v1

Given the risk assessment provided, I do not feel comfortable landing this in Beta41 just yet. Let's uplift to Aurora and determine if there are any unexpected fall outs. Aurora42+ only.

Comment 13

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

Alistair, could you please verify the fix on 09/05 Nightly or later? Thanks in advance.

Comment 14

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

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

Comment 15

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

Comment on attachment 8655506 [details] [diff] [review]
patch v1

This patch has been in Aurora for 3 days so far, seems safe to uplift to Beta41.

Comment 16

[Carsten Book [:Tomcat]]

Hi Mak, i guess this needs a rebase because of the other changes that landed because i got conflicts

merging browser/components/places/tests/browser/browser.ini failed!
merging browser/components/places/tests/browser/head.js                                                                                                                                                                                       
merging toolkit/components/places/PlacesUtils.jsm

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

it's just a conflict with bug 1193621, both are adding a test. I am pushing to try to check everything is fine, then I will land both.

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1620ef4c1341

Comment 19

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

mak, we may want to back this out as it caused regressions reported in bug 1206376 and bug 1206350.  The first regression seems worse to me and a potential last-minute blocker to the 41 release. 

Tim, can you help with this?

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

thanks for the heads up, fwiw it was enough to needinfo in the regression bug.

Comment 21

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed using Firefox 42 beta 6 under Win 7 64-bit and Mac OS X 10.9.5.

I verified this is fixed on Aurora 43.0a2 2015-10-12 too, but only in non-e10s windows (due to bug 1109714), so I will not change the flag there.

Comment 22

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit and Mac OS X 10.9.5 using Firefox 43 Beta 1 (BuildID: 20151103023037).