David Bruant Reporter
	Description • 12 years ago

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

	Jeff Walden [:Waldo]
	Comment 1 • 12 years ago

I know someone who likes to remove useless junk!

	David Bruant Reporter
	Comment 2 • 12 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 12 years ago

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.

	Jeff Walden [:Waldo]
	Comment 4 • 12 years ago

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.

	David Bruant Reporter
	Comment 5 • 12 years ago

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

	Jeff Walden [:Waldo]
	Comment 6 • 12 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 7 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 12 years ago

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

	Andrew McCreight [:mccr8]
	Comment 9 • 12 years ago

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.

	Wladimir Palant
	Comment 10 • 12 years ago

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.

	David Bruant Reporter
	Comment 11 • 12 years ago

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?

	Wladimir Palant
	Comment 12 • 12 years ago

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

	Andrew McCreight [:mccr8]
	Comment 13 • 12 years ago

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+.

	Wladimir Palant
	Comment 14 • 12 years ago

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

	Wladimir Palant
	Comment 15 • 12 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 16 • 12 years ago

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

	Andrew McCreight [:mccr8]
	Comment 17 • 12 years ago

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.

	Wladimir Palant
	Comment 18 • 12 years ago

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

	Charles Scalfani
	Comment 19 • 12 years ago

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

	Wladimir Palant
	Comment 20 • 12 years ago

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

	David Bruant Reporter
	Comment 21 • 12 years ago

(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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 22 • 12 years ago

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.

	Masatoshi Kimura [:emk]
	Comment 23 • 12 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 24 • 12 years ago

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

	Wladimir Palant
	Comment 26 • 11 years ago

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.

	Wladimir Palant
	Comment 27 • 10 years ago

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

	Daniel Glazman (:glazou) (not active in Mozilla any more)
	Comment 28 • 8 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 8 years ago

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 30 • 6 years ago

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.

	Phabricator Automation
	Comment 32 • 6 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 33 • 6 years ago

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.

	Phabricator Automation
	Comment 35 • 6 years ago

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

	Pulsebot
	Comment 36 • 6 years ago

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

	Eliza Balazs [:ebalazs_]
	Comment 37 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a6528c5018d2

	Chris Mills [:cmills]
	Comment 38 • 6 years ago

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!

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 39 • 6 years ago

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!