760940 - Bookmarks and history menus behave incorrectly due to non-node weak map keys

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Josef Vitu]

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)]

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]

Probably regression from Bug 730340 - Don't use expando properties for storing data on places nodes

Comment 3

[Marco Bonardo [:mak]]

I cannot reproduce, for me bookmarks are removed properly in today's Nightly, I see the warnings though.

Comment 4

[Josef Vitu]

(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]]

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 (out of office until 8/21) [:mccr8]]

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]]

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]]

(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]]

(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

[Asaf Romano (gone)]

Attachment: Switch to Map

Comment 11

[Andrew McCreight (out of office until 8/21) [:mccr8]]

(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]]

(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 (out of office until 8/21) [:mccr8]]

(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]]

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

[Asaf Romano (gone)]

Attachment: Switch to Map everywhere

Comment 16

[Marco Bonardo [:mak]]

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

[Asaf Romano (gone)]

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

[Asaf Romano (gone)]

Attachment: patch

Comment 19

[Marco Bonardo [:mak]]

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

[Asaf Romano (gone)]

Sure, I can remove this change, at least for now.

Comment 21

[Marco Bonardo [:mak]]

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

[Asaf Romano (gone)]

Oh, hrm, there's the (now-long-standing) issue with the livemarks service too, right?

Comment 23

[Andrew McCreight (out of office until 8/21) [:mccr8]]

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]]

Mano, why this didn't land yet?

Comment 25

[Asaf Romano (gone)]

Attachment: checked in

http://hg.mozilla.org/integration/mozilla-inbound/rev/012ae8f9eb16

Comment 26

[Marco Bonardo [:mak]]

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]]

https://hg.mozilla.org/mozilla-central/rev/012ae8f9eb16

Comment 28

[Serge Gautherie (:sgautherie)]

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]]

(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 30

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/47a702158c85
https://hg.mozilla.org/releases/mozilla-beta/rev/4b9f6767e721

Comment 31

[Serge Gautherie (:sgautherie)]

(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)]

Verified as fixed on:
Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0

Comment 33

[Asaf Romano (gone)]

Comment on attachment 645292 [details] [diff] [review]
checked in

What Marco Said.