Bob Clary [:bc] (inactive) Reporter
	Description • 15 years ago

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.

	Bob Clary [:bc] (inactive) Reporter
	Comment 1 • 15 years ago

oops, missed part of the error message:

compile actual reason: Expected value 'No Error', Actual value 'SyntaxError: missing ; before statement'

	Brendan Eich [:brendan] Assignee
	Comment 2 • 15 years ago

Wish I had the test before checkin...

/be

	Brendan Eich [:brendan] Assignee
	Comment 3 • 15 years ago

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

	Igor Bukanov
	Comment 4 • 15 years ago

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

	Brendan Eich [:brendan] Assignee
	Comment 5 • 15 years ago

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

	Igor Bukanov
	Comment 6 • 15 years ago

(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.

	Jeff Walden [:Waldo]
	Comment 7 • 15 years ago

(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*

	Brendan Eich [:brendan] Assignee
	Comment 8 • 15 years ago

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

	Brendan Eich [:brendan] Assignee
	Comment 9 • 15 years ago

Harmony proxies: bug 546590.

/be

	Brendan Eich [:brendan] Assignee
	Comment 10 • 15 years ago

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

	Jeff Walden [:Waldo]
	Comment 11 • 15 years ago

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.

	Brendan Eich [:brendan] Assignee
	Comment 12 • 15 years ago

(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

	Jesse Ruderman
	Comment 13 • 14 years ago

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

http://hg.mozilla.org/tracemonkey/rev/b79b34cbd543