377423 - Shutdown crash [@ MemoryElementSet::List::~List]

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect)

Description

[Jesse Ruderman]

When I shut down Firefox (Intel Mac trunk debug), I get this crash about half of the time:

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0xdadadade

Thread 0 Crashed:
0   libgklayout.dylib   	0x15a22b14 MemoryElementSet::List::~List [in-charge]() + 60 (nsRDFConMemberTestNode.cpp:125)
1   libgklayout.dylib   	0x15a22b7d MemoryElementSet::List::Release() + 53 (nsRDFConMemberTestNode.cpp:132)
2   libgklayout.dylib   	0x15a22c5f MemoryElementSet::~MemoryElementSet [in-charge]() + 63 (nsRuleNetwork.h:158)
3   libgklayout.dylib   	0x15a22d33 Instantiation::~Instantiation [in-charge]() + 55 (nsRuleNetwork.h:469)
4   libgklayout.dylib   	0x1576e402 nsXULTemplateResultRDF::~nsXULTemplateResultRDF [in-charge]() + 54 (nsXULTemplateResultRDF.cpp:71)
5   libgklayout.dylib   	0x1576e538 nsXULTemplateResultRDF::Release() + 276 (nsXULTemplateResultRDF.cpp:52)
6   libxpcom_core.dylib 	0x013bbea8 nsCycleCollectionXPCOMRuntime::Unroot(nsDeque const&) + 60 (nsCycleCollector.cpp:539)
7   libxpcom_core.dylib 	0x0135b62d nsCycleCollector::CollectWhite() + 703 (nsCycleCollector.cpp:1050)
8   libxpcom_core.dylib 	0x0135c823 nsCycleCollector::Collect(unsigned) + 317 (nsCycleCollector.cpp:1702)
9   libxpcom_core.dylib 	0x0135c94f nsCycleCollector::Shutdown() + 65 (nsCycleCollector.cpp:1738)
10  libxpcom_core.dylib 	0x0135ca62 nsCycleCollector_shutdown() + 40 (nsCycleCollector.cpp:1928)
11  libxpcom_core.dylib 	0x012f998f NS_ShutdownXPCOM_P + 953 (nsXPComInit.cpp:780)
12  XUL                 	0x00207d41 ScopedXPCOMStartup::~ScopedXPCOMStartup [in-charge]() + 57 (nsAppRunner.cpp:798)
13  XUL                 	0x0020ed74 XRE_main + 5260 (nsAppRunner.cpp:2928)
14  org.mozilla.firefox 	0x00002eec main + 40 (nsBrowserApp.cpp:62)
15  org.mozilla.firefox 	0x00002852 _start + 216
16  org.mozilla.firefox 	0x00002779 start + 41

Comment 1

[Josh Aas]

I get this about 10 times a day.

Comment 2

[Graydon Hoare :graydon]

If I had to guess, judging from reading the code in that vicinity, I'd imagine there's some XUL template code that doesn't respect the lifecycle rules of class MemoryElement (explicit single-ownership) and we're only noting it now because we were leaking one of the erroneous multiple owners in the past, and have stopped leaking it. 

That's just a guess though. The code is hard to follow, I don't know why the author didn't just adopt the same shared ownership model in use everywhere else, given that there's no device to enforce single ownership.

In general I do not think this is "a cycle collection bug". I think this is "a bug in some class' ownership rules".

Comment 3

[Peter Van der Beken [:peterv]]

I get this occasionally but I haven't yet been able to reproduce this with refcount logging turned on. Any steps to reproduce this reliably would be appreciated.

Comment 4

[Graydon Hoare :graydon]

Further reading: the MemoryElementSet::Add method has a test for semantic equality of its argument against each existing list element. When that test succeeds, the method considers the Add operation redundant, and deletes its argument. It does not, however, test whether its argument is actually the same C++ object (memory-wise) as the existing entry; so it's possible that by deleting its argument it invalidates an owned pointer in the list, setting up the future destructor for a double-free error (which this crash sort of looks like).

Try commenting out the 'delete' call on line 87 of nsRuleNetwork.cpp, see if the crash goes away.

Comment 5

[Neil Deakin]

The only MemoryElements used are allocated from a pool, so they should be removed from the pool instead of being deleted. Not sure why that didn't crash before though.

