788914 - Remove dynamic UniversalXPConnect checks in security wrappers

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Now that enablePrivilege is a permanent bit on the compartment, we can encode this information in the wrappers, and ditch a bunch of nasty dynamic checks. Whenever it's enabled, we can just recompute all the wrappers out of the compartment.

Patches should be straightforward, but try will tell...

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=d9110314373f

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=f95bfff16c8e

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove enablePrivilege from bug 585922 tests. v1

This test chokes on the changes in the patch for some reason. Fortunately, since
enablePrivilege now exists solely to make our tests go green, changing its semantics
and removing use of it from anywhere that goes orange is a perfectly acceptable
approach. ;-)

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Generate vanilla cross-compartment wrappers when UniversalXPConnect is enabled. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Recompute cross-compartment wrappers when UniversalXPConnect is enabled. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove dynamic UniversalXPConnect checks sprinkled around the wrapper code. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 5 - Kill partially transparent wrappers. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 6 - Kill the XOW flag. v2

There are really two questions to be asked: is the caller chrome, and does the
caller subsume the callee. We have other, more precise ways of asking both of
these questions.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 5 - Kill partially transparent wrappers. v2

Removed a line of dead code introduced by this patch.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 658944 [details] [diff] [review]
Part 4 - Remove dynamic UniversalXPConnect checks sprinkled around the wrapper code. v1

It's nice to see these all go.

Comment 11

[Bobby Holley (:bholley)]

Thanks for the quick reviews, Blake!

I fixed the oranges from the try push in comment 2, so this should be good to go. Pushed to m-i:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/87aa103d7e87
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/94cfcf5da7c8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8c1ed6327355
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d787279d282c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e8fadd906232
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5853df66d488

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Not really.  mochitest-4 is orange on all platforms.
14967 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug411236.html | an unexpected uncaught JS exception reported through window.onerror - Error: Permission denied to access property 'type' at http://mochi.test:8888/tests/layout/forms/test/test_bug411236.html:70

Comment 13

[Ed Morley [:emorley]]

Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa269335d5b8

Comment 14

[Bobby Holley (:bholley)]

Doh! Sorry guys. I somehow managed to fail to scroll all the way down, and was given false confidence by the fact that a push on top of those patches went green. That other push actually included bug 789494, which fixed this test.

So I'm doing a try push with this bug and bug 789494. It should go green, and then I can just land them together:

https://tbpl.mozilla.org/?tree=Try&rev=e418fdd6aecb

Comment 15

[Bobby Holley (:bholley)]

Looks green this time. Pushed to m-i:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6d74382e1dad
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b3dfdf56f026
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d4d0839c44af
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/130cf7ee659f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/db48c62b2ccc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9ad140c544b9

Comment 16

[Ryan VanderMeulen [:RyanVM]]

The MOZ_ASSERT(rv) added by Part 3 is failing intermittently on inbound.

https://tbpl.mozilla.org/php/getParsedLog.php?id=15055532&tree=Mozilla-Inbound

REFTEST TEST-START | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_8_5/extensions/recursion.js | 3029 / 3199 (94%)
++DOMWINDOW == 74 (0x42e7490) [serial = 5674] [outer = 0x280a7a0]
622167: Handle infinite recursion
Assertion failure: rv, at ../../../js/xpconnect/src/xpcprivate.h:4416
TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_8_5/extensions/recursion.js | Exited with code 1 during test run
INFO | automation.py | Application ran for: 0:11:22.682759
INFO | automation.py | Reading PID log: /tmp/tmpn6rt_Fpidlog
Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-debug/1347045136/firefox-18.0a1.en-US.linux-x86_64.crashreporter-symbols.zip
PROCESS-CRASH | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_8_5/extensions/recursion.js | application crashed (minidump found)
Crash dump filename: /tmp/tmpSWWljo/minidumps/45c1a659-5dcf-4aa4-59989143-2dea5ec0.dmp
Operating system: Linux
                  0.0.0 Linux 2.6.31.5-127.fc12.x86_64 #1 SMP Sat Nov 7 21:11:14 EST 2009 x86_64
CPU: amd64
     family 6 model 23 stepping 10
     2 CPUs

Crash reason:  SIGSEGV
Crash address: 0x0

