Closed Bug 642772 Opened 14 years ago Closed 14 years ago

TM: [infer failure] Missing type at #3:00006 pushed 0: String.split

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: decoder, Unassigned)

References

Details

(Keywords: crash, testcase, Whiteboard: [fixed-in-tracemonkey])

Attachments

(2 files, 2 obsolete files)

The attached test case (run main.js with -n -a) crashes on TI tip, tested on 64 bit.
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
Summary: TI: [infer failure] Missing type at #3:00006 pushed 0: String.split → TM: [infer failure] Missing type at #3:00006 pushed 0: String.split
Assignee: general → pbiggar
Attached patch Kinda ugly fix (obsolete) — Splinter Review
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.
Attachment #527628 - Flags: review?(bhackett1024)
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.
Attachment #527628 - Flags: review?(bhackett1024)
Attached patch updated (obsolete) — Splinter Review
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.
Attachment #527628 - Attachment is obsolete: true
Attachment #527776 - Flags: review?(bhackett1024)
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.
Attachment #527776 - Flags: review?(bhackett1024) → review+
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.
Attachment #527776 - Attachment is obsolete: true
Attachment #528335 - Flags: review?(bhackett1024)
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.
Attachment #528335 - Flags: review?(bhackett1024) → review+
Assignee: pbiggar → general
Whiteboard: [fixed-in-tracemonkey]
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 :)
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug642772-2.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: