Closed Bug 97424 Opened 23 years ago Closed 22 years ago

Expanding pasted folders in tree view results in hang then crash

Categories

(SeaMonkey :: Bookmarks & History, defect)

Product:
SeaMonkey
Component:
Bookmarks & History
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.1alpha

People

(Reporter: cplyon, Assigned: p_ch)

References

Details

(Keywords: crash, fixedOEM, hang)

Attachments

(3 files, 1 obsolete file)

patch - forbid copying to self or its child
2.03 KB, patch
Details | Diff | Splinter Review
Patch v1.1
13.97 KB, patch
timeless
: review+
Details | Diff | Splinter Review
disable copying a folder into its child or itself before Pierre's Big patch is ready
1.93 KB, patch
p_ch
: review+
jag+mozilla
: superreview+
Details | Diff | Splinter Review 
Using build 2001082803 on Win2K

Steps to Reproduce:
1. In the Manage Bookmarks window, create 3 folders
2. Move the folders so they are inside one another:
folder1
    folder2
        folder3
3. Copy folder2 and paste it into folder3
4. Expand folder3 and then all the folder2s you can find (recursively)

Result:
Hang then (wait for it) crash

Talkback IDs:
TB34611702G
TB34659202Q
Adding keywords
Keywords: crash, hang
Here's the stack. I've tagged both incident reports with this bug number as
well. The stack signatures(frm the 2 crashes) are different only because of the
last call on top of the stack.

Incident ID 34659202
Stack Signature RDFServiceImpl::GetResource 72442ac9
Bug ID 97424
Trigger Time 2001-08-28 21:44:41
Email Address cplyon@hotmail.com
User Comments Copying and pasting folders
Build ID 2001082809
Product ID MozillaTrunk
Platform ID Win32
Trigger Reason Stack overflow
Stack Trace
RDFServiceImpl::GetResource
[d:\builds\seamonkey\mozilla\rdf\base\src\nsRDFService.cpp, line 682]
RDFServiceImpl::GetUnicodeResource
[d:\builds\seamonkey\mozilla\rdf\base\src\nsRDFService.cpp, line 815]
nsXULContentUtils::GetResource
[d:\builds\seamonkey\mozilla\content\xul\templates\src\nsXULContentUtils.cpp,
line 675]
nsXULContentUtils::GetResource
[d:\builds\seamonkey\mozilla\content\xul\templates\src\nsXULContentUtils.cpp,
line 645]
nsXULContentBuilder::AddPersistentAttributes
[d:\builds\seamonkey\mozilla\content\xul\templates\src\nsXULContentBuilder.cpp,
line 888]
nsXULContentBuilder::BuildContentFromTemplate
[d:\builds\seamonkey\mozilla\content\xul\templates\src\nsXULContentBuilder.cpp,
line 769]
nsXULContentBuilder::BuildContentFromTemplate
[d:\builds\seamonkey\mozilla\content\xul\templates\src\nsXULContentBuilder.cpp,
line 602]
nsXULContentBuilder::CreateContainerContents
[d:\builds\seamonkey\mozilla\content\xul\templates\src\nsXULContentBuilder.cpp,
line 1289]
nsXULContentBuilder::CreateTemplateAndContainerContents
[d:\builds\seamonkey\mozilla\content\xul\templates\src\nsXULContentBuilder.cpp,
line 1190]
nsXULContentBuilder::CreateContents
[d:\builds\seamonkey\mozilla\content\xul\templates\src\nsXULContentBuilder.cpp,
line 1768]
nsXULElement::EnsureContentsGenerated
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 4010]
nsXULElement::ChildCount
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 2530]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 542]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 568]
nsXULTreeOuterGroupFrame::ComputeTotalRowCount
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsXULTreeOuterGroupFrame.cpp,
line 559]
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1
Paul Chen is now taking Bookmarks bugs. For your convenience, you can filter 
email notifications caused by this by searching for 'ilikegoats'.
Assignee: ben → pchen
Status: ASSIGNED → NEW
2001120703
This crash is happening a lot quicker with outliner.
TB195637M
mass reassign of pchen bookmark bugs to ben
Assignee: pchen → ben
Status: NEW → ASSIGNED
Keywords: nsbeta1
Nav triage team: nsbeta1-
Keywords: nsbeta1 → nsbeta1-
2002061014
MacOS 10.1.5

