Last Comment Bug 767667 - Differential Testing: Getting [object Object] vs null w/without -d
: Differential Testing: Getting [object Object] vs null w/without -d
: regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla16
Assigned To: Luke Wagner [:luke]
: Jason Orendorff [:jorendorff]
Depends on: 769987 771242
Blocks: jsfunfuzz 740446
  Show dependency treegraph
Reported: 2012-06-22 23:17 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2012-07-05 11:23 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix and tests (21.50 KB, patch)
2012-06-23 18:06 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2012-06-22 23:17:49 PDT
(function() {

Using js opt shell on m-i changeset cb74a377095a without -d shows:


but with -d shows:

[object Object]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   92686:fa24b215d49e
user:        Luke Wagner
date:        Mon Apr 02 08:58:30 2012 -0700
summary:     Bug 740446 - make 'arguments' more like a normal local binding (r=bhackett)
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-23 10:31:53 PDT
-d is correct, something's whacked.  Breaking in obj_getProto says that it's being called with Object.prototype as this, which is of course exceedingly strange.  Still looking.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-23 10:43:40 PDT
The problem's obvious enough when it's noted that we have a MagicValue(JS_OPTIMIZED_ARGUMENTS) on the stack.  Getting a property on that resolves through NormalArgumentsObject::optimizedGetElem, which doesn't handle __proto__ itself, but tails to proto->getGeneric(cx, root, vp) where proto is the pretend arguments object's [[Prototype]].

We could add a hack to NormalArgumentsObject::optimizedGetElem for this to fix the instant problem.  But we'd be back to square one for any other getter on Object.prototype, so that's not enough of a fix.  Hmm.  I wonder how other optimized-arguments engines do on this, both for the __proto__ and for the arbitrary-getter cases.
Comment 3 Luke Wagner [:luke] 2012-06-23 16:21:25 PDT
Well, drat, I don't see a quick fix.  This makes GETELEM on 'arguments' just as bad as f.apply(x, arguments) insofar as we can't tell anything based on simple SSA.

I think the fix is to do JSScript::applySpeculationFailed (which would have to be renamed to argumentsSpeculationFailed).  There is also the issue that there can be multiple outstanding MagicValue(JS_OPTIMIZED_ARGUMENTS) floating around on the stack when speculation fails.  Rather than fixing these up on the stack eagerly, we can just have the recipient expressions (there are only a few: apply, getelem, length) check script->needsArgsObj and, if true, fill in fp->hasArgsObj.  This tweak will allow us to remove current hacky restrictions on how the f.apply(arguments) optimization is applied.
Comment 4 Luke Wagner [:luke] 2012-06-23 18:06:40 PDT
Created attachment 636123 [details] [diff] [review]
fix and tests

With this patch, the meaning of script->needsArgsObj() == false (when script->argumentsHasLocalBinding == true) can now be described as: all potential consumers of 'arguments' are barriered.  The barrier both guards that an arguments object isn't needed (f.apply = js_fun_apply and arguments[i] accesses an actual arg) AND guards against lingering MagicValue(JS_OPTIMIZED_ARGUMENTS) after another barrier failed.  Since we know MagicValue(JS_OPTIMIZED_ARGUMENTS) necessarily refers to the initial value of 'arguments' (stored in fp->argsObj), it's always safe to swap it in.
Comment 5 Brian Hackett (:bhackett) 2012-06-28 07:04:50 PDT
Comment on attachment 636123 [details] [diff] [review]
fix and tests

Review of attachment 636123 [details] [diff] [review]:

::: js/src/jsfun.cpp
@@ +683,5 @@
> +         * Can't use cx->fp (since native may be called from C++), so don't call
> +         * IsOptimizedArguments. Luckily, GuardFunApplyArgumentsOptimization
> +         * already did, so we are safe.
> +         */
> +        JS_ASSERT(!cx->fp()->script()->needsArgsObj());

Confused here, the comment talks about not being able to use cx->fp() but the assertion does just that.  Maybe note that JS_OPTIMIZED_ARGUMENTS values will show up only when apply() is being directly called from a script.

Also, what is the correct canonical way of getting the current script now?  cx->fp()->script() doesn't work for JM inlining (which won't happen here) but I don't know if cx->currentScript is forward compatible with IM.
Comment 6 Luke Wagner [:luke] 2012-06-28 12:01:53 PDT
(In reply to Brian Hackett (:bhackett) from comment #5)
Heh, good point, assert removed.

> Also, what is the correct canonical way of getting the current script now? 
> cx->fp()->script() doesn't work for JM inlining (which won't happen here)
> but I don't know if cx->currentScript is forward compatible with IM.

cx->currentScript should be the way to go.  IM should have changed that function to use StackIter which will hand out things like JSScript w/o needing a StackFrame.
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-06-30 12:44:01 PDT

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