Closed Bug 547140 Opened 14 years ago Closed 10 years ago

Remove JSRESOLVE_ASSIGNING and resolve flags

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: Waldo, Assigned: jorendorff)

References

Details

Attachments

(7 files, 1 obsolete file)

The js shell has a bug (bug 547087) that the global object doesn't resolve anything on assigning to a property.  It's fine not to resolve on assignment *only* in the case where the resolved property has the standard attributes (enumerable, configurable, writable) and where the resolved property isn't a getter/setter pair.  With anything else the resolve hook short-circuits, and the assignment defines a property with the wrong attributes (or value if the property wasn't writable), or doesn't invoke a setter, or similar.  Another instance of this mistake occurs in the string-resolve hook, with these consequences:

js> var o = new String("foobar");
js> o[1] = 33
33
js> o[1]
33

(Character properties of string objects are supposed to be non-writable and non-configurable; in the above case both bits aren't respected when the "new" property overwrite happens.)

Also on the subject of weird cases: this should be a TypeError, not a ReferenceError (strict mode makes assignment to non-existing property an error, and the property isn't resolved here because of the assignment nature of the instance):

  "use strict";
  try { undefined = 17; } catch (e) { assert(e instanceof TypeError); }


Brendan suggests removing JSRESOLVE_ASSIGNING.  We have a fair number of cases where this will require some effort to make work, some in wrappers (proxies may address this concern, hopefully).  This may be hard to do, not clear without further analysis.  Alternately we might audit all uses of JSRESOLVE_ASSIGNING in resolve hooks, fixing the ones that define non-"simple" properties to be sure they resolve non-simple properties even on assignment.  Embedders might break on this change; do we care?

Thoughts from other SpiderMonkey hackers on this?  Brendan's position at least is clear.  :-)  I don't know what I think, depends on how break-ful nixing the resolve flag bit is.
What would we replace the nsDOMClassInfo uses with?
JSRESOLVE_ASSIGNING is usually tested to avoid falling into code that reifies the lazy property, on the assumption that creating a new property by assignment is the right thing. This bug shows how it's not the right thing for lazy read-only properties. I suspect the inefficiency of resolving a lazy writable property only to have it overwritten by the RHS is tolerable, but I could be wrong.

Otherwise, there could be latent bugs of any unknown-unknown kind hiding behind current (ab)use of JSRESOLVE_ASSIGNING, but we would be better off (early-ish in a release cycle) finding those bugs by getting rid of this troublesome flag.

/be
OK.  So the current classinfo consumers are:

1)  Avoiding ResolveConstructor when .constructor is being set.
2)  Avoiding the global scope polluter ResolveName/GetElementById call when
    assigning (or declaring/classname/qualified).  Only affects quirks-mode
    pages.
3)  Avoiding looking for child frames when a property whose name is an integer
    is set on the window.
4)  Some sort of special-case of .constructor on Windows.
5)  Avoiding nsWindowSH::GlobalResolve when assigning a property on a Window.
6)  Implementing replaceable properties on Windows.
7)  Avoiding XPConnect overhead on future expando accesses for Window by
    defining on assignment using stub getter/setter.
8)  Avoiding compilation of inline script event handlers when assigning to on*
    on event targets.
9)  Doing no work on assignments to document.all.something.
10) Making assigning to document.something faster.
11) Making assigning to HTMLFormElement.something faster.

I'm not sure what the possible cost in #1 is; Peter might know.

#2 seems like it could hurt.

#3,8,9,10,11 are probably not critical optimizations.

#4 may just be an optimization that can go.

#5 Peter would know better.

#6 we need a different solution; this is NOT a performance optimization as far as I can tell.

#7 could be a significant performance cost if we changed it, depending on whether it makes us fall off trace.
Getting rid of JSRESOLVE_ASSIGNING will be a bit of a trek, so this is a metabug.

Fixing the places that use it incorrectly is high priority -- thanks again to Waldo for finding two (str_resolve in jsstr.cpp, global_resolve in js.cpp). Any more? I vouched for nsWindowSH::NewResolve but mrbkap or bz should take a look.

/be
Depends on: 987007
Summary: How do you solve a problem like JSRESOLVE_ASSIGNING? → Remove JSRESOLVE_ASSIGNING
It's wafer thin. But this is only the beginning.

Note how many jsapi-tests I'm deleting here...
Assignee: general → jorendorff
Attachment #8397245 - Flags: review?(jwalden+bmo)
Comment on attachment 8397245 [details] [diff] [review]
Part 1 - Remove JSRESOLVE_ASSIGNING.

Review of attachment 8397245 [details] [diff] [review]:
-----------------------------------------------------------------

DO IT
Attachment #8397245 - Flags: review?(jwalden+bmo) → review+
>+        // ??? populating a PropertyDescriptor from a LookupProperty result
>+        // should be common enough to have a single implementation somewhere

Oops, stray comment. Removed locally.
Comment on attachment 8397250 [details] [diff] [review]
Part 2 - Remove flags from JS_GetPropertyDescriptor and friends.

Review of attachment 8397250 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +8585,5 @@
>                  Argument('JS::Handle<JSObject*>', 'wrapper'),
>                  Argument('JS::Handle<JSObject*>', 'obj'),
>                  Argument('JS::Handle<jsid>', 'id'),
>                  Argument('JS::MutableHandle<JSPropertyDescriptor>', 'desc'),
> +                Argument('unsigned', 'flags')]

Didn't you mean to flat-out remove this, not change trailing commas?

