Renaming live bookmark while adding it renames the wrong one

VERIFIED FIXED in Firefox 41

Status

()

--
critical
VERIFIED FIXED
3 years ago
a year ago

People

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

Tracking

({dataloss, regression})

40 Branch
mozilla43
dataloss, regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox40+ wontfix, firefox41+ fixed, firefox42+ verified, firefox43+ verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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

3 years ago
[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
Blocks: 1094900
Severity: normal → critical
Status: UNCONFIRMED → NEW
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
tracking-firefox40: --- → ?
tracking-firefox41: --- → ?
tracking-firefox42: --- → ?
tracking-firefox43: --- → ?
Component: Untriaged → Places
Ever confirmed: true
Flags: needinfo?(mak77)
Keywords: dataloss, regression
Product: Firefox → Toolkit
Version: 42 Branch → 40 Branch

Comment 2

3 years ago
Tracked for 41+ as it's a regression.
tracking-firefox41: ? → +
tracking-firefox42: ? → +
tracking-firefox43: ? → +
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?
status-firefox40: affected → wontfix
tracking-firefox40: ? → +
Flags: needinfo?(ttaubert)
(Assignee)

Comment 5

3 years ago
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)
(Assignee)

Comment 6

3 years ago
(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)
(Assignee)

Comment 7

3 years ago
Created attachment 8655506 [details] [diff] [review]
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.
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+
https://hg.mozilla.org/mozilla-central/rev/6dfed2a7a155
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 11

3 years ago
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)
(Assignee)

Comment 17

3 years ago
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)
(Assignee)

Comment 18

3 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/1620ef4c1341
status-firefox41: affected → fixed
(Assignee)

Updated

3 years ago
Blocks: 1197821

Updated

3 years ago
Depends on: 1206350

Updated

3 years ago
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)
(Assignee)

Comment 20

3 years ago
thanks for the heads up, fwiw it was enough to needinfo in the regression bug.
Flags: needinfo?(ttaubert)
Flags: needinfo?(mak77)

Updated

3 years ago
Depends on: 1208063
(Assignee)

Updated

3 years ago
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.
status-firefox42: fixed → verified
Verified fixed on Windows 7 64bit and Mac OS X 10.9.5 using Firefox 43 Beta 1 (BuildID: 20151103023037).
Status: RESOLVED → VERIFIED
status-firefox43: fixed → verified
(Reporter)

Updated

a year ago
Flags: needinfo?(a.j.buxton)
You need to log in before you can comment on or make changes to this bug.