118735 - mozilla crashes on startup if "bad" entry in Personal Toolbar

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[david avery]

mozilla crashes on startup - if "Personal Toolbar Folder" has enough entries to
be wider that window size

Comment 1

[david avery]

Attachment: sample minimal bookmarks.html

Comment 2

[david avery]

change summary

crash is if  either the tinderbox or bugzilla entry in attached bookmark.html is
there

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

What build ID?  Yesterday morning's?

Comment 4

[david avery]

20020108xx tip local build

crashed win32 too

both crashed on clean start( autoimport NS4 profile)

Comment 5

[david avery]

confirmed on 2002010810 win32 official build 

Comment 6

[Morten Nilsen]

confirmed on linux current tip optimized build

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Hyatt was talking about this exact problem yesterday, he was either fixing this,
or breaking it. Reassigning ;)

Comment 8

[Olav Vitters]

Attachment: Stacktrace

adding stacktrace from todays CVS Linux

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

If that's the stack, then it has nothing to do with the window size.

Comment 10

[Robert John Churchill]

The stack indicates its due to a change in RDF's in-memory datasource which went 
in on Sunday.  Patch below:


Index: mozilla/rdf/base/src/nsInMemoryDataSource.cpp
===================================================================
RCS file: /cvsroot/mozilla/rdf/base/src/nsInMemoryDataSource.cpp,v
retrieving revision 1.62
diff -u -2 -r1.62 nsInMemoryDataSource.cpp
--- mozilla/rdf/base/src/nsInMemoryDataSource.cpp	7 Jan 2002 11:18:53 -0000	
1.62
+++ mozilla/rdf/base/src/nsInMemoryDataSource.cpp	8 Jan 2002 21:32:55 -0000
@@ -700,6 +700,4 @@
         if (mAssertion && mAssertion->mHashEntry) {
             // its our magical HASH_ENTRY forward hash for assertions
-            mAssertion = nsnull;
-
             nsresult rv = NS_NewISupportsArray(getter_AddRefs(mHashArcs));
             if (NS_SUCCEEDED(rv)) {
@@ -709,4 +707,5 @@
                     ArcEnumerator, mHashArcs.get());
             }
+            mAssertion = nsnull;
         }
     }

Comment 11

[Robert John Churchill]

However, even with the patch applied, Mozilla then appears to spin forever inside 
of the XUL template builder.  Waterson, do you have a few minutes to look at this 
with me?

Comment 12

[Robert John Churchill]

If I had to take a guess, I'd say from the sample bookmarks file attached to this 
bug that the Personal Toolbar folder doesn't have an "ID" assigned to it in the 
file, so that the bookmark's service is trying to change its URL, and we're 
getting into an infinite loop somewhere between bookmarks, RDF and the XUL 
template builder.

Comment 13

[David Hyatt]

Happy New Year, sugar puff.

Comment 14

[Robert John Churchill]

Hmmm... at second glance, this is now a bug in XBL... reassigning back to Hyatt.  
;)  <hehe>

Just kidding!

Comment 15

[david avery]

the bookmarks file is a imported NS4.7 bookmarks file from starting mozilla with
no profiles available

mozillais now hanging up in the import - after the importing alert closes

^C ing mozilla leaves a bookmarks.html with no ID=S


probably need new bug

Comment 16

[Robert John Churchill]

DOH!  Here we go.  Reviews, please!


Index: mozilla/rdf/base/src/nsInMemoryDataSource.cpp
===================================================================
RCS file: /cvsroot/mozilla/rdf/base/src/nsInMemoryDataSource.cpp,v
retrieving revision 1.62
diff -u -2 -r1.62 nsInMemoryDataSource.cpp
--- mozilla/rdf/base/src/nsInMemoryDataSource.cpp	7 Jan 2002 11:18:53 -0000	
1.62
+++ mozilla/rdf/base/src/nsInMemoryDataSource.cpp	8 Jan 2002 22:49:34 -0000
@@ -700,6 +700,4 @@
         if (mAssertion && mAssertion->mHashEntry) {
             // its our magical HASH_ENTRY forward hash for assertions
-            mAssertion = nsnull;
-
             nsresult rv = NS_NewISupportsArray(getter_AddRefs(mHashArcs));
             if (NS_SUCCEEDED(rv)) {
@@ -709,4 +707,5 @@
                     ArcEnumerator, mHashArcs.get());
             }
+            mAssertion = nsnull;
         }
     }
@@ -758,4 +757,5 @@
         if (itemCount > 0) {
             nsCOMPtr<nsIRDFResource> res = do_QueryInterface(mHashArcs->
ElementAt(itemCount-1));
+            mHashArcs->RemoveElementAt(itemCount-1);
             mCurrent = res;
             NS_IF_ADDREF(mCurrent);

Comment 17

[Chris Waterson]

This first chunk looks fine. The second chunk leaks (always did, my bad for not
catching it). nsISupportsArray::ElementAt() addref's.

Why don't you do this instead:

  if (itemCount > 0)
    PRInt32 last = itemCount - 1;
    mCurrent = NS_STATIC_CAST(nsIRDFResource *,
                              mHashArcs->ElementAt(last));

    mHashArcs->RemoveElementAt(last);
    *aResult = PR_TRUE;
    return NS_OK;
  }

Also saves an unneeded QI, since you _know_ the only things being put in this
array are nsIRDFResource objeccts.

Comment 18

[Robert John Churchill]

OK.


Index: mozilla/rdf/base/src/nsInMemoryDataSource.cpp
===================================================================
RCS file: /cvsroot/mozilla/rdf/base/src/nsInMemoryDataSource.cpp,v
retrieving revision 1.62
diff -u -2 -r1.62 nsInMemoryDataSource.cpp
--- mozilla/rdf/base/src/nsInMemoryDataSource.cpp	7 Jan 2002 11:18:53 -0000	
1.62
+++ mozilla/rdf/base/src/nsInMemoryDataSource.cpp	8 Jan 2002 23:12:23 -0000
@@ -700,6 +700,4 @@
         if (mAssertion && mAssertion->mHashEntry) {
             // its our magical HASH_ENTRY forward hash for assertions
-            mAssertion = nsnull;
-
             nsresult rv = NS_NewISupportsArray(getter_AddRefs(mHashArcs));
             if (NS_SUCCEEDED(rv)) {
@@ -709,4 +707,5 @@
                     ArcEnumerator, mHashArcs.get());
             }
+            mAssertion = nsnull;
         }
     }
@@ -757,7 +756,8 @@
         if (NS_FAILED(rv = mHashArcs->Count(&itemCount)))   return(rv);
         if (itemCount > 0) {
-            nsCOMPtr<nsIRDFResource> res = do_QueryInterface(mHashArcs->
ElementAt(itemCount-1));
-            mCurrent = res;
-            NS_IF_ADDREF(mCurrent);
+            --itemCount;
+            mCurrent = NS_STATIC_CAST(nsIRDFResource *,
+                                      mHashArcs->ElementAt(itemCount));
+            mHashArcs->RemoveElementAt(itemCount);
             *aResult = PR_TRUE;
             return NS_OK;

Comment 19

[Chris Waterson]

sr=waterson

Comment 20

[Robert John Churchill]

Fixed.  Thanks for the reviews, Chris and Darin.