Closed Bug 377423 Opened 13 years ago Closed 13 years ago

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

Categories

(Core Graveyard :: RDF, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: enndeakin)

References

Details

(4 keywords)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
I get this about 10 times a day.
Flags: blocking1.9?
Keywords: dogfood, topcrash
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".
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.
Assignee: dom-to-text → peterv
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.
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.

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.
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.
Assignee: peterv → enndeakin
Status: NEW → ASSIGNED
Attachment #262647 - Flags: superreview?(jonas)
Attachment #262647 - Flags: review?(peterv)
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 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?
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.
Attached patch update patchSplinter Review
bsmedberg said it was ok, but to watch to see if new shutdown crashes occur on mac
Attachment #262647 - Attachment is obsolete: true
Attachment #262759 - Flags: review?(peterv)
Attachment #262647 - Flags: superreview?(jonas)
Attachment #262647 - Flags: review?(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.
Attachment #262759 - Flags: review?(peterv) → review+
Attachment #262759 - Flags: superreview?(jonas)
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.
Attachment #262759 - Flags: superreview?(jonas) → superreview+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Duplicate of this bug: 377402
Crash Signature: [@ MemoryElementSet::List::~List]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.