Open Bug 986115 Opened 12 years ago Updated 3 years ago

Components.utils.getWeakReference() should return the same weak ref on each call with the same target

Categories

(Core :: XPConnect, defect)

Product:
Core
Component:
XPConnect
Type:
defect

Tracking

()

Status:
REOPENED

People

(Reporter: zombie, Unassigned)

Details

not doing so breaks object equality on weakrefs, and prevents them to be used as a simple replacement in all the places standard objects could be used, like keys of a Map, Array.prototype.indexOf(), and probably lots of other..
(In reply to Tomislav Jovanovic [:zombie] from comment #0) > not doing so breaks object equality on weakrefs Well, only if you get the weakref multiple times, right? It seems like consumers that need this should just use a target => WeakRef WeakMap. That's basically what we'd need under the hood if we were to do this automatically.
(CCing Andrew and Kyle in case they have any thoughts)
Yeah, this is what weak maps are good at. :)
i was told (by Boris) to file this as a bug, so i'm going to fight for it some more: 1) it's a method called GETweakReference, not CREATEweakReference or a constructor 2) it's surprising, non-obvious and unexpected behavior (why would there ever be more than one) 3) it's not documented 4) it can lead to subtle, hard to find bugs (in our own code) 5) it's inconsistent with nsISupportsWeakReference.GetWeakReference() this existing behavior has no reason for being (apart from "that's how c++ weak refs behave"), has no use case, and violates the principle of least surprise.
I don't really see why xpcJSWeakReference exists at all. I think we ought to be able to just return the underlying nsIWeakReference, which would give us all the desired semantics.
(In reply to Tomislav Jovanovic [:zombie] from comment #4) > this existing behavior has no reason for being (apart from "that's how c++ > weak refs behave") The reason for being is that it avoids the need for a heavyweight WeakMap on the C++ side, when most of the consumers seem pretty happy without it. (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5) > I don't really see why xpcJSWeakReference exists at all. I think we ought > to be able to just return the underlying nsIWeakReference, which would give > us all the desired semantics. Well, it also supports arbitrary XPCWrappedJSes, right? Though maybe those could just claim to implement nsISupportsWeakReference? mccr8 would know more.
(In reply to Bobby Holley (:bholley) from comment #6) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5) > > I don't really see why xpcJSWeakReference exists at all. I think we ought > > to be able to just return the underlying nsIWeakReference, which would give > > us all the desired semantics. > > Well, it also supports arbitrary XPCWrappedJSes, right? Though maybe those > could just claim to implement nsISupportsWeakReference? > > mccr8 would know more. Sure, but why can't we just XPCWrappedJS whatever the JS object is and hand back the builtin nsIWeakReference? That's basically what xpcJSWeakReference does anyways.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7) > Sure, but why can't we just XPCWrappedJS whatever the JS object is and hand > back the builtin nsIWeakReference? That's basically what xpcJSWeakReference > does anyways. No, the point is that it handles arbitrary JS objects like {foo: 2}, which has no builtin nsIWeakReference. XPCWrappedJS could make it QI to one, which was the subject of my musings in comment 6.
(In reply to Bobby Holley (:bholley) from comment #8) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7) > > Sure, but why can't we just XPCWrappedJS whatever the JS object is and hand > > back the builtin nsIWeakReference? That's basically what xpcJSWeakReference > > does anyways. > > No, the point is that it handles arbitrary JS objects like {foo: 2}, which > has no builtin nsIWeakReference. XPCWrappedJS could make it QI to one, which > was the subject of my musings in comment 6. Sure, but it delegates all the work to XPCWrappedJS anyways. So why not just do that in Cu.getWeakReference instead of xpcJSWeakReference::Init and dispense with the wrapper?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #9) > Sure, but it delegates all the work to XPCWrappedJS anyways. So why not > just do that in Cu.getWeakReference instead of xpcJSWeakReference::Init and > dispense with the wrapper? Probably because we need an intermediate object to prevent XPConnect from just unwrapping the JS object. See the comments around WrappedJSHolder and XPCComponents_Utils::GenerateXPCWrappedJS.
But the XPCWrappedJS's nsIWeakReference is not the XPCWrappedJS itself. It's just another random XPCOM object. Feel free to tell me to go RTFC again though cause I didn't ;-)
Hm, that's a good point. I'm not sure then. There might be some weirdness with using QueryReferent from JS, wherein you end up with a double-wrapped object that you can't unwrap, because it never set the .wrappedJSObject property. But I'm really not sure. You're welcome to try simplifying it. ;-)
(In reply to Tomislav Jovanovic [:zombie] from comment #4) > 5) it's inconsistent with nsISupportsWeakReference.GetWeakReference() this one turned out to be false (my misreading of some code). but i have another to add, discovered while working on bug 807382: 6) workaround requires code that makes Mossop's head hurt :) anyway, can we get any info on the potential for this Kyle?
Flags: needinfo?(khuey)
It's not something I'm likely to have time to work on in the near future, unfortunately.
Flags: needinfo?(khuey)
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.