Last Comment Bug 729589 - fix mochitests for compartment-per-global
: fix mochitests for compartment-per-global
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Bobby Holley (PTO through June 13)
:
Mentors:
Depends on:
Blocks: cpg
  Show dependency treegraph
 
Reported: 2012-02-22 09:38 PST by Luke Wagner [:luke]
Modified: 2012-03-28 14:20 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
cpg folded (137.48 KB, patch)
2012-02-22 10:40 PST, Luke Wagner [:luke]
no flags Details | Diff | Review
Bug 729589 - Fix caps stacktrace test. v1 (1.37 KB, patch)
2012-02-29 11:36 PST, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review
Fix js-ctypes tests for compartment-per-global. v1 (18.81 KB, patch)
2012-02-29 12:32 PST, Bobby Holley (PTO through June 13)
jorendorff: review+
Details | Diff | Review
Fix frameElement wrapping test. v1 (3.18 KB, patch)
2012-03-08 12:02 PST, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review
Fix frameElement wrapping test. v2 (3.70 KB, patch)
2012-03-08 12:32 PST, Bobby Holley (PTO through June 13)
Ms2ger: review+
Details | Diff | Review

Description Luke Wagner [:luke] 2012-02-22 09:38:35 PST
With bug 720580, we can actually remove JS_NewGlobalObject and run tests.  Although a compartment-per-global browser starts up fine and can, e.g., visit every site in membuster without hitting anything, it dies pretty early on mochitests: https://tbpl.mozilla.org/?tree=Try&rev=be3289cdb849.  The tests fail in similar ways so I suspect/hope its just a few core culprits.  A good place to start is:

  python _tests/testing/mochitest/runtests.py --test-path=Harness_sanity

(Note all the XPConnect-looking JS warnings/errors leading up to the timeout.)

Attached is a folded version of what I pushed to try.  It contains the central c-p-g patch and all its dependencies.  It is based on cset 4038ffaa5d82.
Comment 1 Paul Wright 2012-02-22 10:28:26 PST
Nothing attached.
Comment 2 Luke Wagner [:luke] 2012-02-22 10:40:28 PST
Created attachment 599690 [details] [diff] [review]
cpg folded

Oops
Comment 3 Bobby Holley (PTO through June 13) 2012-02-22 19:21:10 PST
Ok, so the (first) issue here is that my patch in bug 720580 was passing |wantXrays = true| during global creation. This causes the mochitest iframe to get an XrayWrapper when accessing its parent, which means that it can't see the |TestRunner| expando. This means that |isPrimaryTestWindow| (in SimpleTest.js) is always false, so we never attach the onload handler to call SimpleTest.finish(). So the tests never go anywhere.

The fix is straightforward - just pass wantXrays = false. This causes the sanity tests to pass (modulo some leaks), and all but one of the XPConnect mochitests to pass. Progress!
Comment 4 Luke Wagner [:luke] 2012-02-22 19:22:07 PST
\o/
Comment 5 Bobby Holley (PTO through June 13) 2012-02-29 11:36:24 PST
Created attachment 601685 [details] [diff] [review]
Bug 729589 - Fix caps stacktrace test. v1

This test is designed to ensure that we get appropriate stacks back from caps exceptions. Unfortunately, stacktraces are cut off at compartment boundaries (see InitExnPrivate in the js engine). So when this test tries to load a forbidden URL in an iframe, the exception is thrown in the iframe's scope. With c-p-g, that iframe now has its own compartment, so the stack trace doesn't include the canary function (inciteCaps).

Given that the load fails anyway, there's no reason not to just attempt the load for the current window, which is what this patch does.

Flagging mrbkap for review.
Comment 6 Bobby Holley (PTO through June 13) 2012-02-29 12:32:19 PST
Created attachment 601710 [details] [diff] [review]
Fix js-ctypes tests for compartment-per-global. v1

