ComputeGlobalThis access checks should be removed

RESOLVED FIXED in mozilla1.9beta4

Status

()

P1
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({perf})

Trunk
mozilla1.9beta4
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

11 years ago
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

11 years ago
I'm assuming you meant this for 1.9 so nominating.  
Flags: blocking1.9?
(Assignee)

Comment 2

11 years ago
Created attachment 286634 [details] [diff] [review]
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
Attachment #286634 - Flags: review?(mrbkap)
Blocking final release, but not blocking M9.
Flags: blocking1.9? → blocking1.9+
Target Milestone: mozilla1.9 M9 → ---

Comment 4

11 years ago
Setting TM to M10 as this would be great for b2
Target Milestone: --- → mozilla1.9 M10

Updated

11 years ago
Keywords: perf
(Assignee)

Comment 5

11 years ago
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

11 years ago
Setting to P1 to make sure this gets looked at for b1
Priority: -- → P1
(Assignee)

Comment 7

11 years ago
Thanks to jst for taking this.

/be
Assignee: brendan → 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?
(Assignee)

Comment 9

11 years ago
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
jst, would you attach the testcase you're using?

Comment 11

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

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

11 years ago
So is this really going to make M10?  We should either close it out or set TM to M11
Moving out to M11.
Target Milestone: mozilla1.9 M10 → mozilla1.9 M11

Comment 15

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

Comment 16

11 years ago
JST/Bkap what's next with this?

Comment 17

11 years ago
Created attachment 296790 [details] [diff] [review]
updated to Trunk

Comment 18

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

More improvement in-browser. > 10%.
Created attachment 296848 [details] [diff] [review]
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.
Assignee: jst → mrbkap
Status: NEW → ASSIGNED
Attachment #296848 - Flags: review?(brendan)
(Assignee)

Comment 20

11 years ago
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
(Assignee)

Comment 21

11 years ago
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
(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?
(Assignee)

Comment 23

11 years ago
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

11 years ago
Bkap - we gonna get an updated patch here?

Updated

11 years ago
Target Milestone: mozilla1.9beta3 → mozilla1.9beta4

Comment 28

11 years ago
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?
(Assignee)

Comment 29

11 years ago
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
Assignee: mrbkap → brendan
Status: ASSIGNED → NEW
(Assignee)

Updated

11 years ago
Attachment #286634 - Attachment is obsolete: true
Attachment #286634 - Flags: review?(mrbkap)
(Assignee)

Comment 31

11 years ago
Created attachment 303150 [details] [diff] [review]
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
Attachment #296790 - Attachment is obsolete: true
Attachment #296848 - Attachment is obsolete: true
Attachment #303150 - Flags: review?(mrbkap)
Attachment #296848 - Flags: review?(brendan)
(Assignee)

Updated

11 years ago
Attachment #303150 - Flags: review?(igor)
(Assignee)

Comment 32

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

/be
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED

Comment 33

11 years ago
(In reply to comment #32)

I can test on linux shortly.
(Assignee)

Comment 34

11 years ago
Created attachment 303164 [details] [diff] [review]
roll in mrbkap's half-way step, cope with lazy call needing to skip a frame
Attachment #303150 - Attachment is obsolete: true
Attachment #303164 - Flags: review?(mrbkap)
Attachment #303150 - Flags: review?(mrbkap)
Attachment #303150 - Flags: review?(igor)
(Assignee)

Updated

11 years ago
Attachment #303164 - Flags: review?(igor)

Comment 35

11 years ago
With the removal of JSOP_GLOBALTHIS, we need an xdr version bump, right?
(Assignee)

Comment 36

11 years ago
Created attachment 303221 [details] [diff] [review]
with JSXDR_BYTECODE_VERSION changed

Thanks, crowder.

/be
Attachment #303164 - Attachment is obsolete: true
Attachment #303221 - Flags: review?(mrbkap)
Attachment #303164 - Flags: review?(mrbkap)
Attachment #303164 - Flags: review?(igor)
(Assignee)

Updated

11 years ago
Attachment #303221 - Flags: review?(igor)

Comment 37

11 years ago
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

11 years ago
> 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.
(Assignee)

Comment 39

11 years ago
Created attachment 303328 [details] [diff] [review]
With jsinterp.h

Sleep is good.

/be
Attachment #303221 - Attachment is obsolete: true
Attachment #303328 - Flags: review?(mrbkap)
Attachment #303221 - Flags: review?(mrbkap)
Attachment #303221 - Flags: review?(igor)
(Assignee)

Updated

11 years ago
Attachment #303328 - Flags: review?(igor)
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.
Attachment #303328 - Flags: review?(mrbkap) → review+

Comment 41

11 years ago
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.
Attachment #303328 - Flags: review?(igor)
(Assignee)

Comment 42

11 years ago
Created attachment 303644 [details] [diff] [review]
final patch for checkin
Attachment #303328 - Attachment is obsolete: true
Attachment #303644 - Flags: review+
Attachment #303644 - Flags: approval1.9+
(Assignee)

Comment 43

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 44

11 years ago
(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

Updated

11 years ago
Depends on: 417916

Updated

11 years ago
Blocks: 417944
No longer blocks: 417944
Depends on: 417944

Comment 45

11 years ago
This caused frequent crashes when loading sites at js_ComputeThis, see bug 417944.
Depends on: 417851

Updated

11 years ago
Flags: in-testsuite-
Flags: in-litmus-

Updated

11 years ago
Depends on: 419221

Comment 46

11 years ago
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.

Mathieu see bug 419221 (listed in the "Depends on:" list), specifically comment 3.
You need to log in before you can comment on or make changes to this bug.