Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 763481 - IonMonkey: Differential Testing: Getting different TypeErrors ((void 0) vs. -1) without/with ion.
: IonMonkey: Differential Testing: Getting different TypeErrors ((void 0) vs. -...
: regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- critical (vote)
: ---
Assigned To: Kannan Vijayan [:djvj]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: langfuzz IonFuzz
  Show dependency treegraph
Reported: 2012-06-11 07:07 PDT by Christian Holler (:decoder)
Modified: 2012-06-26 10:19 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (864 bytes, patch)
2012-06-12 10:03 PDT, Kannan Vijayan [:djvj]
bhackett1024: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-06-11 07:07:16 PDT
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
Comment 1 Kannan Vijayan [:djvj] 2012-06-12 10:03:22 PDT
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).
Comment 2 Brian Hackett (:bhackett) 2012-06-14 15:00:15 PDT
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 Kannan Vijayan [:djvj] 2012-06-19 19:26:58 PDT
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 4 Brian Hackett (:bhackett) 2012-06-23 07:04:34 PDT
Comment on attachment 632303 [details] [diff] [review]

OK, thanks for the explanation.
Comment 5 Kannan Vijayan [:djvj] 2012-06-26 10:19:37 PDT

Note You need to log in before you can comment on or make changes to this bug.