Thread 0 (crashed)
 0  libxul.so!xpc::EnableUniversalXPConnect [xpcprivate.h : 4416 + 0x1c]
    rbx = 0x0280abe0   r12 = 0x2588ebc0   r13 = 0x17b22071   r14 = 0x00000000
    r15 = 0xfe81a578   rip = 0x17b22058   rsp = 0x06db42d0   rbp = 0x06db4300
    Found by: given as instruction pointer in context
 1  libxul.so!netscape_security_enablePrivilege [nsSecurityManagerFactory.cpp : 48 + 0x4]
    rbx = 0x0280abe0   r12 = 0x06db44c0   r13 = 0x17b22071   r14 = 0x00000000
    r15 = 0xfe81a578   rip = 0x17b2207a   rsp = 0x06db4310   rbp = 0x06db4310
    Found by: call frame info
 2  libxul.so!js::CallJSNative [jscntxtinlines.h : 372 + 0x8]
    rbx = 0x0280abe0   r12 = 0x06db44c0   r13 = 0x17b22071   r14 = 0x00000000
    r15 = 0xfe81a578   rip = 0x18834b47   rsp = 0x06db4320   rbp = 0x06db4380
    Found by: call frame info
 3  libxul.so!js::InvokeKernel [jsinterp.cpp : 344 + 0x10]
    rbx = 0x0280abe0   r12 = 0xe41d4e80   r13 = 0x00000000   r14 = 0xe412e740
    r15 = 0x014a0800   rip = 0x1884db60   rsp = 0x06db4390   rbp = 0x06db44b0
    Found by: call frame info
 4  libxul.so!js::Interpret [jsinterp.cpp : 2406 + 0x2c]
    rbx = 0x0280abe0   r12 = 0x00000000   r13 = 0x06db4600   r14 = 0x00000000
    r15 = 0x014a0800   rip = 0x1883b786   rsp = 0x06db44c0   rbp = 0x06db4cb0
    Found by: call frame info
 5  libxul.so!js::RunScript [jsinterp.cpp : 301 + 0xc]
    rbx = 0x0280abe0   r12 = 0xe418ecb8   r13 = 0xfe81a3f8   r14 = 0xfe81a3f8
    r15 = 0x06db4cd0   rip = 0x1884d5d5   rsp = 0x06db4cc0   rbp = 0x06db4d20
    Found by: call frame info
 6  libxul.so!SendToGenerator [jsiter.cpp : 1552 + 0x16]
    rbx = 0x25477a20   r12 = 0x0280abe0   r13 = 0x00000000   r14 = 0x06db4d40
    r15 = 0x00000000   rip = 0x18850380   rsp = 0x06db4d30   rbp = 0x06db4dc0
    Found by: call frame info
 7  libxul.so!generator_next_impl [jsiter.cpp : 1659 + 0x1e]
    rbx = 0xfe81a3e8   r12 = 0x0280abe0   r13 = 0x25477a20   r14 = 0xfe81a3e0
    r15 = 0xfe81a3d8   rip = 0x18852b17   rsp = 0x06db4dd0   rbp = 0x06db4e20
    Found by: call frame info
 8  libxul.so!JS::CallNonGenericMethod<IsGenerator, generator_next_impl> [jsapi.h : 1532 + 0x2d]
    rbx = 0x0280abe0   r12 = 0xe4195588   r13 = 0x18853190   r14 = 0x00000000
    r15 = 0xfe81a3d8   rip = 0x18853167   rsp = 0x06db4e30   rbp = 0x06db4e90
    Found by: call frame info
 9  libxul.so!generator_next [jsiter.cpp : 1670 + 0x16]
    rbx = 0x0280abe0   r12 = 0x06db5090   r13 = 0x18853190   r14 = 0x00000000
    r15 = 0xfe81a3d8   rip = 0x188531c2   rsp = 0x06db4ea0   rbp = 0x06db4ee0

Comment 17

[Bobby Holley (:bholley)]

I think we can live with a failure there. Converted this into a warning:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c06f44d000af

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Sorry, I had to back this out for more assertion failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc73c2c680d1

https://tbpl.mozilla.org/php/getParsedLog.php?id=15060570&tree=Mozilla-Inbound

