Closed Bug 729589 Opened 12 years ago Closed 12 years ago

fix mochitests for compartment-per-global

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: luke, Assigned: bholley)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
Nothing attached.
Attached patch cpg foldedSplinter Review
Oops
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!
Assignee: nobody → bobbyholley+bmo
\o/
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.
Attachment #601685 - Flags: review?(mrbkap)
Attaching a patch to fix the js-ctypes tests. Flagging jorendorff for review.
Attachment #601710 - Flags: review?(jorendorff)
(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.
Attachment #601685 - Flags: review?(mrbkap)
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.
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.
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.)
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
Attachment #604149 - Flags: review?(Ms2ger)
Updating this as Ms2ger's request.



Review: Ms2ger
Attachment #604149 - Attachment is obsolete: true
Attachment #604149 - Flags: review?(Ms2ger)
Attachment #604157 - Flags: review?(Ms2ger)
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 {
Attachment #604157 - Flags: review?(Ms2ger) → review+
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.
jorendorff - ping?
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 on attachment 601710 [details] [diff] [review]
Fix js-ctypes tests for compartment-per-global. v1

Yep.
Attachment #601710 - Flags: review?(jorendorff) → review+
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.
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/b5986b6f6bde
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: