Closed Bug 577648 Opened 12 years ago Closed 12 years ago

arguments.callee.caller does not work in FF 4 under certain circumstances

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: ondras, Assigned: brendan)

References

()

Details

(Keywords: regression, Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 11 obsolete files)

325 bytes, text/plain
Details
68.03 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1 (.NET CLR 3.5.30729)
Build Identifier: 

Under certain circumstances, arguments.callee.caller is not equal to the caller function. However, the ".toSource()" values are equal/

Reproducible: Always

Steps to Reproduce:
1. Open http://tmp.zarovi.cz/ff4.html in Firefox 4
2. Note that the first alert shows "false"
3. A possible workaround is included: after assigning arbitrary property to function, the problem disappears.
Actual Results:  
false, true

Expected Results:  
true, true
Regression range on m-c:  http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=38754465ffde&tochange=3079370d6597

Looks like a likely regression from bug 471214.
Blocks: 471214
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
Keywords: regression
Assignee: general → brendan
Status: NEW → ASSIGNED
OS: Windows XP → All
Priority: -- → P3
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3
Anyone have a reduced testcase?

/be
See URL?

Looks like there needs to be something approximating a method barrier introduced inside functions, to account for access to the .caller property which should end up getting the specific instantiation of the function rather than the generic one.
...and that barrier needs to munge the stack.  Or something.
Waldo: great, reduced test at URL, missed that -- thanks.

Yeah, this should be easy to fix. Working on it now.

/be
Attached patch naive fix (obsolete) — Splinter Review
This doesn't search suspended CallStacks or anything like that. Those stacks cannot be reached by .caller, AFAIK.

ES5 strict makes caller a poison pill, FWIW, too.

/be
Attachment #456637 - Flags: review?(jwalden+bmo)
Attached patch shell version of testcase (obsolete) — Splinter Review
Attached patch naive fix, v2 (obsolete) — Splinter Review
Attachment #456637 - Attachment is obsolete: true
Attachment #456650 - Flags: review?(jwalden+bmo)
Attachment #456637 - Flags: review?(jwalden+bmo)
v1 was a thinko, ignore.

/be
Comment on attachment 456650 [details] [diff] [review]
naive fix, v2

This is going to be hard. We need to identify which calls on the stack were through the Reference (in ES terms: base object, property name) that went through this method barrier.

/be
Attachment #456650 - Attachment is obsolete: true
Attachment #456650 - Flags: review?(jwalden+bmo)
Attachment #456638 - Attachment is obsolete: true
Comment on attachment 456650 [details] [diff] [review]
naive fix, v2

This is the fix after all. The method barrier optimizes all o.m = function (...){...}; and o = {m: function(...){...}}; cases to avoid cloning the function for identity/mutation semantics if the only uses of o.m are o.m(...) calls.

Thus the joined function cannot be in more than exactly one property. Therefore any calls on the stack referring to it as callee (argv[-2], got and set here so I did not obscure its location with fp->calleeObject() for the get -- could add an fp->setCalleeObject though) must be from calls to o.m, not any other property, or a clone created by extracting the method, e.g., f = o.m; f(...).

Whew! I will comment and consider setCalleeObject in a bit, wanted to revive this and get Waldo's review.

