IonMonkey: Differential Testing: Getting different TypeErrors ((void 0) vs. -1) without/with ion.




JavaScript Engine
5 years ago
5 years ago


(Reporter: decoder, Assigned: djvj)


(Blocks: 2 bugs, {regression, testcase})

Other Branch
regression, testcase
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment)



5 years ago
The following testcase shows different behavior with options --ion -n -m --ion-eager vs. --no-ion on ionmonkey revision beabf74b228d:

function enterFunc (funcName) {
  if (!funcName.match(/\(\)$/)) {}
var match = "".toString().search(/ReferenceError/);
enterFunc ('test');
enterFunc ( expect = this, null);

$ debug64/js --ion -n -m --ion-eager test.js
test.js:2: TypeError: -1 is not a function

$ debug64/js --no-ion test.js
test.js:2: TypeError: (void 0) is not a function


5 years ago
Assignee: general → kvijayan

Comment 1

5 years ago
Created attachment 632303 [details] [diff] [review]

This bug is actually occurring in the interpreter.  Ion is doing the right thing, but the interpreter's __noSuchMethod__ handling is causing it to pick up an inconsistent value for for the CALLPROP lookup.  This is is because OnUnknownMethod unconditionally overwrites *vp if the result of a lookup for __noSuchMethod__ fails.

This is a minor patch to avoid clobbering *vp when the __noSuchMethod__ lookup returns undefined (primarily handling the case when __noSuchMethod__ is not bound, but this introduces a slight corner case where having __noSuchMethod__ explicitly bound to undefined will look the same as having __noSuchMethod__ not bound at all).
Attachment #632303 - Flags: review?(bhackett1024)
I'm a little confused by this patch.  Normally the initial value of *vp should already be undefined, so the '*vp = value' would be a no-op if the __noSuchMethod__ lookup gave undefined.  Where is the code that is calling into __noSuchMethod__ which has a non-undefined initial value for *vp and uses that value later?  I fear that there are other cases where property lookups overwrite vp and which will also be subtly broken, and that the right fix is to make sure everyone is using this API consistently.

Comment 3

5 years ago
As far as I can tell, every single use case for OnUnknownMethod is written such that a meaningful Value* is passed in to the call.  Every use case (two in jsinterpinlines.h, one by IonMonkey, one in the methodjit code) does an isPrimitive() check on the the value before calling OnUnknownMethod, so it seems pretty explicitly allow for non-undefined *vps, and OnUnknownMethod seems pretty explicitly to clobber it on entry.

In all cases this is done in response to a CALLPROP opcode either being interpreted or compiled.  The behaviour seems to be: look up the property for the CALLPROP regularly, if the result is a primitive, then OnUnknownMethod is called to do a lookup of __noSuchProperty__.  This happens to clobber the original lookup result.

I believe that this has not shown up in testing for the following reasons:
1. Usually when a primitive is returned for a CALLPROP, it's already undefined (because the CALLPROP was to a nonexistent binding), so the clobbering was idempotent.
2. Even if this DID happen (the property mapped to a primitive, and was then overwritten by undefined), then the decompiler-based construction of the error message in the normal mode (i.e. MORE_DETERMINISTIC=off) prints the _name_ of the lookup that fialed, not the _value_, hiding the error.

The "clean" solution would be to have OnUnknownMethod check for the existence of the __noSuchmethod__ property before clobbering its *vp, but that's a bit tricky since the lookup itself could have side effects, so we don't want to do it twice.

I think everybody is using the API consistently.  This is just a little bug that went unnoticed because of its relative obscurity.  The fix leaves a minor wart (it'll treat a binding of __noSuchMethod__ to undefined as if there existed no binding at all), but aside from that it should do the right thing with minimal tampering to the existing logic.
Comment on attachment 632303 [details] [diff] [review]

OK, thanks for the explanation.
Attachment #632303 - Flags: review?(bhackett1024) → review+

Comment 5

5 years ago
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.