Closed
Bug 465460
Opened 16 years ago
Closed 16 years ago
TM: valueOf ignored on third iteration of loop
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.9.1
People
(Reporter: jruderman, Assigned: brendan)
References
Details
(Keywords: testcase, verified1.9.1, Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 14 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
js> (function(d) { for (let j = 0; j < 5; ++j) { print('' + d); } })({valueOf: function()1}); 1 1 [object Object] 1 1
Assignee | ||
Comment 1•16 years ago
|
||
Fooey, no one uses valueOf :-P. Ok, ok -- I fix. /be
Assignee: general → brendan
Severity: normal → major
Status: NEW → ASSIGNED
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1
Assignee | ||
Comment 2•16 years ago
|
||
ECMA-262 specifies [[DefaultValue]] like so: 8.6.2.6 [[DefaultValue]] (hint) When the [[DefaultValue]] method of O is called with hint String, the following steps are taken: 1. Call the [[Get]] method of object O with argument "toString". 2. If Result(1) is not an object, go to step 5. 3. Call the [[Call]] method of Result(1), with O as the this value and an empty argument list. 4. If Result(3) is a primitive value, return Result(3). 5. Call the [[Get]] method of object O with argument "valueOf". 6. If Result(5) is not an object, go to step 9. 7. Call the [[Call]] method of Result(5), with O as the this value and an empty argument list. 8. If Result(7) is a primitive value, return Result(7). 9. Throw a TypeError exception. When the [[DefaultValue]] method of O is called with hint Number, the following steps are taken: 1. Call the [[Get]] method of object O with argument "valueOf". 2. If Result(1) is not an object, go to step 5. 3. Call the [[Call]] method of Result(1), with O as the this value and an empty argument list. 4. If Result(3) is a primitive value, return Result(3). 5. Call the [[Get]] method of object O with argument "toString". 6. If Result(5) is not an object, go to step 9. 7. Call the [[Call]] method of Result(5), with O as the this value and an empty argument list. 8. If Result(7) is a primitive value, return Result(7). 9. Throw a TypeError exception. When the [[DefaultValue]] method of O is called with no hint, then it behaves as if the hint were Number, unless O is a Date object (section 15.9), in which case it behaves as if the hint were String. The above specification of [[DefaultValue]] for native objects can return only primitive values. If a host object implements its own [[DefaultValue]] method, it must ensure that its [[DefaultValue]] method can return only primitive values. Addition passes no hint, other ops pass "number". We implement Date conversion a bit different, but it works out to the same answers. Please check this and ask questions. I'll invite mrbkap to review too. /be
Attachment #348921 -
Flags: review?(gal)
Assignee | ||
Updated•16 years ago
|
Attachment #348921 -
Flags: review?(mrbkap)
Comment 3•16 years ago
|
||
How do we know that at runtime the objects will have the same combination of methods? We could get entirely different combinations when we run the trace. /me is confused.
Assignee | ||
Comment 4•16 years ago
|
||
(In reply to comment #3) > How do we know that at runtime the objects will have the same combination of > methods? We could get entirely different combinations when we run the trace. > /me is confused. We talked about this in the imacros bug. Shapes guard method *values*, not just names. Tracing guards return type. The has*Method probes are just prolog to the JSOP_CALLPROP ops in the imacros, which indeed trace to shape-guards. /be
Comment 5•16 years ago
|
||
Yes, but what happens if the CALLPROP shape guard fails? How do we get back into the right imacro for this case? (the _other_ path)
Comment 6•16 years ago
|
||
IMO for this to work there should be only one method you call and its implemented differently in each object as needed. But dispatching at recording time doesn't work right as far as I can tell.
Comment 7•16 years ago
|
||
What would also work is to make this decision in the imacro, so jump around in the imacro to select what method to call. But it has to be in the imacro. Since thats where we return to and re-start recording to capture the other path.
Assignee | ||
Comment 8•16 years ago
|
||
Yeah, I need to implement the exact logic cited in comment 2 from ECMA-262 in the imacro body. This will reduce the number of imacros. Thanks, /be
Flags: blocking1.9.1?
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Comment 9•16 years ago
|
||
Comment on attachment 348921 [details] [diff] [review] proposed fix As discussed in the bug this has to all happen inside the same snippet, not by dispatching up front.
Attachment #348921 -
Flags: review?(gal) → review-
Assignee | ||
Comment 12•16 years ago
|
||
This is much more general, but still could be extended to support things like C pre-processor constants as immediate operands, e.g. (see the iter_imacros still hand-coded as C++ array initializers in jstracer.cpp). Alas this does not bootstrap. I really could use some advice from jimb and/or bsmedberg to get it right. I'd like to use Narcissus, if possible. More on IRC. Attaching now to get reviewers started, even though this isn't quite ready for checkin, because it will blow up the build system unless I hardcode to #include "imacros.c" and commit imacros.c.out from the objdir as srcdir/imacros.c! /be
Attachment #348921 -
Attachment is obsolete: true
Attachment #352207 -
Flags: review?(gal)
Attachment #348921 -
Flags: review?(mrbkap)
Assignee | ||
Comment 13•16 years ago
|
||
Many fixes, including separating null from object type -- necessary to pass trace-test.js tests such as testObjectOrderedCmp2. I also moved the lengthy Mandelbrot stuff down to next to last in trace-test.js /be
Attachment #352207 -
Attachment is obsolete: true
Attachment #352281 -
Flags: review?(gal)
Attachment #352207 -
Flags: review?(gal)
Assignee | ||
Updated•16 years ago
|
Attachment #352281 -
Flags: review?(danderson)
Assignee | ||
Comment 14•16 years ago
|
||
Comment on attachment 352281 [details] [diff] [review] proposed fix, take 3 I'd welcome mrbkap's review too. More the merrier, really. /be
Reporter | ||
Comment 15•16 years ago
|
||
> We depend on the snarf function from the js shell, defined
> only for the Narcissus metacircular evaluator (#ifdef NARCISSUS).
Does this mean my shell shouldn't have snarf()? I'm fine with it being defined
for normal js shells, as long as there's a way for paranoid users to disable
load() and snarf().
Reporter | ||
Comment 16•16 years ago
|
||
js> for each (let c in [null, null, null, {}]) { } Assertion failure: *(JSObject**)slot == NULL, at ../jstracer.cpp:1409
Reporter | ||
Comment 17•16 years ago
|
||
js> for each (let j in [null, 2, 3]) { } Assertion failure: (*m == JSVAL_TNULL) ? JSVAL_IS_NULL(*vp) : *m == JSVAL_TAG(*vp), at ../jstracer.cpp:2222
Assignee | ||
Comment 18•16 years ago
|
||
(In reply to comment #15) > > We depend on the snarf function from the js shell, defined > > only for the Narcissus metacircular evaluator (#ifdef NARCISSUS). > > Does this mean my shell shouldn't have snarf()? No, stale comment. I'll fix. > I'm fine with it being defined > for normal js shells, as long as there's a way for paranoid users to disable > load() and snarf(). What's the way to disable load? Whatever it is could apply to snarf too. /be
Reporter | ||
Comment 19•16 years ago
|
||
js> for (var j = 0; j < 3; ++j) { 1 & new Date; } Assertion failed: (!lhs->isQuad() && !rhs->isQuad()) || (lhs->isQuad() && rhs->isQuad()) (../nanojit/Nativei386.cpp:925) js> for (var j = 0; j < 3; ++j) { 1 & Date; } Assertion failure: !JS_ON_TRACE(cx), at ../jsobj.cpp:3776
Reporter | ||
Comment 20•16 years ago
|
||
js> for each (let x in [1, {}, 1, null, 1, {}, 1, null, 1]) { } Assertion failure: !ti->stackTypeMap.matches(ti_other->stackTypeMap), at ../jstracer.cpp:3035
Reporter | ||
Comment 21•16 years ago
|
||
js> e = <x/>; for (j=0;j<3;++j) { 3 | e; } "PASS"; typein:1: TypeError: is not a function
Assignee | ||
Comment 22•16 years ago
|
||
(In reply to comment #18) > > I'm fine with it being defined > > for normal js shells, as long as there's a way for paranoid users to disable > > load() and snarf(). > > What's the way to disable load? Whatever it is could apply to snarf too. Jesse mentioned the one true way: delete load -- delete snarf works too. Debugging the fuzz-found bugs. Keep them coming. /be
Assignee | ||
Comment 23•16 years ago
|
||
Fixed comment 16, comment 17, and comment 20 testcases. /be
Attachment #352281 -
Attachment is obsolete: true
Attachment #352281 -
Flags: review?(gal)
Attachment #352281 -
Flags: review?(danderson)
Assignee | ||
Comment 24•16 years ago
|
||
Bugzilla interdiff shows a crucial fix to a latent JSVAL_TAG abusage bug in TraceRecorder::checkType (look for uint8 vt). /be
Reporter | ||
Comment 25•16 years ago
|
||
js> for each (let _ in [{}, {}, null, null, null, null]) { } Assertion failure: !ti->stackTypeMap.matches(ti_other->stackTypeMap), at ../jstracer.cpp:3037
Assignee | ||
Comment 26•16 years ago
|
||
This fixes all the fuzz-found problems except the E4X one in comment 21. I will deal with that tomorrow morning, but in the mean time, this is ready for review and more fuzz-testing. /be
Attachment #352432 -
Attachment is obsolete: true
Attachment #352501 -
Flags: review?(gal)
Reporter | ||
Comment 27•16 years ago
|
||
js> for (let i in (function() { for (var j = 0; j < 3; ++j) yield; })()) { } Assertion failure: !JS_ON_TRACE(cx), at ../jsobj.cpp:3776
Reporter | ||
Comment 28•16 years ago
|
||
js> for (let i = 0; i < 2; ++i) { ({}) + 3; } Assertion failure: !JS_ON_TRACE(cx), at ../jsobj.cpp:3776
Assignee | ||
Comment 29•16 years ago
|
||
Correct kshape updating logic on "brandfill" in js_FillPropertyCache, for comment 27 and comment 28 testcases. /be
Attachment #352501 -
Attachment is obsolete: true
Attachment #352560 -
Flags: review?(gal)
Attachment #352501 -
Flags: review?(gal)
Reporter | ||
Comment 30•16 years ago
|
||
cat ~/q.js for (let d=0;d<2;++d) for (let a=0;a<1;++a) "" + (d && function(){}); ~/tracemonkey/js/src/debug-valueof6/js -j ~/q.js Assertion failed: "need a way to EOT now, since this is trace end": 0 (../nanojit/LIR.cpp:984)
Assignee | ||
Comment 31•16 years ago
|
||
David, could use your help. Why does the testcase in comment 30 work interactively (as a one-line program typed to the interactive shell, I mean) but assertbotch when sourced from a file? /be
Assignee | ||
Comment 33•16 years ago
|
||
Attachment #352560 -
Attachment is obsolete: true
Attachment #352560 -
Flags: review?(gal)
Assignee | ||
Comment 34•16 years ago
|
||
Attachment #352613 -
Attachment is obsolete: true
Attachment #352631 -
Flags: review?(gal)
Reporter | ||
Comment 35•16 years ago
|
||
js> for (var j = 0; j < 2; ++j) { if (null > "") { } } Assertion failure: JSVAL_IS_NUMBER if int/double, objects should have been handled at start of method, at ../jstracer.cpp:4728 Happens with 6b and 7, both applied to 56029d4f6305. Does not happen with 56029d4f6305 alone, so not simply a regression from Waldo's change in bug 465255.
Assignee | ||
Comment 36•16 years ago
|
||
(Eight takes is nothing. From the wikipedia entry for the film "Shane": "Jack Palance had problems with horses and Alan Ladd with guns. The scene where Shane practices shooting in front of Joey required 116 takes. A scene where Jack Palance mounts his horse was actually a shot of him dismounting, but played in reverse. As well, the original planned introduction of Wilson galloping into town was replaced with him simply walking in on his horse, which was noted as improving the entrance by making him seem more threatening." Next patch will be walked into town. /be)
Attachment #352631 -
Attachment is obsolete: true
Attachment #352639 -
Flags: review?(gal)
Attachment #352631 -
Flags: review?(gal)
Assignee | ||
Comment 37•16 years ago
|
||
(In reply to comment #35) > Happens with 6b and 7, both applied to 56029d4f6305. Does not happen with > 56029d4f6305 alone, so not simply a regression from Waldo's change in bug > 465255. It was a bad merge by yours truly -- Waldo and I collided at a level hg could not see. Fixed by take 8. /be
Comment 38•16 years ago
|
||
Comment on attachment 352639 [details] [diff] [review] proposed fix, take 8 I did not review the js source of the assembler and load/snarf changes.
Attachment #352639 -
Flags: review?(gal) → review+
Assignee | ||
Comment 39•16 years ago
|
||
At Andreas's urging, I committed to tm, a bit undertested. Will file followups on comment 21 and any other probs that don't cause a back-out! http://hg.mozilla.org/tracemonkey/rev/f682453c06d0 /be
Comment 40•16 years ago
|
||
shoot. I had to back this out for three reasons: 1.) fails a scriptaculous mochitest on linux and macintel should be possible to run in isolation with runtests.py --path=... 2.) fails to build on mac ppc (probably an ifdef checking for mac but not intel) 3.) fails to build on windows because off_t is not defined there http://hg.mozilla.org/tracemonkey/rev/5a26ec73cf0d
Comment 41•16 years ago
|
||
(In reply to comment #40) > should be possible to run in isolation with runtests.py --path=... Of course what was meant was --test-path=dom/tests/mochitest/ajax/scriptaculous or somesuch, not --path.
Assignee | ||
Comment 42•16 years ago
|
||
Are 2 and 3 due to the js.cpp changes? I'm reverting them to avoid trouble (it's easy to fix them too but not important right now, given the by-hand generation of imacros.c.out). I'll look at 1 when I have time (off work today). Thanks for the back-out help. /be
Updated•16 years ago
|
Attachment #352639 -
Attachment is obsolete: false
Updated•16 years ago
|
Attachment #352812 -
Attachment is obsolete: true
Comment 45•16 years ago
|
||
Comment on attachment 352812 [details] [diff] [review] v9 Can't use hg diff until the file generated is added.
Assignee | ||
Comment 47•16 years ago
|
||
Unless I goofed up subtly, it looks like mq doesn't keep track of hg add'ed files. /be
Attachment #352829 -
Attachment is obsolete: true
Assignee | ||
Comment 48•16 years ago
|
||
I'm thinking FAIL_VOID is not useful -- we could eliminate it by using FAIL_JSVAL. Comments welcome. /be
Attachment #353094 -
Attachment is obsolete: true
Assignee | ||
Comment 50•16 years ago
|
||
Jason, could you please review the interdiff against last patch (I hope bugzilla doesn't botch it). I didn't remove FAIL_VOID since that's useful for fallibles that return a boolean. I changed OBJECT_FAIL_VOID to be named simply OBJECT_FAIL (as opposed to the extant OBJECT_FAIL_NULL, which means null as failure code where success means a non-null object). OBJECT_FAIL now uses FAIL_JSVAL, so String_p_match* return jsval. Let me know if anything is amiss here, thanks. Jesse, this fixes bug 469239. /be
Attachment #353233 -
Attachment is obsolete: true
Attachment #353316 -
Flags: review?(jorendorff)
Assignee | ||
Comment 51•16 years ago
|
||
Landed in tm again, r=gal -- will fix anything left over that anyone wants in a new bug, assuming this sticks: http://hg.mozilla.org/tracemonkey/rev/ac84a530de97 I removed OBJECT_FAIL altogether, it was misleading to have this duplicate JSVAL_FAIL -- Jason, holler if that's wrong. /be
Assignee | ||
Comment 52•16 years ago
|
||
Attachment #353316 -
Attachment is obsolete: true
Attachment #353316 -
Flags: review?(jorendorff)
Assignee | ||
Comment 53•16 years ago
|
||
Trivial followup fix: http://hg.mozilla.org/tracemonkey/rev/370f4468be2f /be
Assignee | ||
Comment 54•16 years ago
|
||
More from Andreas: http://hg.mozilla.org/tracemonkey/rev/1329b1e3edd3 http://hg.mozilla.org/tracemonkey/rev/7c898d7b2e9e /be
Updated•16 years ago
|
Whiteboard: fixed-in-tracemonkey
Comment 55•16 years ago
|
||
OBJECT_FAIL_VOID was faster than JSVAL_FAIL, which requires unboxing on trace. But, the less bizarre calling conventions, the better. If we're going to use JSVAL_FAIL for String_p_match, I think we could revert my changes in bug 453564.
Assignee | ||
Comment 56•16 years ago
|
||
(In reply to comment #55) > OBJECT_FAIL_VOID was faster than JSVAL_FAIL, which requires unboxing on trace. The split between JSVAL_TNULL and JSVAL_OBJECT in typemap-types requires most of unbox_jsval, plus the resumeAfter logic. So it seemed better to use FAIL_JSVAL and avoid adding more resumeAfter fail-types and special cases. > If we're going to use JSVAL_FAIL for String_p_match, I think we could revert my > changes in bug 453564. Just the first hunk, right? /be
Comment 57•16 years ago
|
||
merged to mc
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 58•16 years ago
|
||
(In reply to comment #56) > (In reply to comment #55) > > If we're going to use JSVAL_FAIL for String_p_match, I think we could revert > > my changes in bug 453564. > > Just the first hunk, right? No, I think we can revert it all. (Well--we'd keep the extern js_String_p_match -> static String_p_match change and the test.) The problem in that bug was that the recorded return type of String.prototype.match was boolean, but the traceable native returned JSObject*. Since we've changed the traceable native to return jsval, that conflict can't happen anymore.
Comment 60•16 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d9084a656268 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d50da676e473 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ff1345ee6580 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a7179884097e
Keywords: fixed1.9.1
Comment 61•16 years ago
|
||
(In reply to comment #21) > js> e = <x/>; for (j=0;j<3;++j) { 3 | e; } "PASS"; > typein:1: TypeError: is not a function I still see this with -j.
Assignee | ||
Comment 62•16 years ago
|
||
(In reply to comment #61) > (In reply to comment #21) > > js> e = <x/>; for (j=0;j<3;++j) { 3 | e; } "PASS"; > > typein:1: TypeError: is not a function > > I still see this with -j. Eek. New bug please? /be
Comment 63•16 years ago
|
||
Checking in js1_8/regress/regress-465460-01.js Checking in js1_8/regress/regress-465460-02.js Checking in js1_8/regress/regress-465460-03.js Checking in js1_8/regress/regress-465460-04.js Checking in js1_8/regress/regress-465460-05.js Checking in js1_8/regress/regress-465460-06.js Checking in js1_8/regress/regress-465460-07.js Checking in js1_8/regress/regress-465460-08.js Checking in js1_8/regress/regress-465460-09.js Checking in js1_8/regress/regress-465460-10.js Checking in js1_8/regress/regress-465460-11.js Checking in js1_8/regress/regress-465460-12.js http://hg.mozilla.org/mozilla-central/rev/dfcbb7037e6c filed Bug 475153 for js1_8/regress/regress-465460-07.js - 'TypeError: is not a function'
Flags: in-testsuite+
Flags: in-litmus-
Comment 64•16 years ago
|
||
v 1.9.1, 1.9.2 modulo Bug 475153 js1_8/regress/regress-465460-07.js
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•