Nightly from 7/14/2012 crashes reproducibly for me while loading http://www.slate.com/blogs/future_tense/2012/07/13/eric_schmidt_on_self_driving_cars_biggest_problem_they_obey_speed_limits.html Crash report: https://crash-stats.mozilla.com/report/index/bp-3b0f2697-cee1-49d9-95ed-035782120714

The crash appears to happen only with the 1Password extension enabled. The 1Password extension doesn't work properly in Nightly anyway, but since it's Add-on SDK-based I suspect that it shouldn't be capable of causing a crash.

First occurred with yesterday's (7/13) Nightly. Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=46804c31366b&tochange=6489be1890c0

It's likely a regression from bug 771908.

(In reply to Scoobidiver from comment #3) > It's likely a regression from bug 771908. On what do you base that assessment? Every implementation of Wrapper::leave is a no-op, that's why it got removed. I have a hard time believing that removing an unused call could cause a regression, so I'd like to see some proof.

(In reply to Eddy Bruel [:ejpbruel] from comment #4) > (In reply to Scoobidiver from comment #3) > > It's likely a regression from bug 771908. > On what do you base that assessment? It was based on jswrapper in the stack trace, but if you removed unused code, it's unlikely the culprit. The other possible culprit is bug 655649 that changed nsPrincipal::SetDomain.

Yes, this is a regression from bug 655649. I'll look into it.

It's #5 top browser crasher in 17.0a1.

I am looking into it. We're somehow ending up with a DeadObjectProxy in the cross-compartment wrapper map.

So this issue here is that we don't ever handle Xray waivers when brain transplanting. If we do |var foo = contentWindow.wrappedJSObject|, we have the following, where => is a same-compartment wrapper and =|> is a cross-compartment wrapper: foo =|> waiver => window But when window navigates, we end up with: foo =|> waiver => transplanted_window =|> window So in this case, js::Unwrap takes us across two compartments, which violates all sorts of assumptions we make. I think we need to remap waiver pointers after transplanting.

This catches the crash when the naughtiness happens, rather than later on.

It's sort of annoying to add this API just for tests, but there's not another great way to trigger a compartment-wide transplant with Xray waivers (since setting document.domain doesn't recompute wrappers to/from chrome, and Xray waivers will stop being accessible to content entirely in bug 742444).

Comment on attachment 644273 [details] [diff] [review] Part 1 - Add some asserts in the brain transplant code. v1 Review of attachment 644273 [details] [diff] [review]: ----------------------------------------------------------------- These are great assertions! Just for my own enlightenment, is it true that if you do wrappedJSObject on an Xray wrapper, you get an Xray waiver wrapper? What does the waiver wrapper do that's different than just having a CCW to the actual object? ::: js/src/jswrapper.cpp @@ +1120,5 @@ > > + // If we're mapping to a different target (as opposed to just recomputing > + // for the same target), we must not have an existing wrapper for the new > + // target, otherwise this will break. > + JS_ASSERT_IF(origTarget != newTarget, !pmap.lookup(ObjectValue(*newTarget))); pmap.has would be a little more idiomatic

Comment on attachment 644276 [details] [diff] [review] Part 4 - Introduce Cu.recomputeWrappers. v1 I'm going to mark this as r+ in order to not get in the way of this landing, but could we use a chrome document.write to achieve the same effect? Alternatively, we could add a chrome-only opt-in API on sandboxes, which seems a little less invasive than adding a new, pretty esoteric, API to Components.utils. Also, please bump the UUID for nsIXPCComponent_Utils if we do go this route.

Comment on attachment 644277 [details] [diff] [review] Part 5 - Tests. v1 Review of attachment 644277 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/tests/chrome/test_bug773962.xul @@ +56,5 @@ > + // Now, recompute some wrappers. > + Cu.recomputeWrappers(); > + > + // First, pat ourselves on the back for not asserting/crashing. That's most of > + // what we're really testing here. We should take care not to hurt our shoulders patting ourselves on the back :-)

(In reply to Blake Kaplan (:mrbkap) from comment #16) > Comment on attachment 644276 [details] [diff] [review] > Part 4 - Introduce Cu.recomputeWrappers. v1 > > I'm going to mark this as r+ in order to not get in the way of this landing, > but could we use a chrome document.write to achieve the same effect? I spent about an hour trying to make this work, but I couldn't get a test working that actually caused the crash. document.write on a chrome HTML document doesn't work because it just causes the wrappers to be transplanted, not recomputed (as this test requires). And document.domain doesn't work for system principal. > Alternatively, we could add a chrome-only opt-in API on sandboxes, which > seems a little less invasive than adding a new, pretty esoteric, API to > Components.utils. I'm not so sure - either way it will be visible to extension developers, so I don't think there's a great reason to put it on sandboxes instead, which makes it harder to use from chrome tests. I'm going to mark the API as "Gecko internal use only - use at your own risk" and go ahead on this. > Also, please bump the UUID for nsIXPCComponent_Utils if we do go this route. Will do.

(In reply to Bill McCloskey (:billm) from comment #15) > Comment on attachment 644273 [details] [diff] [review] > Part 1 - Add some asserts in the brain transplant code. v1 > > Review of attachment 644273 [details] [diff] [review]: > ----------------------------------------------------------------- > > These are great assertions! > > Just for my own enlightenment, is it true that if you do wrappedJSObject on > an Xray wrapper, you get an Xray waiver wrapper? What does the waiver > wrapper do that's different than just having a CCW to the actual object? Sort of. You get a cross-compartment wrapper to a different object on the other side. That object, the "waiver", is just a transparent same-compartment wrapper around the actual object. It exists to give foo.wrappedJSObject a different identity from foo (so we can have cross-compartment wrappers to both at the same time).

Pushed this to try: https://mail.google.com/mail/?ui=2&view=bsp&ver=ohhl4rw8mbn4

Looks green. Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/195065ccf37b http://hg.mozilla.org/integration/mozilla-inbound/rev/325ff8e04bf1 http://hg.mozilla.org/integration/mozilla-inbound/rev/64aba0b5c374 http://hg.mozilla.org/integration/mozilla-inbound/rev/712a40509486 http://hg.mozilla.org/integration/mozilla-inbound/rev/451e63f9381c We should get this on m-a once it bakes a bit.

and a windows linkage bustage fix: http://hg.mozilla.org/integration/mozilla-inbound/rev/4c9ee7535383

https://hg.mozilla.org/mozilla-central/rev/451e63f9381c https://hg.mozilla.org/mozilla-central/rev/712a40509486 https://hg.mozilla.org/mozilla-central/rev/64aba0b5c374 https://hg.mozilla.org/mozilla-central/rev/325ff8e04bf1 https://hg.mozilla.org/mozilla-central/rev/195065ccf37b https://hg.mozilla.org/mozilla-central/rev/4c9ee7535383

Comment on attachment 644275 [details] [diff] [review] Part 3 - Fix up waivers after transplanting. v1 This has baked for a few days on m-c. Requesting approval for patches 1-3. I'm going to skip landing the tests so that we can avoid the iid rev on XPCComponents (though if the release drivers would prefer to rev the iid and take the tests, we can easily do that too). [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 655649 User impact if declined: Crashes Testing completed (on m-c, etc.): baked on m-c Risk to taking this patch (and alternatives if risky): No alternatives. Medium risk. Does some pretty tricky stuff, but it's only for Xray waivers, which only privileged code can create. String or UUID changes made by this patch: None

Also, Scoobidiver, can you verify that this fixed the topcrash?

(In reply to Bobby Holley (:bholley) from comment #25) > Also, Scoobidiver, can you verify that this fixed the topcrash? The latest crashes happened in 17.0a1/20120724.

(In reply to Bobby Holley (:bholley) from comment #24) > Comment on attachment 644275 [details] [diff] [review] > Part 3 - Fix up waivers after transplanting. v1 > > This has baked for a few days on m-c. Requesting approval for patches 1-3. > I'm going to skip landing the tests so that we can avoid the iid rev on > XPCComponents (though if the release drivers would prefer to rev the iid and > take the tests, we can easily do that too). Your reasoning sounds sane here.

Comment on attachment 644275 [details] [diff] [review] Part 3 - Fix up waivers after transplanting. v1 [Triage Comment] Medium risk, but fixes a top crasher. Getting this in sooner is better. Approved for Aurora 16.

Pushed to m-a: http://hg.mozilla.org/releases/mozilla-aurora/rev/657b6a0f0c1c http://hg.mozilla.org/releases/mozilla-aurora/rev/6ac860432dd2 http://hg.mozilla.org/releases/mozilla-aurora/rev/2a8c122b8a60

There are still a couple of these crashes per day on Aurora... but the last build they are showing up in is 7/28, so it looks like this worked. :)