Closed Bug 174578 Opened 22 years ago Closed 6 years ago

added "expando" style object properties are removed after a few seconds

Categories

(Core :: DOM: CSS Object Model, defect, P5)

Product:
Core
Component:
DOM: CSS Object Model
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: m.proctor, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

Mark's testcase
634 bytes, text/html
Details 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1) Gecko/20020826
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1) Gecko/20020826

When adding new properties to a style object, after a few seconds they are
removed causing any references to change to undefined. If you added the property
to CSSStyleDeclaration and then updated it via the style obect, after a few
seconds it would revert back to the CSSStyleDeclaration value. From this I would
judge that it refreshing the style object from the CSSStyleDeclaration, wiping
any user changes.

I need to place hooks into style property changes, so that functions can be
executed on those changes - ie style.left=10 would loop through an array of
functions, calling each function. I create a setter and getter for style.left
but it is wiped after a few seconds unless I define it for all objects via
CSSStyleDeclaration - which then needs other work arounds to localise it to the
correct style object.
//------------------------------------------------
// Example
//------------------------------------------------
<html>
<head>
<script>
function moveDiv(e) {
  div1.style.left = e.clientX +  document.body.scrollLeft;
  div1.style.top= e.clientY +  document.body.scrollTop;
  div1.innerHTML = div1.style.myProperty;
}

//this is optional uncomment to see it rever to this after a few seconds
CSSStyleDeclaration.prototype.myProperty = "goodbye";

onload = function() {
 div1 = document.getElementById("p1");
 div1.style.myProperty = "hello";
 document.addEventListener("mousemove", moveDiv, true);
}
</script>


</head>
<body>
<div id="p1"
style="position:absolute;left:0;top:0;width:50;height:50;background-color:red">
asf
</div>
</body>
</html>


Reproducible: Always

Steps to Reproduce:
1. open the page
2. move the cursor around
3. watch it change from hello to goodbye
4. repeat the process with the CSSStyleDeclaration line commented out, to see it
change from hello to undefined



Expected Results:  
all user "expando" properties and setters/getters should be preserved and work
like they should on any object.
I bet what's happening here is this:

1)  div.style fetches the CSSStyleDeclaration object, wraps it in JS object, and
    returns it
2)  A property is set on the JS object (this, of course, does not set it on the
    CSSStyleDeclaration, since that's not a valid property for
    CSSStyleDeclarations)
3)  No one holds a reference to the JS object
4)  div.style is accessed, finds the cached JS object and uses it.
5)  The JS object gets garbage collected after a bit (since no one is holding a
    ref to it)
6)  div.style is accessed, sees no cached object, fetches the decl and wraps it
    in a new JS object (which obviously does not have the "custom" property).

To test this, I tried creating a global array and pushing div1.style into it in
the onload handler in that testcase.  That fixes the problem....

The only thing I wonder about is why the same does not happen for Node objects
which have gotten JS wrapped (setting myProperty on div1 instead of div1.style
works great in that testcase)... is this a parenting issue of some sort?
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
You're explanation sounds very plausible Boris. Would it be possible for
div.style to keep the pointer to the nsIXPConnectJSObjectHolder returned when
initially wrapped and then subsequent calls just return the JSObject that it
holds. This is just off the top of my head, I don't know much about the CSS code
in question, hoping it might spark a better idea from someone else.
Hmm..  the problem is that the CSS code never gets to see the wrapped JS object.
 If I understand how this all works, the wrapping is essentially done by
DOMClassInfo, right?  Looking at the flags stuff in there, would the
nsIXPCScriptable::WANT_ADDPROPERTY flag be relevant here?  It looks like
nsNodeSH::AddProperty makes the document hold a ref to the
|nsIXPConnectWrappedNative* wrapper|...

Or am I totally on the wrong track here?
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs
*** Bug 221707 has been marked as a duplicate of this bug. ***
Is there any update on this? It seems to have gone dead, seems like core
functionality that shouldn't get forgotten about.
cc'ing brendan for input
Which part is "core functionality"?  This is a nice-to-have, but there is
absolutely no reason this should work other than maybe developer convenience. 
Nothing in the DOM or JS itself implies that this should work.