Assertion failure: !cx->isExceptionPending(), at e:\builds\moz2_slave\m-in-w32-dbg\build\js\src\jscntxtinlines.h:375
TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_8_5/extensions/recursion.js | Exited with code -2147483645 during test run

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d74382e1dad
https://hg.mozilla.org/mozilla-central/rev/b3dfdf56f026
https://hg.mozilla.org/mozilla-central/rev/d4d0839c44af
https://hg.mozilla.org/mozilla-central/rev/130cf7ee659f
https://hg.mozilla.org/mozilla-central/rev/db48c62b2ccc
https://hg.mozilla.org/mozilla-central/rev/9ad140c544b9

Comment 20

[Ryan VanderMeulen [:RyanVM]]

I inadvertently merged this over to m-c. And then backed it back out.
https://hg.mozilla.org/mozilla-central/rev/591eb47f6bd3

Comment 21

[Bobby Holley (:bholley)]

Oh, I see. When we recompute wrappers during the EnableUniversalXPConnect call, we trigger a recursion check in the js engine, and the failing test here is purposely trigger an over-recursion exception. So we just need to return the error rather than squelching it.

Comment 22

[Bobby Holley (:bholley)]

And as for bug 789563, the issue is that Patch 6 should convert the check in ::enumerate to !wrapperSubsumes, not !isChrome.

Blake, can you ack that change?

https://tbpl.mozilla.org/?tree=Try&rev=5da100da05d9

Comment 23

[Alexandre Poirot [:ochameau]]

Bobby, this patch looks 99% good regarding jetpack tests.
The only one percent left is because of this failing assertion:
  error: api-utils: TEST FAILED: test-content-proxy.testXPathResult (failure)
  error: api-utils: fail: XPathResult works correctly on Proxies

I reduced this failing test to this. It appears that we are missing some IDL properties?
  var sb = Components.utils.Sandbox(content, {sandboxPrototype:content});
  var rv = Components.utils.evalInSandbox("XPathResult.UNORDERED_NODE_SNAPSHOT_TYPE", sb);
  Components.utils.reportError(rv); // Print 6 on Nightly, undefined on this build.

Comment 24

[Bobby Holley (:bholley)]

Attachment: interdiff of fixes

Added one more fix for jetpack and pushed to try one last time to be triple-sure:

https://tbpl.mozilla.org/?tree=Try&rev=808400b8885d

Flagging Blake for review on the interdiff. Blake, this interdiff does 2 things:
1 - Makes EnableUniversalXPConnect properly fallible.
2 - Changes a few checks to handle the jetpack wantXray case (same-origin Xrays that don't have XOW-style filtering).

Comment 25

[Bobby Holley (:bholley)]

(In reply to Alexandre Poirot (:ochameau) from comment #23)
> Bobby, this patch looks 99% good regarding jetpack tests.
> The only one percent left is because of this failing assertion:
>   error: api-utils: TEST FAILED: test-content-proxy.testXPathResult (failure)
>   error: api-utils: fail: XPathResult works correctly on Proxies
> 
> I reduced this failing test to this. It appears that we are missing some IDL
> properties?

Sort of. The issue is that there are some properties that aren't defined in IDL that we resolve with scriptable helpers (in nsDOMClassInfo). We've got various detection in there for Xrays, but I managed to bungle the wantXrays case (same-origin Xray wrappers that aren't chrome). I fixed those and verified the fix by running the jetpack tests locally, and pushed again to try.

Comment 26

[Alexandre Poirot [:ochameau]]

(In reply to Bobby Holley (:bholley) from comment #25)
> I fixed those and verified the fix by running the jetpack tests locally, and pushed
> again to try.

In behalf of the whole jetpack team, thanks!!

Comment 27

[Bobby Holley (:bholley)]

Alright, this now has 2 fully green try runs (comment 24 and comment 22). Hopefully it sticks this time:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cefea6ee27bf
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd0e15efc4a7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/382dce79a998
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1cef36c730eb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aa0fba2eb327
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5b4f0ecf01

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cefea6ee27bf
https://hg.mozilla.org/mozilla-central/rev/cd0e15efc4a7
https://hg.mozilla.org/mozilla-central/rev/382dce79a998
https://hg.mozilla.org/mozilla-central/rev/1cef36c730eb
https://hg.mozilla.org/mozilla-central/rev/aa0fba2eb327
https://hg.mozilla.org/mozilla-central/rev/4f5b4f0ecf01