Closed Bug 1417680 Opened 2 years ago Closed 2 years ago

explore the feasibility of making xpcom responsible for shutting down NSS

Categories

(Core :: Security: PSM, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

Currently we have a complicated and buggy infrastructure for ensuring that NSS resources aren't used after NSS has been shut down. The hypothesis is that by removing this and simply never shutting down NSS, we'll avoid use-after-frees of this sort and potentially even get a performance improvement. Since this infrastructure has its serpentine tendrils everywhere in the codebase, it'll probably be easier to see if this will work by removing the guts first and following-up to remove the remnants later if it's a success.
Comment on attachment 8932233 [details]
bug 1417680 - explore the feasibility of not shutting down NSS by no-op-ing the guts of the shutdown infrastructure r?jcj,franziskus

Franziskus Kiefer [:fkiefer or :franziskus] has approved the revision.

https://phabricator.services.mozilla.com/D292#7957
Attachment #8932233 - Flags: review+
Comment on attachment 8932233 [details]
bug 1417680 - explore the feasibility of not shutting down NSS by no-op-ing the guts of the shutdown infrastructure r?jcj,franziskus

J.C. Jones [:jcj] has approved the revision.

https://phabricator.services.mozilla.com/D292#8052
Attachment #8932233 - Flags: review+
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4928928a5e46
explore the feasibility of not shutting down NSS by no-op-ing the guts of the shutdown infrastructure r=jcj r=franziskus
Thanks for the reviews!
Well, the good news is we confirmed our infrastructure correctly detects the leaks that result from not shutting down NSS. The amended plan is to make xpcom responsible for shutting down NSS after it has joined all threads.
Flags: needinfo?(dkeeler)
Summary: explore the feasibility of not shutting down NSS → explore the feasibility of making xpcom responsible for shutting down NSS
Comment on attachment 8932233 [details]
bug 1417680 - explore the feasibility of not shutting down NSS by no-op-ing the guts of the shutdown infrastructure r?jcj,franziskus

:erahm, would you be able to have a look at the additions to xpcom/build/XPCOMInit.cpp ? The idea here is that rather than trying to do this multi-threaded resource coordination thing we just wait for all threads to be joined and JS and XPCOM to be mostly shut down to call NSS_Shutdown.
Attachment #8932233 - Flags: review?(erahm)
:erahm, just wanted to make sure this was still on your radar.
Flags: needinfo?(erahm)
Comment on attachment 8932233 [details]
bug 1417680 - explore the feasibility of not shutting down NSS by no-op-ing the guts of the shutdown infrastructure r?jcj,franziskus

Eric Rahm [:erahm] (please no mozreview requests) has approved the revision.

https://phabricator.services.mozilla.com/D292#8884
Attachment #8932233 - Flags: review+
(In reply to David Keeler [:keeler] (use needinfo) from comment #9)
> :erahm, just wanted to make sure this was still on your radar.

Reviewed, comments in phabricator.
Flags: needinfo?(erahm)
Attachment #8932233 - Flags: review?(erahm)
Comment on attachment 8932233 [details]
bug 1417680 - explore the feasibility of not shutting down NSS by no-op-ing the guts of the shutdown infrastructure r?jcj,franziskus

Franziskus Kiefer [:fkiefer or :franziskus] has been removed from the revision.
J.C. Jones [:jcj] has been removed from the revision.
Eric Rahm [:erahm] (please no mozreview requests) has been removed from the revision.

https://phabricator.services.mozilla.com/D292#8929
Attachment #8932233 - Flags: review+
Comment on attachment 8932233 [details]
bug 1417680 - explore the feasibility of not shutting down NSS by no-op-ing the guts of the shutdown infrastructure r?jcj,franziskus

J.C. Jones [:jcj] has approved the revision.

https://phabricator.services.mozilla.com/D292#8949
Attachment #8932233 - Flags: review+
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28f876e75d47
explore the feasibility of making XPCOM responsible for shutting down NSS r=jcj r=franziskus r=erahm
Backout by aiakab@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24b4c8683fc4
Backed out changeset 28f876e75d47 for failing GTest
FWIW I think you upset the LateWriteChecks observer, probably need to fix the gtests so we can shutdown NSS properly:

23:12:46     INFO -  TEST-PASS | psm_MD4/psm_MD4.RFC1320TestValues/6 | test completed (time: 0ms)
23:12:46     INFO -  TEST-PASS | GTest unit test: passed
23:12:46     INFO -  Passed: 4793
23:12:46     INFO -  Failed: 0
...snip...
23:12:46     INFO -  [7336, Main Thread] WARNING: NSS_Shutdown failed - some NSS resources are still in use (see bugs 1417680 and 1230312): file z:/build/build/src/xpcom/build/XPCOMInit.cpp, line 1021
23:12:46     INFO -  Hit MOZ_CRASH() at z:/build/build/src/xpcom/build/LateWriteChecks.cpp:116
23:12:46     INFO -  #01: `anonymous namespace'::PerThreadData::CallObservers [xpcom/build/IOInterposer.cpp:141]
23:12:46     INFO -  #02: mozilla::IOInterposer::Report(mozilla::IOInterposeObserver::Observation &) [xpcom/build/IOInterposer.cpp:519]
23:12:46     INFO -  #03: mozilla::IOInterposeObserver::Observation::Report() [xpcom/build/IOInterposer.cpp:422]
23:12:46     INFO -  #04: `anonymous namespace'::InterposedNtWriteFile [xpcom/build/PoisonIOInterposerWin.cpp:337]
Duplicate of this bug: 1274138
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c20c33183201
explore the feasibility of making XPCOM responsible for shutting down NSS r=jcj r=franziskus r=erahm
https://hg.mozilla.org/mozilla-central/rev/c20c33183201
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Duplicate of this bug: 1380457
Duplicate of this bug: 1298372
Duplicate of this bug: 1379846
You need to log in before you can comment on or make changes to this bug.