Remove dynamic UniversalXPConnect checks in security wrappers

RESOLVED FIXED in mozilla18

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: Bobby Holley (parental leave - send mail for anything urgent))

Tracking

unspecified
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 1 obsolete attachment)

2.94 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.59 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.12 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
9.56 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
11.39 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
10.71 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.76 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
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...
https://tbpl.mozilla.org/?tree=Try&rev=d9110314373f
https://tbpl.mozilla.org/?tree=Try&rev=f95bfff16c8e
Created attachment 658940 [details] [diff] [review]
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. ;-)
Attachment #658940 - Flags: review?(mrbkap)
Created attachment 658942 [details] [diff] [review]
Part 2 - Generate vanilla cross-compartment wrappers when UniversalXPConnect is enabled. v1
Attachment #658942 - Flags: review?(mrbkap)
Created attachment 658943 [details] [diff] [review]
Part 3 - Recompute cross-compartment wrappers when UniversalXPConnect is enabled. v1
Attachment #658943 - Flags: review?(mrbkap)
Created attachment 658944 [details] [diff] [review]
Part 4 - Remove dynamic UniversalXPConnect checks sprinkled around the wrapper code. v1
Attachment #658944 - Flags: review?(mrbkap)
Created attachment 658945 [details] [diff] [review]
Part 5 - Kill partially transparent wrappers. v1
Attachment #658945 - Flags: review?(mrbkap)
Created attachment 658946 [details] [diff] [review]
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.
Attachment #658946 - Flags: review?(mrbkap)
Created attachment 658949 [details] [diff] [review]
Part 5 - Kill partially transparent wrappers. v2

Removed a line of dead code introduced by this patch.
Attachment #658945 - Attachment is obsolete: true
Attachment #658945 - Flags: review?(mrbkap)
Attachment #658949 - Flags: review?(mrbkap)
Blocks: 784750

Updated

5 years ago
Attachment #658940 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #658942 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #658943 - Flags: review?(mrbkap) → review+
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.
Attachment #658944 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #658949 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #658946 - Flags: review?(mrbkap) → review+
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
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
Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa269335d5b8
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
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

Updated

5 years ago
Depends on: 789563
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
I think we can live with a failure there. Converted this into a warning:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c06f44d000af
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
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
I inadvertently merged this over to m-c. And then backed it back out.
https://hg.mozilla.org/mozilla-central/rev/591eb47f6bd3
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.
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
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.
Created attachment 659807 [details] [diff] [review]
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).
Attachment #659807 - Flags: review?(mrbkap)
(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.
(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!!

Updated

5 years ago
Attachment #659807 - Flags: review?(mrbkap) → review+
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
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

5 years ago
Depends on: 790905
Depends on: 812415

Updated

5 years ago
Depends on: 812942
You need to log in before you can comment on or make changes to this bug.