Attaching a patch to fix the js-ctypes tests. Flagging jorendorff for review.
Comment 7 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-03-05 06:56:58 PST
(In reply to Bobby Holley (:bholley) from comment #5)
> This test is designed to ensure that we get appropriate stacks back from
> caps exceptions. Unfortunately, stacktraces are cut off at compartment
> boundaries (see InitExnPrivate in the js engine). So when this test tries to
> load a forbidden URL in an iframe, the exception is thrown in the iframe's
> scope. With c-p-g, that iframe now has its own compartment, so the stack
> trace doesn't include the canary function (inciteCaps).

I don't think this is the proper way to fix this. We're going to have this problem for regular content too. Authors expect stack traces to be complete, even across iframe boundaries. I think we're going to have to come up with a mechanism to know whether or not to allow a stack track (or callee.caller for that matter) to continue. Unfortunately, we can't simply use subsumes anymore (to protect greasemonkey-like things). Suggestions are welcome, but we might need a bit somewhere in the compartment: "stop stacks here" in addition to a subsumes check.
Comment 8 Luke Wagner [:luke] 2012-03-05 08:59:48 PST
Bobby, Blake: I agree this sounds like a problem that needs a proper fix.  What about just changing InitExnPrivate to cross compartments and use JS_WrapValue on the saved values.  (Note 'values' will have to be an AutoValueVector.)  IIRC, this is the way it was before the compartment check so there should be no loss in security.
Comment 9 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-03-05 09:03:47 PST
Luke, that would work for now (with a subsumes thrown in to preserve the old behavior). But IMO, if content is called from GreaseMonkey directly (via a SJOW/cross origin wrapper) new Error().stack should hide GreaseMonkey's presence.
Comment 10 Luke Wagner [:luke] 2012-03-05 09:16:53 PST
I don't recall a subsumes ever being removed, do you?  What about the 'checkAccess' currently there?  Does 'subsume' subsume 'checkAccess'?  (On the subject, is the check access hook still necessary?  I see 10 or so uses that __proto__, getters/setters, that I would have guessed unnecessary with security wrappers.  I have no idea what 'checkAccess' means or when it should be used.)
Comment 11 Bobby Holley (PTO through June 13) 2012-03-08 12:02:15 PST
Created attachment 604149 [details] [diff] [review]
Fix frameElement wrapping test. v1

This test currently uses DOMWindowUtils to check when we're getting cross-compartment wrappers, which makes no sense in a c-p-g world. Flagging Ms2ger for review.


Review: Ms2ger
Comment 12 Bobby Holley (PTO through June 13) 2012-03-08 12:32:09 PST
Created attachment 604157 [details] [diff] [review]
Fix frameElement wrapping test. v2

Updating this as Ms2ger's request.



Review: Ms2ger
Comment 13 :Ms2ger 2012-03-08 12:35:03 PST
Comment on attachment 604157 [details] [diff] [review]
Fix frameElement wrapping test. v2

Review of attachment 604157 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

::: dom/tests/mochitest/general/file_frameElementWrapping.html
@@ +26,5 @@
> +        var pass = check(frameElement, sameOrigin, 'src');
> +        if (!pass) {
> +            sendMessage(false, sameOrigin, 'src');
> +        }
> +        else {

} else {
Comment 14 Bobby Holley (PTO through June 13) 2012-03-08 12:57:15 PST
Pushed the frameElement test to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/869326012e89

This is a metabug, so please leave it open after m-c merge.
Comment 15 Marco Bonardo [::mak] 2012-03-09 05:24:37 PST
https://hg.mozilla.org/mozilla-central/rev/869326012e89
Comment 16 Bobby Holley (PTO through June 13) 2012-03-16 09:41:46 PDT
jorendorff - ping?
Comment 17 Bobby Holley (PTO through June 13) 2012-03-22 10:36:28 PDT
jorendorff - this is kind of time-sensitive, because I'm trying to get compartment-per-global landed early next week. The review here should be a 2-minute rubber-stamp (we talked about the patch already on IRC a while back). Do you think you can get to it by the end of the week?
Comment 18 Jason Orendorff [:jorendorff] 2012-03-27 10:23:38 PDT
Comment on attachment 601710 [details] [diff] [review]
Fix js-ctypes tests for compartment-per-global. v1

Yep.
Comment 19 Bobby Holley (PTO through June 13) 2012-03-27 11:37:01 PDT
Thanks jorendorff.

Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/b5986b6f6bde

I think we don't need to leave this bug open. I'll re-open if we need it again.
Comment 20 Ed Morley [:emorley] 2012-03-28 14:20:39 PDT
https://hg.mozilla.org/mozilla-central/rev/b5986b6f6bde

Note You need to log in before you can comment on or make changes to this bug.