::: js/src/jsapi.cpp
@@ +3203,5 @@
>  }
>  
>  static bool
> +GetPropertyDescriptorById(JSContext *cx, HandleObject obj, HandleId id,
> +                          MutableHandle<PropertyDescriptor> desc)

Removing |own| and converting JS_GOPD to call GOPD as part of this patch seems unideal to me, but oh well.

@@ +3208,5 @@
>  {
>      RootedObject obj2(cx);
>      RootedShape shape(cx);
>  
> +    if (!LookupPropertyById(cx, obj, id, 0, &obj2, &shape))

This going to be another followup patch, probably, to remove the 0?

@@ +3232,5 @@
>                  desc.value().set(obj2->nativeGetSlot(shape->slot()));
>          }
>      } else {
>          if (obj2->is<ProxyObject>()) {
> +            JSAutoResolveFlags rf(cx, 0);

And to remove this as well?  And to remove JSARF since I don't remember it (and cx->resolveFlags) being removed elsewhere yet?

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1824,5 @@
>          return true;
>      }
>  
>      // Nothing in the cache. Call through, and cache the result.
> +    if (!Traits::singleton.resolveNativeProperty(cx, wrapper, holder, id, desc, 0))

And removing this flags argument as another followup?
Attachment #8397250 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> ::: dom/bindings/Codegen.py
> > +                Argument('unsigned', 'flags')]
> 
> Didn't you mean to flat-out remove this, not change trailing commas?

I have, in a later patch in this stack---the one for resolve hooks. There are quite a few different families of functions that passed the resolve flags around, and it was easier to cope doing a few at a time.

> > +    if (!LookupPropertyById(cx, obj, id, 0, &obj2, &shape))
> This going to be another followup patch, probably, to remove the 0?
> > +            JSAutoResolveFlags rf(cx, 0);
> And to remove this as well?  And to remove JSARF since I don't remember it
> (and cx->resolveFlags) being removed elsewhere yet?
> > +    if (!Traits::singleton.resolveNativeProperty(cx, wrapper, holder, id, desc, 0))
> And removing this flags argument as another followup?

Yes.
Attachment #8397647 - Flags: review?(jwalden+bmo)
wrong patch
Attachment #8397647 - Attachment is obsolete: true
Attachment #8397647 - Flags: review?(jwalden+bmo)
Attachment #8397649 - Flags: review?(jwalden+bmo)
Comment on attachment 8397642 [details] [diff] [review]
Part 3 - Remove flags argument from DefineNativeProperty.

Review of attachment 8397642 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobj.cpp
@@ +3732,5 @@
>  
>      AutoRooterGetterSetter gsRoot(cx, attrs, &getter, &setter);
>  
>      RootedShape shape(cx, JSObject::putProperty<mode>(cx, obj, id, getter, setter,
> +                                                      SHAPE_INVALID_SLOT, attrs, 0));

This flags goes away later too?

@@ +3797,5 @@
>          /*
>           * If we are defining a getter whose setter was already defined, or
>           * vice versa, finish the job via obj->changeProperty.
>           */
> +        if (!NativeLookupOwnProperty(cx, obj, id, 0, &shape))

And this.

::: js/src/jsobj.h
@@ +1405,5 @@
> +    /*
> +     * Unqualified property set.  Only used in the defineHow argument of
> +     * js_SetPropertyHelper.
> +     */
> +    DNP_UNQUALIFIED  = 2

0x1 and 0x2 might be better indicative of bitfielding here.
Attachment #8397642 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8397643 [details] [diff] [review]
Part 4 - Remove flags argument from resolve hooks.

Review of attachment 8397643 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobj.h
@@ -1588,5 @@
>  js_ReportGetterOnlyAssignment(JSContext *cx, bool strict);
>  
> -extern unsigned
> -js_InferFlags(JSContext *cx, unsigned defaultFlags);
> -

\o/

::: js/src/shell/js.cpp
@@ +2952,5 @@
>      RootedObject obj2(cx);
>  
>      objp.set(nullptr);
>      if (referent->isNative()) {
> +        if (!LookupPropertyWithFlags(cx, referent, id, 0, &obj2, &shape))

And this?
Attachment #8397643 - Flags: review?(jwalden+bmo) → review+
Attachment #8397645 - Flags: review?(jwalden+bmo) → review+
Attachment #8397649 - Flags: review?(jwalden+bmo) → review+
Attachment #8397650 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> >      RootedShape shape(cx, JSObject::putProperty<mode>(cx, obj, id, getter, setter,
> > +                                                      SHAPE_INVALID_SLOT, attrs, 0));
> 
> This flags goes away later too?

Not putProperty. It's a different kind of flags: Shape flags rather than resolve flags.

I discovered while doing this patch that having thirty totally unrelated things, all called "flags" and with effectively the same C++ type, can make things confusing.

> > +        if (!NativeLookupOwnProperty(cx, obj, id, 0, &shape))
> 
> And this.

For sure.

> > +    DNP_UNQUALIFIED  = 2
> 
> 0x1 and 0x2 might be better indicative of bitfielding here.

Turns out DNP_DONT_PURGE is unused. In bug 988751, I remove it and convert DNP_UNQUALIFIED to an enum-bool.
Summary: Remove JSRESOLVE_ASSIGNING → Remove JSRESOLVE_ASSIGNING and resolve flags
Blocks: 988751
Blocks: 993026
You need to log in before you can comment on or make changes to this bug.