679945 - Add DLL Blocklist telemetry

Status: RESOLVED INVALID

(Toolkit :: Blocklist Policy Requests, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We should add telemetry for the DLL blocklist.  Specifically of interest is whether or not the hook installed properly and which DLLs if any were blocked from loading.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This reports whether or not the blocklist hook was installed successfully.  Reporting DLLs blocked would be a lot trickier.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/projects/build-system/rev/1b2c92b52298

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/1b2c92b52298

Comment 4

[Mike Hommey [:glandium]]

As mentioned on IRC the following may be required for comm-central:

    diff --git a/toolkit/xre/nsWindowsDllBlocklist.cpp b/toolkit/xre/nsWindowsDllBlocklist.cpp
    --- a/toolkit/xre/nsWindowsDllBlocklist.cpp
    +++ b/toolkit/xre/nsWindowsDllBlocklist.cpp
    @@ -221,14 +221,18 @@ WindowsDllInterceptor NtDllIntercept;
     
     void
     XRE_SetupDllBlocklist()
     {
       NtDllIntercept.Init("ntdll.dll");
     
       bool ok = NtDllIntercept.AddHook("LdrLoadDll", reinterpret_cast<intptr_t>(patched_LdrLoadDll), (void**) &stub_LdrLoadDll);
     
    +#ifdef XRE_WANT_DLL_BLOCKLIST
    +  XRE_TelemetryAccumulate(Telemetry::DLLBLOCKLIST_HOOK_INSTALLED, ok ? 1 : 0);
    +#else
       Telemetry::Accumulate(Telemetry::DLLBLOCKLIST_HOOK_INSTALLED, ok ? 1 : 0);
    +#endif
     #ifdef DEBUG
       if (!ok)
         printf_stderr ("LdrLoadDll hook failed, no dll blocklisting active\n");
     #endif
     }

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/d9b9693feb46

Comment 6

[Mark Banner (:standard8)]

(In reply to Mike Hommey [:glandium] from comment #4)
> As mentioned on IRC the following may be required for comm-central:

Thunderbird is up to date wrt this code now and I just did a compile with the patch and it succeeded.

Dunno about SeaMonkey (I suspect they are still blocking this, or you need the ifdef).

Comment 7

[Justin Wood (:Callek)]

(In reply to Mark Banner (:standard8) from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #4)
> > As mentioned on IRC the following may be required for comm-central:
> 
> Thunderbird is up to date wrt this code now and I just did a compile with
> the patch and it succeeded.
> 
> Dunno about SeaMonkey (I suspect they are still blocking this, or you need
> the ifdef).

That (c#4) would be required for SeaMonkey in the short term, failing that I'm ok with using the "introduced for seamonkey" define for this, or changing the define name.

(http://hg.mozilla.org/mozilla-central/rev/1985330ca298)

I am _hoping_ to get this code up to date for SeaMonkey in the Gecko 10 cycle.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is Seamonkey ready to go?  I'd like to land this at some point.

Comment 9

[Justin Wood (:Callek)]

(In reply to Justin Wood (:Callek) from comment #7)
> I am _hoping_ to get this code up to date for SeaMonkey in the Gecko 10
> cycle.

Unfortunately, I did not get SeaMonkey ready yet, it is on my priority list, but got sidetracked with critical releng machine failures and bringing them back.

I am ok with landing this with ifndef MOZ_SUITE around the telemetry stuff if you like, or we can do c#4's solution if it works.

Comment 10

[Nathan Froyd [:froydnj]]

We do want this; it'd be nice to get whatever issues are still outstanding sorted.  Patch will require some updating.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Do we still want this and should move this to a more fitting component?
Or should we close it?

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> Do we still want this and should move this to a more fitting component?
> Or should we close it?

I think we still want this, and we should do the following:

1. Reland the original patch.  Thunderbird apparently works OK with this; SeaMonkey can submit patches to fix compilation problems.
2. Move the "report DLLs blocked" telemetry to a different bug.
3. Close this bug.

Comment 13

[Georg Fritzsche [:gfritzsche]]

This doesn't apply anymore as is.
This uses TelemetryHistograms.h, we now have to register histograms in Histograms.json [0].
This will also need an owner and data collection review [1].

Moving to the blocklisting component.

0: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json
1: https://wiki.mozilla.org/Firefox/Data_Collection

Comment 14

[Jorge Villalobos [:jorgev] (he/him)]

Nathan, is there any interest in updating this patch? This bug hasn't had any activity for a very long time.

Comment 15

[Jorge Villalobos [:jorgev] (he/him)]

Closing old blocklist requests that shouldn't be valid after the move to WebExtensions-only in Firefox 57. Please comment if you think this bug is still valid and should be reopened.