Closed Bug 788914 Opened 8 years ago Closed 8 years ago

Remove dynamic UniversalXPConnect checks in security wrappers

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(7 files, 1 obsolete file)

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...
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)
Attachment #658945 - Flags: review?(mrbkap)
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)
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
Attachment #658940 - Flags: review?(mrbkap) → review+
Attachment #658942 - Flags: review?(mrbkap) → review+
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+
Attachment #658949 - Flags: review?(mrbkap) → review+
Attachment #658946 - Flags: review?(mrbkap) → review+
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
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
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
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.
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!!
Attachment #659807 - Flags: review?(mrbkap) → review+
Depends on: 790905
Depends on: 812942
You need to log in before you can comment on or make changes to this bug.