DataStorage nsThread isn't shut down in xpcshell runs

RESOLVED FIXED in Firefox 49

Status

()

Core
XPCOM
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.)
(Assignee)

Comment 1

2 years ago
Created attachment 8750438 [details] [diff] [review]
name and cleanup SSL DataStorage thread when running XPCshell

MozReview-Commit-ID: 2brXgEcp91J
Attachment #8750438 - Flags: review?(nfroyd)
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...
Attachment #8750438 - Flags: review?(nfroyd)
Attachment #8750438 - Flags: review?(dkeeler)
Attachment #8750438 - Flags: review+
(Assignee)

Comment 3

2 years ago
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.
(Assignee)

Updated

2 years ago
Summary: Flag threads that aren't shut down before nsThreadManager::Shutdown() → DataStorage nsThread isn't shut down in xpcshell runs
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.
Attachment #8750438 - Flags: review?(dkeeler) → review+

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b4ddf7b098af
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.