Closed
Bug 739674
Opened 12 years ago
Closed 12 years ago
test_tmpl_storage_bad_parameters_2.xul (and others) don't close their database
Categories
(Core :: XUL, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: espindola, Assigned: espindola)
References
Details
Attachments
(2 files, 5 obsolete files)
5.16 KB,
patch
|
Details | Diff | Splinter Review | |
4.07 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
This is similar to bug 739656, but in this case the database is created implicitly, so it is harder to know how to close it.
Assignee | ||
Updated•12 years ago
|
Summary: est_tmpl_storage_bad_parameters_2.xul (and others) don't close their database → test_tmpl_storage_bad_parameters_2.xul (and others) don't close their database
Assignee | ||
Comment 1•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=81cbb2d8c27a
Assignee | ||
Comment 2•12 years ago
|
||
This is just a hack patch I used it to reproduce the windows failure. It is a useful debugging hack to replace a T* with at nsCOMPtr<T> just to check the refcount.
Assignee | ||
Comment 3•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=62dd7535e3bc
Attachment #610537 -
Attachment is obsolete: true
Attachment #610537 -
Flags: review?
Attachment #610661 -
Flags: review?(enndeakin)
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #610661 -
Attachment is obsolete: true
Attachment #610661 -
Flags: review?(enndeakin)
Attachment #610662 -
Flags: review?(enndeakin)
Comment 5•12 years ago
|
||
Comment on attachment 610662 [details] [diff] [review] close connection You may also want to clear mQueryProcessor at the end of nsXULTemplateBuilder::ContentRemoved as it calls Uninit(false) Just to be safe, can you also add a null check (returning 0) to nsXULTreeBuilder::CompareResults before mQueryProcessor is accessed as that can indirectly be called by script.
Attachment #610662 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 6•12 years ago
|
||
Would you mind checking if I got the code review right? https://tbpl.mozilla.org/?tree=Try&rev=c7ec73bf07e1
Attachment #610662 -
Attachment is obsolete: true
Attachment #610894 -
Flags: review?(enndeakin)
Updated•12 years ago
|
Attachment #610894 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 7•12 years ago
|
||
Looks like try found a problem. Trying the hack xpcom hack as before.
Comment 8•12 years ago
|
||
The crash stack suggests that the query processor is being accessed by the call to UninitFalse from nsXULTemplateBuilder::ContentRemoved after the query processor has been set to null. The issue is that the script runner delays calling UninitFalse because it isn't safe to run script. It may be possible to just add a variation of UninitFalse which clears mQueryProcessor afterwards or just remove that change again.
Assignee | ||
Comment 9•12 years ago
|
||
What is happening is that nsINode::doRemoveChildAt looks like if (aNotify) { nsNodeUtils::ContentRemoved(this, aKid, aIndex, previousSibling); } aKid->UnbindFromTree(); On the ContentRemoved call we get to nsXULTemplateBuilder::ContentRemoved which in the new patch sets mQueryProcessor to null. On the call to UnbindFromTree we get to nsXULTemplateBuilder::Uninit which calls mMatchMap.Enumerate(DestroyMatchList, &mPool); which will end up deleting a nsRDFQuery whose destructor will access the deleted nsXULTemplateQueryProcessorRDF. The two simple fixes I see are * Don't set mQueryProcessor to null in nsXULTemplateBuilder::ContentRemoved (i.e., go with the old patch). * Run mMatchMap.Enumerate(DestroyMatchList, &mPool) in nsXULTemplateBuilder::ContentRemoved. (can we do that?)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #610660 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 years ago
|
||
I did a try push to https://tbpl.mozilla.org/?tree=Try&rev=cc011e039cc2 and it looks green. I will remove the debug bits and send for review.
Assignee | ||
Comment 12•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f9fddcb2f788
Attachment #610894 -
Attachment is obsolete: true
Attachment #611854 -
Flags: review?(enndeakin)
Comment 13•12 years ago
|
||
Comment on attachment 611854 [details] [diff] [review] close connection Seems ok, although I wonder why the script runner was necessary.
Attachment #611854 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 14•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=5d82421a984d
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5d82421a984d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in
before you can comment on or make changes to this bug.
Description
•