The default bug view has changed. See this FAQ.

Crash @ JSObject::swap

VERIFIED FIXED in Firefox 16

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Simon Kornblith, 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)

Description

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

Comment 1

5 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

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

Comment 3

5 years ago
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.

Comment 5

5 years ago
(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
tracking-firefox16: --- → +
tracking-firefox17: --- → +
Whiteboard: [js:p1]

Comment 7

5 years ago
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.
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)
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)
Created attachment 644275 [details] [diff] [review]
Part 3 - Fix up waivers after transplanting. v1
Attachment #644275 - Flags: review?(mrbkap)
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)
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+

Updated

5 years ago
Attachment #644274 - Flags: review?(mrbkap) → review+

Updated

5 years ago
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).
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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?

Comment 26

5 years ago
(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+
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
status-firefox16: --- → fixed
Depends on: 779122
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.