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)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Attached patch Implementation v0.1 (obsolete) — Splinter Review
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
Attached patch Implementation v1 (obsolete) — Splinter Review
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.
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)
Flags: blocking1.8.1.3?
Flags: blocking1.8.0.11?
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
(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.
Flags: blocking1.8.1.3?
Flags: blocking1.8.0.11?
Comment on attachment 254784 [details] [diff] [review]
Implementation v1

New patch coming based on nit?

/be
Attachment #254784 - Flags: review?(brendan)
Comment on attachment 254784 [details] [diff] [review]
Implementation v1

Sorry, meant to + with nit fix assumed.

/be
Attachment #254784 - Flags: review+
Blocks: 363530
Fixing bug 370372 is necessary to make sure that assignments xml_list.function::name works using generic XML support.
Depends on: 370372
Attached patch Implementation v2 (obsolete) — Splinter Review
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 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+
Patch to commit with comments added.
Attachment #255880 - Attachment is obsolete: true
Attachment #255934 - Flags: review+
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
Attached file stack trace
this checkin is causing a crash during the unit test run
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?
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
(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
(In reply to comment #19)
> Crazy: someone has 0 in a jsid and is calling InternNonXmlObjectId:

Yep. And jsid should never be 0, right?
(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
(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.
Attachment #255942 - Flags: review?(brendan)
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+
(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.  
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 ago17 years ago
Resolution: --- → FIXED
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
crash on startup (in -safe-mode/normal mode)
TB29556357G and others, identical
(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.
(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.  
Blocks: 370016
/cvsroot/mozilla/js/tests/e4x/Regress/regress-369740.js,v  <--  regress-369740.js
Flags: in-testsuite+
verified fixed 1.9.0 20070226 windows/mac*/linux
Status: RESOLVED → VERIFIED
Blocks: 370048
No longer blocks: 370048
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.

Attachment

General

Creator:
Created:
Updated:
Size: