Closed Bug 1194568 Opened 9 years ago Closed 9 years ago

Renaming live bookmark while adding it renames the wrong one


(Toolkit :: Places, defect)

40 Branch
Not set



Tracking Status
firefox40 + wontfix
firefox41 + fixed
firefox42 + verified
firefox43 + verified


(Reporter: a.j.buxton, Assigned: mak)



(Keywords: dataloss, regression)


(1 file)

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
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.
[Tracking Requested - why for this release]: Data loss

1. Delete all bookmarks from the bookmarks toolbar.
2. Navigate to an RSS feed eg
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


Regressed by:
6126a2cc97c6	Marco Bonardo — Bug 1094900 - Livemarks service should use the new Bookmarks.jsm. r=ttaubert
Blocks: 1094900
Severity: normal → critical
Component: Untriaged → Places
Ever confirmed: true
Flags: needinfo?(mak77)
Keywords: dataloss, regression
Product: Firefox → Toolkit
Version: 42 Branch → 40 Branch
I gave this a try in the current Nightly, build 20150820030202 and could not get a response from clicking "Subscribe" at all.
This is too late for 40 but we should take a patch in 41.
Tim, do you know who could help with this bug?
Flags: needinfo?(ttaubert)
taking, but no promises on an ETA cause I'm overloaded at the moment, if anyone wants to contribute a patch, feel free to.
Assignee: nobody → mak77
Flags: needinfo?(mak77)
Flags: needinfo?(ttaubert)
(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)
Attached patch patch v1Splinter Review
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.
Attachment #8655506 - Flags: review?(ttaubert)
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
Attachment #8655506 - Flags: review?(ttaubert) → review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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
Attachment #8655506 - Flags: approval-mozilla-beta?
Attachment #8655506 - Flags: approval-mozilla-aurora?
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.
Attachment #8655506 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Alistair, could you please verify the fix on 09/05 Nightly or later? Thanks in advance.
Flags: needinfo?(a.j.buxton)
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.
Attachment #8655506 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
Blocks: 1197821
Depends on: 1206350
Depends on: 1206376
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?
Flags: needinfo?(ttaubert)
Flags: needinfo?(mak77)
thanks for the heads up, fwiw it was enough to needinfo in the regression bug.
Flags: needinfo?(ttaubert)
Flags: needinfo?(mak77)
Depends on: 1208063
No longer depends on: 1208063
Flags: qe-verify+
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.
Verified fixed on Windows 7 64bit and Mac OS X 10.9.5 using Firefox 43 Beta 1 (BuildID: 20151103023037).
Flags: needinfo?(a.j.buxton)
You need to log in before you can comment on or make changes to this bug.