Similar (same?) problem:  Create a new folder in the bookmark manager, select
and then copy it, then paste while it is still selected.  Folder now has itself
as a child and expanding it will cause an immediate crash.
See bug 113554 for comment #7.
Let me take care of this one. Add ben@netscape.com (Ben Goodger) to the cc list.
OS: Windows 2000 → All
Hardware: PC → All
=>
Assignee: ben → Henry.Jia
Status: ASSIGNED → NEW
Forbid the operation if the source folder is parent, parent's parent, etc or
the same as the destination folder.

Please r=/sr=.
Status: NEW → ASSIGNED
Keywords: review
Nominate the bug.

Please r=/sr= the patch.
Keywords: nsbeta1- → nsbeta1
pch: thoughts?
I don't think that pasting a folder into itself or into one of its child should
be forbidden. But even if want to forbid it, we should forbid it at a higher
level in the CM. In addition, isParentOrSelf is not really efficient since is
scans the tree downwards instead of only considering the parent containers of
the target folder.

In fact, there is a bug in the recursion. Basically, the folder returned by
BookmarksUtils.cloneFolder has already been inserted in the the rdf ds. That's
why it provokes infinite duplication of the folder into itself. I noted this
problem when I fixed bug 144992, though, I did not feel myself enough confident
to change the cpp code.

The patch I will attach will allow to paste a folder into itself: it will be
correctly cloned and will modify the cpp methods createFolderWithDetails and
createGroupWithDetails so that if the parent resource is null, it will not
automatically insert the newly created folder into the parent.
The insertion in the ds will be done in BookmarksUtils.cloneFolder at the
appropriate place in the recursion process.
I checked all the places where clonefolder was used and found one occurence
where the newly created folder was inserted twice (ctrl-drag)

Since createFolder and createGroup were nearly not used in the js code, and
because they were striclty equivalent to create*WithDetails called with
aIndex=-1 (append), I changed their meanings:
 folder = BMSVC.createFolder(folderName)
 group  = BMSVC.createGroup (groupName)
will create a group or a folder that will not be inserted in the ds.
I also changed createBookmark to be consistent (not used in js)
Since I was touching clonefolder, I also fixed bug 144238 (pasted group is a
folder).
Attached patch Patch v1.0 (obsolete) — — Splinter Review 

    
    

    
  
Comment on attachment 93138 [details] [diff] [review]
patch - forbid copying to self or its child

1. Do we really want a bookmark folder to be copied to itself or its child?
It's confusing when mozilla forbid such operation elsewhere, such as mail
folder, etc. So I still stick to this patch.

2. Not sure your saying "isParentOrSelf is not really efficient since is
scans the tree downwards instead of only considering the parent containers of
the target folder". Can you tell me the method a little more? Thx.

        Attachment #93138 -
        Attachment description: patch → patch - forbid copying to self or its child

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v1.1
        — 
          — Splinter Review
      

    


  
I think it's a bad idea to morph an alrealy existing method.
that's why I renamed the new methods:
create(folder|group|bookmark)Resource: create a resource that is not included
in the bookmark ds.
create***WithDetails: same as before, except that if the parent resource is
null, the resource will not be included in the ds.
The methods create(Folder|Group|Bookmark) (almost unused and nearly identical
with the ***WithDetails ones) are dropped.
I also fixed a js warning with the bundle var and used the newly defined
isFolderGroup method in bm-props.js

Henry: the current recursion in BookmarksUtils.cloneFolder is not correctly
performed. We have to fix it independently of whether we forbid or not the
paste into itself of child folder.
1) You're right, we should forbid it, but not in cloneFolder. this method
should clone any folder, without considering where we will insert it. With my
patch, the simpler new synthax will be:
newFolder = BookmarksUtils.cloneFolder(folder);
RDFC.AppendElement(newFolder);

We should forbid this in the caller of the paste function. I am currently
working on having the infrastructure to do so in a easy way (bug 160019, see
how the move into itself or child is forbidden for the move action)

