1605881 - When trying to save a new bookmark, folder selection changes to the same folder no matter which folder is picked.

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Vitaly]

Attachment: Screencast 2019-12-24 11:40:55.mp4

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

Trying to save a new bookmark to a specific folder either by clicking on the start icon in the url bar or by pressing Ctrl + D.
Issue is shown in the attached video.

Actual results:

A new bookmark menu appeared, when selecting which folder to save the bookmark to, folder changes by itself a second after selection.

Expected results:

Folder should not automatically change and should stay at whichever folder was picked by the user.

Comment 1

[Mark Banner (:standard8)]

I can confirm this on latest nightly on Mac, it looks like it is related to Firefox Sync, as I can't reproduce without that disabled.

Approximate STR:

1. Have Sync enabled (especially bookmark sync).
2. Create a couple of sub-folders in other bookmarks.
3. Use the Star UI to save a bookmark into those folders, this is to pre-seed the Folder menu with a couple of "recent" entries.
4. Visit a page and create a bookmark, save it into one of the new folders.
5. Open up the Star UI again, and from the folder menu select "Other Bookmarks" or another top-level folder (I'm not sure if top-level folder is critical here, other folders seem to generally trigger this as well).

Expected Results

The bookmark is moved correctly.

Actual Results

After a second or so, the bookmark reverts back to the original folder.

I did this a few times and could repeat it. I also tried turning off the bookmarks sync a couple of times, and when I did, then I couldn't reproduce it any more.

Comment 3

[Molly Harris]

Experiencing same (with great irritation) and I just closed my own report (#1609777) as a dupe of this.

I have tried sort of wiping my Sync clean, by wiping out home and work installs' bookmarks, deleting Sync account, and importing the bookmarks from HTML, but it's ineffective: either the problem is recreated from some other unknown causative factor, or I'm somehow missing a step (in which case it would be useful to have something step-by-step laid out by more knowledgeable people).

I do think the issue is somewhere administrative on Mozilla's end, as the problem just began occurring out of nowhere, somewhere around the same time (or earlier?) as this thread's reporter.

Comment 4

[Molly Harris]

I will add to this bug report some findings that might be of use:

(1) As far as I can tell, the problem does not occur if the bookmark tree is expanded within the Star UI.

(2) As far as I can tell, the problem does not occur outside of the Star UI, i.e., in another bookmark UI.

(3) In case it in any way helps track down the cause, I used the Firefox recommended extension "Default Bookmark Folder". When these problems began, I uninstalled it in case it was the source of the problem. The extension's function itself (to set a different default than 'Other' -- and, really, sorry for the aside but I have no idea why that's been set as the default!) has no effect in repairing this issue. But perhaps installing that somehow causes the problem ...

Comment 5

[Kestrel]

Several reports of this on reddit in the last month, earliest mid-December:
Video (since deleted) (2020-01-02)
Mentions using Sync (2020-01-01)
Video with Sync enabled (2019-12-22)
Mentions it occurring with new profiles (2019-12-13)

I can reproduce it myself with Sync enabled and it does not appear to be a regression in Firefox itself.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

I'm still trying to get my head around this, but out of time for this week, so looking for a little help to get me kick-started next week :)

I'm somewhat confident that there's some batching of tree view events which is interacting strangely with sync. Sync will notify of onBeginUpdateBatch/onEndUpdateBatch and I believe this reproduces when the move happens in between. The stack which causes the "bad" move is:

...
promise callback*enqueue@resource://gre/modules/PlacesTransactions.jsm:483:33
transact@resource://gre/modules/PlacesTransactions.jsm:566:42
transact@resource://gre/modules/PlacesTransactions.jsm:222:36
onFolderMenuListCommand@chrome://browser/content/places/editBookmark.js:867:10
oncommand@chrome://browser/content/browser.xhtml:1:18
onFolderTreeSelect@chrome://browser/content/places/editBookmark.js:908:16
onselect@chrome://browser/content/browser.xhtml:1:18
PTV__batching@chrome://browser/content/places/treeView.js:1179:7
notifyObserver@resource://gre/modules/SyncedBookmarksMirror.jsm:2479:29
notifyBookmarkObservers@resource://gre/modules/SyncedBookmarksMirror.jsm:2464:12
...
_processIncoming@resource://services-sync/engines/bookmarks.js:888:39
...
async*syncIfMPUnlocked/<@resource://services-sync/policies.js:669:20

However, I'm confident this is a "delayed" async stack - _processIncoming has returned at this point - and I suspect it's actually from a sync that's fully completed, but not sure. Best I can tell we are awaiting in all the right places - so I believe the issue is related to PTV__batching@chrome://browser/content/places/treeView.js:1179:7 and some magic that is queuing events while a batch is in progress.

However, line 1179 is this line - which doesn't make sense - best I can tell, some bool properties (not setters) are being set. I can't make sense of the batching code in that file or how these events are queued.

Mark or Mak, if either of you understand how that works and can help me work out what the intention behind this._inBatchMode, selectEventsSuppressed and the queuing mechanism in general, or you know who would know, I'd really appreciate it.

Comment 7

[Mark Banner (:standard8)]

Thanks for looking into this Mark.

The this.selection.selectEventSuppressed is actually a setter on the tree view.

That setter will cause the selection to be updated. The basic intent here being that we're not spending time continuously changing the selection on the UI whilst handling multiple updates during the batch. I think most of that is handled within the tree view, but I'm not fully sure.

iirc most (all?) of places doesn't do batch handling now, that happened as part of the transition to async. The new notification system can handle its own batching but we've only migrated a couple of callers over to it and we're not really using the batching yet. This is probably why we don't see it with Sync disabled.

I'm not sure if it'd be suitable for Sync to drop it - presumably we'd still want it for some cases at least.

Maybe Marco will have some ideas here as well.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Banner (:standard8) from comment #7)

Thanks for looking into this Mark.

The this.selection.selectEventSuppressed is actually a setter on the tree view.

Right! And when that's set to false, it unconditionally sends a "select" event to the tree view. This is the root of the problem and helped me work out how to reproduce this without Sync, although it's difficult - involves lots of "disable autohide" and console commands - but I can describe it. I can't, however, work out who exactly to blame :)

• If you add a dump() to the tree view's owner's tree setter you will be able to observe that when you click on the bookmark Star and expand the tree view, the tree is first created.

• If you click outside the bookmark editor, the popup is no longer shown - but that dump will not fire - the bookmark editor stays alive and its tree view stays alive. This is important, because below, we will see how it may take a kind of "deferred action" even if it is not visible.

• If the tree view's owner is alive when an onBeginUpdateBatch notification is received, then 'select' events are suppressed - so selecting a new folder in the tree view will not move the bookmark after this notification.

• It's now possible to have that treeview hide, but stay alive. Using the other widget in the editor, you can perform a move of the item - that move happens immediately, because it's not via a tree-view widget, just via the tree "dropdown" (it's complicated that there are, basically, 2 ways to specify the parent in that dialog)

• Now if the tree view observes onEndUpdateBatch it unsuppresses select events, so a select event is fired - but that tree view no longer accurately reflects the parent - the end result is another move! The stack for the move then looks like the one above (ie, via PTV_Batching)

So I'm now convinced the problem is simply that the batching mechanisms are simply broken. Other ways they are broken include the fact that if a treeview is creating during a batch some wrong things may happen.

So given:

(In reply to Mark Banner (:standard8) from comment #7)

iirc most (all?) of places doesn't do batch handling now, that happened as part of the transition to async. The new notification system can handle its own batching but we've only migrated a couple of callers over to it and we're not really using the batching yet. This is probably why we don't see it with Sync disabled.

and after chatting to Lina, I believe the best thing to do now is to simply stop Sync from sending the batching notification. I'll put a patch up tomorrow, so speak now or forever hold your peace ;)

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1605881 - have Sync not send batch notifications to places to avoid spurious bookmark moves. r?lina

Comment 10

[Marco Bonardo [:mak]]

(In reply to Mark Hammond [:markh] [:mhammond] from comment #8)

• If you click outside the bookmark editor, the popup is no longer shown - but that dump will not fire - the bookmark editor stays alive and its tree view stays alive. This is important, because below, we will see how it may take a kind of "deferred action" even if it is not visible.

You are right, the folder tree is never killed. I was expecting us to kill it and recreate it at open, especially because thhe starring operation doesn't happen so often. If it doesn't cause bad things, we should really destroy and recreate the tree in editBookmark.js::toggleFolderTreeVisibility.

• If the tree view's owner is alive when an onBeginUpdateBatch notification is received, then 'select' events are suppressed - so selecting a new folder in the tree view will not move the bookmark after this notification.
• It's now possible to have that treeview hide, but stay alive. Using the other widget in the editor, you can perform a move of the item - that move happens immediately, because it's not via a tree-view widget, just via the tree "dropdown" (it's complicated that there are, basically, 2 ways to specify the parent in that dialog)
• Now if the tree view observes onEndUpdateBatch it unsuppresses select events, so a select event is fired - but that tree view no longer accurately reflects the parent - the end result is another move!

The tree is not directly observing onEndUpdateBatch, it just waits for Batching(false) to be invoked by the result (that is what is actually observing onEndUpdateBatch). At the time onEndUpdateBatch is invoked, the underlying result should have updated itself, so Batching(false) should actually act on an updated tree.
Why do you say the tree view does no longer reflect the parent? It should be an updated tree.
Isn't what happens just that when we unsuppress select events the tree ends up reselecting the same element as before, but actually it should instead select a different (newly inserted) element?

So I'm now convinced the problem is simply that the batching mechanisms are simply broken. Other ways they are broken include the fact that if a treeview is creating during a batch some wrong things may happen.

I don't think batching is broken, I think we are not handling it properly.

There are a couple things we can do here, if I correctly understood all of your debugging (and many thanks for doing it, it's extremely valuable);

1. when the foldertree is hidden, destroy the tree, recreate it on showing.
2. when the foldertree is hidden, bailout of onFolderTreeSelect() asap. And imo this is the real bug, why are we handling events by something that is hidden to the user?!?

Long term, we should do what me and Standard8 discussed multiple times, STOP having live-update dialogs. The star, Library and Properties dialogs should only act on confirmation.

and after chatting to Lina, I believe the best thing to do now is to simply stop Sync from sending the batching notification. I'll put a patch up tomorrow, so speak now or forever hold your peace ;)

Note this may have visible ui perf implications yet (but I can't tell by how much). Surely the long term plan is to remove batch notifications, we're just not there, so it's really hard atm to tell what breaks.

Comment 11

[Marco Bonardo [:mak]]

It may also be interesting to distinguish cases where the user selects, VS cases where the tree selects, and invoke onFolderTreeSelect only in the former case. We should not really act on events that are not caused by an explicit user action.
This is all caused by the fancy way editBookmark.js uses the treeview.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d78fe33901aa
have Sync not send batch notifications to places to avoid spurious bookmark moves. r=lina

Comment 15

[Razvan Maries]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 9126255 [details]
Bug 1605881 - have Sync not send batch notifications to places to avoid spurious bookmark moves. r?lina

Beta/Release Uplift Approval Request

• User impact if declined: Sync users may find that bookmarks they move revert back to the original folder for no apparent reason
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: As described in the bug. Note that this is somewhat difficult to reproduce, hence me saying manual testing is valuable - while I can no longer repro it, someone more determined may be able to.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Limited to sync users. A first sync for these users when they have some bookmarks UI open may be slower as the UI may update as bookmarks come in via sync rather than at the end - although I suspect this will not be noticeable in practice.

Re automated tests: the code is covered by tests, but there are no new tests as part of the patch.

• String changes made/needed: None

Comment 18

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9126255 [details]
Bug 1605881 - have Sync not send batch notifications to places to avoid spurious bookmark moves. r?lina

Looks safe for beta, uplift approved for 74.0b6, thanks.

Comment 19

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/releases/mozilla-beta/rev/02ed219795f9

Comment 20

[Timea Cernea [:tbabos][inactive]]

Reproduced on affected Release 73.0.1 (64-bit) and Beta 74.0b5 on Windows 10 x64.
Verified- fixed on latest Nightly 75.0a1 (2020-02-20) (64-bit) and latest Beta 74.0b6 on Windows 10 x64, MacOS 10.15 and Ubuntu 18.04.