Closed
Bug 369740
Opened 17 years ago
Closed 17 years ago
Using generic code for function:: (e4x/Regress/regress-369740.js)
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
People
(Reporter: igor, Assigned: igor)
References
Details
Attachments
(3 files, 3 obsolete files)
22.29 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
37.26 KB,
text/plain
|
Details | |
1.25 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
Currently function:: prefix in SpiderMonkey is implemented either through a special bytecode constructing a qname with an internal function namespace or through GET_METHOD/SET_METHOD bytecode. But the latter is redundant since the special namespace can also be used to implement object.function:: operations. Thus I suggest to remove special handling of object.function:: for the price of more expensive runtime support for this rarely used operation. This should also simplify patches for bug 363530 and bug 362910.
Assignee | ||
Comment 2•17 years ago
|
||
The initial patch that removes JSOP_(GET|SET)METHOD support in the decompiler, renames JSOP_GETMETHOD into JSOP_CALLPROP to be in sync with bug 363530 and adds support in the interpreter for getelem of qname with the function namespace. The patch does not work correctly as with it the following one line script generates an error an the first access: ~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js js> Math.function::sin Math.function::sin typein:1: TypeError: Math has no properties js> Math.function::sin Math.function::sin function sin() { [native code] }
Assignee: general → igor
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•17 years ago
|
||
In this version I removed CALLELEM bytecode as the patch does not need it even if patches for bug 363530 or bug 362910 will introduce it.
Assignee | ||
Comment 4•17 years ago
|
||
Comment on attachment 254784 [details] [diff] [review] Implementation v1 The patch does not bring any new regressions against the test suite.
Attachment #254784 -
Flags: review?(brendan)
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.8.1.3?
Flags: blocking1.8.0.11?
Comment 6•17 years ago
|
||
Comment on attachment 254784 [details] [diff] [review] Implementation v1 Looks good, only a nit and a question: Nit: "CheckIf" should be "CheckWhether" by preferred English Usage, but that's just long for "Is" -- so how about "IsFunctionQName" for the whole method naming convention? Question: does this still work with the patch: js> x = <xml/> js> x.function::toString = function(){return "moo"} function () { return "moo"; } js> x moo ? /be
Assignee | ||
Comment 7•17 years ago
|
||
(In reply to comment #6) > Question: does this still work with the patch: > > js> x = <xml/> > > js> x.function::toString = function(){return "moo"} > function () { > return "moo"; > } > js> x > moo Yes as a consequence of http://lxr.mozilla.org/seamonkey/source/js/src/jsxml.c#4633. But that also points out to the problem with the patch with XMLList: it does not work with that as PutProperty does not check for funid for xml lists. On the other hand such bug already presents in SM as the following example demonstrates: ~/m/trunk/mozilla/js/src $ cat ~/s/y.js var x = <><a/><b/></> with (x) { function::toString = function() { return "test"; } } print(x) ~/m/trunk/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/s/y.js /home/igor/s/y.js:4: TypeError: can't set property @mozilla.org/js/function::toString in XMLList I will file tomorrow a bug about it.
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.8.1.3?
Flags: blocking1.8.0.11?
Comment 9•17 years ago
|
||
Comment on attachment 254784 [details] [diff] [review] Implementation v1 New patch coming based on nit? /be
Attachment #254784 -
Flags: review?(brendan)
Comment 10•17 years ago
|
||
Comment on attachment 254784 [details] [diff] [review] Implementation v1 Sorry, meant to + with nit fix assumed. /be
Attachment #254784 -
Flags: review+
Assignee | ||
Comment 12•17 years ago
|
||
Fixing bug 370372 is necessary to make sure that assignments xml_list.function::name works using generic XML support.
Depends on: 370372
Assignee | ||
Comment 13•17 years ago
|
||
This is the previous attachment updated to sync with the trunk and with CheckIfFunctionQName renamed into IsFunctionQName.
Attachment #254424 -
Attachment is obsolete: true
Attachment #254784 -
Attachment is obsolete: true
Attachment #255880 -
Flags: review?(brendan)
Comment 14•17 years ago
|
||
Comment on attachment 255880 [details] [diff] [review] Implementation v2 Still looks good. One nit: comment js_IsFunctionQName in jsxml.h to talk about the *funid == 0 meaning (might use funidp as the formal parameter name, too). /be
Attachment #255880 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 15•17 years ago
|
||
Patch to commit with comments added.
Attachment #255880 -
Attachment is obsolete: true
Attachment #255934 -
Flags: review+
Assignee | ||
Comment 16•17 years ago
|
||
I committed the patch from comment 15 to the trunk: Checking in js.c; /cvsroot/mozilla/js/src/js.c,v <-- js.c new revision: 3.133; previous revision: 3.132 done Checking in jsemit.c; /cvsroot/mozilla/js/src/jsemit.c,v <-- jsemit.c new revision: 3.236; previous revision: 3.235 done Checking in jsemit.h; /cvsroot/mozilla/js/src/jsemit.h,v <-- jsemit.h new revision: 3.72; previous revision: 3.71 done Checking in jsinterp.c; /cvsroot/mozilla/js/src/jsinterp.c,v <-- jsinterp.c new revision: 3.331; previous revision: 3.330 done Checking in jsopcode.c; /cvsroot/mozilla/js/src/jsopcode.c,v <-- jsopcode.c new revision: 3.208; previous revision: 3.207 done Checking in jsopcode.tbl; /cvsroot/mozilla/js/src/jsopcode.tbl,v <-- jsopcode.tbl new revision: 3.87; previous revision: 3.86 done Checking in jsparse.c; /cvsroot/mozilla/js/src/jsparse.c,v <-- jsparse.c new revision: 3.269; previous revision: 3.268 done Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.143; previous revision: 3.142 done Checking in jsxml.h; /cvsroot/mozilla/js/src/jsxml.h,v <-- jsxml.h new revision: 3.16; previous revision: 3.15 done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 17•17 years ago
|
||
this checkin is causing a crash during the unit test run
Updated•17 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 18•17 years ago
|
||
(In reply to comment #17) > Created an attachment (id=255939) [details] > stack trace > > this checkin is causing a crash during the unit test run How can I see which unit test failed?
Comment 19•17 years ago
|
||
Crazy: someone has 0 in a jsid and is calling InternNonXmlObjectId: 1 libmozjs.dylib 0x0105ec5e InternNonXmlObjectId + 31 (jsinterp.c:1971) 2 libmozjs.dylib 0x0106d50b js_Interpret + 59487 (jsinterp.c:3825) What are script->filename and script->lineno in that js_Interpret frame? /be
Comment 20•17 years ago
|
||
(In reply to comment #18) > > How can I see which unit test failed? I you click on L- and View Brief Log, and scroll down to the bottom, you'll find: *** 0 INFO SimpleTest START *** 1 INFO Running /tests/content/base/test/test_bug218236.html... *** 2 INFO PASS | event sequence for '200 OK' should be 1, 2, 3, 4, load *** 3 INFO PASS | event sequence for '404 Not Found' should be 1, 2, 3, 4, load *** 4 INFO PASS | event sequence for 'connection error' should be 1, 2, 4, error *** 5 INFO PASS | event sequence for 'abort() call on readyState = 1' should be 1, 4 *** 6 INFO PASS | event sequence for 'abort() call on readyState = 2' should be 1, 2, 4 *** 8 INFO Running /tests/content/base/test/test_bug218277.html... *** 9 INFO PASS | nbsp preserved in form submissions *** 11 INFO Running /tests/content/base/test/test_bug238409.html... *** 12 INFO PASS | parsing table with cellspacing='0' *** 13 INFO PASS | parsing table with cellspacing='2' *** 14 INFO PASS | parsing table without cellspacing *** 15 INFO PASS | parsing table with malformed cellspacing *** 17 INFO Running /tests/content/base/test/test_bug276037-1.html... FAIL Exited with code 10 during test run kill TERM 3264 Process killed. Took 1 second to die. started: Wed Feb 21 13:37:47 2007 finished: Wed Feb 21 13:37:51 2007 which is this file: http://lxr.mozilla.org/seamonkey/source/content/base/test/test_bug276037-1.html
Assignee | ||
Comment 21•17 years ago
|
||
(In reply to comment #19) > Crazy: someone has 0 in a jsid and is calling InternNonXmlObjectId: Yep. And jsid should never be 0, right?
Assignee | ||
Comment 22•17 years ago
|
||
(In reply to comment #21) > (In reply to comment #19) > > Crazy: someone has 0 in a jsid and is calling InternNonXmlObjectId: > > Yep. And jsid should never be 0, right? > It can be due to the optimizer work: js> const xhtmlNS = null; const xhtmlNS = null; js> this[xhtmlNS] = {}; this[xhtmlNS] = {}; Segmentation fault
Assignee | ||
Comment 23•17 years ago
|
||
(In reply to comment #20) > (In reply to comment #18) > > > > How can I see which unit test failed? > > I you click on L- and View Brief Log, and scroll down to the bottom, you'll > find: Thanks, that allowed to find the regression. Patch in a moment.
Assignee | ||
Comment 24•17 years ago
|
||
Attachment #255942 -
Flags: review?(brendan)
Comment 25•17 years ago
|
||
Comment on attachment 255942 [details] [diff] [review] Fix for regression r=me, I shoulda caught that last time. /be
Attachment #255942 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 26•17 years ago
|
||
(In reply to comment #22) > > Yep. And jsid should never be 0, right? > > > > It can be due to the optimizer work: > > js> const xhtmlNS = null; > const xhtmlNS = null; > js> this[xhtmlNS] = {}; > this[xhtmlNS] = {}; In fact the source of the bug is that JSID_IS_OBJECT(id) returns true when id is 0. This is different from JSVAL_IS_OBJECT behavior and that test case should byte anybody who compiles SM without XML support as jsinterp.c contains: #if JS_HAS_XML_SUPPORT ... #define CHECK_ELEMENT_ID(obj, id) \ ... #else #define CHECK_ELEMENT_ID(obj, id) JS_ASSERT(!JSID_IS_OBJECT(id)) #endif That would assert with the test case.
Assignee | ||
Comment 27•17 years ago
|
||
I committed the regression fix: Checking in jsinterp.c; /cvsroot/mozilla/js/src/jsinterp.c,v <-- jsinterp.c new revision: 3.332; previous revision: 3.331 done
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Comment 28•17 years ago
|
||
JSVAL_IS_OBJECT(JSVAL_NULL) => true, always has. The JSID_* macros match, bug for bug compatibility! Can you fix the latent bogus-assertion bug (#if !JS_HAS__XML_SUPPORT) too? /be
Comment 29•17 years ago
|
||
crash on startup (in -safe-mode/normal mode) TB29556357G and others, identical
Assignee | ||
Comment 30•17 years ago
|
||
(In reply to comment #26) > (In reply to comment #22) > > > Yep. And jsid should never be 0, right? > > > > > > > It can be due to the optimizer work: > > > > js> const xhtmlNS = null; > > const xhtmlNS = null; > > js> this[xhtmlNS] = {}; > > this[xhtmlNS] = {}; > > In fact the source of the bug is that JSID_IS_OBJECT(id) returns true when id > is 0. This is different from JSVAL_IS_OBJECT behavior and that test case should > byte anybody who compiles SM without XML support as jsinterp.c contains: > > #if JS_HAS_XML_SUPPORT > ... > #define CHECK_ELEMENT_ID(obj, id) \ > ... > #else > #define CHECK_ELEMENT_ID(obj, id) JS_ASSERT(!JSID_IS_OBJECT(id)) > #endif > > That would assert with the test case. Please ignore that stupid text. There is no bug in non-xml case due to the code logic in FETCH_ELEMENT_ID (which the assert enforces) and JSID_IS_OBJECT behaves in the same way as JSVAL_IS_OBJECT.
Assignee | ||
Comment 31•17 years ago
|
||
(In reply to comment #28) > JSVAL_IS_OBJECT(JSVAL_NULL) => true, always has. The JSID_* macros match, bug > for bug compatibility! Right, Java programming experience still bytes me with its (null instanceof Type) == false.
Comment 32•17 years ago
|
||
/cvsroot/mozilla/js/tests/e4x/Regress/regress-369740.js,v <-- regress-369740.js
Flags: in-testsuite+
Comment 33•17 years ago
|
||
verified fixed 1.9.0 20070226 windows/mac*/linux
Status: RESOLVED → VERIFIED
Updated•17 years ago
|
Summary: Using generic code for function:: → Using generic code for function:: (e4x/Regress/regress-369740.js)
You need to log in
before you can comment on or make changes to this bug.
Description
•