Last Comment Bug 763381 - GC Hazard with new Xray Expando Architecture (Jetpack compartment mismatch)
: GC Hazard with new Xray Expando Architecture (Jetpack compartment mismatch)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: x86_64 Windows 8
: P1 normal with 1 vote (vote)
: mozilla16
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 763818 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-10 23:16 PDT by Wes Kocher (:KWierso)
Modified: 2012-07-23 23:41 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Part 1 - Pass cx around in more places. v1 (6.52 KB, patch)
2012-06-12 06:38 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Part 2 - Fix up compartment situation for expando objects. v1 (5.56 KB, patch)
2012-06-12 06:39 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Wes Kocher (:KWierso) 2012-06-10 23:16:15 PDT
Starting with this push ( https://tbpl.mozilla.org/?noignore=1&tree=Mozilla-Inbound&rev=828da55601e7 )on inbound, all Jetpack test runs on debug builds are failing with a timeout error.

Assertion failure: (obj2)->compartment()->isCollecting(), at ../../../js/src/gc/Marking.cpp:1142
Traceback (most recent call last):
  File "bin/cfx", line 33, in <module>
    cuddlefish.run()
  File "/home/cltbld/talos-slave/test/build/addon-sdk-07137b290b6d/python-lib/cuddlefish/__init__.py", line 517, in run
    test_all(env_root, defaults=options.__dict__)
  File "/home/cltbld/talos-slave/test/build/addon-sdk-07137b290b6d/python-lib/cuddlefish/__init__.py", line 345, in test_all
    test_all_packages(env_root, defaults)
  File "/home/cltbld/talos-slave/test/build/addon-sdk-07137b290b6d/python-lib/cuddlefish/__init__.py", line 408, in test_all_packages
    env_root=env_root)
  File "/home/cltbld/talos-slave/test/build/addon-sdk-07137b290b6d/python-lib/cuddlefish/__init__.py", line 786, in run
    mobile_app_name=options.mobile_app_name)
  File "/home/cltbld/talos-slave/test/build/addon-sdk-07137b290b6d/python-lib/cuddlefish/runner.py", line 675, in run_app
    OUTPUT_TIMEOUT)
Exception: Test output exceeded timeout (60s).


There have been two pushes to inbound since that one, and they both have all debug builds failing:
https://tbpl.mozilla.org/?noignore=1&tree=Mozilla-Inbound&jobname=jetpack&rev=61fd66629c4f
https://tbpl.mozilla.org/?noignore=1&tree=Mozilla-Inbound&jobname=jetpack&rev=4c34e1a4cbb5
Comment 1 Bill McCloskey (:billm) 2012-06-11 10:28:39 PDT
How do I run these tests myself? Tinderbox is running a script called run_jetpack.py, but I don't have that in my tree.
Comment 2 Wes Kocher (:KWierso) 2012-06-11 12:05:23 PDT
(In reply to Bill McCloskey (:billm) from comment #1)
> How do I run these tests myself? Tinderbox is running a script called
> run_jetpack.py, but I don't have that in my tree.

You can download the SDK source from here: http://hg.mozilla.org/projects/addon-sdk/archive/07137b290b6d.tar.bz2 and unpack it somewhere. 

Go into the unpacked directory and then do the following on the command line (assuming you're on Linux or Mac):
source bin/activate
cfx testall


This isn't exactly what Tinderbox does, but I think it'd be about the same end result.

When you're done you can just type | deactivate | to get rid of the extra stuff added to the command line.
Comment 3 Wes Kocher (:KWierso) 2012-06-11 13:42:25 PDT
Ooh, you'd probably need to do "cfx testall -b /path/to/debug/binary" to get it to actually use the debug build.
Comment 4 Bill McCloskey (:billm) 2012-06-11 15:10:36 PDT
Bobby, could you take a look at this? It looks like a regression from bug 758415.

When I run the test, there's XrayExpandoObject pointing at a Sandbox object in a different compartment. It looks like it's via the JSSLOT_EXPANDO_EXCLUSIVE_GLOBAL slot. I assume this is intentional, but nevertheless it's a problem for the GC.

If you can guarantee that the value in the JSSLOT_EXPANDO_EXCLUSIVE_GLOBAL slot will outlive the XrayExpandoObject, then you could put a PrivateValue rather than an ObjectValue in the slot. You just have to worry about what might happen if the old exclusive global is freed and something else is allocated in its place--then you might get false matches.

The alternative is to wrap the global before sticking it in there.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-06-12 06:38:54 PDT
Created attachment 632229 [details] [diff] [review]
Part 1 - Pass cx around in more places. v1
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-06-12 06:39:10 PDT
Created attachment 632230 [details] [diff] [review]
Part 2 - Fix up compartment situation for expando objects. v1
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2012-06-12 07:06:34 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9ca60839bec3

Sigh, sorry for making such a dumb mistake, bill.

I can't seem to reproduce this crash locally. Can you double-check that the patches here fix the problem?
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-06-12 07:08:44 PDT
(assuming you can easily reproduce it yourself. We can also check the results of the try push with jetpack unhidden: https://tbpl.mozilla.org/?tree=Try&rev=9ca60839bec3&noignore=1 )
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2012-06-12 10:16:21 PDT
MOZ_ASSERT_IF is nice, btw.
Comment 10 Bill McCloskey (:billm) 2012-06-12 11:42:37 PDT
The JP tests are green, so I don't think there's any need to test locally. Thanks, Bobby.
Comment 11 Bill McCloskey (:billm) 2012-06-15 10:30:40 PDT
It seems likely that this is causing bug 763818. Can you get to this soon, Blake?
Comment 12 Blake Kaplan (:mrbkap) 2012-06-15 14:08:10 PDT
Comment on attachment 632230 [details] [diff] [review]
Part 2 - Fix up compartment situation for expando objects. v1

Review of attachment 632230 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +134,5 @@
> +    // work needs to happen there.
> +    JSAutoEnterCompartment ac;
> +    if (!ac.enter(cx, target) ||
> +        !JS_WrapObject(cx, &exclusiveGlobal))
> +        return NULL;

Nit: need braces around the consequent here.
Comment 15 :Ms2ger (⌚ UTC+1/+2) 2012-06-19 06:36:26 PDT
Comment on attachment 632230 [details] [diff] [review]
Part 2 - Fix up compartment situation for expando objects. v1

Review of attachment 632230 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +212,5 @@
>  {
> +    // Expando objects live in the target compartment.
> +    JSAutoEnterCompartment ac;
> +    if (!ac.enter(cx, target))
> +        return false;

false ain't no JSObject*...
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 14:58:32 PDT
Comment on attachment 632229 [details] [diff] [review]
Part 1 - Pass cx around in more places. v1

Nominating per bug 758415 comment 56.
Comment 17 Alex Keybl [:akeybl] 2012-06-19 20:19:09 PDT
Comment on attachment 632229 [details] [diff] [review]
Part 1 - Pass cx around in more places. v1

[Triage Comment]
In support of a tracked bug, approved for Aurora 15.
Comment 18 Bill McCloskey (:billm) 2012-06-20 13:07:03 PDT
*** Bug 763818 has been marked as a duplicate of this bug. ***

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