Last Comment Bug 760940 - Bookmarks and history menus behave incorrectly due to non-node weak map keys
: Bookmarks and history menus behave incorrectly due to non-node weak map keys
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: Firefox 17
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
Mentors:
: 772065 (view as bug list)
Depends on:
Blocks: 730340 761620 777005
  Show dependency treegraph
 
Reported: 2012-06-03 02:08 PDT by Josef Vitu
Modified: 2013-02-11 16:18 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
fixed


Attachments
Switch to Map (1.06 KB, patch)
2012-06-05 08:28 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
Switch to Map everywhere (2.53 KB, patch)
2012-06-05 09:38 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: review+
Details | Diff | Review
patch (4.46 KB, patch)
2012-06-06 07:44 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: review+
Details | Diff | Review
checked in (5.92 KB, patch)
2012-07-24 07:22 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
asaf: feedback+
gavin.sharp: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Josef Vitu 2012-06-03 02:08:32 PDT
The Bookmarks and History menus behave incorrectly in the recent nightly build, e.g. deleting a bookmark via the Bookmarks menu is not reflected in the menu (only in the Library). Tested with a clean profile.

Opening Bookmarks menu causes these warnings:

Warning: Error: Failed to preserve wrapper of wrapped native weak map key.
Source File: chrome://browser/content/places/browserPlacesViews.js
Line: 72, 330, 344

Opening History menu causes these warnings:

Warning: Error: Failed to preserve wrapper of wrapped native weak map key.
Source File: chrome://browser/content/places/browserPlacesViews.js
Line: 72, 344

Opening Bookmarks Manager (alias Library) causes several of these warnings:

Warning: Error: Failed to preserve wrapper of wrapped native weak map key.
Source File: chrome://browser/content/places/treeView.js
Line: 1211

Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120602134306
Comment 1 Mihaela Velimiroviciu (:mihaelav) 2012-06-04 01:37:54 PDT
Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/15.0 Firefox/15.0a1

I was able to reproduce the issue on Linux i686, as well.
Steps:
1. Open a page
2. Click on the star icon in the url bar (or CTRL+D) to create a bookmark
3. In the Page Bokkmark dialog, select "Bookmarks Menu" as a folder and submit the dialog
4. Go to Bookmarks menu
5. Right click on the bookmark and select "Delete" from the context menu

Actual result: Bookmark is not removed from Bookmarks menu, but it is removed from Library.
Also, when opening the History menu, I get the following error in Error Console:

