Last Comment Bug 642772 - TM: [infer failure] Missing type at #3:00006 pushed 0: String.split
: TM: [infer failure] Missing type at #3:00006 pushed 0: String.split
Status: RESOLVED FIXED
[fixed-in-tracemonkey]
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: ---
Assigned To: general
:
:
Mentors:
: 651199 651232 653399 (view as bug list)
Depends on: 654073
Blocks: infer-regress langfuzz
  Show dependency treegraph
 
Reported: 2011-03-18 05:21 PDT by Christian Holler (:decoder)
Modified: 2013-01-14 08:20 PST (History)
8 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
shell testcase, unpack, chdir and run main.js with options "-n -a" (834 bytes, application/x-compressed-tar)
2011-03-18 05:21 PDT, Christian Holler (:decoder)
no flags Details
Kinda ugly fix (2.79 KB, patch)
2011-04-21 12:51 PDT, Paul Biggar
no flags Details | Diff | Splinter Review
updated (9.12 KB, patch)
2011-04-22 08:27 PDT, Paul Biggar
bhackett1024: review+
Details | Diff | Splinter Review
Add proxy and reflect (12.88 KB, patch)
2011-04-26 10:08 PDT, Paul Biggar
bhackett1024: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2011-03-18 05:21:18 PDT
Created attachment 520178 [details]
shell testcase, unpack, chdir and run main.js with options "-n -a"

The attached test case (run main.js with -n -a) crashes on TI tip, tested on 64
bit.
Comment 1 Brian Hackett (:bhackett) 2011-03-19 12:57:29 PDT
This looks like a bug in TM related to lazy standard classes.

JS_EnumerateStandardClasses calls obj->nativeContains for each standard atom to figure out whether that class needs initialization.  If the class had previously been initialized and then deleted, a new version of it (and all its associated natives, etc.) will be created and attached to the global, confusing inference (which thinks that compileAndGo code will only ever have a single String/Number/Boolean/etc.)

With a TM shell:

js> String
function String() {[native code]}
js> delete String
true
js> String
typein:3: ReferenceError: String is not defined
js> for (x in this) {}
js> String
function String() {[native code]}

Also:

var one = String.prototype.split;
delete String;
var two = ("foo").split;
for (x in this) {}
var three = ("foo").split;
print(one === two);
print(one === three);

d8/jsc:
true
true

js:
true
false
Comment 2 Brian Hackett (:bhackett) 2011-04-19 11:42:16 PDT
*** Bug 651119 has been marked as a duplicate of this bug. ***
Comment 3 Brian Hackett (:bhackett) 2011-04-19 11:43:21 PDT
*** Bug 651199 has been marked as a duplicate of this bug. ***
Comment 4 Brian Hackett (:bhackett) 2011-04-19 23:20:01 PDT
*** Bug 651232 has been marked as a duplicate of this bug. ***
Comment 5 Paul Biggar 2011-04-21 12:51:48 PDT
Created attachment 527628 [details] [diff] [review]
Kinda ugly fix

I'm not 100% happy with this solution, but it works. I tried the way you suggested, which was to add a prototype and a constructor, but that was user visible and didn't match the spec (in that JSON.prototype was not undefined anymore).

I have more tests to add here, from the duplicated bugs.
Comment 6 Paul Biggar 2011-04-21 13:48:37 PDT
Comment on attachment 527628 [details] [diff] [review]
Kinda ugly fix

Gonna see if I can't fix:

delete JSON;
for (x in this);
assertEq(JSON, undefined);

while I'm at it.
Comment 7 Paul Biggar 2011-04-22 08:27:28 PDT
Created attachment 527776 [details] [diff] [review]
updated

This makes it more explicit what we're doing, and handles one case we didn't before, and adds many more tests. I didn't include the tests from the duplicates - I think I need to try them on the JM branch first.

Note this patch is for TM.
Comment 8 Brian Hackett (:bhackett) 2011-04-22 10:27:17 PDT
Comment on attachment 527776 [details] [diff] [review]
updated


>+        /*
>+         * If the constructor is undefined, then it hasn't been initialized
>+         * (see also js::MarkInitializedNoProto.
>+         */

Missing a ')', also MarkInitializedNoProto doesn't seem to exist.

>+void
>+DefineNoConstructorAndPrototype(JSContext* cx, JSProtoKey key)
>+{
>+    /*
>+     * Lazy standard classes need a way to indicate if they have been
>+     * initialized. Otherwise, when we delete them, we might accidentally
>+     * recreate them via a lazy initialization. We use the presence of a ctor
>+     * or proto in the globalObject's slot to indicate that they've been
>+     * constructed, but this only works for classes which have a proto and
>+     * ctor. Classes which don't have one can call this function.
>+     *
>+     * We use True so that it's obvious what we're doing (instead of, say, Null).
>+     * If the class has no constructor (JSON and Math), then our test
>+     */
>+    if (cx->globalObject->getReservedSlot(key) == UndefinedValue())
>+        cx->globalObject->setSlot(key, BooleanValue(true));
>+}*

DefineNoConstructorAndPrototype should have a better name ("if it doesn't define a constructor and prototype, then what *does* it do?").  MarkStandardClassInitializedNoProto?  Also, the last sentence in the comment trails off.

>-fails-if(xulRuntime.shell) script regress-633741.js
>+script regress-633741.js

Nice.

I think Proxy follows the same pattern as Math and JSON and should mark the class as initialized.  No others I'm aware of, but you might want to double check.

r+ with these addressed.
Comment 9 Paul Biggar 2011-04-26 10:08:27 PDT
Created attachment 528335 [details] [diff] [review]
Add proxy and reflect

Just sending this for another round of review since I've change how it works a bit. You were right about proxy, but Reflect was also like that, and I needed to refactor things a little bit to make that work in a non-ugly way.
Comment 10 Brian Hackett (:bhackett) 2011-04-26 10:19:29 PDT
Comment on attachment 528335 [details] [diff] [review]
Add proxy and reflect

Review of attachment 528335 [details] [diff] [review]:

::: js/src/jsobj.h
@@ +1549,5 @@
                               JSPropertySpec *ps, JSFunctionSpec *fs,
                               JSPropertySpec *static_ps, JSFunctionSpec *static_fs);
+
+bool
+isStandardClassResolved(JSObject *obj, js::Class *clasp);

I think the convention here is that non-member functions should start with an uppercase.
Comment 12 Brian Hackett (:bhackett) 2011-04-28 08:48:32 PDT
*** Bug 653399 has been marked as a duplicate of this bug. ***
Comment 13 Christian Holler (:decoder) 2011-05-01 10:25:54 PDT
Brian merged this and other changes to TI at 

http://hg.mozilla.org/projects/jaegermonkey/rev/02c4a0f752f3

and I can no longer reproduce this on TI trunk, so I assume this is fixed now also on that branch :)
Comment 14 Christian Holler (:decoder) 2013-01-14 08:20:04 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug642772-2.js.

Note You need to log in before you can comment on or make changes to this bug.