Comment 6

[Peter Van der Beken [:peterv]]

Neil: could you take this one?

There seems to be something wrong with the ownership model. As the trace shows this ownership chain: nsXULTemplateResultRDF -> Instantiation -> MemoryElementSet -> MemoryElementSet::List is what keeps the MemoryElementSet::List alive. But I don't see who's supposed to make sure the pool isn't destroyed in that case (nsXULTemplateQueryProcessorRDF owns the pool), and nobody ever removes any of these objects from the pool.

Comment 7

[Neil Deakin]

Attachment: make sure the objects are removed from the pool properly

just make the pool a static member, and make sure the MemoryElement objects are deleted from the pool properly. I confirmed that the destructors are being called and the allocated count is 0 when exiting.

Comment 8

[timeless]

Comment on attachment 262647 [details] [diff] [review]
make sure the objects are removed from the pool properly

>+PRBool
>+MemoryElement::Init()
>+{
>+    if (!gPoolInited) {
>+        const size_t bucketsizes[] = {
>+            sizeof (nsRDFConMemberTestNode::Element),
>+            sizeof (nsRDFPropertyTestNode::Element)
>+        };
>+
>+        if (NS_FAILED(gPool.Init("MemoryElement", bucketsizes, 1, 256)))
>+            return PR_FALSE;
>+
>+        gPoolInited = PR_TRUE;
>+    }
>+
>+    return PR_TRUE;
>+}

>Index: content/xul/templates/src/nsXULTemplateQueryProcessorRDF.cpp
>@@ -203,16 +203,18 @@ nsXULTemplateQueryProcessorRDF::InitGlob
>+    MemoryElement::Init();

this should handle the false case...

>+
>     return NS_OK;
> }

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 262647 [details] [diff] [review]
make sure the objects are removed from the pool properly

>Index: content/xul/templates/src/nsRuleNetwork.cpp
>===================================================================

>@@ -65,31 +65,55 @@ extern PRLogModuleInfo* gXULTemplateLog;

>+MemoryElement::Init()
>+{
>+    if (!gPoolInited) {
>+        const size_t bucketsizes[] = {
>+            sizeof (nsRDFConMemberTestNode::Element),
>+            sizeof (nsRDFPropertyTestNode::Element)
>+        };
>+
>+        if (NS_FAILED(gPool.Init("MemoryElement", bucketsizes, 1, 256)))

Don't you want 2 bucket sizes?

>Index: content/xul/templates/src/nsXULTemplateQueryProcessorRDF.cpp
>===================================================================

>+    MemoryElement::Init();

Probably better to do (although nsFixedSizeAllocator::Init apparently can't really fail)

    return MemoryElement::Init() ? NS_OK : NS_ERROR_FAILURE;

>+
>     return NS_OK;

Could you run this past bsmedberg to make sure it's ok to have a static nsFixedSizeAllocator?

Comment 10

[Neil Deakin]

OK, I'll fix both of those issues.

There are several places where static nsFixedSizedAllocators are used already, nsINodeInfo::sNodeInfoPool for instance, so I assume this is OK, but I'll ask to make sure.

Comment 11

[Neil Deakin]

Attachment: update patch

bsmedberg said it was ok, but to watch to see if new shutdown crashes occur on mac

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 262759 [details] [diff] [review]
update patch

(In reply to comment #10)
> There are several places where static nsFixedSizedAllocators are used already,
> nsINodeInfo::sNodeInfoPool for instance, so I assume this is OK, but I'll ask
> to make sure.

Note that sNodeInfoPool is |static nsFixedSizeAllocator*|. I'm fine with this if it doesn't cause leak stats to go up.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 262759 [details] [diff] [review]
update patch

>Index: content/xul/templates/src/nsRuleNetwork.cpp
>+MemoryElement::Init()
>+{
>+    if (!gPoolInited) {
>+        const size_t bucketsizes[] = {
>+            sizeof (nsRDFConMemberTestNode::Element),
>+            sizeof (nsRDFPropertyTestNode::Element)
>+        };
>+
>+        if (NS_FAILED(gPool.Init("MemoryElement", bucketsizes, 2, 256)))

Use NS_ARRAY_LENGTH(bucketsizes) instead of 2

sr=me with that.