564577 - __noSuchMethod__ no longer invoked for defined non-function properties

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Matthew Draper]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-au) AppleWebKit/531.2+ (KHTML, like Gecko) Safari/531.2+ Debian/squeeze/sid () Epiphany/2.30.2
Build Identifier: ca591da19a26

Trying to call a property that was set to a non-function value used to invoke __noSuchMethod__, but now it just throws "is not a function". Brendan's comments in bug 224956, comment 15 suggest the previous behaviour was a conscious choice.

Reproducible: Always

Steps to Reproduce:
js> var o = { a: 'xx', __noSuchMethod__: function() { return 'nsm'; } };
js> o.a()
Actual Results:  
typein:2: TypeError: o.a is not a function

Expected Results:  
nsm

Bisecting suggests the change occurred in bug 420399

Comment 1

[Matthew Draper]

Attachment: naive patch

Seems to restore the previous behaviour, in my testing: __noSuchMethod__ is called if the property is undefined, null, a number, or a string -- but not if it's an object or a regexp. Or a function. :)

I actually wonder whether js_OnUnknownMethod should get a chance in js_Call, so any object without a JSClass.call gets treated the same way as a primitive... but that's really a separate matter.

Comment 2

[Matthew Draper]

Attachment: potential tests

Comment 3

[Igor Bukanov]

Comment on attachment 444249 [details] [diff] [review]
naive patch

This should restore the old behavior.

Comment 4

[Matthew Draper]

Attachment: updated patch

Comment 5

[Jonathan Watt [:jwatt]]

I'm searching for patches with checkin-needed to check in at the same time as my patches. This bug should not have that keyword set since it doesn't have approval and isn't a blocker, so the keyword is just creating noise and wasting other people's time. Please remove it and - assuming it's there to keep this bug on someone's radar - use your own whiteboard flags for that purpose.

Comment 6

[Phil Ringnalda (:philor)]

Brendan, Igor: any reason why this patch (which should have been going into TM anyway, and thus shouldn't have gotten yelled at for not having the approval it would have only needed if it was headed into mozilla-central) shouldn't land?

Comment 7

[Phil Ringnalda (:philor)]

tryservered in http://tbpl.mozilla.org/?tree=MozillaTry&rev=653854347eef

Comment 8

[Phil Ringnalda (:philor)]

Attachment: unrotted, folded, and exported

Just a tiny bit of bitrot in the context, and I folded them into a single patch so we're less likely to fail to push one, and hg exported them so we're less likely to wind up gumming up your name/email/commit message.

Comment 9

[Phil Ringnalda (:philor)]

Comment on attachment 524801 [details] [diff] [review]
unrotted, folded, and exported

Could have answered my own question, just by waiting for tryserver results: according to the jsreftests, "o.fff is not a function item 6."

Comment 10

[Jonathan Watt [:jwatt]]

Matthew, someone just pointed out to me that comment 5 came across as sounding curt and a bit rude to them, and in retrospect I have to agree. More to the point I failed to notice that you're a new contributor and shouldn't have been expected to know the subtleties of checkin-needed in use at the time. Apologies for the flyby comment. Had I realized you were just getting started I'd have made a much more friendly and helpful comment. I hope my carelessness doesn't put you off, since your involvement in Mozilla is certainly very welcome. :)

Comment 11

[Matthew Draper]

Attachment: fully unrotted

Addressed new instances of HAS_NO_SUCH_METHOD in methodjit/, and updated test to reflect regular expressions not being callable (#582717; the error mentioned in comment 9).

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 527616 [details] [diff] [review]
fully unrotted

I'm guessing this needs review so as not to fall off the radar. Please cancel the review request if I'm wrong.

Comment 13

[Paul Biggar]

Comment on attachment 527616 [details] [diff] [review]
fully unrotted

This probably needs to be igor.

Comment 14

[Paul Biggar]

http://hg.mozilla.org/tracemonkey/rev/72a5969b0c29

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/72a5969b0c29