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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[ondra zara]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Looks like a likely regression from bug 471214.

Comment 2

[Brendan Eich [:brendan]]

Anyone have a reduced testcase?

/be

Comment 3

[Jeff Walden [:Waldo]]

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.

Comment 4

[Jeff Walden [:Waldo]]

...and that barrier needs to munge the stack.  Or something.

Comment 5

[Brendan Eich [:brendan]]

Waldo: great, reduced test at URL, missed that -- thanks.

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

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: naive fix

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

Comment 7

[Brendan Eich [:brendan]]

Attachment: shell version of testcase

Comment 8

[Brendan Eich [:brendan]]

Attachment: naive fix, v2

Comment 9

[Brendan Eich [:brendan]]

v1 was a thinko, ignore.

/be

Comment 10

[Brendan Eich [:brendan]]

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

Comment 11

[Brendan Eich [:brendan]]

Attachment: shell testcase, reduced slightly

Comment 12

[Brendan Eich [:brendan]]

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

Comment 13

[Brendan Eich [:brendan]]

(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

Comment 14

[Brendan Eich [:brendan]]

Attachment: complete fix, with info-hiding and testsuite addition

Comment 15

[Jeff Walden [:Waldo]]

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.

Comment 16

[Brendan Eich [:brendan]]

Waldo, thanks for the great review and testcase -- comment 10 was right after all. Patch in a sec.

/be

Comment 17

[Brendan Eich [:brendan]]

Attachment: more complete fix, two testcases

Method extraction is rare, stacks are shallow. Stats in a bit, have to head home.

/be

Comment 18

[Brendan Eich [:brendan]]

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

Comment 19

[Brendan Eich [:brendan]]

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

Comment 20

[Brendan Eich [:brendan]]

Attachment: no stack walk in method read barrier, perf win on v8 (1-2%)

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

Comment 21

[Brendan Eich [:brendan]]

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

Comment 22

[Brendan Eich [:brendan]]

Attachment: 458156: no stack walk in method read barrier, perf win on v8 (1-2%)

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

/be

Comment 23

[Brendan Eich [:brendan]]

Attachment: no stack walk in method read barrier, perf win on v8 (1-2%), + comment fixes

Comment 24

[Brendan Eich [:brendan]]

/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 25

[Jeff Walden [:Waldo]]

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.

Comment 26

[Brendan Eich [:brendan]]

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

Comment 27

[Jeff Walden [:Waldo]]

(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.

Comment 28

[Brendan Eich [:brendan]]

Attachment: revised per review comments

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

Comment 29

[Jeff Walden [:Waldo]]

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...

Comment 30

[Brendan Eich [:brendan]]

(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

Comment 31

[Brendan Eich [:brendan]]

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

/be

Comment 32

[Brendan Eich [:brendan]]

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

Comment 33

[Brendan Eich [:brendan]]

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

Comment 34

[Jeff Walden [:Waldo]]

Later in the day tomorrow, yes -- after 17:00 or so.

Comment 35

[Brendan Eich [:brendan]]

Attachment: fixed not to leak #ifdef DEBUG

Comment 36

[Brendan Eich [:brendan]]

Attachment: fixed for xpconnect/crashtests/117307-1.html

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

Comment 37

[Brendan Eich [:brendan]]

Attachment: fix for if (!fun) case, take two

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

Comment 38

[Brendan Eich [:brendan]]

Tryserver victory ([e-mail] Sun Jul 25 14:12:10 2010 PDT), modulo failed builds due to insufficient disk space and some flappy-looking orangeness.

/be

Comment 39

[Jeff Walden [:Waldo]]

...plus removing the JS_FUNCTION_METERING #define and just using DEBUG directly.

Comment 40

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/80382d88b92c

/be

Comment 41

[Brendan Eich [:brendan]]

Looks like this landed in m-c:

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

/be

Comment 42

[ivan]

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.

Comment 43

[Brendan Eich [:brendan]]

(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

Comment 44

[ivan]

Thanks, filed bug 586482