__noSuchMethod__ no longer invoked for defined non-function properties

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Matthew Draper, Assigned: Matthew Draper)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

7 years ago
Created attachment 444249 [details] [diff] [review]
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.
(Assignee)

Comment 2

7 years ago
Created attachment 444301 [details] [diff] [review]
potential tests
(Assignee)

Updated

7 years ago
Attachment #444249 - Flags: review?(igor)
(Assignee)

Updated

7 years ago
Attachment #444301 - Flags: review?(igor)

Comment 3

7 years ago
Comment on attachment 444249 [details] [diff] [review]
naive patch

This should restore the old behavior.
Attachment #444249 - Flags: review?(igor) → review+
(Assignee)

Comment 4

7 years ago
Created attachment 491109 [details] [diff] [review]
updated patch
Attachment #444249 - Attachment is obsolete: true

Updated

7 years ago
Attachment #491109 - Flags: review+

Updated

7 years ago
Attachment #444301 - Flags: review?(igor) → review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
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.
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
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?
Assignee: general → matthew
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Version: unspecified → Trunk
tryservered in http://tbpl.mozilla.org/?tree=MozillaTry&rev=653854347eef
Created attachment 524801 [details] [diff] [review]
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.
Attachment #444301 - Attachment is obsolete: true
Attachment #491109 - Attachment is obsolete: true
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."
Attachment #524801 - Flags: review-
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. :)
(Assignee)

Comment 11

6 years ago
Created attachment 527616 [details] [diff] [review]
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 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.
Attachment #527616 - Flags: review?(philringnalda)

Comment 13

6 years ago
Comment on attachment 527616 [details] [diff] [review]
fully unrotted

This probably needs to be igor.
Attachment #527616 - Flags: review?(philringnalda) → review?(igor)

Updated

6 years ago
Attachment #527616 - Flags: review?(igor) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [c-n: tracemonkey]

Comment 14

6 years ago
http://hg.mozilla.org/tracemonkey/rev/72a5969b0c29
Keywords: checkin-needed
Whiteboard: [c-n: tracemonkey] → [fixed-in-tracemonkey]
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/72a5969b0c29
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.