Last Comment Bug 739674 - test_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
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
: 739656 (view as bug list)
Depends on:
Blocks: 711076
  Show dependency treegraph
 
Reported: 2012-03-27 09:44 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2012-04-04 11:53 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
close connection earlier (1.16 KB, patch)
2012-03-29 07:29 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
hack patch to reproduce the windows failures (3.91 KB, patch)
2012-03-29 13:21 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
close connection earlier (6.18 KB, patch)
2012-03-29 13:23 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
close connection (1.12 KB, patch)
2012-03-29 13:25 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
enndeakin: review+
Details | Diff | Splinter Review
close connection (2.52 KB, patch)
2012-03-30 08:37 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
enndeakin: review+
Details | Diff | Splinter Review
hack patch to reproduce the windows failures with the new patch (5.16 KB, patch)
2012-03-30 13:50 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
close connection (4.07 KB, patch)
2012-04-03 09:50 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
enndeakin: review+
Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-03-27 09:44:34 PDT
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.
Comment 1 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-03-29 07:29:00 PDT
Created attachment 610537 [details] [diff] [review]
close connection earlier

https://tbpl.mozilla.org/?tree=Try&rev=81cbb2d8c27a
Comment 2 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-03-29 13:21:48 PDT
Created attachment 610660 [details] [diff] [review]
hack patch to reproduce the windows failures

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.
Comment 3 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-03-29 13:23:56 PDT
Created attachment 610661 [details] [diff] [review]
close connection earlier

https://tbpl.mozilla.org/?tree=Try&rev=62dd7535e3bc
Comment 4 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-03-29 13:25:36 PDT
Created attachment 610662 [details] [diff] [review]
close connection
Comment 5 Neil Deakin 2012-03-30 07:01:31 PDT
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.
Comment 6 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-03-30 08:37:24 PDT
Created attachment 610894 [details] [diff] [review]
close connection

Would you mind checking if I got the code review right?
https://tbpl.mozilla.org/?tree=Try&rev=c7ec73bf07e1
Comment 7 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-03-30 11:44:55 PDT
Looks like try found a problem. Trying the hack xpcom hack as before.
Comment 8 Neil Deakin 2012-03-30 11:52:31 PDT
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.
Comment 9 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-03-30 12:39:22 PDT
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?)
Comment 10 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-03-30 13:50:32 PDT
Created attachment 611016 [details] [diff] [review]
hack patch to reproduce the windows failures with the new patch
Comment 11 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-04-03 06:58:27 PDT
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.
Comment 12 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-04-03 09:50:20 PDT
Created attachment 611854 [details] [diff] [review]
close connection

https://tbpl.mozilla.org/?tree=Try&rev=f9fddcb2f788
Comment 13 Neil Deakin 2012-04-03 09:58:32 PDT
Comment on attachment 611854 [details] [diff] [review]
close connection

Seems ok, although I wonder why the script runner was necessary.
Comment 14 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-04-03 14:13:55 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=5d82421a984d
Comment 15 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-04 04:54:42 PDT
https://hg.mozilla.org/mozilla-central/rev/5d82421a984d
Comment 16 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-04-04 11:53:03 PDT
*** Bug 739656 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.