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)
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)
2.52 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
Component: Untriaged → Bookmarks & History
Comment 1•8 years ago
|
||
(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•8 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.
Comment 3•8 years ago
|
||
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.
Updated•8 years ago
|
Reporter | ||
Comment 4•8 years ago
|
||
And no, it doesn't dismiss the menu.
Assignee | ||
Comment 5•8 years ago
|
||
Reporter | ||
Comment 6•8 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #5) > Created attachment 8738986 [details] [diff] [review] > patch Perfect. Thank you!
Comment 7•8 years ago
|
||
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+
Comment 9•8 years ago
|
||
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•8 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)
Comment 11•8 years ago
|
||
(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•8 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.
Comment 13•8 years ago
|
||
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•8 years ago
|
||
I removed the removeObserver call from onItemRemoved. This fixes the failure.
Attachment #8738986 -
Attachment is obsolete: true
Attachment #8741640 -
Flags: review?(mak77)
Comment 15•8 years ago
|
||
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)
Comment 16•8 years ago
|
||
fwiw, filed bug 1264904
Updated•8 years ago
|
Attachment #8741640 -
Flags: review?(mak77) → review+
Backed this out for mochitest-bc leaks: https://hg.mozilla.org/integration/fx-team/rev/355e9b83a662 https://treeherder.mozilla.org/logviewer.html#?job_id=8732732&repo=fx-team
Flags: needinfo?(dao+bmo)
Updated•8 years ago
|
Priority: -- → P5
Updated•8 years ago
|
status-firefox46:
--- → unaffected
status-firefox47:
--- → disabled
status-firefox48:
--- → affected
Reporter | ||
Comment 19•8 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•8 years ago
|
||
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•8 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)
Comment 22•8 years ago
|
||
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•8 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.
Comment 24•8 years ago
|
||
you could store bookmarksObserver in BookmarkingUI._recentlyBookmarkedObserver and it would be kept alive with the window.
Flags: needinfo?(mak77)
Assignee | ||
Comment 25•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f38c1159471
Attachment #8748081 -
Attachment is obsolete: true
Attachment #8748596 -
Flags: review?(mak77)
Comment 26•8 years ago
|
||
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•8 years ago
|
||
Another Try run with the explicit removeObserver call: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d568c1e5bbd
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/26ac3fcfa7f3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 30•8 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)
Comment 31•8 years ago
|
||
filed bug 1272013, I wonder if that ended up creating some cycle the collector couldn't break.
Flags: needinfo?(mak77)
Comment 32•8 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]
Comment 33•8 years ago
|
||
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!
You need to log in
before you can comment on or make changes to this bug.
Description
•