Using generic code for function:: (e4x/Regress/regress-369740.js)

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
enhancement
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

11 years ago
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

11 years ago
Created attachment 254424 [details] [diff] [review]
Implementation v0.1

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

11 years ago
Created attachment 254784 [details] [diff] [review]
Implementation v1

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

11 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

11 years ago
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
(Assignee)

Comment 7

11 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

11 years ago
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+
(Assignee)

Updated

11 years ago
Blocks: 363530
(Assignee)

Comment 12

11 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

11 years ago
Created attachment 255880 [details] [diff] [review]
Implementation v2

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+
(Assignee)

Comment 15

11 years ago
Created attachment 255934 [details] [diff] [review]
Implementation v2b

Patch to commit with comments added.
Attachment #255880 - Attachment is obsolete: true
Attachment #255934 - Flags: review+
(Assignee)

Comment 16

11 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
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 17

11 years ago
Created attachment 255939 [details]
stack trace

this checkin is causing a crash during the unit test run

Updated

11 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

11 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?
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

11 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

11 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

11 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

11 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

11 years ago
Created attachment 255942 [details] [diff] [review]
Fix for regression
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+
(Assignee)

Comment 26

11 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

11 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
Last Resolved: 11 years ago11 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
(Assignee)

Comment 30

11 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

11 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.  
(Assignee)

Updated

11 years ago
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
(Assignee)

Updated

11 years ago
Blocks: 370048
(Assignee)

Updated

11 years ago
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.