Closed Bug 520778 Opened 13 years ago Closed 12 years ago

__noSuchMethod__ is no longer called for |new|

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: bc, Assigned: brendan)

References

Details

(Keywords: regression, testcase)

tracemonkey js1_5/extensions/regress-429739.js linux at least.

Do not assert: OBJ_GET_CLASS(cx, obj)->flags & JSCLASS_HAS_PRIVATE reason: Expected value ' function anonymous ( y ) { } ', Actual value ' TypeError : o . y is not a constructor '

regression changeset: 33392:e6f2cbdfce26 user: Brendan Eich <brendan@mozilla.org> date: Mon Oct 05 16:55:21 2009 -0700 summary: Fix constructor method (foo.bar/foo[baz] initialized from a lambda) invocation to go through the method read barrier (518103, r=jorendorff).

private since bug 518103 is still private.
Flags: in-testsuite+
oops, missed part of the error message:

compile actual reason: Expected value 'No Error', Actual value 'SyntaxError: missing ; before statement'
Wish I had the test before checkin...

/be
Assignee: general → brendan
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P1
Hardware: x86 → All
This is a __noSuchMethod__ regression from the patch for bug 518103. It is not a security-sensitive bug, so it would be best if this bug could be opened up and a testcase that does not trigger bug 429739 were attached for this bug only.

The bug is not a surprise. The patch for bug 518103 despecialized from JOF_CALLOP flavored ops (JSOP_CALLPROP, e.g.) to "get + null" to push the constructor and a placeholder for its |this| parameter (the newborn instance created by the VM). But __noSuchMethod__ support applies only for JSOP_CALLPROP and JSOP_CALLELEM.

Igor, any thoughts? I'm half-inclined to WONTFIX. IIRC the Tibet guys do not use __noSuchMethod__ for constructors, only for methods. I should check but not via cc'ing them while this bug is restricted.

/be
(In reply to comment #3)
> Igor, any thoughts? I'm half-inclined to WONTFIX.

IMO we should not just randomly disable a particular SM extension if it hurts performance. Why not then just disable E4X? Surely it would improve some benchmarks. Also what if we did have that test case in test suite?

As regarding the fix for this bug what about keeping using CALLOPS for the new and just wrap the compiler function in JSOP_NEW?
The fix that regressed this was about correctness, but it was a followup to a perf bug indeed.

We can't wrap in JSOP_NEW because the property slot won't be updated -- we'll wrap on each new call and lose the wrapper. We have to unjoin on get. I thought about complicating the CALLOPs (either by parameterizing their code with a new immediate, or splittin them into CALLOPs and NEWOPs) but that's overkill.

Could we make __noSuchMethod__ work for JSOP_GETPROP and JSOP_GETELEM too? Isn't that an open bug that you can't extract a no-such-method and call it later?

/be
(In reply to comment #5)
> The fix that regressed this was about correctness, but it was a followup to a
> perf bug indeed.
> 
> We can't wrap in JSOP_NEW because the property slot won't be updated -- we'll
> wrap on each new call and lose the wrapper. 

Right - wrapping in JSOP_NEW is too late.

> We have to unjoin on get. I thought
> about complicating the CALLOPs (either by parameterizing their code with a new
> immediate, or splittin them into CALLOPs and NEWOPs) but that's overkill.
> 
> Could we make __noSuchMethod__ work for JSOP_GETPROP and JSOP_GETELEM too?
> Isn't that an open bug that you can't extract a no-such-method and call it
> later?

I could not find one. IIRC once I thought about filing one about extracting the method using function::, but I guess I have not done that. Still extending __noSuchMethod__ so it would work for JSOP_GETELEM rather than keeping this bug open may even make things worse  for __noSuchMethod__ users. That method will be called when it was not before potentially breaking things.

So lets make this bug public with an attached test case and fixed title like "__noSuchMethod__ is no longer called for new". If embbeders would complain, we can evaluate options to address it via one of the above suggestions.
Group: core-security
Summary: js1_5/extensions/regress-429739.js FAIL → __noSuchMethod__ is no longer called for |new|
(In reply to comment #4)
> IMO we should not just randomly disable a particular SM extension if it hurts
> performance. Why not then just disable E4X? Surely it would improve some
> benchmarks. Also what if we did have that test case in test suite?

*bitingmytonguebitingmytonguebitingmytongue*
The future is

http://wiki.ecmascript.org/doku.php?id=strawman:proxies

instead of __noSuchMethod__ (this needs a bug, mrbkap or gal will file). We can remove __noSuchMethod__ once people have had a release or two with proxies supported, standardized by the second release, and we have at least the TIBET guys on board.

As for E4X, Igor is on the hook to desugar its syntax to builtin calls (see bug 441416). These will not perform as well but who cares?

AFAIK, E4X doesn't slow down fast paths. Anyone have evidence to the contrary?

/be
Harmony proxies: bug 546590.

/be
The future is 546590. I'm not going to fix this, and I don't think anyone else should work on it. But I do think the proxies proposal should add a methodMissing (like invoke in the proposal, but evaluated in order as __noSuchMethod__ is) handler trap.

/be
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Depends on: harmony:proxies
http://wiki.ecmascript.org/doku.php?id=harmony:proxies

claims to already support __noSuchMethod__-alike functionality on a per-proxy basis; I haven't read closely enough to evaluate the claim.
(In reply to comment #11)
> http://wiki.ecmascript.org/doku.php?id=harmony:proxies
> 
> claims to already support __noSuchMethod__-alike functionality on a per-proxy
> basis; I haven't read closely enough to evaluate the claim.

That's what comment 10 alludes to -- it does not support a convenience out of the box, but you can "build it yourself". Users shouldn't have to reinvent stdio for C (I lived through that in the '80s), or __noSuchMethod__ on top of proxies. But this is for another bug.

/be
Rather than leave the test disabled, I updated it with a new expect string.

http://hg.mozilla.org/tracemonkey/rev/b79b34cbd543
Depends on: 566818
No longer depends on: 566818
You need to log in before you can comment on or make changes to this bug.