614347 - XPConnect-wrapped JSObjects must clear their gray bit when they are handed out

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Now that CC and GC are split we need to make sure that the JSObject colors stay in sync if we hand out a gray object after GC. I fixed XPCWrappedNative, but peterv believes that we should be doing something similar with tearoffs and XPCWrappedJS too.

Comment 1

[Johnny Stenback (:jst)]

We need this before release to possibly avoid JS objects being collected too early. Marking this a regression, which it sort of is, though not strictly. This is needed because of the gc/cc split work that's already been done.

Comment 2

[Johnny Stenback (:jst)]

Ben, you'll get first chops at this one, but if you think peterv needs to own this, feel free to move it to him.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: XPCWrappedJS changes

Comment 4

[Johnny Stenback (:jst)]

This doesn't need a beta.

Comment 5

[Johnny Stenback (:jst)]

Downgrading this to a softblocker per discussion with bent.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Based on the stack traces in bug 630517 and bug 630519, those might
depend on this one.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

...and if that is the case, this should be a hardblocker.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Other option is to back out the changes to cycle collector scheduling.

Comment 9

[Johnny Stenback (:jst)]

Marking hardblocker per smaug's observations.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #8)
> Other option is to back out the changes to cycle collector scheduling.

No, let's fix this bug.

/be

Comment 11

[Andreas Gal :gal]

This always smelled like a hardblocker to me.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

This fixes the bug for real. Andreas and I realized that we were doing this wrong before - we don't want to make sure the object is marked black, rather that the gray bit is cleared. Marking black in some cases leads to crashes, and leaving the gray bit set will allow the cycle collector to collect the object when it shouldn't.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

I was going to push the patch to tryserver, but it doesn't apply cleanly to trunk.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #13)
> I was going to push the patch to tryserver, but it doesn't apply cleanly to
> trunk.

I did already. There are some build errors, but mochitests look ok on mac. I'll see if I can figure those our today.

Comment 15

[Peter Van der Beken [:peterv]]

New patch that fixes the build errors?

Also, don't you need to either do this recursively to everything traced by that JSObject or change nsXPConnect::Traverse, given that nsXPConnect::Traverse doesn't traverse objects that don't have the gray bit set. It's a performance optimisation in nsXPConnect::Traverse to avoid retraversing all JS objects that the last GC already traversed (traced) to mark them black. Anything hanging off of them should be black anyway, and not recording edges from those JS objects to C++ should just lead to missing refcounts (and black C++ objects). But given that we now can run JS between the GC and the CC I suppose we really need to traverse, in case some of the edges have changed (object hanging off of a gray object, so gray itself, now hanging off of a black object). It'll probably make CC a lot slower again :-(.

Comment 16

[Peter Van der Beken [:peterv]]

Patch looks ok otherwise, though probably need to comment next to the various JSObject members that they should only be accessed through one of the getters. Don't think we can easily enforce that :-(.

Comment 17

[Peter Van der Beken [:peterv]]

Comment on attachment 508951 [details] [diff] [review]
Patch, v1

-ing for now, given comment 15.

Comment 18

[Andreas Gal :gal]

Not traversing means we can't find a cycle there. Shouldn't it always be safe to not traverse? We marked one member of the cycle black so we will never find any cycle with that member in it.

Comment 19

[Peter Van der Beken [:peterv]]

Not for GC-ed objects, think about a cycle that's held with an additional edge from an XPCWrappedNative's JS object. The XPCWrappedNative's JS object and the JS objects in the cycle are gray. Now you mark the XPCWrappedNative's JS object non-gray. If we don't traverse then the CC won't know about the edge from the XPCWrappedNative's JS object to the cycle, and so we'll happily collect that cycle even though it's still held by an object that's not collected.

Comment 20

[Peter Van der Beken [:peterv]]

The GC will mark all the JS objects, but we will unlink any XPCOM objects in the cycle or held by it.

Comment 21

[Andreas Gal :gal]

I see. We only unset the gray bit on the suspected object. Whe we Traverse we can skip past that even if its gray but then stop along any path that is not gray? That should preserve the shortcut but still find that link. No?

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

This implements our latest plan, but something is leaking...

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2.1

