Crash @ JSObject::swap

VERIFIED FIXED in Firefox 16

Status

()

--
critical
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: simon, Assigned: bholley)

Tracking

({crash, regression, topcrash})

16 Branch
mozilla17
crash, regression, topcrash
Points:
---

Firefox Tracking Flags

(firefox16+ fixed, firefox17+ verified)

Details

(Whiteboard: [js:p1], crash signature)

Attachments

(5 attachments)

(Reporter)

Comment 1

7 years ago
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.
(Reporter)

Comment 2

7 years ago
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
(Assignee)

Comment 6

7 years ago
Yes, this is a regression from bug 655649. I'll look into it.
Assignee: general → bobbyholley+bmo
tracking-firefox16: --- → +
tracking-firefox17: --- → +
Whiteboard: [js:p1]
It's #5 top browser crasher in 17.0a1.
Keywords: topcrash
(Assignee)

Comment 8

7 years ago
I am looking into it. We're somehow ending up with a DeadObjectProxy in the cross-compartment wrapper map.
(Assignee)

Comment 9

7 years ago
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.
(Assignee)

Comment 10

7 years ago
Created attachment 644273 [details] [diff] [review]
Part 1 - Add some asserts in the brain transplant code. v1

This catches the crash when the naughtiness happens, rather than later on.
Attachment #644273 - Flags: review?(wmccloskey)
(Assignee)

Comment 11

7 years ago
Created attachment 644274 [details] [diff] [review]
Part 2 - Refactor Xray waiving logic to allow simple lookups in the waiver map without creating a waiver. v1
Attachment #644274 - Flags: review?(mrbkap)
(Assignee)

Comment 12

7 years ago
Created attachment 644275 [details] [diff] [review]
Part 3 - Fix up waivers after transplanting. v1
Attachment #644275 - Flags: review?(mrbkap)
(Assignee)

Comment 13

7 years ago
Created attachment 644276 [details] [diff] [review]
Part 4 - Introduce Cu.recomputeWrappers. v1

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)
(Assignee)

Comment 14

7 years ago
Created attachment 644277 [details] [diff] [review]
Part 5 - Tests. v1
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+
(Assignee)

Comment 18

7 years ago
(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.
(Assignee)

Comment 19

7 years ago
(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).
(Assignee)

Comment 24

7 years ago
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?
(Assignee)

Comment 25

7 years ago
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
status-firefox17: --- → 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.