395993 - ComputeGlobalThis access checks should be removed

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

The solution is cross-origin wrappers (XOWs), but we may need to teach XOWs about allAccess/noAccess properties too. This bug will need help in xpconnect (probably) for wrapper automation at every edge in the JS object graph that crosses a trust domain. It should be possible to assert in ComputeGlobalThis that we have access, in DEBUG builds, and do no checks in release builds.

Similar bug(s) should be filed on xpconnect, we think. It pessimistically calls the caps security manager through its interface indirection on every method call, etc. I'll let mrbkap file that bug.

A metabug couldn't hurt, but I'll file this first.

/be

Comment 1

[Mike Schroepfer]

I'm assuming you meant this for 1.9 so nominating.  

Comment 2

[Brendan Eich [:brendan]]

Attachment: DEBUG build insists on access, release just walks up parent

Find me on IRC if this is going to open a hole even with XOWs in 1.9 (if true, we must lack automation to make XOWs where needed, and we would need a blocking bug).

/be

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking final release, but not blocking M9.

Comment 4

[Mike Schroepfer]

Setting TM to M10 as this would be great for b2

Comment 5

[Brendan Eich [:brendan]]

jst, mrbkap: one of you should probably take this from my list and bring it home (along with other wrapper-only security check work).

/be

Comment 6

[Mike Schroepfer]

Setting to P1 to make sure this gets looked at for b1

Comment 7

[Brendan Eich [:brendan]]

Thanks to jst for taking this.

/be

Comment 8

[Johnny Stenback (:jst)]

So brendan, this patch pretty consistently shows a *slowdown* in a testcase that runs an empty loop in the scope of a function, about 10%! It speeds up the same loop running in the global scope, by about 10% also. This slowing down the loop in a function scope makes no sense to me. That's not expected from this is it?

Comment 9

[Brendan Eich [:brendan]]

That makes no sense -- can you profile (shark on Mac?) with and without? What happens if you make the function a stub that just returns cx->globalObject (for the shell, this is fine)?

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

jst, would you attach the testcase you're using?

Comment 11

[Mike Schroepfer]

See bug 375225 for some testcases (not what JST is using) and shark profiles.

Comment 12

[Johnny Stenback (:jst)]

Blake, I was using attachment 259731 [details], from bug 375470. But I don't really believe in the numbers I was seeing earlier when I wrote comment 8, but verification of that would be nice of course.

Comment 13

[Mike Schroepfer]

So is this really going to make M10?  We should either close it out or set TM to M11

Comment 14

[Johnny Stenback (:jst)]

Moving out to M11.

Comment 15

[Robert Sayre]

SunSpider showed a 3-5% improvement with this patch. 

Comment 16

[Mike Schroepfer]

JST/Bkap what's next with this?

Comment 17

[Robert Sayre]

Attachment: updated to Trunk

Comment 18

[Robert Sayre]