/be
Attachment #456650 - Attachment is obsolete: false
Attachment #456650 - Flags: review?(jwalden+bmo)
(In reply to comment #12)
> Thus the joined function cannot be in more than exactly one property. Therefore
> any calls on the stack referring to it as callee (argv[-2], got and set here so
> I did not obscure its location with fp->calleeObject() for the get -- could add
> an fp->setCalleeObject though) must be from calls to o.m, not any other
> property, or a clone created by extracting the method, e.g., f = o.m; f(...).

Forgot to add (or clarify -- pre-caffeine here, sorry!) that if an activation of the function was via f = o.m; f(...) then there'd already be a clone due to the method barrier being crossed at f = o.m, and no need or way to go through the barrier patched here again.

/be
Attachment #456650 - Attachment is obsolete: true
Attachment #456697 - Flags: review?(jwalden+bmo)
Attachment #456650 - Flags: review?(jwalden+bmo)
Comment on attachment 456697 [details] [diff] [review]
complete fix, with info-hiding and testsuite addition

Introducing a stackwalk into every method barrier can't be good.  But maybe it's not that bad.  Color me skeptical; I'd want perf numbers demonstrating it's not bad before granting review, but...

I alluded to the below behavior (seen with your patch applied) in comment 3, it seems overly opaquely:

[jwalden@find-waldo-now tests]$ ../dbg/js -j
js> var o = { f: function() { return o.g(); }, g: function() { return arguments.callee.caller; } };
js> var c = o.f();
js> c
(function () {return o.g();})
js> o.f
(function () {return o.g();})
js> c === o.f
false

Your patch appears to fix the barrier occurring via generic property lookup, but it doesn't fix it occurring through fun.caller, which the above example triggers.
Attachment #456697 - Flags: review?(jwalden+bmo) → review-
Waldo, thanks for the great review and testcase -- comment 10 was right after all. Patch in a sec.

/be
Attached patch more complete fix, two testcases (obsolete) — Splinter Review
Method extraction is rare, stacks are shallow. Stats in a bit, have to head home.

/be
Attachment #456697 - Attachment is obsolete: true
Attachment #457219 - Flags: review?(jwalden+bmo)
No method read barrier crossings (method extraction, get-not-call) in SunSpider.

v8 has 175 hits (testing the old copy of v8 benchmarks under js/src -- if this copy is as old and musty as it seems, I'll redo tomorrow on the latest v8 suite).

/be
Target Milestone: mozilla1.9.3 → mozilla2.0
Update: closing on perf wins to balance the stack walk. It indeed hurts v8, which extracts methods in several ways, one hard to optimize away:

Object.extend = function(destination, source) {
  for (var property in source) {
    destination[property] = source[property];
  }
  return destination;
}

Without a way to chain joined function object references together for patching when unjoining later due to mutation or extraction, the read of a method via source[property] must clone (unjoin).

/be
This was a wild ride, including the mq rebase on top of the fatvals landing. More when I've slept, it passes trace-tests and jsreftests.

/be
Attachment #457219 - Attachment is obsolete: true
Attachment #458156 - Flags: review?(jwalden+bmo)
Attachment #457219 - Flags: review?(jwalden+bmo)
The perf win is due to recognizing more cases where a null closure lambda is used in a way that can't be observed to differ if the implementation joins function objects (as ES3 tried to allow in observable situations too; ES5 got rid of this mistake), namely as a parameter to the original String.prototype.replace native, or similarly for Array.prototype.sort's comparator funarg, or for the "module pattern" lambda-immediately-applied case.

/be
Same as last patch but with better parallel structure between interp and tracer, and a warning comment about keeping it that way. Zzzz...

/be
Attachment #458156 - Attachment is obsolete: true
Attachment #458159 - Flags: review?(jwalden+bmo)
Attachment #458156 - Flags: review?(jwalden+bmo)
Attachment #458159 - Attachment is patch: true
Attachment #458159 - Attachment mime type: application/octet-stream → text/plain
Attachment #458159 - Attachment is obsolete: true
Attachment #458220 - Flags: review?(jwalden+bmo)
Attachment #458159 - Flags: review?(jwalden+bmo)
/tmp/function.stats file for SunSpider, run interp-only to get complete states (we don't generate trace-JITted code to update rt->functionMeter):

function meter (t/string-validate-input.js):
allfun              232
heavy               9
nofreeupvar         56
onlyfreevar         127
display             0
flat                5
setupvar            0
badfunarg           35
joinedsetmethod     28
joinedinitmethod    0
joinedreplace       170
joinedsort          1
joinedmodulepat     4
mreadbarrier        0
mwritebarrier       0
mwslotbarrier       5
unjoined            12

method read barrier count map:

unjoined function count map:
         2 t/string-unpack-code.js:66
         2 t/string-unpack-code.js:51
         2 t/string-unpack-code.js:30
         2 t/string-unpack-code.js:16
         1 t/string-tagcloud.js:40
         1 t/string-tagcloud.js:229
         1 t/date-format-xparb.js:45
         1 t/date-format-xparb.js:45

I manually sort -nr'ed the bottom indented lines of hashmap dump. This is a win over the stats without the patch fully enabled, particularly for string.replace calls in crypto-aes.js:

12,14c12,14
< joinedreplace       0
< joinedsort          0
< joinedmodulepat     0
---
> joinedreplace       170
> joinedsort          1
> joinedmodulepat     4
18c18
< unjoined            187
---
> unjoined            12
23,25d22
<         85 t/crypto-aes.js:283
<         85 t/crypto-aes.js:279
<          4 t/access-nbody.js:157
31d27
<          1 t/string-tagcloud.js:235

Still, SS timing is so noisy even on my MBP with SSD and all apps shut down that I see only a marginal win. More like 1.5% on v8, details next.

/be
Comment on attachment 458220 [details] [diff] [review]
no stack walk in method read barrier, perf win on v8 (1-2%), + comment fixes

>diff --git a/js/src/jsarray.h b/js/src/jsarray.h

>+/* JSFastNative entry point for Array.prototype.sort. */
>+extern JSBool
>+js_array_sort(JSContext *cx, uintN argc, js::Value *vp);

Is the comment necessary?  Seems obvious enough to me, accords with normal tradition for other extern'd built-in function native-code definitions, seems unnecessary.

Minor tweak: this should be js::array_sort.


>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp

>+                    } else if (sprop->hasSlot()) {
>+                        Value v = thisp->getSlot(sprop->slot);
>+                        JSObject *clone;
>+
>+                        if (IsFunctionObject(v, &clone) && GET_FUNCTION_PRIVATE(cx, clone) == fun) {
>+                            JS_ASSERT(clone != funobj);
>+                            *vp = v;
>+                            setCalleeObject(*clone);
>+                        }
>+                    }

This code looks mostly okay, but this bit raises my hackles.  In particular: is it possible to change the value stored in the slot to *another* clone of the primordial function here?  Initially this should be fine, because it'll look up the original clone.  But if I get ahold of a completely different clone, and I overwrite this value somehow, I will get that different clone back -- not the one I really should get (because that different clone, and the real one I should get, will both have the same private JSFunction*).  I tried for awhile to make a testcase to cause this to happen, but I wasn't successful.  Maybe it's not possible, but it is not immediately obvious to me that this is the case.  Perhaps you (or jorendorff, to whom I've forwarded a r? to get a second mind on this) can explain why my worries are unfounded?


>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp

>+                    if (op2 == JSOP_CALL) {
>+                        /*
>+                         * Array.prototype.sort and String.prototype.replace are
>+                         * optimized as if they are special form. We know that they
>+                         * won't leak the joined function object in obj, therefore
>+                         * we don't need to clone that compiler- created function
>+                         * object for identity/mutation reasons.
>+                         */

Sigh, a characteristically opaque optimization that nevertheless is claimed to improve performance (haven't tested, quite believable that it does).

I'm not sure whether I believe the optimization is worth the complexity.  I'm sort of leaning toward resigned acceptance, given that it does show overall gains, and it's not incredibly inscrutable when you think about it.  But it does require that double-(triple?)-take, and it makes our property access strategy even more special-cased.  If jorendorff can live with it, I guess that'll settle it from my point of view.

I assume profiling or somesuch pointed out these two optimization opportunities -- have you considered extending this to the array extras as well?  filter/forEach/every/some/map/reduce/reduceRight could all benefit from this optimization as well, although to be sure they're -- currently -- less used on the web.


>+                        int iargc = GET_ARGC(pc2);
>+
>+                        /*
>+                         * Note that we have not pushed obj as the final argument
>+                         * yet, so regs.sp[-iargc - 1], not regs.sp[-iargc - 2], is
>+                         * the callee for this JSOP_CALL.
>+                         */
>+                        JSFunction *calleeFun =
>+                            GET_FUNCTION_PRIVATE(cx, &regs.sp[-iargc - 1].toObject());

I would prefer [+1 - iargc - 2] to make the necessarily-different adjustment more understandable (perhaps without the +, I'm uncertain how much it improves clarity).


>diff --git a/js/src/jsobj.h b/js/src/jsobj.h

>+inline bool JSObject::isObject() const { return getClass() == &js_ObjectClass; }

I could go for isPlainObject, perhaps, here.  The current name just makes me burst out laughing -- but I can't think of a sufficiently better one to strongly request it.


>diff --git a/js/src/jsobjinlines.h b/js/src/jsobjinlines.h

> inline bool
>+JSObject::canHaveMethodBarrier() const
>+{
>+    return isObject() || isFunction() || isPrimitive() || isDate();

isRegExp() too, or no?


r=me as much as I can, although I am not completely convinced on the clone-confusion point -- hopefully others can correct or elaborate upon the point.
Attachment #458220 - Flags: review?(jwalden+bmo)
Attachment #458220 - Flags: review?(jorendorff)
Attachment #458220 - Flags: review+
Good point on js::array_sort (same for str_replace), minimizes the diff. I got rid of that useless one line comment (I think I was gonna say why array_sort is exported, but that's for jsarray.h to comment on).

Re: [+1 - iargc - 2], I tried that and [(-iargc - 2) + 1] but neither was an improvement, since the reader has to do the CSE, or else grok the -argc - 2 based on (JSOP_CALL's case):

    vp = regs.sp - (argc + 2);

This suggests 1 - (argc + 2) vs. 0 - (argc + 2), which I've used elsewhere. But again readers who do CSE in their heads will have to waste time doing that and checking number-line relations. Still, I'd rather repeat (argc + 2) if it helps. What do you think?

Great catch on the "wrong clone" issue (you can't trust clones ;-). I will work on that and put up a better patch. Thanks,

/be
(In reply to comment #26)
> This suggests 1 - (argc + 2) vs. 0 - (argc + 2), which I've used elsewhere.

1 - (argc + 2) seems better, yes -- I like that the most of the mooted possibilities.
blocking2.0: ? → betaN+
Attached patch revised per review comments (obsolete) — Splinter Review
I didn't optimize null closure lambdas passed to the Array extras; they aren't used in benchmarketing yet and this bug's patch is big enough. I'll file another bug on addressing them either specifically or (ideally) generally.

See the revised testcase for case analysis and coverage. Thanks again for the great review. If you could take one more look, I bet you'd be convinced without needing to summon jorendorff.

/be
Attachment #458220 - Attachment is obsolete: true
Attachment #458922 - Flags: review?(jwalden+bmo)
Attachment #458220 - Flags: review?(jorendorff)
Comment on attachment 458922 [details] [diff] [review]
revised per review comments

Yeah, I think I'm good with this now.  Maybe in several years we might be able to remove fun.caller and a lot of this convolution...
Attachment #458922 - Flags: review?(jwalden+bmo) → review+
(In reply to comment #29)
> Comment on attachment 458922 [details] [diff] [review]
> revised per review comments
> 
> Yeah, I think I'm good with this now.  Maybe in several years we might be able
> to remove fun.caller and a lot of this convolution...

That would be great -- ES5 strict will help to the extent that it's used.

/be
http://hg.mozilla.org/tracemonkey/rev/ff7262da5290

/be
Whiteboard: fixed-in-tracemonkey
Out again:

http://hg.mozilla.org/tracemonkey/rev/c421366b52a5

Leak-o-rama, I suspect due to JSSLOT_FUN_METHOD_OBJ entraining the object owning the joined function object.

/be
Whiteboard: fixed-in-tracemonkey
Of course the patch breaks debug leak tests: the JS_FUNCTION_METERING stuff marks functions entered into methodReadBarrierCountMap and unjoinedFunctionCountMap. The envar testing does not prevent this, it controls only whether we dump these maps. Waldo, I will tryserver a fix and if it is good, put it up for review. You around this weekend?

/be
Later in the day tomorrow, yes -- after 17:00 or so.
Attached patch fixed not to leak #ifdef DEBUG (obsolete) — Splinter Review
Attachment #458922 - Attachment is obsolete: true
Attachment #460127 - Flags: review?(jwalden+bmo)
The if (!fun) early return in JSStackFrame::getValidCalleeObject was badly translated from the FUN_CALLER: case in fun_getProperty.

I did not rename JSStackFrame::getValidCalleeObject to getValidCalleeValue, which has one too many "Val*" words -- instead I commented both jsinterp.h and jsdbgapi.h.

If you have a better name, lay it on me. Thanks,

/be
Attachment #460127 - Attachment is obsolete: true
Attachment #460162 - Flags: review?(jwalden+bmo)
Attachment #460127 - Flags: review?(jwalden+bmo)
Pre-caffeine me thought there was an argv test in calleeValue, as there is in callee. These JSStackFrame methods need at least naming help (use of maybe for non-asserting null-checking methods, e.g. callee -> maybeCallee, could help) but I will leave that for another bug.

/be
Attachment #460162 - Attachment is obsolete: true
Attachment #460169 - Flags: review?(jwalden+bmo)
Attachment #460162 - Flags: review?(jwalden+bmo)
Tryserver victory (brendan@mozilla.com Sun Jul 25 14:12:10 2010 PDT), modulo failed builds due to insufficient disk space and some flappy-looking orangeness.

/be
Attachment #460169 - Flags: review?(jwalden+bmo) → review+
...plus removing the JS_FUNCTION_METERING #define and just using DEBUG directly.
http://hg.mozilla.org/tracemonkey/rev/80382d88b92c

/be
Whiteboard: fixed-in-tracemonkey
Looks like this landed in m-c:

http://hg.mozilla.org/mozilla-central/log?rev=577648

/be
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I think this is still buggy. See:

http://ludios.net/browser_bugs/ff4_arguments_callee_bug.html

I see `false' in the nightly I just tried, Mozilla/5.0 (Windows NT 5.1; rv:2.0b4pre) Gecko/20100810 Minefield/4.0b4pre

It's `true` everywhere else. I hope I'm not misinterpreting things.
(In reply to comment #42)
> I think this is still buggy. See:
> 
> http://ludios.net/browser_bugs/ff4_arguments_callee_bug.html
> 
> I see `false' in the nightly I just tried, Mozilla/5.0 (Windows NT 5.1;
> rv:2.0b4pre) Gecko/20100810 Minefield/4.0b4pre
> 
> It's `true` everywhere else. I hope I'm not misinterpreting things.

You are not -- thanks. Please file a new bug, one patch per bug. Your test shows the prototype chain in action, where the method barrier is on the proto of the |this| object. Should be easy to fix, I'll take the bug.

I don't believe bug 586453 (also filed today) is related, as I can't reduce it to a shell testcase and it seems not to involve any joined function objects (from "null closure" lambda expressions assigned to properties in assignment statements or object initialisers).

/be
Thanks, filed bug 586482
Depends on: 586482
Depends on: 623430
You need to log in before you can comment on or make changes to this bug.