To make way for the work on bug 1435827, we must add a shutdown method to complement DllBlocklist_Initialize().

Can you please give me some more context around the need for this patch? Why do we now need to explicitly shut down the blocklist?

It feels like a natural pairing with DllBlocklist_Initialize(), so anything we add there we now have a place to clean it up. That's the hypothetical justification. The practical justification is that the work for bug 1435827 will need a cleanup function. Considering this patch is basically just a carefully-placed NOP, do you recommend we just roll it into bug 1435827?

Comment on attachment 8986740 [details] Bug 1467736: Add support for DllBlocklist_Shutdown; https://reviewboard.mozilla.org/r/252036/#review261396 It's fine to do this in a separate bug, but I am going to mark r- because I don't want us to export this routine; it makes it too easy for somebody to just call it randomly and disable our blocklist. I think the best thing to do here is to add it to DLL services as a new API. Take a look at what we do with the Authenticode interface in DllServicesBase for prior art.

Comment on attachment 8986740 [details] Bug 1467736: Add support for DllBlocklist_Shutdown; https://reviewboard.mozilla.org/r/252036/#review261396 New revision moves the shutdown to a new export, DllBlocklist_GetAPI(), which provides the same obfuscation as DllBlocklist_SetDllServices(), except stays a bit separate from DllServices. The issue with DllServices is that it's used for multiple unrelated things, but calling DllBlocklist_SetDllServices is designed to operate with only one DllService. This way we can be sure we're not clobbering any previous calls.

Comment on attachment 8986740 [details] Bug 1467736: Add support for DllBlocklist_Shutdown; https://reviewboard.mozilla.org/r/252036/#review262696 I really don't want any new exports from mozglue for this functionality. Is there a particular reason why it cannot be added to DllServicesBase?

Comment on attachment 8986740 [details] Bug 1467736: Add support for DllBlocklist_Shutdown; https://reviewboard.mozilla.org/r/252036/#review262696 The obfuscation provided by DllServices is basically a vtable layout, which is the idea behind DllBlocklist_GetAPI. It would be better if the obfuscation was stronger indeed. How about we make DllBlocklist_Initialize() return a random number "cookie" that must be passed to GetAPI() in order for it to work. That would increase obfuscation a lot. The problem with DllServices is that subsequent calls to SetDllServices invalidates previous calls. So it's designed to really be set from a single place in code. It would probably work here because this is the final shutdown call so we don't expect to use DllServices further. But it adds some weird relationships that I don't like. In effect DllServices is currently acting like a two-way API for mozglue. My GetAPI() call is basically the same thing, but only one way to eliminate any problems of overwriting previous calls.

aklotz: do you think the "API cookie" idea is sufficient for this? It would surpass the level of obfuscation provided by DllServices and prevent added complexity.

(In reply to Carl Corcoran [:ccorcoran] from comment #8) OK, it looks like we've been talking past each other, but now I see where our confusion is. > The problem with DllServices is that subsequent calls to SetDllServices > invalidates previous calls. So it's designed to really be set from a single > place in code. It would probably work here because this is the final > shutdown call so we don't expect to use DllServices further. But it adds > some weird relationships that I don't like. To prevent this very problem, we have the concrete https://searchfox.org/mozilla-central/source/toolkit/xre/WinDllServices.h#17 on the XUL side, which acts as a reference-counted singleton for DLL services. It is first instantiated on the main thread at https://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4392 and remains live until XRE_mainRun goes out of scope. It may then be accessed from anywhere within XUL, such as what I did for the modules ping in telemetry: https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#812 If you add the API to DllServicesBase, then the XUL singleton will then automagically gain the new API and you won't have any lifetime issues as long as you use mozilla::DllServices::Get() to retrieve the instance. (Note that we shouldn't piggyback blocklist shutdown atop SetDllServices(nullptr), since that just creates another backdoor for somebody to do something malicious) I should have gone into more detail about how the Gecko front-end to DllServices works, sorry about that. > aklotz: do you think the "API cookie" idea is sufficient for this? I'd still like us to use DLL Services, but I *do* like the idea of using a cookie for the blocklisting APIs! Please file a follow-up bug so that we can explore that further.

A shutdown call doesn't always have access to XUL (e.g. https://searchfox.org/mozilla-central/source/ipc/app/MozillaRuntimeMain.cpp#18), so we can't use WinDllServices.h for this. Presumably the reason DllServices is a base class is to specifically address the issue of callers having different requirements / dependencies, so my instinct was to use the appropriate DllServices child: BasicDllServices, which doesn't depend on XUL. But that leads to the issue I mentioned where subsequent calls to SetDllServices will cause side-effects (at least in principle). We could just try and keep XUL-dependent-DllServices calls within XUL, but that feels a bit dirty & arbitrary, and in this specific case may force us to Shutdown() earlier than we want. I still feel a mozglue-owned API is the cleanest and least-complex solution.

Comment on attachment 8986740 [details] Bug 1467736: Add support for DllBlocklist_Shutdown; https://reviewboard.mozilla.org/r/252036/#review267920 All of your callsites need to be updated with the revised name, as well as checking for DEBUG.

clearing ni?

Comment on attachment 8986740 [details] Bug 1467736: Add support for DllBlocklist_Shutdown; https://reviewboard.mozilla.org/r/252036/#review267920 DllBlocklist_DebugShutdown is an inline function encapsulating the DEBUG check and the call to the exported function. So the callsites don't need to be updated.

MozReview-Commit-ID: 9FCEeInmopX

Comment on attachment 9004892 [details] Bug 1467736: Add support for DllBlocklist_Shutdown;r=aklotz Aaron Klotz [:aklotz] has approved the revision.

Pushed by aklotz@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bda9e323c7fb Add support for DllBlocklist_Shutdown;r=aklotz