Closed
Bug 767667
Opened 11 years ago
Closed 11 years ago
Differential Testing: Getting [object Object] vs null w/without -d
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: gkw, Assigned: luke)
References
Details
(Keywords: regression, testcase, Whiteboard: [js:t])
Attachments
(1 file)
21.50 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
(function() { print(arguments["__proto__"]) })() Using js opt shell on m-i changeset cb74a377095a without -d shows: null 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•11 years ago
|
||
-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.
Assignee: general → jwalden+bmo
OS: Windows 7 → All
Hardware: x86 → All
Comment 2•11 years ago
|
||
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.
Updated•11 years ago
|
Whiteboard: js-triage-needed → js-triage-done
![]() |
Assignee | |
Comment 3•11 years ago
|
||
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.
Assignee: jwalden+bmo → luke
![]() |
Assignee | |
Comment 4•11 years ago
|
||
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.
Attachment #636123 -
Flags: review?(bhackett1024)
Updated•11 years ago
|
Whiteboard: js-triage-done → [js:t]
Comment 5•11 years ago
|
||
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.
Attachment #636123 -
Flags: review?(bhackett1024) → review+
![]() |
Assignee | |
Comment 6•11 years ago
|
||
(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.
![]() |
Assignee | |
Comment 7•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ac6ac2e618
Target Milestone: --- → mozilla16
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d4ac6ac2e618
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•