Last Comment Bug 773962 - Crash @ JSObject::swap
: Crash @ JSObject::swap
Status: VERIFIED FIXED
[js:p1]
: crash, regression, topcrash
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 16 Branch
: All All
: -- critical (vote)
: mozilla17
Assigned To: Bobby Holley (busy)
:
Mentors:
Depends on: 779122
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-14 07:30 PDT by Simon Kornblith
Modified: 2012-08-09 11:30 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
verified


Attachments
Part 1 - Add some asserts in the brain transplant code. v1 (2.79 KB, patch)
2012-07-20 05:51 PDT, Bobby Holley (busy)
wmccloskey: review+
Details | Diff | Review
Part 2 - Refactor Xray waiving logic to allow simple lookups in the waiver map without creating a waiver. v1 (6.36 KB, patch)
2012-07-20 05:51 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
Part 3 - Fix up waivers after transplanting. v1 (6.60 KB, patch)
2012-07-20 05:57 PDT, Bobby Holley (busy)
mrbkap: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
Part 4 - Introduce Cu.recomputeWrappers. v1 (3.45 KB, patch)
2012-07-20 05:57 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
Part 5 - Tests. v1 (4.16 KB, patch)
2012-07-20 05:58 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review

Comment 1 Simon Kornblith 2012-07-14 07:40:40 PDT
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.
Comment 2 Simon Kornblith 2012-07-14 07:55:50 PDT
First occurred with yesterday's (7/13) Nightly. Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=46804c31366b&tochange=6489be1890c0
Comment 3 Scoobidiver (away) 2012-07-14 12:07:27 PDT
It's likely a regression from bug 771908.
Comment 4 Eddy Bruel [:ejpbruel] 2012-07-14 13:23:12 PDT
(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 Scoobidiver (away) 2012-07-14 14:55:14 PDT
(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.
Comment 6 Bobby Holley (busy) 2012-07-15 15:44:36 PDT
Yes, this is a regression from bug 655649. I'll look into it.
Comment 7 Scoobidiver (away) 2012-07-19 00:09:53 PDT
It's #5 top browser crasher in 17.0a1.
Comment 8 Bobby Holley (busy) 2012-07-19 01:53:11 PDT
I am looking into it. We're somehow ending up with a DeadObjectProxy in the cross-compartment wrapper map.
Comment 9 Bobby Holley (busy) 2012-07-20 01:22:05 PDT
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.
Comment 10 Bobby Holley (busy) 2012-07-20 05:51:15 PDT
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.
Comment 11 Bobby Holley (busy) 2012-07-20 05:51:53 PDT
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
Comment 12 Bobby Holley (busy) 2012-07-20 05:57:09 PDT
Created attachment 644275 [details] [diff] [review]
Part 3 - Fix up waivers after transplanting. v1
Comment 13 Bobby Holley (busy) 2012-07-20 05:57:47 PDT
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).
Comment 14 Bobby Holley (busy) 2012-07-20 05:58:41 PDT
Created attachment 644277 [details] [diff] [review]
Part 5 - Tests. v1
Comment 15 Bill McCloskey (:billm) 2012-07-20 10:28:56 PDT
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 16 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-07-20 12:59:29 PDT
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 17 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-07-20 13:01:48 PDT
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 :-)
Comment 18 Bobby Holley (busy) 2012-07-23 02:59:40 PDT
(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.
Comment 19 Bobby Holley (busy) 2012-07-23 03:05:12 PDT
(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 20 Bobby Holley (busy) 2012-07-23 04:46:07 PDT
Pushed this to try: https://mail.google.com/mail/?ui=2&view=bsp&ver=ohhl4rw8mbn4
Comment 22 Bobby Holley (busy) 2012-07-23 09:00:12 PDT
and a windows linkage bustage fix:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4c9ee7535383
Comment 24 Bobby Holley (busy) 2012-07-27 03:20:19 PDT
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
Comment 25 Bobby Holley (busy) 2012-07-27 03:21:12 PDT
Also, Scoobidiver, can you verify that this fixed the topcrash?
Comment 26 Scoobidiver (away) 2012-07-27 03:31:00 PDT
(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.
Comment 27 Alex Keybl [:akeybl] 2012-07-27 16:52:06 PDT
(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 28 Alex Keybl [:akeybl] 2012-07-27 16:52:39 PDT
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.
Comment 30 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-08-09 11:30:36 PDT
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. :)

Note You need to log in before you can comment on or make changes to this bug.