Closed Bug 612617 Opened 10 years ago Closed 10 years ago

Assertions due to thread-safety issues on Places branch

Categories

(Toolkit :: Places, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 3 obsolete files)

Spun out of bug 599969.
Blocks: 599969
blocking2.0: --- → betaN+
Depends on: 575188
Attached patch v1.0 (obsolete) — Splinter Review
Fix it by never ever passing an nsIURI object to the background thread.  Try results:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=81cb6a299a2a
Attachment #491053 - Flags: review?(mak77)
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.
Attachment #491053 - Flags: review?(mak77)
Ah, the crash is at shutdown, so I bet the statement got finalized.  We need bug 612455 to fix that.
Attached patch v1.1 (obsolete) — Splinter Review
Addresses feedback.
Attachment #491053 - Attachment is obsolete: true
Attachment #491214 - Flags: review?(mak77)
Attached patch v1.1 (obsolete) — Splinter Review
Rebased on top of places tip.
Attachment #491214 - Attachment is obsolete: true
Attachment #491215 - Flags: review?(mak77)
Attachment #491214 - Flags: review?(mak77)
No longer depends on: 575188
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.
Attachment #491215 - Flags: review?(mak77) → review+
(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
Attached patch v1.2Splinter Review
per comments
Attachment #491215 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/61e07145a35f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-places]
Target Milestone: --- → mozilla2.0b9
You need to log in before you can comment on or make changes to this bug.