354034 - 20060924NB crashes on quitting

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Eiichi]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1) Gecko/20060924 Camino/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1) Gecko/20060924 Camino/1.0+

20060924NB crashes on quitting every time.
For reference, I'm using Mac OS X Version 10.3.9 (Build 7W98).

Crash.log is forthcoming.

Reproducible: Always

Comment 1

[Eiichi]

Attachment: Camino.crash.log

Comment 2

[Chris Lawson (gone)]

Does this affect *only* the 1.0+ builds, or does it affect branch (1.1+) and trunk (1.2+) as well?

Comment 3

[Peter Jaros (:peeja)]

I'm seeing this too.  The crash does not trigger Talkback, if that's any use.

Comment 4

[Eiichi]

(In reply to comment #2)
> Does this affect *only* the 1.0+ builds, or does it affect branch (1.1+) and
> trunk (1.2+) as well?

Both branch and trunk.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Considering it crashes on branch and trunk, and it's mentioning xpcom string crap, it's probably also fallout from the autocomplete rewrite.

Comment 6

[Håkan Waara]

Using global static literal strings in XPCOM is apparently considered harmful on OS X, see bug 234916.

From empiric evidence, everyone is reproducing it on 10.2-10.3.x. I think both crashes are related to this.

Patch coming up

Comment 7

[Håkan Waara]

Attachment: Stop using global static literal strings

Please try this, and see if it fixes the crash(es).

Comment 8

[Håkan Waara]

Comment on attachment 239961 [details] [diff] [review]
Stop using global static literal strings

Smokey tested this, and it fixes both reported crashes.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: branch version of the patch

There was a #@$@#$ comment (line 107/108 on the branch) that was out-of-sync that caused the first hunk to fail on the branch.  This should apply cleanly.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

BTW, this bug is fairly benign, but bug 354062 (which this patch also fixes) renders Camino branch and trunk unusable on 10.3.x

Comment 11

[Simon Fraser [no longer active]]

Comment on attachment 239961 [details] [diff] [review]
Stop using global static literal strings

Why not just use a static const string array just like kIgnoredPrefixes? Two levels of allocations seems like overkill.

Comment 12

[Håkan Waara]

(In reply to comment #11)
> (From update of attachment 239961 [details] [diff] [review] [edit])
> Why not just use a static const string array just like kIgnoredPrefixes? Two
> levels of allocations seems like overkill.

How? I must not use a static class array, because that will be inited at the same time as other global-static things, that's why I'm doing it lazily (which means doing it on the heap).

Comment 13

[Simon Fraser [no longer active]]

In-function statics are initialized on the first call to that function, which is fine.

Comment 14

[Håkan Waara]

(In reply to comment #13)
> In-function statics are initialized on the first call to that function, which
> is fine.

Note that the two remaining arrays in the new struct are used at two places, so they can't be in-function statics.  They can't be static const arrays in the class either, as that would be the same as before.

Comment 15

[Håkan Waara]

Attachment: Patch v2

I suppose I can save us 4 bytes doing it this way though. (Keep the pointers directly on nsSimpleGlobalHistory instead.)

Comment 16

[Simon Fraser [no longer active]]

Comment on attachment 240174 [details] [diff] [review]
Patch v2

>? crashfix.diff
>? foo
>Index: nsSimpleGlobalHistory.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/history/nsSimpleGlobalHistory.cpp,v
>retrieving revision 1.27
>diff -u -8 -p -r1.27 nsSimpleGlobalHistory.cpp
>--- nsSimpleGlobalHistory.cpp	23 Sep 2006 16:41:00 -0000	1.27
>+++ nsSimpleGlobalHistory.cpp	26 Sep 2006 17:42:46 -0000
>@@ -1,9 +1,9 @@
>-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* ***** BEGIN LICENSE BLOCK *****
>  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>  *
>  * The contents of this file are subject to the Mozilla Public License Version
>  * 1.1 (the "License"); you may not use this file except in compliance with
>  * the License. You may obtain a copy of the License at
>  * http://www.mozilla.org/MPL/
>  *
>@@ -88,37 +88,16 @@
> #define HISTORY_EXPIRE_NOW_TIMEOUT (3 * PR_MSEC_PER_SEC)
> 
> static const PRInt64 MSECS_PER_DAY = LL_INIT(20, 500654080);  // (1000000LL * 60 * 60 * 24)
> 
> PRInt32 nsSimpleGlobalHistory::gRefCnt;
> nsIMdbFactory* nsSimpleGlobalHistory::gMdbFactory = nsnull;
> nsIPrefBranch* nsSimpleGlobalHistory::gPrefBranch = nsnull;
> 
>-static const nsLiteralCString kSchemePrefixes[] = {
>-  NS_LITERAL_CSTRING("http://"),
>-  NS_LITERAL_CSTRING("https://"),
>-  NS_LITERAL_CSTRING("ftp://")
>-};
>-
>-static const nsLiteralCString kHostNamePrefixes[] = {
>-  NS_LITERAL_CSTRING("www."),
>-  NS_LITERAL_CSTRING("ftp.")
>-};
>-
>-// Prefixes to ignore in autocomplete.
>-static const nsLiteralCString kIgnoredPrefixes[] = { 
>-  NS_LITERAL_CSTRING("http://www."),
>-  NS_LITERAL_CSTRING("http://"),
>-  NS_LITERAL_CSTRING("https://www."),
>-  NS_LITERAL_CSTRING("https://"),
>-  NS_LITERAL_CSTRING("ftp://ftp."),
>-  NS_LITERAL_CSTRING("ftp://")
>-};
>-
> // list of terms, plus an optional groupby column
> struct SearchQueryData {
>   nsVoidArray   terms;            // array of searchTerms
>   mdb_column    groupBy;           // column to group by
> };
> 
> 
> // individual search term, pulled from token/value structs
>@@ -476,16 +455,24 @@ nsSimpleGlobalHistory::nsSimpleGlobalHis
>     mBatchesInProgress(0),
>     mDirty(PR_FALSE),
>     mPagesRemoved(PR_FALSE),
>     mEnv(nsnull),
>     mStore(nsnull),
>     mTable(nsnull)
> {
>   LL_I2L(mFileSizeOnDisk, 0);
>+  
>+  // these strings can't be static global, because then they'd
>+  // be inited before XPCOM, and things will crash.
>+  mSchemePrefixes = new nsCStringArray(3);
>+  mSchemePrefixes->ParseString("http:// https:// ftp://", " ");
>+
>+  mHostNamePrefixes = new nsCStringArray(2);
>+  mHostNamePrefixes->ParseString("www. ftp.", " ");
> }
> 
> nsSimpleGlobalHistory::~nsSimpleGlobalHistory()
> {
>   nsresult rv;
>   rv = CloseDB();
> 
>   NS_IF_RELEASE(mTable);
>@@ -2726,63 +2713,63 @@ nsSimpleGlobalHistory::AutoCompleteSearc
> // then mark their index in an AutocompleteExcludeData struct.
> void
> nsSimpleGlobalHistory::AutoCompleteGetExcludeInfo(const nsACString& aURL, AutocompleteExcludeData* aExclude)
> {
>   aExclude->schemePrefix = -1;
>   aExclude->hostnamePrefix = -1;
>   
>   PRInt32 index = 0;
>-  PRUint32 i;
>-  for (i = 0; i < sizeof(kSchemePrefixes)/sizeof(kSchemePrefixes[0]); ++i) {
>-    const nsACString& string = kSchemePrefixes[i];    
>+  PRInt32 i;
>+  for (i = 0; i < mSchemePrefixes->Count(); ++i) {
>+     nsACString& string = *mSchemePrefixes->CStringAt(i);    
>     if (StringBeginsWith(aURL, string)) {
>       aExclude->schemePrefix = i;
>       index = string.Length();
>       break;
>     }
>   }
>   
>-  for (i = 0; i < sizeof(kHostNamePrefixes)/sizeof(kHostNamePrefixes[0]); ++i) {
>-    const nsACString& string = kHostNamePrefixes[i];
>+  for (i = 0; i < mHostNamePrefixes->Count(); ++i) {
>+    const nsACString& string = *mHostNamePrefixes->CStringAt(i);
>     if (Substring(aURL, index, string.Length()).Equals(string)) {
>       aExclude->hostnamePrefix = i;
>       break;
>     }
>   }
> }
> 
> // Cut any protocol and domain prefixes from aURL, except for those which
> // are specified in aExclude
> void
> nsSimpleGlobalHistory::AutoCompleteCutPrefix(nsACString& aURL, AutocompleteExcludeData* aExclude)
> {
>   // This comparison is case-sensitive.  Therefore, it assumes that aUserURL is a 
>   // potential URL whose host name is in all lower case.
>   PRInt32 idx = 0;
>-  PRUint32 i;
>+  PRInt32 i;
>   
>-  for (i = 0; i < sizeof(kSchemePrefixes)/sizeof(kSchemePrefixes[0]); ++i) {
>-    if (aExclude && i == aExclude->schemePrefix)
>+  for (i = 0; i < mSchemePrefixes->Count(); ++i) {
>+    if (aExclude && i == (PRInt32)aExclude->schemePrefix)
>       continue;
>-    const nsACString &string = kSchemePrefixes[i];    
>+    const nsACString &string = *mSchemePrefixes->CStringAt(i);    
>     if (StringBeginsWith(aURL, string)) {
>       idx = string.Length();
>       break;
>     }
>   }
> 
>   if (idx > 0)
>     aURL.Cut(0, idx);
> 
>   idx = 0;
>-  for (i = 0; i < sizeof(kHostNamePrefixes)/sizeof(kHostNamePrefixes[0]); ++i) {
>-    if (aExclude && i == aExclude->hostnamePrefix)
>+  for (i = 0; i < mHostNamePrefixes->Count(); ++i) {
>+    if (aExclude && i == (PRInt32)aExclude->hostnamePrefix)
>       continue;
>-    const nsACString &string = kHostNamePrefixes[i];    
>+    const nsACString &string = *mHostNamePrefixes->CStringAt(i);    
>     if (StringBeginsWith(aURL, string)) {
>       idx = string.Length();
>       break;
>     }
>   }
> 
>   if (idx > 0)
>     aURL.Cut(0, idx);
>@@ -2878,16 +2865,26 @@ nsSimpleGlobalHistory::AutoCompleteSortC
>     if (isPath1 && !isPath2) return -1; // url1 is a website/path, url2 isn't
>     if (!isPath1 && isPath2) return  1; // url1 isn't a website/path, url2 is
> 
>     // We have two websites/paths.. ignore "http[s]://[www.]" & "ftp://[ftp.]"
>     // prefixes.  Find a starting position in the string, just past any of the 
>     // above prefixes.  Only check for the prefix once, in the far left of the 
>     // string - it is assumed there is no whitespace.
>     PRInt32 postPrefix1 = 0, postPrefix2 = 0;
>+    
>+    // Prefixes to ignore in autocomplete.
>+    static const nsLiteralCString kIgnoredPrefixes[] = { 
>+      NS_LITERAL_CSTRING("http://www."),
>+      NS_LITERAL_CSTRING("http://"),
>+      NS_LITERAL_CSTRING("https://www."),
>+      NS_LITERAL_CSTRING("https://"),
>+      NS_LITERAL_CSTRING("ftp://ftp."),
>+      NS_LITERAL_CSTRING("ftp://")
>+    };
> 
>     const unsigned int kNumPrefixes = sizeof(kIgnoredPrefixes)/sizeof(kIgnoredPrefixes[0]);
>     size_t i;
>     // iterate through our prefixes looking for a match
>     for (i = 0; i < kNumPrefixes; i++)
>     {
>       // Check if string is prefixed.  Note: the parameters of the Find() 
>       // method specify the url is searched at the 0th character and if there
>Index: nsSimpleGlobalHistory.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/history/nsSimpleGlobalHistory.h,v
>retrieving revision 1.10
>diff -u -8 -p -r1.10 nsSimpleGlobalHistory.h
>--- nsSimpleGlobalHistory.h	23 Sep 2006 16:41:00 -0000	1.10
>+++ nsSimpleGlobalHistory.h	26 Sep 2006 17:42:47 -0000
>@@ -263,16 +263,19 @@ protected:
>   void      SwapBytes(const PRUnichar *source, PRUnichar *dest, PRInt32 aLen);
> 
> protected:
> 
>   static PRInt32      gRefCnt;
> 
>   static nsIMdbFactory* gMdbFactory;
>   static nsIPrefBranch* gPrefBranch;
>+  
>+  nsCStringArray      *mHostNamePrefixes;
>+  nsCStringArray      *mSchemePrefixes;
> 
>   PRInt64             mFileSizeOnDisk;
>   PRInt32             mExpireDays;
> 
>   PRInt32             mBatchesInProgress;
>   PRBool              mDirty;             // if we've changed history
>   PRBool              mPagesRemoved;      // true if we've removed pages but not committed.
>   nsCOMPtr<nsITimer>  mSyncTimer;

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: branch version of patch v2

Branch version to ease landing; I also confirmed that the v2 patches do still fix the crashes.

We're going to see if we can't get one of our US-based chicken-wranglers to get these landed in the next hour or so in order to make the morning's nightlies :)

Comment 18

[Simon Fraser [no longer active]]

Sorry about the waste-of-space patch quote. Brain fart on my part.

Comment 19

[Eiichi]

Confirmed by latest-1.1-M1.8 (20060927).
Thank you for the quick fix.