Accessing XML properties from .() affects function namespace

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
12 years ago
5 years ago

People

(Reporter: Igor Bukanov, Unassigned)

Tracking

(Depends on: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

31.04 KB, patch
brendan
: review-
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
Consider the following example:

~/m/trunk/mozilla/js/src> cat ~/m/files/y.js
x = <a><fun>b</fun></a>;

function f() {}

x.function::fun = f;
if (x.function::fun !== f)
  throw "can not assign function::fun";

x.(fun == "b");

if (x.function::fun !== f) {
  throw "Using property in XML filter change its function value to "+
        uneval(x.function::fun);
}

~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/files/y.js
uncaught exception: Using property in XML filter change its function value to undefined

That is, after x.(fun == "b") x.function::fun becomes undefined.
(Reporter)

Updated

11 years ago
Assignee: general → igor
(Reporter)

Updated

11 years ago
Blocks: 373082
(Reporter)

Comment 1

11 years ago
The reason for the bug is that during x.(fun == "b") xml_lookupProperty defines a native property corresponding to fun. This removes any previous value of the property thus loosing x.function::fun. GetFunction in jsxml.c contains a hack that ignores such native properties during the method lookup so they would not hide methods stored in XML.prototype, but that obviously does not work when the function was stored in the object itself.

There are 2 ways to fix this bug:

1. Change OBJ_LOOKUP_PROPERTIES callers to deal with XML objects specially so xml_lookupProperty would not need to to create native properties. This simplifies jsxml.c and allows to remove a few lines in the file dealing with those properties for the price of few extra if(xml) checks.

2. Make jsxml.c non-native and store function properties in an extra object stored in a private slot. This is a cleaner approach, but it is non-trivial to  confirm to lookupProperty prototype where JSProperty is supposed to be returned even for non-native objects. Since XML does not store any id, providing JSProperty requires some hacks.

For now I will provide a patch for the first option.  

 
(Reporter)

Comment 2

11 years ago
Created attachment 258168 [details] [diff] [review]
Fix v1

The patch adds explicit checks for xml objects for OBJ_LOOKUP_PROPERTY users. To offset the performance lost due to the extra checks the patch consolidates js_FindProperty/action pairs into single js_SimpleNameOp method that calls the appropriate operation in addition to the property lookup.

The patch passes the test suite without regressions.
Attachment #258168 - Flags: review?(brendan)
The reason I hacked xml_lookupProperty and GetFunction in the first place was to avoid crudding up the generic code in SpiderMonkey with if (xml) wart(); else norm(); control flow forks. I really do object to the idea still. We are paying for the JSObjectOps leaky abstraction already. If it can't be used as is to front for E4X, then let's extend it, not add if-else forks on top of it.

/be
Comment on attachment 258168 [details] [diff] [review]
Fix v1

Sorry, E4X is just not important enough, and the function:: extension is not even standard. I'm glad you are looking for a cleaner way that meets all requirements, but as noted in the previous comment I don't want it to be this one.

/be
Attachment #258168 - Flags: review?(brendan) → review-
And yeah, I knew about this bug all along -- my dirty little secret (one of them). But E4X is kind of dirty too.

/be
(Reporter)

Comment 6

11 years ago
(In reply to comment #3)
> The reason I hacked xml_lookupProperty and GetFunction in the first place was
> to avoid crudding up the generic code in SpiderMonkey with if (xml) wart();
> else norm(); control flow forks. 

Interpreter is already littered with direct or indirect xml checks. IMO the extra checks that the patch adds for JSOP_IN  and JSOP_NAME just follows the same pattern while fixing the bug, reducing the total complexity of the code and shrinking the binary size.

> I really do object to the idea still. We are
> paying for the JSObjectOps leaky abstraction already. If it can't be used as is
> to front for E4X, then let's extend it, not add if-else forks on top of it.

The problem with extending JSObjectOps is that it is only necessary to support E4X and it is unclear if such extensions would be of any use for anything else.  Yet they would definitely bring the complexity of a "generic solution". If/switch may look ugly, but they remind about how badly E4X integrates with normal JS idioms.  

For me this leaky abstraction is another example that http://www.geocities.com/tablizer/myths.htm is quite right in its critique of "Myth: OOP eliminates the "complexity" of "case" or "switch" statements". See, in particular, http://www.geocities.com/tablizer/shapes.htm which gives nice examples when if/switch leads to significantly simpler and easier to maintain code.
$ grep '#if JS_HAS_XML_SUPPORT' jsinterp.c | wc -l
       7

Not exactly littered, but not pristine either. Ok, I will reconsider the patch.

You don't have to sell me on anti-OOP. I remember a colleague at SGI 15 years ago praising "the mighty if statement" over against virtual method logic splitting ;-).

/be
Note also that it's hard to get rid of JSObjectOps -- we use it as a way to layer on top of native object ops, to good effect. E.g., we don't want XPConnect or LiveConnect if or case statements in the core engine. It's hard to get rid of, and I was hoping we could reform it to leak less. That may still be worthwhile, even assuming OOPness is folly.

/be
(Reporter)

Updated

11 years ago
No longer blocks: 373082
(Reporter)

Comment 9

11 years ago
(In reply to comment #8)
>  E.g., we don't want
> XPConnect or LiveConnect if or case statements in the core engine. 

Out of curiosity, JSObjectOps appeared before or during/after XPConnect/LiveConnect surfaced?

> It's hard to
> get rid of, and I was hoping we could reform it to leak less. That may still be
> worthwhile, even assuming OOPness is folly.

The main problem with JSObjectOps is its lookupProperty method. If instead it would just define hasProperty together with extending getProperty with an option  to get the value only if it exists (to optimize the name lookup in the scope chain), this bug would not happen. Surely it would make calling getAttribute followed by setAttribute non-atomic, but that can be worked around via replacing setAttribute by doAttributeOperation where the operation can be one of the necessary set operations. 
(In reply to comment #9)
> (In reply to comment #8)
> >  E.g., we don't want
> > XPConnect or LiveConnect if or case statements in the core engine. 
> 
> Out of curiosity, JSObjectOps appeared before or during/after
> XPConnect/LiveConnect surfaced?

JSObjectOps was done by Scott Furman and myself when Scott did the modern version of LiveConnect, 1998 or so.

> The main problem with JSObjectOps is its lookupProperty method. If instead it
> would just define hasProperty together with extending getProperty with an
> option  to get the value only if it exists (to optimize the name lookup in the
> scope chain), this bug would not happen.

The optimization is not just to get the value only if it exists, but to avoid rehashing. But that could be done several ways.

It's even possible to use a (JSProperty *)1 hack if you are careful. Not sure how many people depend on JSObjectOps outside of Mozilla code, though.

> Surely it would make calling
> getAttribute followed by setAttribute non-atomic, but that can be worked around
> via replacing setAttribute by doAttributeOperation where the operation can be
> one of the necessary set operations. 

If you are motivated to clean up JSObjectOps, I will help. File a new bug?

In the mean time, consider (JSProperty *)1 expedient measures?

/be
(Reporter)

Comment 11

11 years ago
(In reply to comment #10)
> JSObjectOps was done by Scott Furman and myself when Scott did the modern
> version of LiveConnect, 1998 or so.

So that mean that JSObjectOps was designed to suite LiveConnectes needs. Then it only natural that it does not work well with E4X that sets a different set of requirements.

> 
> > The main problem with JSObjectOps is its lookupProperty method. If instead it
> > would just define hasProperty together with extending getProperty with an
> > option  to get the value only if it exists (to optimize the name lookup in the
> > scope chain), this bug would not happen.
> 
> The optimization is not just to get the value only if it exists, but to avoid
> rehashing.

After removal of the property cache that rehashing avoidance is only used during name lookup (and only with JSOP_NAME, JSOP_BINDNAME/JSOP_DELNAME do not use it) and when manipulating attributes.    

> 
> It's even possible to use a (JSProperty *)1 hack if you are careful. Not sure
> how many people depend on JSObjectOps outside of Mozilla code, though.

The problem with that is that it would require to make XML objects non-native as there are assumptions in the current code that for native objects JSProperty is JSScopeProperty when OBJ_IS_NATIVE is true. But with non-native XML objects functions:: properties can not be stored is the map and require a separated storage (like a hidden object). It requires a lot of code. Surely it would only present in jsxml.c, but that is a part of SpiderMonkey in any case so I do not see the benefits.

> If you are motivated to clean up JSObjectOps, I will help. File a new bug?

I thought that JSObjectOps is untouchable for 1.9. For Mozilla 2.0 it is too early to do it since it is not known that any redesign at this stage would not require another one later.

(In reply to comment #11)
> (In reply to comment #10)
> > JSObjectOps was done by Scott Furman and myself when Scott did the modern
> > version of LiveConnect, 1998 or so.
> 
> So that mean that JSObjectOps was designed to suite LiveConnectes needs. Then
> it only natural that it does not work well with E4X that sets a different set
> of requirements.

This is obviously true in light of JSXMLObjectOps ;-).

JSObjectOps served XPConnect too, without much (if any) change. LiveConnect, XPConnect, and other such bridges have similar requirements.

> > The optimization is not just to get the value only if it exists, but to avoid
> > rehashing.
> 
> After removal of the property cache that rehashing avoidance is only used
> during name lookup (and only with JSOP_NAME, JSOP_BINDNAME/JSOP_DELNAME do not
> use it) and when manipulating attributes.    

Sure, JSOP_NAME. Perhaps it could stand the rehashing costs in real workloads.

> > If you are motivated to clean up JSObjectOps, I will help. File a new bug?
> 
> I thought that JSObjectOps is untouchable for 1.9. For Mozilla 2.0 it is too
> early to do it since it is not known that any redesign at this stage would not
> require another one later.

Not clear what can be done in 1.9, or that Mozilla 2 gives us freedom to change the JS API in large incompatible ways. But JSObjectOps is misplaced in the public API, and is hairy enough that I think we can treat it as "unfrozen". So we might clean it up for 1.9. Given other work, is it worth doing?

/be
Ok, I will review an updated patch.

/be
(Reporter)

Comment 14

11 years ago
Created attachment 263871 [details] [diff] [review]
fix v2

Here is an updated patch. In the new version in js_SimpleNameOp I check the map, not the class, to detect the with object, so compiler can optimize the following OBJ_IS_NATIVE. The new version also calls js_LookupProperty directly for native objects to avoid function call overhead of OBJ_LOOKUP_PROPERTY.
Attachment #258168 - Attachment is obsolete: true
Attachment #263871 - Flags: review?(brendan)
Comment on attachment 263871 [details] [diff] [review]
fix v2


>@@ -2837,33 +2845,30 @@ interrupt:
>               case JSOP_FORPROP:
>                 /*
>                  * We fetch object here to ensure that the iterator is called
>                  * even if lval is null or undefined that throws in
>                  * FETCH_OBJECT. See bug 372331.
>                  */
>                 FETCH_OBJECT(cx, -1, lval, obj);
>-                goto set_for_property;

Nit: blank line here before comment line or major (multiline) comment:

>+                /* Set the variable obj[id] to refer to rval. */
>+                fp->flags |= JSFRAME_ASSIGNING;
>+                ok = OBJ_SET_PROPERTY(cx, obj, id, &rval);
>+                fp->flags &= ~JSFRAME_ASSIGNING;

Note how JSFRAME_ASSIGNING is set only during OBJ_SET_PROPERTY, which looks along proto chain, not parent chain, calling resolve hooks if defined and if property not found already.

>+                if (!ok)
>+                    goto out;
>+                break;
> 
>               case JSOP_FORNAME:
>                 /*
>                  * We find property here after the iterator call to ensure
>                  * that we take into account side effects of the iterator
>                  * call. See bug 372331.
>                  */
>-
>-                ok = js_FindProperty(cx, id, &obj, &obj2, &prop);
>-                if (!ok)
>-                    goto out;
>-                if (prop)
>-                    OBJ_DROP_PROPERTY(cx, obj2, prop);
>-
>-              set_for_property:
>-                /* Set the variable obj[id] to refer to rval. */
>                 fp->flags |= JSFRAME_ASSIGNING;
>-                ok = OBJ_SET_PROPERTY(cx, obj, id, &rval);
>+                ok = js_SimpleNameOp(cx, id, JSNAME_SET, NULL, &rval);

Here js_SimpleNameOp is called with JSFRAME_ASSIGNING for all objects searched on scope chain, for all protos searched from those objects. Different from before, where (as above under the JSOP_FORPROP case) JSFRAME_ASSIGNING is set only for the proto chain search.

Is this a bug fix? Not clear, but I wanted to point it out. It's an incompatibility.


>+JS_FRIEND_API(JSBool)
>+js_SimpleNameOp(JSContext *cx, jsid id, JSNameOp nameop, JSObject **objp,
>+                jsval *vp)
>+{
>+    JSObject *obj, *lastobj, *proto, *pobj;
>+    JSProperty *prop;
>+    JSScope *scope;
>+
>+    JS_ASSERT(JSID_IS_ATOM(id));
>+    JS_ASSERT_IF(nameop != JSNAME_GET, objp == NULL);
>+    obj = cx->fp->scopeChain;
>+    do {
>+        lastobj = obj;
>+        if (obj->map->ops == &js_WithObjectOps) {
>+            do {
>+                proto = OBJ_GET_PROTO(cx, obj);
>+                if (!proto)
>+                    break;
>+                obj = proto;
>+            } while (obj->map->ops == &js_WithObjectOps);

You can't just skip with objects, you must look in each with object's proto for id. Somehow this requirement was lost -- is it in conflict with the XML testcase? Maybe only if id has a function namespace?

>+                  case JSNAME_SET:
>+                    /*
>+                     * Can not call NativeSet here as it does not check

Nit: "Cannot". Perhaps also a blank line before each non-initial case label in this switch would be easier to read.

>+                     * for read-only attributes etc.
>+                     */
>+                    JS_UNLOCK_SCOPE(cx, scope);
>+                    return js_SetProperty(cx, obj, id, vp);

This comment and code suggests that the js_FindPropertyBase/js_SetProperty pair is better. But it's not a big deal for efficiency. The real question is when JSFRAME_ASSIGNING should be visible to resolve hooks: scope as well as proto chain search, or only proto chain search once the target object in the scope chain in which to set id has been found (at the end of the scope chain, presumably -- otherwise id was resolved during the scope chain search).

>+        JS_ASSERT(0);   /* unreachable */

Spin-off fodder: time for JS_NOTREACHED() ?

>+      case JSNAME_BIND:
>+      case JSNAME_SET:
>+        /*
>+         * Give a strict warning if binding an undeclared top-level variable.
>+         */
>+        if (JS_HAS_STRICT_OPTION(cx)) {
>+            JSString *str = JSVAL_TO_STRING(ID_TO_VALUE(id));
>+            const char *bytes = js_GetStringBytes(cx, str);
>+
>+            if (!bytes ||
>+                !JS_ReportErrorFlagsAndNumber(cx,
>+                                              JSREPORT_WARNING
>+                                              | JSREPORT_STRICT,

Nit: could crunch around | to fit on one line; or (I think prevailing style says) break after |.

>+                                              js_GetErrorMessage, NULL,
>+                                              JSMSG_UNDECLARED_VAR, bytes)) {
>+                return JS_FALSE;
>+            }
>+        }

This case used to be only in js_FindIdentifierBase, called only from JSOP_BINDNAME (if I'm reading my old code correctly). Why is it needed now for the JSNAME_SET case?

/be
Attachment #263871 - Flags: review?(brendan) → review-

Updated

10 years ago
Depends on: 408416

Updated

10 years ago
Blocks: 246441
(Reporter)

Updated

10 years ago
Blocks: 355233
(Reporter)

Updated

10 years ago
Assignee: igor → general
e4x is going to be removed soon (bug 788293).
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.