466781 - TM: inconsistent (0 in d) where d is a String

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

js> for each (let d in [new String('q'), new String('q'), new String('q')]) { print('' + (0 in d)); }

true
true
false

Comment 1

[Robert Sayre]

WFM on tm.

Comment 2

[Robert Sayre]

oops, no, it is still broken

Comment 3

[Andreas Gal :gal]

Note that these are string objects, not primitive strings. This calls into HasNamedProperty.

Comment 4

[Robert Sayre]

Attachment: back traces for each str_resolve

Look at the resolve flags being passed from the third, traced property lookup.

Looks like cx->resolveFlags is not right.

Comment 5

[Robert Sayre]

InferFlags() in jsobj.cpp doesn't seem to get the right answer on trace.

Comment 6

[Robert Sayre]

The copy of js_GetTopStackFrame in jstracer.cpp references bug 462027. Does this mean we're broken for anything calling InferFlags, and objects with custom JSResolveOps in this particular case?

Comment 7

[Robert Sayre]

OK, after talking to jorendorff, I think this depends on bug 462027, so we can side exit in case there's a resolve op that re-enters. However, it seems to me that InferFlags() needn't interfere with us in this particular case, and we can get around it by setting the resolve op.

Comment 8

[Robert Sayre]

Attachment: set resolve flags

I think this will work because these functions are called only from a recorded record_JSOP_IN. 

Seems like this should be an improvement, but not a perfect fix without a way to bail if we hit a meddling resolve implementation.

Comment 9

[Jason Orendorff [:jorendorff]]

This patch is missing trace-tests!

Rob, you can make the builtin fail if anything about the situation smells fishy. (Non-native object, class has a resolve hook, whatever.) Then we'll just bail out and retry from the interpreter.

I think a lot of builtins (and more all the time) are "optimistic" about doing stuff that could examine the stack, call random native code, or reenter the interpreter. The #ifdef DEBUG_jason assertion in js_GetTopStackFrame catches those cases, and I hit it a lot. That assert is going to be turned on for everyone in bug 462027 -- brace yourselves.

Comment 10

[Robert Sayre]

Attachment: bail on unknown resolve ops, set the resolve flags

now with bailing and tests

Comment 11

[Robert Sayre]

Attachment: consolidate some more code

Comment 12

[Robert Sayre]

Attachment: consolidate some more code

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 356133 [details] [diff] [review]
consolidate some more code

>+static JSBool HasProperty(JSContext* cx, JSObject* obj, jsid id)

House style wants function name on its own line after qualifiers/type/mode.

> {
>+    // check whether we know how the resolve op will behave
>+    JSClass* clasp = OBJ_GET_CLASS(cx, obj);
>+    if (clasp->resolve != JS_ResolveStub && clasp != &js_StringClass)
>         return JSVAL_TO_BOOLEAN(JSVAL_VOID);
>+    JSAutoResolveFlags rf(cx, JSRESOLVE_QUALIFIED); 
> 

No trailing whitespace please!

Prettier to put the blank line before rf's definition, not after.

r=me with nits picked.

/be

Comment 14

[Robert Sayre]

Attachment: nits picked for checkin

Comment 15

[Robert Sayre]

http://hg.mozilla.org/tracemonkey/rev/080ccddea3b5

Comment 16

[Jason Orendorff [:jorendorff]]

I'm not sure this is good enough--property lookups can also call resolve hooks for other objects up the prototype chain.

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> I'm not sure this is good enough--property lookups can also call resolve hooks
> for other objects up the prototype chain.

You're right, this could be an issue for XPConnect and the DOM. For the core engine it shouldn't be. Separated bug needed.

/be

Comment 18

[Jason Orendorff [:jorendorff]]

Filed bug 473075.

Comment 19

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/080ccddea3b5

Comment 20

[Bob Clary [:bc] (inactive)]

test updated in http://hg.mozilla.org/mozilla-central/rev/7eb907886ae7 
and  /cvsroot/mozilla/js/tests/js1_8_1/trace/trace-test.js,v  <-- 
trace-test.js
new revision: 1.9; previous revision: 1.8


verified 1.9.1 tracemonkey, 1.9.2.

Comment 21

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5d4969d1620b

Comment 22

[Bob Clary [:bc] (inactive)]

v 1.9.1