415028 - Startup assertions and crash compiling proto properties with gczeal == 2

Status: VERIFIED FIXED

(Core :: XBL, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

The problem is that we end up doing a GC trace while compiling a property, trace into the nsXBLProtoImpl, that has an mClassObject already set, so we go ahead and trace into stuff.. but said stuff is uncompiled (asserts fire in a debug build), so we treat random PRUnichar*s as void* gcthings, and bad stuff happens.

Fix coming up.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is more annoying than I thought, but Peter has graciously offered to take it.  From our IRC conversation:

> here's the problem
> XBL has a bunch of members
> it goes through and compiles them
> While it's doing this, some are compiled, some are not
> So there are basically 3 states:
> 1)  None compiled
> 2)  Some compiled
> 3)  All compiled
<peterv> 'k
> Right now there is a boolean switch that decides whether we trace
> that is "false" in state 1 and "true" in state 2 & 3
> peterv: so states 1 and 3 are ok
> peterv: but in state 2, if we trace we crash
> peterv: because there are these unions conditioned on the same boolean
> peterv: that can point to PRUnichar* or JSObject*
<peterv> bz: oh, not good
> peterv: apparently we don't GC much in state 2 usually.  But with
          gczeal == 2, we of course do
<peterv> bz: ok, please file a bug then :-)
> peterv: my initial reaction was to make the boolean "true" in state 3 and
          false in 1,2
> peterv: but then I get the above assert, since we collect the stuff during that state 2 gc
<peterv> yes
> peterv: and then the next gc we trace it and die
> bug's already filed
<peterv> so that won't work :-)
<peterv> ok
> 415028
> You want it?
<peterv> I can look at it tomorrow if you want
<peterv> sure
* bz was gonna do it until he realized the annoyance
> ok, I'll reassign
<peterv> unless you have a fix :-D
> I don't
> I'll copy/paste this in the bug...
<peterv> thanks
> we might just want to make mCompiled a non-debug member or something.  :(
> or tag pointers, or some such evil
<peterv> I can do evil

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 3

[Peter Van der Beken [:peterv]]

I wonder if it would make sense to invert the setting of the bit, to set it in mUncompiledMethod as long as the method is not compiled. Accessing mUncompiledMethod should be the less common case.

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v2

I think I like this better.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 302474 [details] [diff] [review]
v2

>Index: content/xbl/src/nsXBLProtoImplMethod.h
>+    return (nsXBLUncompiledMethod*)(mUncompiledMethod & ~BIT_UNCOMPILED);

reinterpret_cast?

>Index: content/xbl/src/nsXBLProtoImplProperty.cpp
>@@ -334,20 +319,18 @@ nsXBLProtoImplProperty::CompileMember(ns
>-  
>+

Let the whitespace be, please.

>+  if (mJSAttributes & JSPROP_GETTER != 0) {

This is wrong due to the operator precedence mess... Just lose the "!= 0" part?

>+  if (mJSAttributes & JSPROP_SETTER != 0) {

Same here.

With those nits, looks pretty good...

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Comment 7

[timeless]

Comment on attachment 302540 [details] [diff] [review]
v2.1

unless i'm missing something this is LLP64 (=w64) unsafe.

http://blogs.msdn.com/oldnewthing/archive/2005/01/31/363790.aspx

+typedef unsigned long PtrBits;
+    mUncompiledMethod = PtrBits(aUncompiledMethod) | BIT_UNCOMPILED;

neil suggests this is a nice instant crash on w64 :)

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 302540 [details] [diff] [review]
v2.1

I'll use PRUptrdiff, but it's mostly irrelevant because there's a dozen or so other places that'll do exactly the same thing.

Comment 9

[Peter Van der Beken [:peterv]]

Hopefully this will cause the number of crashes in js_GetGCThingTraceKind to go down, most seemed to come from nsXBLProtoImplMethod::Trace.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Verified that I no longer die in this code.  I'll file more bugs for the places I do die.  :(