Closed Bug 773962 Opened 8 years ago Closed 8 years ago

Crash @ JSObject::swap

Categories

(Core :: JavaScript Engine, defect, critical)

16 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla17
Tracking Status
firefox16 + fixed
firefox17 + verified

People

(Reporter: simon, Assigned: bholley)

References

Details

(Keywords: crash, regression, topcrash, Whiteboard: [js:p1])

Crash Data

Attachments

(5 files)

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.
Blocks: 771908
Status: UNCONFIRMED → NEW
Crash Signature: [@ JSObject::swap(JSContext*, JSObject*)] [@ JSObject::swap]
Ever confirmed: true
Keywords: crash, regression
OS: Mac OS X → All
Hardware: x86_64 → All
Summary: Crash [@ JSObject::swap ] → Crash @ JSObject::swap
Version: Trunk → 16 Branch
(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.
No longer blocks: 771908
Yes, this is a regression from bug 655649. I'll look into it.
Assignee: general → bobbyholley+bmo
Whiteboard: [js:p1]
It's #5 top browser crasher in 17.0a1.
Keywords: topcrash
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.
Attachment #644273 - Flags: review?(wmccloskey)
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).
Attachment #644276 - Flags: review?(mrbkap)
Attachment #644277 - Flags: review?(mrbkap)
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
Attachment #644273 - Flags: review?(wmccloskey) → review+
Attachment #644274 - Flags: review?(mrbkap) → review+
Attachment #644275 - Flags: review?(mrbkap) → review+
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.
Attachment #644276 - Flags: review?(mrbkap) → review+
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 :-)
Attachment #644277 - Flags: review?(mrbkap) → review+
(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).
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
Attachment #644275 - Flags: approval-mozilla-aurora?
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.
Status: RESOLVED → VERIFIED
(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.
Attachment #644275 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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. :)
You need to log in before you can comment on or make changes to this bug.