Closed
Bug 729589
Opened 12 years ago
Closed 12 years ago
fix mochitests for compartment-per-global
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: luke, Assigned: bholley)
References
Details
Attachments
(4 files, 1 obsolete file)
137.48 KB,
patch
|
Details | Diff | Splinter Review | |
1.37 KB,
patch
|
Details | Diff | Splinter Review | |
18.81 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
3.70 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Nothing attached.
Reporter | ||
Comment 2•12 years ago
|
||
Oops
Assignee | ||
Comment 3•12 years ago
|
||
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
Reporter | ||
Comment 4•12 years ago
|
||
\o/
Assignee | ||
Comment 5•12 years ago
|
||
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)
Assignee | ||
Comment 6•12 years ago
|
||
Attaching a patch to fix the js-ctypes tests. Flagging jorendorff for review.
Attachment #601710 -
Flags: review?(jorendorff)
Comment 7•12 years ago
|
||
(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.
Updated•12 years ago
|
Attachment #601685 -
Flags: review?(mrbkap)
Reporter | ||
Comment 8•12 years ago
|
||
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•12 years ago
|
||
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.
Reporter | ||
Comment 10•12 years ago
|
||
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.)
Assignee | ||
Comment 11•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Attachment #604149 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 12•12 years ago
|
||
Updating this as Ms2ger's request. Review: Ms2ger
Attachment #604149 -
Attachment is obsolete: true
Attachment #604149 -
Flags: review?(Ms2ger)
Assignee | ||
Updated•12 years ago
|
Attachment #604157 -
Flags: review?(Ms2ger)
Comment 13•12 years ago
|
||
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+
Assignee | ||
Comment 14•12 years ago
|
||
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.
Assignee | ||
Comment 16•12 years ago
|
||
jorendorff - ping?
Assignee | ||
Comment 17•12 years ago
|
||
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•12 years ago
|
||
Comment on attachment 601710 [details] [diff] [review] Fix js-ctypes tests for compartment-per-global. v1 Yep.
Attachment #601710 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 19•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Target Milestone: --- → mozilla14
Comment 20•12 years ago
|
||
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.
Description
•