The security DataStorage thread doesn't cleanly exit if profile-before-change isn't sent, and it isn't sent for xpcshell (for example when running ./mach package). Also, the thread doesn't have a name, making it hard to figure out what it is in a log message or debugger. (And the setting of the thread name is late in SocketTransportThread, though that doesn't matter a lot.)

MozReview-Commit-ID: 2brXgEcp91J

Comment on attachment 8750438 [details] [diff] [review] name and cleanup SSL DataStorage thread when running XPCshell Review of attachment 8750438 [details] [diff] [review]: ----------------------------------------------------------------- I think the idempotency thing below is actually OK, but I'm not terribly familiar with the DataStorage code, so I'm going to ask keeler to take a look. ::: security/manager/ssl/DataStorage.cpp @@ +866,5 @@ > MutexAutoLock lock(mMutex); > mPrivateDataTable.Clear(); > + } else if (strcmp(aTopic, "profile-before-change") == 0 || > + (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0 && > + XRE_IsParentProcess())) { Is this shutdown process idempotent? We're now doing this twice where we were only doing it once before. e.g. there's a Shutdown of mWorkerThread a couple of lines below here...I think the AsyncWriteData is idempotent...

We could tweak it to trivially to clear the xpcom-shutdown observer on profile-before-change, or to clear mWorkerThread and test that before calling Shutdown, if those are needed.

Comment on attachment 8750438 [details] [diff] [review] name and cleanup SSL DataStorage thread when running XPCshell Review of attachment 8750438 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/DataStorage.cpp @@ +105,5 @@ > mInitCalled = true; > > nsresult rv; > if (XRE_IsParentProcess()) { > + rv = NS_NewNamedThread("SSL DataStorage", getter_AddRefs(mWorkerThread)); nit: I would just call this "DataStorage" - it's meant to be contents-agnostic. @@ +866,5 @@ > MutexAutoLock lock(mMutex); > mPrivateDataTable.Clear(); > + } else if (strcmp(aTopic, "profile-before-change") == 0 || > + (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0 && > + XRE_IsParentProcess())) { I believe this whole section is idempotent. AsyncWriteData returns early if mShuttingDown is true (which gets set just after calling it the first time). DispatchShutdownTimer causes mTimer to be nulled (after waiting on the thread). Also, it looks like mThread->Shutdown() just returns NS_OK if called multiple times. Finally, sDataStorages->Clear() is just a hash table clear and should be fine to call multiple times.