469717 - Crash when using `xf:delete`followed by `xf:send`

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[John L. Clark]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.6pre) Gecko/2008121115 Minefield/3.0.6pre
Build Identifier: CVS trunk

In some cases, when you dispatch an `xf:delete` action followed by an `xf:send` action, Firefox crashes in XPath processing of the binding on the `xf:submission` element that was selected by the `xf:send` action.  This happens in both current CVS trunk (as of 2008-12-15) and Firefox 3.0.4 with version 0.8.6ff3 of the Mozilla XForms Extension.

I will post a test case shortly.

Reproducible: Always

Comment 1

[John L. Clark]

Attachment: Test case that demonstrates this bug

Comment 2

[Dion Sole]

Attachment: Simplified test case with successful delete+send

This is a simpler test case which also includes an example of delete+send that does work.
Also, if you include an <xf:output ref="deleteme"/> in the page, the "I Do Crash" trigger will work correctly (i.e. not crash).

I had a look into this, but I'm still far away from understanding the tx code, so I couldn't figure out if it's a bug in there or in XForms.
The crash occurs (I think) because the txResultRecycler tries to re-use a node-set that still refers to the deleted node, and thus when it tries to call that txXPathNode's destructor, the code causes a seg fault in nsINode::GetNodeParent.
Sadly, this is as far as I can go until I understand how things work more, so any help would be appreciated.

Comment 3

[aaronr]