Timestamp: 06/04/2012 11:30:41 AM
Warning: Error: Failed to preserve wrapper of wrapped native weak map key.
Source File: chrome://browser/content/places/browserPlacesViews.js
Line: 344
Comment 2 Philip Chee 2012-06-04 02:39:17 PDT
Probably regression from Bug 730340 - Don't use expando properties for storing data on places nodes
Comment 3 Marco Bonardo [::mak] 2012-06-04 06:56:43 PDT
I cannot reproduce, for me bookmarks are removed properly in today's Nightly, I see the warnings though.
Comment 4 Josef Vitu 2012-06-04 07:21:54 PDT
(In reply to Marco Bonardo [:mak] from comment #3)
> I cannot reproduce, for me bookmarks are removed properly in today's
> Nightly, I see the warnings though.

I can confirm that the problem concerning bookmark deletion seems to be resolved (although warnings remain).

Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120604030527
Comment 5 Marco Bonardo [::mak] 2012-06-05 03:40:22 PDT
So, we get a lot of these warnings, for basically each Places node we add to the weakmap, and we have no idea what the warning means and what's the action we should take there.
May someone who originally implemented it explain it to us?
Comment 6 Andrew McCreight [:mccr8] 2012-06-05 07:55:14 PDT
Yeah, sorry, I really need to make that message more comprehensible.

What it means is that the only C++ things you should add as weakmap keys are DOM nodes.  If you add other C++ things, then they may disappear without warning from the weak map, due to garbage collection and cycle collection.

In hindsight, maybe I should just make it deterministic that they are never even added.

What kind of things are you using as weak map keys?
Comment 7 Marco Bonardo [::mak] 2012-06-05 08:10:48 PDT
each DOM node in the browser ui is associated with a Places node (nsINavHistoryResultNode), due to GPC and cycles we moved from using expando to use WeakMaps, but now looks like WeakMaps can't be used as well?

Without both of those we have no more alternatives to keep the association.

(In reply to Andrew McCreight [:mccr8] from comment #6)
> What it means is that the only C++ things you should add as weakmap keys are
> DOM nodes.  If you add other C++ things, then they may disappear without
> warning from the weak map, due to garbage collection and cycle collection.

What does disappear mean, if there's something else keeping the object alive would it still disappear?
So, the warning means the weakmap won't keep the object used as key alive (and we'd be happy about that) or that it uses a special wrapper of that object and this may go away while the object is still alive?
Comment 8 Marco Bonardo [::mak] 2012-06-05 08:11:10 PDT
(In reply to Marco Bonardo [:mak] from comment #7)
> each DOM node in the browser ui

clearly in a Places view (menupopup, toolbar)
Comment 9 Marco Bonardo [::mak] 2012-06-05 08:20:28 PDT
(In reply to Marco Bonardo [:mak] from comment #7)
> Without both of those we have no more alternatives to keep the association.

We may use Map, though we also lose the nice weak side of WeakMaps.
Comment 10 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-05 08:28:58 PDT
Created attachment 630175 [details] [diff] [review]
Switch to Map
Comment 11 Andrew McCreight [:mccr8] 2012-06-05 08:30:24 PDT
(In reply to Marco Bonardo [:mak] from comment #7)
> each DOM node in the browser ui is associated with a Places node
> (nsINavHistoryResultNode), due to GPC and cycles we moved from using expando
> to use WeakMaps, but now looks like WeakMaps can't be used as well?

Can you get to the DOM node from the Places node?  In that case, you could use the underlying DOM node as the key.

> (In reply to Andrew McCreight [:mccr8] from comment #6)
> > What it means is that the only C++ things you should add as weakmap keys are
> > DOM nodes.  If you add other C++ things, then they may disappear without
> > warning from the weak map, due to garbage collection and cycle collection.
> 
> What does disappear mean, if there's something else keeping the object alive
> would it still disappear?
> So, the warning means the weakmap won't keep the object used as key alive
> (and we'd be happy about that) or that it uses a special wrapper of that
> object and this may go away while the object is still alive?
The weak map never keeps the key alive.  The problem is that the map may "forget" about the mapping that you added.

If you do this: map.set(key, data)

and then get the error message, then some time later when you look up |key| again using get, you may not get |data| back.  Instead it won't find anything.
Comment 12 Marco Bonardo [::mak] 2012-06-05 08:34:50 PDT
(In reply to Andrew McCreight [:mccr8] from comment #11)
> (In reply to Marco Bonardo [:mak] from comment #7)
> > each DOM node in the browser ui is associated with a Places node
> > (nsINavHistoryResultNode), due to GPC and cycles we moved from using expando
> > to use WeakMaps, but now looks like WeakMaps can't be used as well?
> 
> Can you get to the DOM node from the Places node?  In that case, you could
> use the underlying DOM node as the key.

these maps are our way to get the DOM node from the Places node.

> > (In reply to Andrew McCreight [:mccr8] from comment #6)
> If you do this: map.set(key, data)
> 
> and then get the error message, then some time later when you look up |key|
> again using get, you may not get |data| back.  Instead it won't find
> anything.

I understood this, what I don't get if it is sure to happen later, or if something else is keeping the key object alive it will just keep working.
Comment 13 Andrew McCreight [:mccr8] 2012-06-05 08:42:21 PDT
(In reply to Marco Bonardo [:mak] from comment #12)
> > Can you get to the DOM node from the Places node?  In that case, you could
> > use the underlying DOM node as the key.
> 
> these maps are our way to get the DOM node from the Places node.

Ah, okay.  That's a problem then.

> > and then get the error message, then some time later when you look up |key|
> > again using get, you may not get |data| back.  Instead it won't find
> > anything.
> 
> I understood this, what I don't get if it is sure to happen later, or if
> something else is keeping the key object alive it will just keep working.

It will stay alive if a real JS object holds onto the key object.  If the only reference to the key is from the C++ side, then it will go away.  Each C++ object used in JS has a "wrapper" that is a JS object.  The wrapper is used as the key, and is thrown away if only the C++ object holds it alive (this is an optimization).
Comment 14 Marco Bonardo [::mak] 2012-06-05 08:54:19 PDT
Thanks for explanation, now it's clear.
I think we'll just have to switch all of our WeakMaps to Maps.  This may cause some unwanted leaks if add-ons don't correctly use the APIs (like forgetting to close a container, or to unregister an observer), but I don't see any other ways out.

Basically all of these http://mxr.mozilla.org/mozilla-central/search?string=WeakMap&find=places
Comment 15 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-05 09:38:05 PDT
Created attachment 630194 [details] [diff] [review]
Switch to Map everywhere
Comment 16 Marco Bonardo [::mak] 2012-06-05 12:09:22 PDT
Comment on attachment 630194 [details] [diff] [review]
Switch to Map everywhere

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

r=me provided the tryrun is happy and doesn't show new leaks in mochitest-other
Comment 17 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-06 06:59:31 PDT
With this patch, test_bug549192.xul failed for good reason. I fixed that, but I would like to explained what happened:

1. That test calls view.treeIndexForNode for removed nodes, expecting it to return INDEX_INVISIBLE.
2. For different reasons, getRowForNode (called by treeIndexForNode) throws in this case for both the expandos-based and the weakmap-based implementations of treeView.
2.1. In the weakmap case, it threw because we were trying to call hasCachedLivemarkInfo for node.parent (which is null for removed nodes). Weakmaps don't allow that (key cannot be null).
3. So, the exception was caught, and we got to |return Ci.nsINavHistoryResultTreeViewer.INDEX_INVISIBLE;| in treeIndexForNode. That's apparently what has always happened.
4. With the Map-based implementation, nothing throws (key can be null), and getRowForNode just returned the default value for the row variable there... -1.
5. But Ci.nsINavHistoryResultTreeViewer.INDEX_INVISIBLE is not -1!

So, the fix:
1. Check in getRowForNode if the node is removed (node.parent != null)
2. report the exception we swallow in treeIndexForNode.
Comment 18 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-06 07:44:57 PDT
Created attachment 630567 [details] [diff] [review]
patch
Comment 19 Marco Bonardo [::mak] 2012-06-06 14:58:01 PDT
Comment on attachment 630567 [details] [diff] [review]
patch

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

::: browser/components/places/content/treeView.js
@@ +1126,5 @@
>      try {
>        return this._getRowForNode(aNode, true);
>      }
> +    catch(ex) {
> +      Components.utils.reportError(ex);

Though, we should probably not report the exception for invisible nodes, since those are expected... and based on the fix also removed nodes may be expected.
So is not this going to pollute the console while using the treeview normally?
Comment 20 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-11 04:40:46 PDT
Sure, I can remove this change, at least for now.
Comment 21 Marco Bonardo [::mak] 2012-06-14 05:33:06 PDT
Comment on attachment 630567 [details] [diff] [review]
patch

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

yeah, please remove the reportError for now, sounds like may just be problematic
Comment 22 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-14 05:52:16 PDT
Oh, hrm, there's the (now-long-standing) issue with the livemarks service too, right?
Comment 23 Andrew McCreight [:mccr8] 2012-06-14 07:00:37 PDT
Could Bug 730340 have caused leaks?  Perhaps something would end up not getting cleaned up?  I have a user reporting slow CC times, and they have this message in their log, but it could just be a red herring.
Comment 24 Marco Bonardo [::mak] 2012-07-09 11:29:48 PDT
Mano, why this didn't land yet?
Comment 25 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-07-24 07:22:55 PDT
Created attachment 645292 [details] [diff] [review]
checked in

http://hg.mozilla.org/integration/mozilla-inbound/rev/012ae8f9eb16
Comment 26 Marco Bonardo [::mak] 2012-07-24 07:36:39 PDT
Comment on attachment 645292 [details] [diff] [review]
checked in

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 730340 
User impact if declined: WeakMap keys can disappear at any time, that means we may consider a node nonexisting when it really exists. The UI consequences are unknown, likely bookmark/history UI pieces may stop working.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Should not be much risky, we are replacing wekmaps with maps, the worse thing would be a leak, but didn't show any in tests.
String or UUID changes made by this patch: none
Comment 27 Ed Morley [:emorley] 2012-07-25 08:13:29 PDT
https://hg.mozilla.org/mozilla-central/rev/012ae8f9eb16
Comment 28 Serge Gautherie (:sgautherie) 2012-07-25 08:55:09 PDT
Comment on attachment 645292 [details] [diff] [review]
checked in

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

::: browser/components/places/content/treeView.js
@@ +164,5 @@
> +      throw new Error("Removed node passed to _getRowForNode");
> +    }
> +
> +    // Ensure that the entire chain is open, otherwise that node is invisible.
> +    for (let ancestor of ancestors) {

Did you actually mean to s/in/of/?
Comment 29 Marco Bonardo [::mak] 2012-07-26 14:42:40 PDT
(In reply to Serge Gautherie (:sgautherie) from comment #28)
> Did you actually mean to s/in/of/?

it's an array now, while before was a generator, so using "of" is fine.
Comment 31 Serge Gautherie (:sgautherie) 2012-07-29 17:46:58 PDT
(In reply to Marco Bonardo [:mak] (Away 28 Jul - 12 Aug) from comment #29)
> it's an array now, while before was a generator, so using "of" is fine.

Thanks.
Comment 32 Ioana (away) 2012-08-09 07:17:08 PDT
Verified as fixed on:
Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0
Comment 33 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-08-17 10:50:45 PDT
Comment on attachment 645292 [details] [diff] [review]
checked in

What Marco Said.
Comment 34 Marco Bonardo [::mak] 2013-02-11 16:18:18 PST
*** Bug 772065 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.