the 5 recent bookmarks menu doesn't automatically update when one of those bookmarks is deleted

VERIFIED FIXED in Firefox 49

Status

()

defect
P5
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: ht990332, Assigned: dao)

Tracking

(Blocks 1 bug)

47 Branch
Firefox 49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 unaffected, firefox47 disabled, firefox48 affected, firefox49 verified)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

3 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160222170014

Steps to reproduce:

1) Delete a bookmark to http://some-news-website.com from a bookmark folder called News.


Actual results:

Notice that the link doesn't disappear from the new "Recently Bookmarked" menu until the I dismiss the bookmarks menu and open it again from the bookmarks toolbar icon.
Reporter

Updated

3 years ago
Component: Untriaged → Bookmarks & History
(In reply to Hussam Al-Tayeb from comment #0)
> Notice that the link doesn't disappear from the new "Recently Bookmarked"
> menu until the I dismiss the bookmarks menu and open it again from the
> bookmarks toolbar icon.

how do you remove the bookmark without dismissing the menu?
Btw no, those entries doon't live-update.
Reporter

Comment 2

3 years ago
(In reply to Marco Bonardo [::mak] from comment #1)
> (In reply to Hussam Al-Tayeb from comment #0)
> > Notice that the link doesn't disappear from the new "Recently Bookmarked"
> > menu until the I dismiss the bookmarks menu and open it again from the
> > bookmarks toolbar icon.
> 
> how do you remove the bookmark without dismissing the menu?
> Btw no, those entries doon't live-update.

I clicked on the bookmarks toolbar icon. I navigated to "News" folder. I deleted a link. The link disappeared from that folder but was still in the 'recently bookmarked' section.
At first glance, users will think their bookmark was not fully deleted.
I fear there's no "trivial" solution, live-update is only supported in places views and here we deal with static content.
The only workaround may be to add a bookmarks observer while the menu is open... not exactly future proof, but may work.
Blocks: 1219804
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter

Comment 4

3 years ago
And no, it doesn't dismiss the menu.
Assignee

Comment 5

3 years ago
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8738986 - Flags: review?(mak77)
Reporter

Comment 6

3 years ago
(In reply to Dão Gottwald [:dao] from comment #5)
> Created attachment 8738986 [details] [diff] [review]
> patch

Perfect. Thank you!
Comment on attachment 8738986 [details] [diff] [review]
patch

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

::: browser/base/content/browser-places.js
@@ +1414,5 @@
> +      onBeginUpdateBatch() {},
> +      onEndUpdateBatch() {},
> +      onItemRemoved() {
> +        removeObserver();
> +        BookmarkingUI._updateRecentBookmarks(aHeaderItem, extraCSSClass);

Clearly this is not going to work on Mac native menubar since it doesn't live update.
Not a big deal, I don't think it's so common to exactly remove one of the recently added bookmarks... But may be worth a comment.

@@ +1420,5 @@
> +      onItemChanged() {},
> +      onItemVisited() {},
> +      onItemMoved() {}
> +    };
> +    PlacesUtils.addLazyBookmarkObserver(observer, false);

I don't think in this case it is useful to use a lazy observer, since we just used the bookmarks service to populate the menu.
Thus there's no risk of initializing the bookmarks service inadvertently.
Attachment #8738986 - Flags: review?(mak77) → review+
Backed this out for test failures in browser-chrome browser_views_liveupdate.js, OS X xpcshell test_browserGlue_migrate.js and maybe more

Backout: https://hg.mozilla.org/integration/fx-team/rev/9c492faa4c39
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=1ea84d818c7d58b4bde2766fa9f6e64818d6822d

Bug 1248617 got also backed out, likely cause for the M(5) failures.
Flags: needinfo?(dao)
Assignee

Comment 10

3 years ago
Seems like adding a bookmarks observer that removes itself in onItemRemoved makes browser_views_liveupdate.js fail. Is the bookmarks observer backend just broken, Marco?
Flags: needinfo?(dao+bmo) → needinfo?(mak77)
(In reply to Dão Gottwald [:dao] from comment #10)
> Seems like adding a bookmarks observer that removes itself in onItemRemoved
> makes browser_views_liveupdate.js fail. Is the bookmarks observer backend
> just broken, Marco?

I don't think it's broken, when notifying we use a copy of the observers arrays, so the fact it's removed shouldn't matter...

I rather suspect the problem is in browser_views_liveupdate.js itself, since it looks for the actual DOM node in the menu, and it could try to use  the recent bookmark entry instead of the entry it was expecting to manage (the real one).
I'll try to reproduce this and check if I'm right, ideally we should be able to only walk the non-static part of the menu instead of all the menu childNodes.
Assignee

Comment 12

3 years ago
(In reply to Marco Bonardo [::mak] from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > Seems like adding a bookmarks observer that removes itself in onItemRemoved
> > makes browser_views_liveupdate.js fail. Is the bookmarks observer backend
> > just broken, Marco?
> 
> I don't think it's broken, when notifying we use a copy of the observers
> arrays, so the fact it's removed shouldn't matter...
> 
> I rather suspect the problem is in browser_views_liveupdate.js itself, since
> it looks for the actual DOM node in the menu, and it could try to use  the
> recent bookmark entry instead of the entry it was expecting to manage (the
> real one).

I modified the patch such that it doesn't do anything beyond adding and removing the observer. The test still failed.
That's surprising! Off-hand I don't know why that may be happening, I can try to do some debugging tomorrow, and will let you know what I find.
Assignee

Comment 14

3 years ago
Posted patch patch v2 (obsolete) — Splinter Review
I removed the removeObserver call from onItemRemoved. This fixes the failure.
Attachment #8738986 - Attachment is obsolete: true
Attachment #8741640 - Flags: review?(mak77)
ok, I was wrong, we DO NOT copy the array before notifying. That means when you remove an observer during a notification, we will skip the next one.
On the other side, making a copy would still notify a future observer in case you remove it in a previous observer.
Both approaches have their pro/cons.

Though, now that we are exposing a getObservers() API to JS, we return a copy from it.
So it makes sense to be consistent and always use a copied array.
I'll file a bug for that, in the meanwhile your workaround should be fine.
Flags: needinfo?(mak77)
fwiw, filed bug 1264904
Attachment #8741640 - Flags: review?(mak77) → review+
Reporter

Comment 19

3 years ago
Is it possible that we can get a new patch against current trunk?
Or does this patch currently cause regressions?
Thank you.
Assignee

Comment 20

3 years ago
Posted patch patch v3 (obsolete) — Splinter Review
Updated after bug 1248268, needs fresh try run, probably still suffers from the leak
Attachment #8741640 - Attachment is obsolete: true
Flags: needinfo?(dao+bmo)
Assignee

Comment 21

3 years ago
Yep, the leak is still there:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec1979763cf6&selectedJob=20291108

The updated patch is trivial and I don't see what could have gone wrong there. Could this be another bug in the bookmarks observer backend?
Flags: needinfo?(mak77)
does it leak if you use a weak reference (second argument to true)?
That may be the simplest fix and we'd have time to investigate it later.
Assignee

Comment 23

3 years ago
I haven't tried that yet. I was hesitant to use a weak reference, since I'm not sure what would hold on to the observer here and when it would go away.
you could store bookmarksObserver in  BookmarkingUI._recentlyBookmarkedObserver and it would be kept alive with the window.
Flags: needinfo?(mak77)
Comment on attachment 8748596 [details] [diff] [review]
patch v4

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

Ok, let's see what Try thinks.

::: browser/base/content/browser-places.js
@@ +1414,5 @@
>        if (event.target == event.currentTarget) {
>          updatePlacesContextMenu(true);
>  
>          Services.prefs.removeObserver(this.RECENTLY_BOOKMARKED_PREF, prefObserver, false);
> +        this._recentlyBookmarkedObserver = null;

I'd also still invoke removeObserver before nulling it out, so we don't depend too much on GC timings. Otherwise we may still receive notifications until gc collects it.
Attachment #8748596 - Flags: review?(mak77) → review+
Assignee

Comment 27

3 years ago
Another Try run with the explicit removeObserver call: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d568c1e5bbd

Comment 29

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/26ac3fcfa7f3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee

Comment 30

3 years ago
(In reply to Marco Bonardo [::mak] from comment #22)
> does it leak if you use a weak reference (second argument to true)?
> That may be the simplest fix and we'd have time to investigate it later.

NEEDINFO in case you still want to investigate this further / file a bug on it.
Flags: needinfo?(mak77)
Blocks: 1272013
filed bug 1272013, I wonder if that ended up creating some cycle the collector couldn't break.
Flags: needinfo?(mak77)

Comment 32

3 years ago
I have reproduced this bug with Nightly 47.0a1 (2016-02-22)on Windows 8.1 64 bit!

This bug's fix is verified on Latest Aurora 49.0a2.

Build ID : 20160720004018
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0.

[bugday-20160720]
Reproduced this issue in firefox nightly 47.0a1 (2016-02-22) with ubuntu 16.04 (64 bit)

Verified as this issue fixed with latest firefox aurora 49.0a2 (Build ID: 20160722004032)
Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0

As it is also verified on windows (Comment 32), Marking it as verified!
Status: RESOLVED → VERIFIED
QA Whiteboard: [testday-20160722]
You need to log in before you can comment on or make changes to this bug.