(In reply to comment #15)
> SunSpider showed a 3-5% improvement with this patch. 
> 

More improvement in-browser. > 10%.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Halfway step

I offered to take this from jst.

Getting rid of the access checks entirely via wrapping harder is a nobel goal, but we're not there yet and want some benefit from this bug. This patch is an optimization that keeps the access checks, but only does them once. All of the other times through the loop, we're guaranteed to get the same answer.

Comment 20

[Brendan Eich [:brendan]]

Nobel Prize? Oh, "noble" :-P. I think we should fix this for real. What remains to do with wrappers?

In the mean time, please burn a JSStackFrame flag bit to record that we've checked access along the scope chain once, so don't need to check again. Scope chains are immutable so we should not have to do the check more than once per activation.

/be

Comment 21

[Brendan Eich [:brendan]]

Also, this is performance critical code -- is JS_GetGlobalForObject going to spill stuff and otherwise cost noticeably more than the inline loop we had before?

/be

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #20)
> Nobel Prize? Oh, "noble" :-P. I think we should fix this for real. What remains
> to do with wrappers?

Currently it's possible for extensions to stick privileged functions in places where content can reach them. At that point content would be able to use the lack of security checks here to reach out of the sandbox. We need to ensure that in places where content wants to do this that we are able to wrap the functions (and the return values from those functions!).

> In the mean time, please burn a JSStackFrame flag bit to record that we've
> checked access along the scope chain once, so don't need to check again. Scope
> chains are immutable so we should not have to do the check more than once per
> activation.

But the bit would have be be per callable object. Although the parent links are immutable, there might be multiple lexical scopes per stack frame... or am I misunderstanding you?

Comment 23

[Brendan Eich [:brendan]]

The fast case, the one we don't want useless access checks for, involves code like this (from a benchmark, from memory):

  function foo() {
    ...
    var sin = Math.sin;
    for (big loop head) {
      ... sin(x) ...
  }

Since Math.sin is not this-bound, extracting and calling via local variable must use null as the nominal this, which is replaced per ECMA-262 with the ancestral, global object for the Math.sin function object.

It should not be necessary to do any security checks when walking up the scope chain from Math.sin to Math to its parent, the global object. If we can't tell this _a priori_ (we ought to be able to), then at we should try to cache security judgments that can't vary.

That was what I was getting at with the flag idea. In this case the flag would be a set of flags, one per called object, set in the current activation. We wouldn't want a flag per called object, that would be a pigeon hole that other control flows (not to say threads; just sub-flows) would race to test and set.

And for called objects such as sin (Math.sin), which are (grand-)parented by the same window that parents foo, could we have a faster special case? I.e. inline the parent chain loop in ComputeGlobalThis, but test in the loop body when you see a parent (could be the global, as in this example) that is the parent of the current frame's callee (argv[-2]). If you see that parent, no need for an access check -- if you don't, then do the access check after the parent-chain loop in ComputeGlobalThis.

/be

Comment 24

[Mike Schroepfer]

Bkap - we gonna get an updated patch here?

Comment 28

[Robert Sayre]

Hey, we need to unwedge this, pronto. 

Let's not let these reviews sit passive. r- each other's, if you must. :)

Comment 23 has a long suggestion. Is the proposal viable?

Comment 29

[Brendan Eich [:brendan]]

Blake, mind if I have a go? Inspired by ECMA-262, my plan is to make global this computation lazy. I thought there was an order-of-evaluation spec requiring it to be eager, but it's not true.

/be

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Go for it.

Comment 31

[Brendan Eich [:brendan]]

Attachment: Follow ECMA and be even lazier

Igor, I'd appreciate your review too.

Sayrer, this should no longer show up on benchmarks -- holler if it does.

/be

Comment 32

[Brendan Eich [:brendan]]

Passes the testsuite (Bob, please confirm when you can -- thanks).

/be

Comment 33

[Bob Clary [:bc] (inactive)]

(In reply to comment #32)

I can test on linux shortly.

Comment 34

[Brendan Eich [:brendan]]

Attachment: roll in mrbkap's half-way step, cope with lazy call needing to skip a frame

Comment 35

[Brian Crowder]

With the removal of JSOP_GLOBALTHIS, we need an xdr version bump, right?

Comment 36

[Brendan Eich [:brendan]]

Attachment: with JSXDR_BYTECODE_VERSION changed

Thanks, crowder.

/be

Comment 37

[Bob Clary [:bc] (inactive)]

patch in comment 36 fails to build w gcc 4.1.1/4.1.2

jsapi.c: In function 'js_generic_fast_native_method_dispatcher':
jsapi.c:4275: error: too many arguments to function 'js_ComputeThis'
jsapi.c: In function 'js_generic_native_method_dispatcher':
jsapi.c:4331: warning: passing argument 2 of 'js_ComputeThis' makes pointer from integer without a cast
jsapi.c:4331: error: too many arguments to function 'js_ComputeThis'

Comment 38

[Robert Sayre]

> Sayrer, this should no longer show up on benchmarks -- holler if it does.

Patch works like a charm.

> jsapi.c:4331: error: too many arguments to function 'js_ComputeThis'

Except you forgot jsinterp.h changes.

Comment 39

[Brendan Eich [:brendan]]

Attachment: With jsinterp.h

Sleep is good.

/be

Comment 40

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 303328 [details] [diff] [review]
With jsinterp.h

>+         * FIXME: nnnnnn -- this access check should not be required, as it

Make sure this gets filed.

Comment 41

[Igor Bukanov]

Comment on attachment 303328 [details] [diff] [review]
With jsinterp.h

My general perception for the patch is that it does the right things. But the real review requires to dig few more days for me into this. 

So please go ahead and check in without my r+ for now. I will add that later next week or file any raised issues if any as separated bugs.

Comment 42

[Brendan Eich [:brendan]]

Attachment: final patch for checkin

Comment 43

[Brendan Eich [:brendan]]

Fixed:

js/src/jsapi.c 3.414
js/src/jsdbgapi.c 3.127
js/src/jsemit.c 3.305
js/src/jsinterp.c 3.429
js/src/jsinterp.h 3.74
js/src/jsopcode.c 3.291
js/src/jsopcode.tbl 3.108
js/src/jsxdrapi.h 1.48

/be

Comment 44

[Brendan Eich [:brendan]]

(In reply to comment #40)
> (From update of attachment 303328 [details] [diff] [review])
> >+         * FIXME: nnnnnn -- this access check should not be required, as it
> 
> Make sure this gets filed.

Bug 417851.

/be

Comment 45

[Steffen Wilberg]

This caused frequent crashes when loading sites at js_ComputeThis, see bug 417944.

Comment 46

[Mathieu Johnson]

This has caused a regression, facebook friends don't appear anymore.

"20080215_1801 - Worked correctly using a new profile setup when I began regression hunting.

20080215_1847 - Using the same profile as above, displays the incorrect behaviour, with nothing showing up in any of the 'Friends' sections (ie. online now, everyone, etc.). "

A broken site report was filed, but I comment here to see if something can be done about the cause of the regression.

Comment 47

[Steve England [:stevee]]

Mathieu see bug 419221 (listed in the "Depends on:" list), specifically comment 3.