Even if we fix this specific case (for node.style), "expando"s on other
JSObjects will continue to behave as they do now -- they will disappear when the
JSObject is garbage collected.  So anythign we do would be a .style-specific
hack.  The question is how much people need this (I don't know) and how painful
it would be to do (per comment 4, very, if I understand this stuff at all).
Don't we keep permanent expandos for DOM nodes?
Yes, we do.  That's hacked in inside classinfo.  See comment 4.
"Expando" is an MS term that I would prefer everyone eschew.  jband called them
"ad hoc properties" when doing XPConnect.

Can someone point me to the JSClass getProperty hook, or per-property getter,
that is defined for the div.style, etc.?  Or rather, to the JS_DefineProperty
call or the JS_InitClass call that creates the .style property?

/be
Brendan, the JS_InitClass call happens in nsDOMClassInfo::InitDOMJSClass, as far
as I can tell, which is where it happens for all DOM objects.
And the property never gets defined on the Node JSObject that I can see....
maybe I'm wrong...

Perhaps that's what we should do?  When we get through XPConnect, we should
define the property on the object we got from; that will keep things alive
automatically (in this case the node would hold a ref to the style JSObject, and
the node's JSObject would be kept alive by parenting).
InitDOMJSClass defines the DOM classes (prototypes of the objects) so that's not
what you're looking for I think. The property hooks for wrapped natives are in
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappednativejsops.cpp#776
and those call through to the nsIXPCScriptable methods defined in nsDOMClassInfo
and its descendants. As bz points out, for nodes we hold a reference to the
wrappers in a hash in the document, see nsNodeSH::AddProperty
(http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsDOMClassInfo.cpp#4502)
which calls nsDocument::AddReference
(http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.cpp#3802).
I suppose we could do the same for other wrappers, not sure how costly that'll
be. Note that it means you'd have to find the document from every object wrapper
so that might be a lot of code to add.
Brendan: accessing div.style results in a call to XPCWrappedNative::GetAttribute
which ends up calling XPCWrappedNative::CallMethod where the resulting native
object gets wrapped
(http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappednative.cpp#2109).
XPConnect caches that wrapper so the next time it gets accessed it comes from
the cache. However, if a gc happens and the JSObject gets finalized, the wrapper
gets released and removes itself from the hash. The wrapper doesn't get added as
a property to the object, since we want the getter and setter to get called when
that property is accessed. At least, that's my understanding of it :-/.
> ...  The wrapper doesn't get added as a property to the object, since we want
> the getter and setter to get called when that property is accessed. At least,
> that's my understanding of it :-/.

Getters and setters are called on every get or set, respectively.  The issue of
ephemeral JS objects for certain DOM properties seems like a bug to me.  The PLA
is being violated for some objects, while other DOM objects are not ephemeral.

An XPConnect-level solution might continue to use getters and setters, but lose
JSPROP_SHARED, which is an attribute that means "don't allocate a slot to keep
the value last got or set", wherefore the ephemeral nature of the wrapper.  But
then you should worry about entrainment of large garbage object graphs hanging
off of certain slots, although in this case, perhaps that's not likely.

The DOM could keep its own strong refs, as you note, but that sounds hard if the
locus of such rooting is the document, and the instances that need to add and
remove roots are far away from it, of don't have affine lifetimes.

dbradley, jband: thoughts?

/be
Another option here would be to have the DOMCSSDeclaration be an nsIDOMGCParticipant and for nsGenericElement to put it in the list when it gets AppendReachableList() called.  That would mean an extra 4 bytes for each DOMCSSDeclaration, though...
*** Bug 351367 has been marked as a duplicate of this bug. ***
Assignee: general → nobody
QA Contact: ian → general
QA Contact: general → style-system
Priority: -- → P5
Was this fixed by conversion to the new (Paris) bindings?
Yes, it was.  We now do wrapper caching and preservation as needed for everything.
Status: NEW → RESOLVED
Closed: 6 years ago
Depends on: ParisBindings
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: