Closed Bug 1250203 Opened 8 years ago Closed 8 years ago

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

Categories

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

47 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 49
Tracking Status
firefox46 --- unaffected
firefox47 --- disabled
firefox48 --- affected
firefox49 --- verified

People

(Reporter: ht990332, Assigned: dao)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
(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
And no, it doesn't dismiss the menu.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8738986 - Flags: review?(mak77)
(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)
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.
(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.
Attached 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+
Is it possible that we can get a new patch against current trunk?
Or does this patch currently cause regressions?
Thank you.
Attached 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)
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.
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+
Another Try run with the explicit removeObserver call: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d568c1e5bbd
https://hg.mozilla.org/mozilla-central/rev/26ac3fcfa7f3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
(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)
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.