DOM3 had node.getUserData (http://www.w3.org/TR/DOM-Level-3-Core/core.html#Node3-getUserData) and node.getUserData (http://www.w3.org/TR/DOM-Level-3-Core/core.html#Node3-getUserData) methods. These have been retired in DOM4: http://www.w3.org/TR/domcore/#dom-core They were pretty useless anyway. Chrome does not implement these methods, so it's unlikely that the web depends on them. It seems they can be safey removed

I know someone who likes to remove useless junk!

Setting blocked bugs. Fixing this one may either invalidate or fix them.

If you plan to remove this, you first need to come up with an alternate way to allow extensions like AdBlock plus to store metadata on nodes. Attributes are out. Weakmaps would work, I think, if they actually worked, but last I checked I was told they don't.

While you'd run the risk of inducing leaks, and perhaps for that reason it's impractical, Map would do the trick now. Although Map's still in experimental, not-shipped-enabled state, so it's probably out until that changes.

(In reply to Boris Zbarsky (:bz) from comment #3) > If you plan to remove this, you first need to come up with an alternate way > to allow extensions like AdBlock plus to store metadata on nodes. > Attributes are out. Weakmaps would work, I think, if they actually worked, > but last I checked I was told they don't. WeakMaps seems to be the perfect candidate indeed (with MutationObservers if people really care about the third setUserData argument). What is it that doesn't work with them? Each time I use them, they do work.

WeakMaps don't work correctly when the key is a DOM object, if the weak map is the only thing with a reference to the DOM object, in certain circumstances, due to a (I think) misguided optimization. It's all clownshoes, sadly, not sure what the current state of fixing it is. Also I can't find the bug offhand, either. :-\ mccr8 would know, I'm sure.

The only C++ objects you can use as weak map keys are those that are wrapper cached. I think we even only limit this to nodes right now, as that's the most common use case, but that could be expanded to other wrapper cached things. But they should work on nodes, and have worked since last year, IIRC.

Interesting. Wladimir, do you use user data in adblock? Could you use a weakmap instead, if so?

I think ABP uses them, because Olli had to add support to them while he was working on CC optimizations. I think I've also seen userdata used on IRCcloud, for whatever odd reason. I don't think I was running an addon that would have caused it when I noticed it.

Yes, we use user data. I guess that we could switch to weak maps - after some transition period because they aren't really usable in browser versions we currently support.

Are there bugs related to improving how weakmaps work with DOM nodes? If so, can these be added as blocking this one? (In reply to Wladimir Palant from comment #10) > Yes, we use user data. I guess that we could switch to weak maps - after > some transition period because they aren't really usable in browser versions > we currently support. Trying to use document.setUserData on Chrome console doesn't work. Chrome doesn't seem to support setUserData. How do you work around that?

(In reply to David Bruant from comment #11) > Are there bugs related to improving how weakmaps work with DOM nodes? I think that bug 673468 and bug 680937 are relevant here. > Trying to use document.setUserData on Chrome console doesn't work. Chrome > doesn't seem to support setUserData. How do you work around that? We don't - we don't have this functionality in Chrome yet. But the idea was to use expando properties. From what I understood, expando properties from a content script won't be visible to the web page but they also won't be suddenly garbage collected even if the content script doesn't have references to the node.

Bug 673468 only applies to pure JS objects being used as keys, not nodes. The equivalent bug for nodes is bug 655297. Also related is bug 668855. I think nodes as weakmap keys should work in Firefox 11+.

Ok, Firefox 11 is something that I can already set as minVersion, I thought that I would need Firefox 13. Will try that.

I've apparently hit bug 673468 hard. I am doing something like this: > var windowStats = new WeakMap(); > ... > windowStats.set(wnd.document, stats); > ... > windowStats.get(wnd.document); Here wnd is a content window meaning that it is accessed across compartments. XPCWrappedNative.unwrap() doesn't help, I can only get hold of a cross-compartment wrapper and that one gets garbage-collected despite the node that it refers to still being alive. From what I can see there are no work-arounds which means that weak maps really are only usable starting with Firefox 13 for me. I will have to keep using user data or expando properties for Firefox 12 and below.

I guess that makes sense. That's too bad. But you aren't encountering any problems in 13 so far?

That isn't a huge deal, as we aren't going to get rid of getUserData and setUserData until 15, and more realistically 16 or later.

(In reply to Andrew McCreight [:mccr8] from comment #16) > I guess that makes sense. That's too bad. But you aren't encountering any > problems in 13 so far? Not sure about Firefox 13 - I am usually only testing in stable and nightly. But Firefox 15 seems to work fine. Also, I am using weak maps with chrome nodes elsewhere (meaning no cross-compartment wrappers in Firefox 12) and that seems to work fine as well. So far I didn't look at memory leaks too closely however.

PLEASE do NOT remove getUserData & setUserData without first considering the impact on extension development. Without these functions, there will be no way to do communication between the extension and the page. If you do remove this, at least make sure you provide functionality like chrome.extension.sendMessage. References: https://developer.mozilla.org/en-US/docs/Code_snippets/Interaction_between_privileged_and_non-privileged_pages?redirectlocale=en-US&redirectslug=Code_snippets%3AInteraction_between_privileged_and_non-privileged_pages http://developer.chrome.com/extensions/messaging.html

(In reply to Charles Scalfani from comment #19) > https://developer.mozilla.org/en-US/docs/Code_snippets/ > Interaction_between_privileged_and_non-privileged_pages Ouch, lots of bad code. Somebody apparently failed to understand the solutions proposed on that page and added his own - actually passing objects from chrome to content via user data and IMHO reintroducing the very security risks that the proposed approach was supposed to avoid (never mind the potential memory leaks). I changed the code to make sure that it follows the same approach as the rest of the page - communication happens through the DOM, there is no direct object passing in any direction. There is still some potential for memory leaks there, I don't see any obvious solution to that (from what I remember, Chromium was struggling with that issue as well).

(In reply to Wladimir Palant from comment #20) > There is still some potential for memory > leaks there, I don't see any obvious solution to that (from what I remember, > Chromium was struggling with that issue as well). The idea is to do the scripting equivalent to the hueyfix [1] It can be achieved using proxies [2][3] implementing the caretaker pattern [4] (and revoking so that untrusted code can hold a reference to things that would need garbage collection). I'll leave it to that to not further pollute this bug, but I'm happy to continue this conversation elsewhere if you want to (you have my e-mail address) [1] http://blog.kylehuey.com/post/21892343371/fixing-the-memory-leak [2] http://wiki.ecmascript.org/doku.php?id=harmony:direct_proxies [3] https://bugzilla.mozilla.org/show_bug.cgi?id=703537 [4] http://c2.com/cgi/wiki?RevokableCapabilities

Anything preventing us from getting rid of these functions now? They are adding complexity and security bugs. The longer we keep them around, the longer we run the risk of further compatibility problems when removing them. At the very least adding a warning seems appropriate at this time.

We need to removed in-tree callers first. https://mxr.mozilla.org/mozilla-central/search?string=getUserData Notably, devtools and pdf.js are using userData right now.

Significant amount userdata usage in addons https://mxr.mozilla.org/addons/search?string=getUserData https://mxr.mozilla.org/addons/find?text=setUserData&string=

Weak maps are once again broken in Firefox 18 (bug 819131) and right now the only work-around I can see is reverting to getUserData/setUserData.

We tried to get rid of getUserData/setUserData once more but hit bug 982561 and had to revert again.

FWIW, BlueGriffon relies quite heavily on setUserData/getUserData because it's a super convenient way of making a node hold complex data that are never serialized w/o having an extra object serving as a map or an extra kung-fu death grip. Dataset cannot help us, and a transition to WeakMap would be quite a lot of work.

Why would a transition to WeakMap hanging off the node in an expando property be a lot of work, exactly? Or even just a transition to a Map hanging off the node? Those would have the additional benefit of being way faster than the kludge that set/getUserData is at this point.

I was going through nsIDocument today in bug 1446568 and I saw the property tables, which carried me to Get/SetUserData, which seem unused now. I think we can get rid of them.

Comment on attachment 8959759 [details] Bug 749981: Remove Node.getUserData / setUserData. r=smaug Olli Pettay [:smaug] has approved the revision. https://phabricator.services.mozilla.com/D749

Comment on attachment 8959759 [details] Bug 749981: Remove Node.getUserData / setUserData. r=smaug this review flag setup is so broken. Anyhow, gave r+ in Phabricator, but it is conditional, since I think the patch was missing some test removals/changes, so there needs to be another patch for those.

Comment on attachment 8959826 [details] Bug 749981: Remove setUserData from a couple tests. r=smaug Olli Pettay [:smaug] has approved the revision. https://phabricator.services.mozilla.com/D755

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a6528c5018d2 Remove Node.getUserData / setUserData. r=smaug

These methods are already marked as obsolete on https://developer.mozilla.org/en-US/docs/Web/API/Node. I've also updated the compat data on https://developer.mozilla.org/en-US/docs/Web/API/Node/getUserData https://developer.mozilla.org/en-US/docs/Web/API/Node/setUserData and added to note to the Fx61 rel notes: https://developer.mozilla.org/en-US/Firefox/Releases/61#DOM Let me know if this looks OK, thanks!

Those methods weren't accessible to content anyway from a long time ago, so not sure it needs a relnote, but other than that it looks good, thanks!