Fixed the leak! I was getting the IsGray check backwards :(

I think this is ready to go, I've pushed to try as well so fingers crossed!
http://tbpl.mozilla.org/?tree=MozillaTry&rev=450475da8eb7

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Bingo. Tryserver is happy.

Comment 25

[Steve Fink [:sfink] [:s:]]

I applied to v2.1 to my tracemonkey tree. I had one compile problem -- xpcjsruntime.cpp still had a reference to GetFlatJSObjectAndMark() that I replaced with GetFlatJSObject(). I am seeing a number of occurrences of this assert with only this patch + the fix applied:

  SuspectWrappedNative attempting to touch dead object

Every one I've looked at have been Window objects.

I see it on 3 different profiles, one with no extensions. I am rebuilding now without this patch to check if I get the asserts without it.

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Wha?! Tryserver like it just fine... Did it rot already?

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #25)
>   SuspectWrappedNative attempting to touch dead object

Wait, I removed that assertion. Did you do a full rebuild?

Comment 28

[Steve Fink [:sfink] [:s:]]

(In reply to comment #27)
> (In reply to comment #25)
> >   SuspectWrappedNative attempting to touch dead object
> 
> Wait, I removed that assertion. Did you do a full rebuild?

Yes. (I had to for it to compile.)

You removed that assertion in this patch? I don't see that in the v2.1 attached to this bug, nor in the version you pushed to try. (The version pushed to try seems to match v2.1 at least for that file.)

Maybe it's a tracemonkey vs m-c difference?

Comment 29

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Must be a m-c vs. tm difference. This patch is meant to apply to tm.

Comment 30

[Steve Fink [:sfink] [:s:]]

I'm using tm.

Your push to try had a parent of db8be4e3f373. The assert was added later, in e039358204f5. As was the code that doesn't compile for me.

Yer bits are rotten, sir.

Comment 31

[Steve Fink [:sfink] [:s:]]

Err... or maybe I'm full of it. e039358204f5 is from Feb 2, db8be4e3f373 from Feb 4. Let me look again.

Comment 32

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2.2

Unrotted.

Comment 33

[Andreas Gal :gal]

Comment on attachment 510484 [details] [diff] [review]
Patch, v2.2

Steve, can you test this?

Comment 34

[Johnny Stenback (:jst)]

With v2.2 I get:

../../../../../mozilla/js/src/xpconnect/wrappers/../src/xpcinlines.h: In member function ‘void XPCLazyCallContext::SetWrapper(XPCWrappedNative*, XPCWrappedNativeTearOff*)’:
../../../../../mozilla/js/src/xpconnect/wrappers/../src/xpcinlines.h:794:40: error: ‘class XPCWrappedNative’ has no member named ‘GetFlatJSObjectAndMark’

Comment 35

[Johnny Stenback (:jst)]

Er, nm, that's due to another conflicting patch in my queue.

Comment 36

[Steve Fink [:sfink] [:s:]]

(In reply to comment #33)
> Comment on attachment 510484 [details] [diff] [review]
> Patch, v2.2
> 
> Steve, can you test this?

I can verify that it applies, compiles, and no longer shows the assert. The last doesn't mean much, though, since the assert is removed as part of the patch. :-) I can't speak for whether it works or not; it does not resolve the problem for which I was applying it, but that was just a shot in the dark anyway.

Comment 37

[Peter Van der Beken [:peterv]]

Comment on attachment 510484 [details] [diff] [review]
Patch, v2.2

Could we move the XPC_GC_COLOR_* to xpcpublic.h?
Could we move nsWrapperCache::GetWrapper to xpcpublic.h? If not, please s/nsWrapperCache-inl.h/nsWrapperCacheInlines.h/.
Please add comments to the *PreserveColor functions to explain how they're different from the non-PreserveColor variant: that they don't make sure that the object returned will be kept alive past the next CC/GC and they should only be used if we are certain that the return value won't be passed into a JS API function or stored without rooting it or without signaling the stored value to the CC.

Comment 38

[Andreas Gal :gal]

Good call on beta+.

Comment 39

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This bounced off tracemonkey, something in just the debug mochitest-chrome failed.

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1297227039.1297230992.22103.gz&fulltext=1#err2

JavaScript strict warning: chrome://browser/content/tabview.js, line 418: reference to undefined property this[0]
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_apptabs.js | Console message: [JavaScript Warning: "Error in parsing value for '-moz-column-count'.  Declaration dropped." {file: "chrome://browser/content/tabview.html" line: 0}]
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_apptabs.js | Console message: [JavaScript Warning: "WARN addons.manager: Exception calling callback: TypeError: PREF_GETADDONS_CACHE_ENABLED is undefined" {file: "chrome://mozapps/content/extensions/extensions.js" line: 1657}]
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_apptabs.js | Console message: trace: tabview assert: does not yet support multi-objects (or null objects)
iQClass_width()@chrome://browser/content/tabview.js:392
GroupItem_getContentBounds()@chrome://browser/content/tabview.js:2607
GroupItem_arrange([object Object])@chrome://browser/content/tabview.js:3384
GroupItem_reorderTabItemsBasedOnTabOrder()@chrome://browser/content/tabview.js:3853
([object Object],0,[object Array])@chrome://browser/content/tabview.js:7532
UI_showTabView(true)@chrome://browser/content/tabview.js:7531
([object Event])@chrome://browser/content/tabview.js:7217
()@chrome://browser/content/browser.js:4879
TabView__initFrame((function () {var event = document.createEvent("Events");event.initEvent("tabviewshow", false, false);dispatchEvent(event);}))@chrome://browser/content/browser.js:4833
()@chrome://browser/content/browser.js:4876
()@chrome://browser/content/browser.js:4898
test()@chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_apptabs.js:43
Tester_execTest()@chrome://mochikit/content/browser-test.js:235
Test_realNextTest()@chrome://mochikit/content/browser-test.js:196
([object Proxy],29)@chrome://mochikit/content/browser-test.js:110
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_apptabs.js | Console message: [JavaScript Warning: "reference to undefined property this[0]" {file: "chrome://browser/content/tabview.js" line: 418}]
NEXT ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_apptabs.js | Test timed out

Comment 40

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Er, mochitest-browser-chrome.

Comment 41

[mdew]

Did this patch cause a regression? http://arewefastyet.com/ seemed to have jumped up 600ms due to this patch

Comment 42

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #41)
> Did this patch cause a regression? http://arewefastyet.com/ seemed to have
> jumped up 600ms due to this patch

It was backed out, so I have no idea why the numbers moved.

Comment 43

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2.3

This fixes XPCVariant and the test failures.

Comment 44

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 511119 [details] [diff] [review]
Patch, v2.3

This bounced again yesterday. Something is wrong, the variant thing seems to be a red herring.

Comment 45

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v2.4

This patch fixes XPCWrappedNative's "wrapper wrapper" (BLEGH!) and the expando maps too, but it still doesn't fix the tabcandy mochitests. We're missing something still.

Comment 46

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/bfadd8743281
http://hg.mozilla.org/mozilla-central/rev/050d216ec2a2
http://hg.mozilla.org/mozilla-central/rev/42063595d186 (backout)
http://hg.mozilla.org/mozilla-central/rev/10ebc5ea11ac
http://hg.mozilla.org/mozilla-central/rev/c64656593906 (backout)
Note: not marking as fixed because last changeset is a backout.

Comment 47

[Sheila Mooney]

What's the status on this? If this indeed fixes bug 630517, it fixes the #3 crash on Beta11.

Comment 48

[Andreas Gal :gal]

No good progress, we still have an undiagnosed problem here. I will confirm with bent and come up with an emergency plan if we can't fix this in time.

Comment 49

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We actually have a much better idea of what's going wrong, patch waiting on tryserver now.

Comment 50

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Sad news, http://hg.mozilla.org/try/rev/22cb19dd492f failed again. Next idea is to recursively un-gray objects.

Comment 51

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v3

Woo! This one passed the tryserver for the tabcandy tests that were failing before. Full tryserver run is in progress, http://hg.mozilla.org/try/rev/ae0c55d09422 but assuming that passes let's get some eyes on this.

Now we ungray all objects traced from the object we're handing out. Dbaron pointed out a hole in our logic before so I think we have to do this. We're also correctly telling the CC about black objects now... Before we were claiming that the black objects were GCUnmarked :-(

Comment 52

[Peter Van der Beken [:peterv]]

(In reply to comment #51)
> Dbaron pointed out a hole in our logic before

What's the hole?

Comment 53

[Ben Turner (not reading bugmail, use the needinfo flag!)]

What's that?! You want ASCII art?!

Basically we have a cycle of objects exactly like you describe in comment 19, something like this (where A, B, and C are all XPConnect roots with their own JSObjects, and D is some JS object found from JS roots):

  XPConnect roots:    JS roots:

  A -> B ---+         D
       ^    |
       |    |
       C <--+

(trying to show A owns B, B owns C, C owns B, D is off in its own happy world. A, B, and C are gray, D is black.)

If we hand out the JSObject for A (turning it black, of course) then script could do anything it wants with it, like doing several GetProperty calls and finding the JSObject for C. If it then defines that JSObject for C as a new property on D, and then drops the link from A to B, we have created a new owning reference reachable only through JS roots:

  XPConnect roots:    JS roots:

  A    B ---+         D
       ^    |         |
       |    |         |
       C <--+         |
       ^              |
       |              |
       +--------------+

So now the cycle collector will see B and C as collectable. The only way to prevent this is to mark B and C black when we hand out A.

Comment 54

[Andreas Gal :gal]

Comment on attachment 512704 [details] [diff] [review]
Patch, v3


> #ifdef DEBUG
>     if (!mWillReparent) {
>       // We really shouldn't have a wrapper here but if we do we need to make sure
>       // it has the correct parent.
>-      JSObject *obj = GetWrapper();
>+      JSObject *obj = GetWrapperPreserveColor();

We preserve the color here because its DEBUG right?

>+// static
>+void
>+nsXPConnect::UnmarkGrayInternal(void *thing)
>+{

XXXInternal, MyXXX, DoXXX are really bad names. How about RecursiveUnmarkGrayObject?

>+    // Remove the gray color from the given GCThing and any other GCThings that
>+    // can be reached through it.
>+    static void UnmarkGray(void *thing)
>+        {if(thing && nsXPConnect::IsGray(thing)) UnmarkGrayInternal(thing);}
>+

thing is really always an object, so we could also type it at such.

Comment 55

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v3.1

Fixed a crash, and addressed gal's comments. Yeah, DEBUG only code shouldn't change color imo, otherwise we'd get weird behavioral differences maybe.

Comment 56

[Ben Turner (not reading bugmail, use the needinfo flag!)]

New tryserver run, http://hg.mozilla.org/try/rev/05d49ce3152b

Comment 57

[Brendan Eich [:brendan]]

Awesome work, bent et al.!

/be

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 512718 [details] [diff] [review]
Patch, v3.1

I tested builds (64bit opt Linux builds which have Bug 630932) with and without this patch.
I didn't see any major changes.
I tested FF startup, Google Maps, Gmail, BBC news opened in 78 tabs,
closing those tabs, the testcase for bug 631494 (with and without ABP)
and then also on OSX 10.5 FOTN.

I think there was a small CC perf regression noticeable in the case when 78 tabs were opened.
Few % or so. But could be just noise.

Comment 59

[Peter Van der Beken [:peterv]]

Comment on attachment 512718 [details] [diff] [review]
Patch, v3.1

>diff --git a/dom/base/nsDOMClassInfo.h b/dom/base/nsDOMClassInfo.h

>   static nsresult WrapNative(JSContext *cx, JSObject *scope,
>                              nsISupports *native, nsWrapperCache *cache,
>                              const nsIID* aIID, jsval *vp,
>                              nsIXPConnectJSObjectHolder** aHolder,
>-                             PRBool aAllowWrapping)
>-  {
>-    if (!native) {

This is annoying, IIRC inlining was a performance improvement here.

>diff --git a/dom/base/nsWrapperCache.cpp b/dom/base/nsWrapperCache.cpp

>+JSObject*
>+nsWrapperCache::GetWrapper() const

Same here :-(.

>diff --git a/dom/base/nsWrapperCache.h b/dom/base/nsWrapperCache.h

>+  /**
>+   * This getter clears the gray bit before handing out the JSObject which means
>+   * that the object is guaranteed to be kept alive past the next CC.
>+   *
>+   * Implemented in xpcpublic.h because we have to include some JS headers that
>+   * don't play nicely with the rest of the codebase. Include xpcpublic.h if you
>+   * need to call this method.

That's not correct anymore. Can you now remove a bunch of xpcpublic.h includes that you added?

>diff --git a/js/src/xpconnect/src/nsXPConnect.cpp b/js/src/xpconnect/src/nsXPConnect.cpp

>@@ -675,17 +720,17 @@ nsXPConnect::Traverse(void *p, nsCycleCo
>             PL_DHashTableOperate(&mJSRoots, p, PL_DHASH_LOOKUP);
>         type = markJSObject || PL_DHASH_ENTRY_IS_BUSY(entry) ? GCMarked :
>                                                                GCUnmarked;
>     }
>     else
> #endif
>     {
>         // Normal codepath (matches non-DEBUG_CC codepath).
>-        type = !markJSObject && IsGray(p) ? GCUnmarked : GCMarked;
>+        type = !markJSObject && IsGray(obj) ? GCUnmarked : GCMarked;

This change is wrong (why is it even needed?), obj might be uninitialized.

>@@ -756,31 +801,24 @@ nsXPConnect::Traverse(void *p, nsCycleCo

>-    // There's no need to trace objects that have already been marked by the JS
>-    // GC. Any JS objects hanging from them will already be marked. Only do this
>-    // if DEBUG_CC is not defined, else we do want to know about all JS objects
>-    // to get better graphs and explanations.
>-    if(!cb.WantAllTraces() && type == GCMarked)
>-        return NS_OK;

Why do we still need this change, given that we're now doing the recursive ungraying in the getters?

>diff --git a/js/src/xpconnect/src/xpcjsruntime.cpp b/js/src/xpconnect/src/xpcjsruntime.cpp

>@@ -498,28 +500,22 @@ XPCJSRuntime::SuspectWrappedNative(JSCon

>-    // Only record objects that might be part of a cycle as roots, unless
>-    // the callback wants all traces (a debug feature).
>-    if(nsXPConnect::IsGray(obj) || cb.WantAllTraces())
>-        cb.NoteRoot(nsIProgrammingLanguage::JAVASCRIPT, obj,
>-                    nsXPConnect::GetXPConnect());
>+    cb.NoteRoot(nsIProgrammingLanguage::JAVASCRIPT, obj,
>+                nsXPConnect::GetXPConnect());

Why do we still need this change, given that we're now doing the recursive ungraying in the getters?

>diff --git a/js/src/xpconnect/src/xpcwrappednativescope.cpp b/js/src/xpconnect/src/xpcwrappednativescope.cpp

> static JSDHashOperator
> WrappedNativeSuspecter(JSDHashTable *table, JSDHashEntryHdr *hdr,
>                        uint32 number, void *arg)
> {
>     XPCWrappedNative* wrapper = ((Native2WrappedNativeMap::Entry*)hdr)->value;
> 
>-    if(wrapper->HasExternalReference())
>+    if(wrapper->HasExternalReference() ||
>+       !nsXPConnect::IsGray(wrapper->GetFlatJSObjectPreserveColor()))

Why do we still need this change, given that we're now doing the recursive ungraying in the getters?

Comment 60

[Peter Van der Beken [:peterv]]

Also, UnmarkGrayChildren casts a void* potentially pointing to a JSXML object to JSObject*, how does that work?

Comment 61

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v3.2

Should address all comments.

Comment 62

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #58)
> I tested builds (64bit opt Linux builds which have Bug 630932) with and without
> this patch.
> I didn't see any major changes.

Thanks a ton smaug!

Comment 63

[Peter Van der Beken [:peterv]]

Comment on attachment 512840 [details] [diff] [review]
Patch v3.2

>diff --git a/js/src/xpconnect/src/nsXPConnect.cpp b/js/src/xpconnect/src/nsXPConnect.cpp

> static void
> NoteJSChild(JSTracer *trc, void *thing, uint32 kind)
> {
>+    TraversalTracer *tracer = static_cast<TraversalTracer*>(trc);
>+
>+    // There's no point in further traversing a non-gray object here unless we
>+    // explicitly want to see all traces.
>+    if(!xpc_IsGrayGCThing(thing) && !tracer->cb.WantAllTraces())
>+        return;
>+
>     if(ADD_TO_CC(kind))
>     {
>-        TraversalTracer *tracer = static_cast<TraversalTracer*>(trc);

You need to move the |if(!xpc_IsGrayGCThing(thing) && !tracer->cb.WantAllTraces())| inside this block.
Now it just needs to build on Windows.

Comment 64

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed that, and fixed the windows build.

http://hg.mozilla.org/tracemonkey/rev/14dda9fd9686

Comment 65

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/mozilla-central/rev/52246c1b1799

Comment 66

[Mike Beltzner [:beltzner, not reading bugmail]]

Restoring BetaN+ flag that seems to have been accidentally removed.