2) see method checkTargetContainer in the patch in bug 160019. An additional
problem with your patch is that you use a recursive function |isParentOrSelf|
(that looks up all the child in the folder to be cloned) in cloneFolder that is
also recursive.

        Attachment #94017 -
        Attachment is obsolete: true

    
  

        Attachment #94253 -
        Flags: review+

    
    

    
  
*** Bug 113554 has been marked as a duplicate of this bug. ***

    
    

    
  
taking
Assignee: Henry.Jia → chanial
Status: ASSIGNED → NEW
Depends on: 160019

    
    

    
  
Pierre, do you have plan about when to check in this patch? It is needed in the
OEM branch, which will be closed soon. Thanks.  


    
    

    
  
Bolian, I have incorporated patch v1.1 in the one in bug 160019. It's a rather
large one and I don't expect it to land in the trunk very soon. Since patch v1.1
fixes the crash but still allows pasting a folder into itself and I recognize we
should prevent it, and since a quick band-aid is needed on the OEM branch, then
r=me for checking in Henry's patch on the OEM branch.

    
    

    
  


  
this patch use upward checking, it is more efficient. Pierre, would you mind to
giva a r=?

    
    

    
  
Comment on attachment 96647 [details] [diff] [review]
disable copying a folder into its child or itself before Pierre's Big patch is ready

r=chanial@noos.fr

        Attachment #96647 -
        Flags: review+

    
    

    
  
Comment on attachment 96647 [details] [diff] [review]
disable copying a folder into its child or itself before Pierre's Big patch is ready

sr=jag

        Attachment #96647 -
        Flags: superreview+

    
  
Whiteboard: branchOEM

    
    

    
  
checked in Trunk (attachment 96647 [details] [diff] [review]).

    
    

    
  
Wasn't this only for the OEM branch, and not the trunk?

    
    

    
  
I think it is also needed in Trunk before Pierre's big fix.

    
  
Whiteboard: branchOEM → branchOEM+

    
    

    
  
checked in NETSCAPE_7_0_OEM_BRANCH (attachment 96647 [details] [diff] [review]).
Whiteboard: branchOEM+ → branchOEM+, fixedOEM

    
    

    
  
*** Bug 167479 has been marked as a duplicate of this bug. ***

    
  
Keywords: review → fixedOEM
Whiteboard: branchOEM+, fixedOEM

    
    

    
  
*** Bug 174791 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 176131 has been marked as a duplicate of this bug. ***

    
    

    
  
I can no longer reproduce this (Build 2003011604 on Win2k) (although I can
reproduce my bug that was marked as a dupe of this one (bug 176131)).  It looks
like a patch was checked in for this.  Should it be closed?  If so, 176131
should be reopened as it is still a problem.

    
    

    
  
yeah  looks like a patch was checked in for this in Sept '02.
This should be marked fixed, and yes, if u can still recreate the
other, it should be reopened (un-duped)



    
    

    
  
The patch for this bug is in. Marking resolved.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED

    
    

    
  
I see the fix was checked in to the NETSCAPE_7_0_OEM_BRANCH.
But didit ever hit the mozilla trnk? Or make it into Mozilla 1.2.1?
What looks like this bug is now being reported again in bug 191888, Mozilla 1.2.1

    
    

    
  
rkaa: the bug has been fixed for the trees, but there's still a bug in the
personal toolbar (and menus). In mozilla, they don't share the same code at all.

    
    

    
  
so.. is bug 197291 a dup of this?

    
    

    
  
Regarding comment 36:
I really don't understand this bug. It is labeled Version: Trunk, and is
resolved as "Fixed". Do I understand comment 36 correct if I think it says that
this bug is NOT fixed on the trunk? Shouldn't this bug be reopened then?

    
  
Summary: Expanding pasted folders results in hang then crash → Expanding pasted folders in tree view results in hang then crash

    
    

    
  
rkaa: this bug related to pasting the PTF into itself is fixed on the trunk, but
it only applied to the bookmark manager and the bookmarks sidebar (xul trees).
The problem still exists when pasting the PTF on the personal toolbar and has
never been fixed, nowhere except phoenix.
Anyways, it will be fixed when the bookmark branch will land (1.4b early)

    
    

    
  
*** Bug 196200 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: