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

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Josef Vitu, Assigned: mano)

Tracking

({regression})

Trunk
Firefox 17
All
Linux
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15+ verified, firefox16+ fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Keywords: regression
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: x86_64 → All

Comment 2

5 years ago
Probably regression from Bug 730340 - Don't use expando properties for storing data on places nodes
Blocks: 730340
I cannot reproduce, for me bookmarks are removed properly in today's Nightly, I see the warnings though.
(Reporter)

Comment 4

5 years ago
(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
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?
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?
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?
(In reply to Marco Bonardo [:mak] from comment #7)
> each DOM node in the browser ui

clearly in a Places view (menupopup, toolbar)
(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.
Created attachment 630175 [details] [diff] [review]
Switch to Map
Assignee: nobody → mano
Status: NEW → ASSIGNED
Attachment #630175 - Flags: review?(mak77)
(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.
(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.
Status: ASSIGNED → NEW
(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).
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
Created attachment 630194 [details] [diff] [review]
Switch to Map everywhere
Attachment #630175 - Attachment is obsolete: true
Attachment #630175 - Flags: review?(mak77)
Attachment #630194 - Flags: review?(mak77)
tracking-firefox15: --- → ?
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
Attachment #630194 - Flags: review?(mak77) → review+
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.
Created attachment 630567 [details] [diff] [review]
patch
Attachment #630194 - Attachment is obsolete: true
Attachment #630567 - Flags: review?(mak77)
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?
Sure, I can remove this change, at least for now.
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
Attachment #630567 - Flags: review?(mak77) → review+
Oh, hrm, there's the (now-long-standing) issue with the livemarks service too, right?
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.

Updated

5 years ago
tracking-firefox15: ? → +
tracking-firefox16: --- → +
Summary: Failed to preserve wrapper of wrapped native weak map key → Bookmarks and history menus behave incorrectly due to non-node weak map keys
Mano, why this didn't land yet?
Blocks: 772065
Created attachment 645292 [details] [diff] [review]
checked in

http://hg.mozilla.org/integration/mozilla-inbound/rev/012ae8f9eb16
Attachment #630567 - Attachment is obsolete: true
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
Attachment #645292 - Flags: approval-mozilla-beta?
Attachment #645292 - Flags: approval-mozilla-aurora?
Blocks: 761620
Attachment #645292 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Blocks: 777005
https://hg.mozilla.org/mozilla-central/rev/012ae8f9eb16
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
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/?
Attachment #645292 - Flags: feedback?(mano)
Attachment #645292 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
https://hg.mozilla.org/releases/mozilla-aurora/rev/47a702158c85
https://hg.mozilla.org/releases/mozilla-beta/rev/4b9f6767e721
status-firefox15: --- → fixed
status-firefox16: --- → fixed
(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

5 years ago
Verified as fixed on:
Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0
status-firefox15: fixed → verified
Comment on attachment 645292 [details] [diff] [review]
checked in

What Marco Said.
Attachment #645292 - Flags: feedback?(mano) → feedback+
Duplicate of this bug: 772065
No longer blocks: 772065
You need to log in before you can comment on or make changes to this bug.