Spun out of bug 599969.

Fix it by never ever passing an nsIURI object to the background thread. Try results: http://tbpl.mozilla.org/?tree=MozillaTry&rev=81cb6a299a2a

looks like there is a crash in SetPageTitle 0 mozsqlite3.dll!vdbeUnbind + 0x192 eip = 0x6edcac04 esp = 0x0826f6d8 ebp = 0x0826f6f4 ebx = 0x080f0f08 esi = 0x00000001 edi = 0x00000000 eax = 0x00000000 ecx = 0x000008cc edx = 0x00000000 efl = 0x00010246 Found by: given as instruction pointer in context 1 mozsqlite3.dll!sqlite3_bind_text [sqlite3.c:81cb6a299a2a : 59139 + 0xd] eip = 0x6ed92438 esp = 0x0826f6fc ebp = 0x0826f724 Found by: previous frame's frame pointer 2 xul.dll!mozilla::storage::variantToSQLiteT<mozilla::storage::`anonymous namespace'::BindingColumnData>(mozilla::storage::`anonymous namespace'::BindingColumnData,nsIVariant *) [variantToSQLiteT_impl.h:81cb6a299a2a : 109 + 0x17] eip = 0x69c35f62 esp = 0x0826f72c ebp = 0x0826f864 Found by: previous frame's frame pointer 3 xul.dll!mozilla::storage::BindingParams::bind(sqlite3_stmt *) [mozStorageBindingParams.cpp:81cb6a299a2a : 254 + 0x10] eip = 0x69c35ddf esp = 0x0826f86c ebp = 0x00000008 Found by: previous frame's frame pointer 4 xul.dll!mozilla::storage::Statement::ExecuteStep(int *) [mozStorageStatement.cpp:81cb6a299a2a : 607 + 0x21] eip = 0x69c1e643 esp = 0x0826f884 ebp = 0x080f0f08 Found by: call frame info with scanning 5 xul.dll!mozilla::places::`anonymous namespace'::SetPageTitle::Run() [History.cpp:81cb6a299a2a : 757 + 0x4c] eip = 0x69c87ec9 esp = 0x0826f8a0 ebp = 0x0826f898 Found by: call frame info with scanning

Comment on attachment 491053 [details] [diff] [review] v1.0 >diff --git a/toolkit/components/places/src/Helpers.cpp b/toolkit/components/places/src/Helpers.cpp >--- a/toolkit/components/places/src/Helpers.cpp >+++ b/toolkit/components/places/src/Helpers.cpp >@@ -201,14 +201,13 @@ URIBinder::Bind(mozIStorageBindingParams > nsresult > GetReversedHostname(nsIURI* aURI, nsString& aRevHost) > { >- nsCAutoString forward8; >- nsresult rv = aURI->GetHost(forward8); >+ nsCAutoString forward; nit: since changing the name, what about forwardHost? >@@ -220,6 +219,14 @@ GetReversedHostname(const nsString& aFor >+GetReversedHostname(const nsCString& aForward, >+ nsString& aRevHost) nit: and aForwardHost here. The point is that we have a GetReversedHostname that takes a uri, and 2 that take strings, it's confusing since I could try to pass the spec to it. Indeed the 2 utils should probably be called ReverseHostName since they are not getting the host from the url... >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >--- a/toolkit/components/places/src/History.cpp >+++ b/toolkit/components/places/src/History.cpp >@@ -206,19 +206,19 @@ struct VisitData { > , visitId(aOther.visitId) > , sessionId(aOther.sessionId) > , spec(aOther.spec) >+ , hostname(aOther.hostname) Even if we could end up doing it when it's not needed, I'd prefer if we stored the revHost here, it would be easier when we read the value from the db or when we have to bind it. The cost to reverse a short string (the host) should be really ignorable on today's hardware. > , hidden(aOther.hidden) > , typed(aOther.typed) > , transitionType(aOther.transitionType) > , visitTime(aOther.visitTime) > { >- uri.swap(aOther.uri); > } You can probably kill this copy constructor > nsAutoString revHost; >- nsresult rv = GetReversedHostname(mPlace.uri, revHost); >- NS_ENSURE_SUCCESS(rv, rv); >+ GetReversedHostname(mPlace.hostname, revHost); just comparing these 2, you can see what I mean when I say GetReversedHostname is confusing :) >@@ -829,39 +815,18 @@ public: > mozIStorageConnection* mDBConn; > >- nsCOMPtr<nsIURI> mURI; >+ const nsCString mSpec; > const nsString mTitle; > > /** >@@ -1083,6 +1048,8 @@ History::VisitURI(nsIURI* aURI, > VisitData place; > rv = aURI->GetSpec(place.spec); > NS_ENSURE_SUCCESS(rv, rv); >+ rv = aURI->GetHost(place.hostname); >+ NS_ENSURE_SUCCESS(rv, rv); Does this create problems to uris not having a host, like local pages? is this a behavioral change from before? it should not warn at least nothing major, but I'm just clearing the flag since I'd like to have the crash clarified.

Crash happened here: http://hg.mozilla.org/try/file/81cb6a299a2a/toolkit/components/places/src/History.cpp#l756 Not sure why yet...

Ah, the crash is at shutdown, so I bet the statement got finalized. We need bug 612455 to fix that.

Addresses feedback.

Rebased on top of places tip.

Comment on attachment 491215 [details] [diff] [review] v1.1 >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp > PRInt64 placeId; > PRInt64 visitId; > PRInt64 sessionId; > nsCString spec; >- nsCOMPtr<nsIURI> uri; >+ nsString reversedHost; nit: if you use revHost it is coherent with icons, but I don't have preferences, you can let it like it is >@@ -1101,6 +1044,7 @@ History::VisitURI(nsIURI* aURI, > VisitData place; > rv = aURI->GetSpec(place.spec); > NS_ENSURE_SUCCESS(rv, rv); >+ GetReversedHostname(aURI, place.reversedHost); nit: cast to void (I seem to recall this returns a nsresult Regarding the crash I'm fine if we handle it in bug 612455. I think there is more than just finalizing on the right thread, I sent you some message on irc, I'll also post there.

(In reply to comment #8) > nit: cast to void (I seem to recall this returns a nsresult Oy. The string one returns void, the other nsresult: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/Helpers.h#161

per comments