This sounds familiar.  Try this out on FF 3.5 (might need Philipp Wagner to test it for you if you don't build the mercurial tree) and see if it still happens.  I seem to remember not pursuing a bug that I suspected was in the xpath code because it was fixed in the current code.

Comment 4

[Philipp Wagner [:imphil]]

This bug is different than the one you remember (http://groups.google.com/group/mozilla.dev.tech.xforms/msg/ebc8916cc4ad2d70), and FF 3.5 still crashes.

Comment 5

[Dion Sole]

(In reply to comment #3)
> This sounds familiar.  Try this out on FF 3.5 (might need Philipp Wagner to
> test it for you if you don't build the mercurial tree) and see if it still
> happens.  I seem to remember not pursuing a bug that I suspected was in the
> xpath code because it was fixed in the current code.
The stack trace for that crash is almost exactly the same, except that instead of nsXFormsModel::ProcessBinds(), it's nsXFormsSubmissionElement::GetBoundInstanceData() that starts off the path to the crash.

Comment 6

[Sergey Reymerov]

Crash appears when txNodeSet clears (in submit action). 
I analyzed callgraph and txNodeSet usings. I had thought that error occurs in case of wrong txXPathNode appends to txNodeSet. Thus I had looked at nsXFormsXPathFunctions::Instance. And I removed next code(because this code appends XPath of instance node into txNodeSet):
> if (root) {
>     result->Add(root);
> }
It removed instance-function at all. But crashes didn't occur.

So, I was looking then. I had looked at code:
> while (aStart < aEnd) {
>     aStart->~txXPathNode();
>     ++aStart;
> }
I'm surprized that when Firefox crashes pointers (aStart and aEnd) are equal. How could it be? Why when aStart equals aEnd cycle continues?

Comment 7

[Sergey Reymerov]

I've just forgotten that second code patch is from txNodeSet::destroyElements.

Comment 8

[Sergey Reymerov]

I was cought every call of txXPathNode::txXPathNode(const txXPathNode& aNode) and txXPathNode::~txXPathNode(). And I had seen next error (construcotr is "+", destructor is "-", hex code - is address of Root() that NS_ADDREFS in constructor and NS_RELEASE in destructor):
> +0x7fffdf1ba000
> -0x7fffdf1ba000
> +0x7fffdf1ba000
> +0x7fffdf1ba000
> -0x7fffdf1ba000
Next -0x7fffdf1ba000 crashes Firefox.

Comment 9

[Sergey Reymerov]

I've studied crash. It happens in next reason:
In nsXFormsXPathFunctions::Instance:298 instance node adds to txINodeSet. Also this sets aKeepRootAlive flag.
Next it creates txXPathNode for instance node and in case of aKeepRootAlive (mRefCountRoot) is true constructor does NS_ADDREF(Root()).
Next in nsXFormsInsertDeleteElement::HandleAction:670 makes UnbindFromTree that break parent-child relation.
Next destructor of txINodeSet makes NS_RELEASE(Root()) that crashes Firefox because Root() couldn't process.

Comment 10

[alexander :surkov (:asurkov)]

My stack trace is a bit different:

 	xul.dll!nsINode::GetNodeParent()  Line 620 + 0x3 bytes	C++
 	xul.dll!txXPathNode::RootOf(nsINode * aNode=0x050a28f8)  Line 104 + 0x8 bytes	C++
 	xul.dll!txXPathNode::Root()  Line 111 + 0xb bytes	C++
>	xul.dll!txXPathNode::~txXPathNode()  Line 359 + 0x8 bytes	C++
 	xul.dll!txXPathNode::`scalar deleting destructor'()  + 0xf bytes	C++
 	xul.dll!txNodeSet::destroyElements(const txXPathNode * aStart=0x06a156e0, const txXPathNode * aEnd=0x06a156e8)  Line 233	C++
 	xul.dll!txNodeSet::clear()  Line 444 + 0x13 bytes	C++
 	xul.dll!txResultRecycler::getNodeSet(txNodeSet * * aResult=0x0012add0)  Line 189	C++
 	xul.dll!LocationStep::evaluate(txIEvalContext * aContext=0x0012ae7c, txAExprResult * * aResult=0x0012ae60)  Line 67 + 0x2c bytes	C++
 	xul.dll!PathExpr::evaluate(txIEvalContext * aContext=0x0012af6c, txAExprResult * * aResult=0x0012af64)  Line 125 + 0x40 bytes	C++
 	xul.dll!nsXPathExpression::EvaluateWithContext(nsIDOMNode * aContextNode=0x050a261c, unsigned int aContextPosition=0x00000001, unsigned int aContextSize=0x00000001, unsigned short aType=0x0009, nsISupports * aInResult=0x00000000, nsISupports * * aResult=0x0012b2bc)  Line 149 + 0x43 bytes	C++
 	xforms.dll!nsXFormsUtils::EvaluateXPath(const nsAString & aExpression={...}, nsIDOMNode * aContextNode=0x050a261c, nsIDOMNode * aResolverNode=0x0509b00c, unsigned short aResultType=0x0009, nsIDOMXPathResult * * aResult=0x0012b484, int aContextPosition=0x00000001, int aContextSize=0x00000001, nsCOMArray<nsIDOMNode> * aSet=0x00000000, nsStringArray * aIndexesUsed=0x00000000)  Line 596 + 0x50 bytes	C++
 	xforms.dll!nsXFormsUtils::EvaluateNodeBinding(nsIDOMElement * aElement=0x0509b00c, unsigned int aElementFlags=0x00000000, const nsString_external & aBindingAttr={...}, const nsString_external & aDefaultRef={...}, unsigned short aResultType=0x0009, nsIModelElementPrivate * * aModel=0x0012b4f8, nsIDOMXPathResult * * aResult=0x0012b500, int * aUsesModelBind=0x0012b504, nsIXFormsControl * * aParentControl=0x00000000, nsCOMArray<nsIDOMNode> * aDeps=0x00000000, nsStringArray * aIndexesUsed=0x00000000)  Line 788 + 0x46 bytes	C++
 	xforms.dll!nsXFormsSubmissionElement::GetBoundInstanceData(nsIDOMNode * * result=0x0012b604)  Line 1105 + 0x6a bytes	C++
 	xforms.dll!nsXFormsSubmissionElement::Submit()  Line 757 + 0x26 bytes	C++

Comment 11

[alexander :surkov (:asurkov)]

So it looks like nsINode::GetNodeParent() crashes, also the nsINode object is in bad shape since it hasn't node info, mParentPtrBits cannot be evaluated in debugger.

I think txXPathNode makes bad things when it addrefs and releases the result of Root() computations. Since the parent chain of the mNode can be changed then Root() might fail when it tries to release the object what should lead to crash.

Comment 12

[alexander :surkov (:asurkov)]

cc'ing Peter and Jonas.

Comment 13

[Peter Van der Beken [:peterv]]

Our XPath engine has never been able to deal with changes to the DOM while an XPath expression is being evaluated, it assumes a static DOM.

Comment 14

[alexander :surkov (:asurkov)]

That's was an assumption, possibly wrong. Peter, could you please try the testcase? (however you will need to build firefox with xforms and schema-validation extensions).

Comment 15

[Peter Van der Beken [:peterv]]

It'll be a while before I can try that out. Comment 9 seems to imply that nsXFormsInsertDeleteElement::HandleAction is called during XPath evaluation?

Comment 16

[Sergey Reymerov]

(In reply to comment #15)
> It'll be a while before I can try that out. Comment 9 seems to imply that
> nsXFormsInsertDeleteElement::HandleAction is called during XPath evaluation?

No, XPath evaluation is in HandleAction.

Comment 17

[aaronr]

What I get from what you are saying in comment 8 and 9 is that when the NS_RELEASE(Root()) happens that crashes, it still has a refcount of 1, right?  Which means that it shouldn't crash because according to the refcount, the object should still be alive.  But if it is not, you should look into why that is.  For example, at what point does the data in the object go bad/get overwritten?  It must be some time between the last NS_RELEASE that crashes and the one before that which had no problem, right?

Comment 18

[Sergey Reymerov]

Attachment: bad patch that solves problem

As I wrote that problem is that HandleAction destroy node is root of nodes which will destroy later.
Simple solution of this is make don't adding refs and release of Root() of instance. 
I did this patch to certain of problem above.
May be this patch is normal because I don't know for what txNodeSetAdaptor::Add creates txXPathNativeNode with aKeepRootAlive flag.

Comment 19

[Sergey Reymerov]

Why node creates with aKeepRootAlive == true in this:
txNodeSetAdaptor::Add(nsIDOMNode *aNode) {
    NS_ENSURE_TRUE(mWritable, NS_ERROR_FAILURE);
    nsAutoPtr<txXPathNode> node(txXPathNativeNode::createXPathNode(aNode, PR_TRUE));
 
And how I could make adding of node to txNodeSetAdaptor with no aKeepRootAlive?

Comment 20

[Peter Van der Beken [:peterv]]

(In reply to comment #19)
> Why node creates with aKeepRootAlive == true in this:

Because we need to keep ancestors alive. Holding a node alive without holding the root of the subtree that that node is in will cause dangling parent pointers and crashes.

> And how I could make adding of node to txNodeSetAdaptor with no aKeepRootAlive?

You shouldn't, you should not disconnect the root from the node while the txNodeSetAdaptor is alive. Or if you do, you should addref the new root and release the old one.

Comment 21

[Peter Van der Beken [:peterv]]

(In reply to comment #9)
> Next destructor of txINodeSet makes NS_RELEASE(Root()) that crashes Firefox
> because Root() couldn't process.

When does this destructor run? The XPathResult doesn't keep the txNodeSet alive specifically for this reason, so why is it alive while nsXFormsInsertDeleteElement::HandleAction removes the node from its parent?

Comment 22

[Sergey Reymerov]

Callstack looks like this:
> RootExpr::evaluate
> txResultRecycler::getNodeSet
> txNodeSet::clear
> txNodeSet::destoyElements
> ~txXPathNode
> txXPathNode::RootOf
> <and there is crash>

Comment 23

[Sergey Reymerov]

Another note about bug. I've found that creating of txXPathNode and it's destoying in txNodeSet) occurs in RootExpr::evaluate.
> txXPathTreeWalker walker(aContext->getContextNode()); -- there is a creating
> walker.moveToRoot();
> return aContext->recycler()->getNodeSet(walker.getCurrentPosition(), aResult);  -- there is a destroying

walker.moveToRoot() is not cause of crash.

Comment 24

[Sergey Reymerov]

In tx nodes add to the nodeset that stores in txResultRecycler object. So nodeset from InsertDeleteElement::HandleAction moves to Submit via txResultRecycler (same txResultRecycler object that in HandleAction and Submit). In submit nodeset tries to clear itself and firefox crashes because one parent node of node from nodeset already has been destroyed in InsertDeleteElement::HandleAction.
Could I make to clear recycler in InsertDeleteElement?

Comment 25

[Sergey Reymerov]

Why I can't do somethink like this in InsertDeleteElement ?: 
> nsCOMPtr<nsIXPathResult> xpathResult = do_QueryInterface(nodeset);
> xpathResult->recycler();
I have errors that no nsIXPathResult. How could I connect such headers and is it possible?

Comment 26

[aaronr]

Hi Sergey, I think that you are looking at this from the wrong angle.  You seem to be trying to avoid the crash from the XForms point of view.  The recycler is internal to XPath.  I doubt that it should be acted on directly by an outside user like XForms.

If I've been following this thread correctly, somehow this root is being freed before it has a zero reference count, right?  And that when it finally does have a zero reference count and goes through the destructor like it is supposed to, it crashes because it has already been freed.  This should never happen for a xpcom object.  So I think tracking down why it is freed before its reference count is zero is the way to go.

Comment 27

[Sergey Reymerov]

Attachment: patch

I just add refs to removed node. And I think that then recycler release this node and destructor process it. 

And is it normal that handling of insert and delete node in same class? It's done by next code in handling:
> if (mIsInsert) {
>   ...
> } else {
>   ...
> }

Comment 28

[alexander :surkov (:asurkov)]

Comment on attachment 431832 [details] [diff] [review]
patch


>             rv = parentNode->RemoveChild(locationNode, getter_AddRefs(resNode));
>+            NS_ADDREF(resNode);

It must be a leak.

Comment 29

[Sergey Reymerov]

Simple calls-tracing of txXPathNode() and ~txXPathNode() show that this node normally destroying in txNodeSet::clear()

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #29)
> Simple calls-tracing of txXPathNode() and ~txXPathNode() show that this node
> normally destroying in txNodeSet::clear()

For all possible cases? For example, I'm a bit sceptic because of line
NS_ENSURE_SUCCESS(rv, rv);
if it fails then will it destroyed as well?

Also it would be nice to have some portion of comments, at least it's not evident why it doesn't leak.

Comment 31

[Sergey Reymerov]

Attachment: patch 2

I put comments.

Comment 32

[alexander :surkov (:asurkov)]

Comment on attachment 431860 [details] [diff] [review]
patch 2

I'm not sure this way is exactly what Aaron suggested, so it's better to get his first review.

Actually it looks unsafe to addref in XForms module and except it will be released outside of XForms because the proposed way is not standard convention of addref/release.

Comment 33

[aaronr]

Comment on attachment 431860 [details] [diff] [review]
patch 2

But what happens in all the cases that DON'T crash firefox?  Does that mean that in those cases we'll be leaking this node?  Remember, we only seem to be crashing when instance() is used which is certainly not all of the cases.

Comment 34

[Sergey Reymerov]

(In reply to comment #33)
> (From update of attachment 431860 [details] [diff] [review])
> But what happens in all the cases that DON'T crash firefox?  Does that mean
> that in those cases we'll be leaking this node?  Remember, we only seem to be
> crashing when instance() is used which is certainly not all of the cases.

When node removes it stay in nodeset. Nodeset lives from nsXFormsInsertDeleteElement::HandleAction to nsXFormsSubmitElement::HandleAction or any other that uses tx. Destructor for node calls in InsertDeleteElement because reference count is zero but node is still alive in nodeset. So I think that this is normal solution to addref because node is still alive. 
It could make memory leak 'til next txNodeset using. 
Bug appears due to using of instance() makes nodes adding to nodeset with addrefing root of them. If no instance() function then nodes adding don't add refs to roots of nodes.

Comment 35

[aaronr]

Feel free to run this by Olli (whose xpcom isn't nearly as rusty as mine), but I don't think that addref'ing here is the right solution.  If the reference count for the node should be > 0 after the delete because it still lives in a xpath structure, then it should have been > 1 before we deleted it.  Doing an addref here might get us around a crash, but I think it would just be hiding whatever the real problem is.  But again, ask Olli's opinion.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

If you add an NS_ADDREF, you need to add also NS_RELEASE somewhere.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

...otherwise you leak.

XPCOM_MEM_LEAK_LOG=1 environment variable is useful for detecting memory leaks.
Set the variable, start browser, do something which may leak, wait a bit
so that cycle collector runs (15 seconds or so doing *nothing*) and close the
browser. If there are leaks, those should be reported in the console.
You may need to re-do the same steps to make sure the leak is really
reproducable.

Comment 38

[Peter Van der Beken [:peterv]]

Yeah, the NS_ADDREF is definitely wrong. The right solution is probably to make txResultRecycler::recycle call clear() on mNodeSetResults before pushing it to mNodeSetResults. Does that work?

Comment 39

[Peter Van der Beken [:peterv]]

(In reply to comment #38)
> call clear() on mNodeSetResults

Er, make that "call clear() on static_cast<txNodeSet*>(aResult)"

Comment 40

[Sergey Reymerov]

(In reply to comment #39)
> (In reply to comment #38)
> > call clear() on mNodeSetResults
> 
> Er, make that "call clear() on static_cast<txNodeSet*>(aResult)"

This midification solve FF crashes problem. But I don't think that tx modification can be possible.

Comment 41

[Sergey Reymerov]

Attachment: tx patch

Comment 42

[Peter Van der Beken [:peterv]]

Comment on attachment 433537 [details] [diff] [review]
tx patch

> void
> txResultRecycler::recycle(txAExprResult* aResult)
> {
>+    static_cast<txNodeSet*>(aResult)->clear();

This is wrong, at this point you could have a non-nodeset aResult. This needs to only happen in the nodeset case.
It also doesn't remove the clear calls from all the getNodeSet implementations that aren't needed anymore.

(In reply to comment #40)
> But I don't think that tx
> modification can be possible.

I have no idea what that means.

Comment 43

[Sergey Reymerov]

Attachment: tx patch

Comment 44

[Peter Van der Beken [:peterv]]

Comment on attachment 433547 [details] [diff] [review]
tx patch

See comment 42. BTW, I didn't say anything about checking for isEmpty, it's unnecessary.

Comment 45

[Sergey Reymerov]

Attachment: tx patch

Remove clear calls from getNodeset and remove isEmpty.

Comment 46

[Sergey Reymerov]

Peter, can you look at this again?

Comment 47

[Sergey Reymerov]

Peter, who could be co-reviwer in that patch?

Comment 48

[Peter Van der Beken [:peterv]]

No one, this patch is good to go. Get someone to check it in (set checkin-needed keyword if necessary).

Comment 49

[alexander :surkov (:asurkov)]

try server build - http://build.mozilla.org/tryserver-builds/[e-mail]-try-ecbc97ebc3c9

Comment 50

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/aba6af3a5ad1

Comment 51

[Sergey Reymerov]

Comment on attachment 433887 [details] [diff] [review]
tx patch

That patch in 1.9.2 need for fix crash that appears in XForms. It would be nice to fix that bug in  nearest release of XForms. I developed this patch in 1.9.2 and test it. Also this patch makes safe changes because nodeset::clear() calls move to another (in callstack) methods (getNodeSet).

Comment 52

[Sergey Reymerov]

Hmmm. I mean:
It would be nice to fix that bug in nearest release of Firefox.

Comment 53

[aaronr]

I'd rather not wait since there is no guarantee this will be approved.  We can always refresh the xforms release with this patch included if it does get approved.  What does everyone else think?

Comment 54

[Philipp Wagner [:imphil]]

Yes, I guess the new XForms addon 0.8.7 is already in the AMO review queue (if Doron uploaded it). We can always do a stability release if necessary.

Comment 55

[LegNeato]

Comment on attachment 433887 [details] [diff] [review]
tx patch

We won't be taking this in 1.9.2.6...we already have too many fixes and a tight schedule. Feel free to nominate it for .7 though

Comment 56

[Daniel Veditz [:dveditz]]

Comment on attachment 433887 [details] [diff] [review]
tx patch

Approved for 1.9.2.9, a=dveditz for release-drivers

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c750acc6f11b