Closed Bug 1391393 Opened 7 years ago Closed 7 years ago

Canceling Properties of bookmark dialog makes undoing previous editing

Categories

(Firefox :: Bookmarks & History, defect, P1)

Product:
Firefox
Component:
Bookmarks & History
Version:
57 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 + verified
firefox58 --- verified

People

(Reporter: alice0775, Assigned: standard8)

References

Details

(Keywords: dataloss, regression, Whiteboard: [fxsearch])

Attachments

(1 file, 1 obsolete file)

Bug 1391393 - Cancelling the bookmarks properties dialog shouldn't undo when no changes have been made.
59 bytes, text/x-review-board-request
mak
: review+
Details 
@Mupasa reported, see http://forums.mozillazine.org/viewtopic.php?p=14761724#p14761724

Reproducible: always

Steps To Reproduce:
1. Delete, Add or Edit any bookmarks
2. Right click on any bookmarks and choose Properties
3. Cancel the dialog

Actual Results:
The previously edited bookmarks are unexpectedly undone.

Expected Results:
Do nothing


Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0da00124af43d44fed96133300ba5e32b0d821a8&tochange=0a4690dfd7b383e2f732210cf8906ce51a5b2433

Regressed by:0a4690dfd7b3	Mark Banner — Bug 1071513 - Enable async PlacesTransactions for nightly builds. r=mak
[Tracking Requested - why for this release]: dataloss
tracking-firefox57: --- → ?
Priority: -- → P1
Whiteboard: [fxsearch]
More detailed STR:

1) Display the bookmarks toolbar.
2) Add bookmarks to the toolbar if necessary.
3) Right-click on a bookmark
4) Select Delete
-> Bookmark is deleted

5) Right-click on a different bookmark
6) Select Properties.
7) Select `Cancel` on the Bookmark Properties dialog.

Expected Results:

-> Nothing changes at this stage

Actual Results

-> The bookmark deleted at step 4 comes back again.
The steps in comment 2 also work with the sidebar as well the toolbar. They might also work on Windows/Linux in the Bookmarks menu.
Assignee: nobody → standard8
I've just realised the important part here, is that in comment 2, there are no actions between steps 6 and 7 - i.e. the properties dialog doesn't have anything changed.

What is happening is that the properties dialog is shown, so it sets up a new batch mode, when it gets cancelled, it ends the batch, and then assumes it can undo it. However, the batch is empty, so doesn't actually exist, and it ends up undoing the previous action - the delete of the bookmark.

I suspect the old transaction code would have stored an entry regardless, which is why it wasn't an issue there.

However, I think it would be better if the dialog didn't call undo if no actions have taken place. I think we can keep a record of the top undo entry when starting the batch, and see if it is the same or not after the batch has finished.
Status: NEW → ASSIGNED
Comment on attachment 8900686 [details]
Bug 1391393 - Cancelling the bookmarks properties dialog shouldn't undo when no changes have been made.

https://reviewboard.mozilla.org/r/172124/#review177484

::: browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js:61
(Diff revision 1)
> +        await BrowserTestUtils.waitForCondition(() => !acceptButton.disabled,
> +          "The accept button should be enabled");
> +      }
> +    );
> +
> +    Assert.ok(PlacesTransactions.undo.notCalled, "undo should not have been called");

may we also check the bookmark we removed is not there?

::: browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js:83
(Diff revision 1)
> +          "The accept button should be enabled");
> +
> +        let promiseKeywordNotification = PlacesTestUtils.waitForNotification(
> +          "onItemChanged", (itemId, prop, isAnno, val) => prop == "keyword" && val == "kw");
> +
> +        fillBookmarkTextField("editBMPanel_keywordField", "kw", dialogWin);

I'd prefer if you'd change the title, keywords may change in the future, the title is unlikely.
Attachment #8900686 - Flags: review?(mak77) → review+
Comment on attachment 8902440 [details]
Bug 1391393 - When waiting on notifications, ensure async updates have completed before moving on.

Bah, these browser places tests really don't want to play nice - I've now got a memory leak showing up on one of them.
Attachment #8902440 - Flags: review?(mak77)
Bug 1395994 fixes the memory leak.
Depends on: 1395994
Attachment #8902440 - Attachment is obsolete: true
Try build with bug 1395994 and the updated patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e124d6f0244f080e2b87e5217d9fe64f3b547865

The updated patch includes a couple of test-only tweaks to ensure tests correctly pass and don't hit intermittent issues.
One more minor update to skip the test when async transactions are turned off.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ef718d6269b
Cancelling the bookmarks properties dialog shouldn't undo when no changes have been made. r=mak
https://hg.mozilla.org/mozilla-central/rev/4ef718d6269b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Flags: qe-verify+
Build ID: 20171016220427
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0

Verified as fixed on Firefox Nightly 58.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox58: --- → verified
Also verified that this issue is fixed using Firefox 57 beta 10 across platforms (Windows 10 64bit, macOS 10.12.6 and Ubuntu 16.04 32bit).
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: