263865 - Crash in rdf.dll if I try UNDO after delete , sort operations in Bookmarks manager.

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect)

Description

[Muni Sekhar Reddy]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040910
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040616

This problem is seen Mozilla 1.7 and Mozilla 1.6 also. But the same is solved in
Mozilla 1.8a1 onwards.

Reproducible: Always
Steps to Reproduce:
1. Start Mozilla
2. Open Bookmarks Manager ( ctrl +b) 
3. Click on New Folder button.
4. Click on OK button in  Properties for "New Folder" dialog
5. Click on Delete button
6. Select "Bookmarks for default" (assuming that you are using default profile)
7. Click on New Folder button.
8. Click on OK button in  Properties for "New Folder" dialog.
9. Sort the bookmarks on Name ( Click on "Name").  Select ok on the Confirm dialog.
10. Edit -> Undo

Mozilla Crashes now.

Actual Results:  
Mozilla Crashes in RDF.DLL

Expected Results:  
Undo the last operation.

This problem happens only ath above recreation steps are followed exactly.This
problem does not occur on Mozilla 1.8a1, 1.8a2, 1.8a3 and 1.8a4.  This sems to
be fixed by bugid 221619. This is a huge patch to port.

The problem is because of NS_RDF_NO_VALUE being returned from
InMemoryDataSource::GetTarget(), which is called from
RDFContainerImpl::RemoveElementAt(PRInt32 aIndex, PRBool aRenumber, nsIRDFNode**
_retval). The failing instruction is NS_ADDREF(*_retval) in RemoveElementAt().

Comment 1

[Muni Sekhar Reddy]

Is it correct to return error value when ever NS_RDF_NO_VALUE is seen in the
RemoveElementAt() routine. Because we are performing a NS_ADDREF() on the error
value which seems to be incorrect. Please advice. 

Comment 2

[Axel Hecht]

This happens if the rdf:_n arcs inside a container get discontinuous.
No idea how bug 221619 may have fixed this, but it didn't fix the potential crash
inside nsRDFContainerImpl. We can get around an AddRef-Release cycle, too.
Patch coming up.

Comment 3

[Axel Hecht]

Attachment: don't addref null, use swap instead.

Comment 4

[Axel Hecht]

Comment on attachment 162062 [details] [diff] [review]
don't addref null, use swap instead.

I smell that some folks may want this on the branches, should it bake on the
trunk first?

Comment 5

[Mike Kaply [:mkaply]]

IF the crash isn't on the trunk, I'm not sure what baking will do.

Could this cause any potential problem?

Comment 6

[Axel Hecht]

I don't see any. Note that the trunk apparently changed the codepath which
exposes this crasher. It doesn't fix the crasher itself.

Comment 7

[Axel Hecht]

fixed.

Comment 8

[Mike Kaply [:mkaply]]

Comment on attachment 162062 [details] [diff] [review]
don't addref null, use swap instead.

We need this on the 1.7 branch as well if you get a chance.

Comment 9

[Axel Hecht]

checked in on the 1.7 branch