Add a UMA stat to track if the Browser blacklist is Set on the Renderer

Add a UMA stat to track if the Browser blacklist is Set on the Renderer

This shouldn't be happening, but we got some crash reports suggesting it
does. Unable to repo locally so this stat will verify it does occur and
then can be used to verify our fixes actually fix it.

BUG=329023
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250828

[robertshield, 2014-02-06 18:55:23]

https://codereview.chromium.org/140763008/diff/1/chrome_elf/blacklist/blackli...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/140763008/diff/1/chrome_elf/blacklist/blackli...
chrome_elf/blacklist/blacklist.cc:32: bool blacklist_patched = false;
g_blacklist_patched

https://codereview.chromium.org/140763008/diff/1/chrome_elf/blacklist/blackli...
File chrome_elf/blacklist/blacklist.h (right):

https://codereview.chromium.org/140763008/diff/1/chrome_elf/blacklist/blackli...
chrome_elf/blacklist/blacklist.h:23: // Record if the blacklist was patched in
so processes can easily determine
s/patched in/successfully initialized/

https://codereview.chromium.org/140763008/diff/1/content/renderer/renderer_ma...
File content/renderer/renderer_main.cc (right):

https://codereview.chromium.org/140763008/diff/1/content/renderer/renderer_ma...
content/renderer/renderer_main.cc:247: if (blacklist::blacklist_patched)
I don't believe this will work. blacklist_patched here will refer to a copy of
the variable linked into chrome.dll, not the one in chrome_elf.dll that actually
gets set.

[csharp, 2014-02-06 20:03:57]

https://codereview.chromium.org/140763008/diff/1/chrome_elf/blacklist/blackli...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/140763008/diff/1/chrome_elf/blacklist/blackli...
chrome_elf/blacklist/blacklist.cc:32: bool blacklist_patched = false;
On 2014/02/06 18:55:23, robertshield wrote:
> g_blacklist_patched

Done.

https://codereview.chromium.org/140763008/diff/1/chrome_elf/blacklist/blackli...
File chrome_elf/blacklist/blacklist.h (right):

https://codereview.chromium.org/140763008/diff/1/chrome_elf/blacklist/blackli...
chrome_elf/blacklist/blacklist.h:23: // Record if the blacklist was patched in
so processes can easily determine
On 2014/02/06 18:55:23, robertshield wrote:
> s/patched in/successfully initialized/

Done.

https://codereview.chromium.org/140763008/diff/1/content/renderer/renderer_ma...
File content/renderer/renderer_main.cc (right):

https://codereview.chromium.org/140763008/diff/1/content/renderer/renderer_ma...
content/renderer/renderer_main.cc:247: if (blacklist::blacklist_patched)
On 2014/02/06 18:55:23, robertshield wrote:
> I don't believe this will work. blacklist_patched here will refer to a copy of
> the variable linked into chrome.dll, not the one in chrome_elf.dll that
actually
> gets set.

Ah yup. Fixed (and tested to make sure it works).

[robertshield, 2014-02-06 20:30:38]

On 2014/02/06 20:03:57, csharp wrote:
>
https://codereview.chromium.org/140763008/diff/1/chrome_elf/blacklist/blackli...
> File chrome_elf/blacklist/blacklist.cc (right):
> 
>
https://codereview.chromium.org/140763008/diff/1/chrome_elf/blacklist/blackli...
> chrome_elf/blacklist/blacklist.cc:32: bool blacklist_patched = false;
> On 2014/02/06 18:55:23, robertshield wrote:
> > g_blacklist_patched
> 
> Done.
> 
>
https://codereview.chromium.org/140763008/diff/1/chrome_elf/blacklist/blackli...
> File chrome_elf/blacklist/blacklist.h (right):
> 
>
https://codereview.chromium.org/140763008/diff/1/chrome_elf/blacklist/blackli...
> chrome_elf/blacklist/blacklist.h:23: // Record if the blacklist was patched in
> so processes can easily determine
> On 2014/02/06 18:55:23, robertshield wrote:
> > s/patched in/successfully initialized/
> 
> Done.
> 
>
https://codereview.chromium.org/140763008/diff/1/content/renderer/renderer_ma...
> File content/renderer/renderer_main.cc (right):
> 
>
https://codereview.chromium.org/140763008/diff/1/content/renderer/renderer_ma...
> content/renderer/renderer_main.cc:247: if (blacklist::blacklist_patched)
> On 2014/02/06 18:55:23, robertshield wrote:
> > I don't believe this will work. blacklist_patched here will refer to a copy
of
> > the variable linked into chrome.dll, not the one in chrome_elf.dll that
> actually
> > gets set.
> 
> Ah yup. Fixed (and tested to make sure it works).

Can this be added to the existing unit tests?

[csharp, 2014-02-06 21:36:19]

Add the initialized check to the test and moved around where the variable was
defined a bit (reduced the scope of it).

[robertshield, 2014-02-06 21:41:57]

lgtm

[csharp, 2014-02-06 21:49:01]

asvitkine@ for owner's approval of histograms.xml
jamesr@ for owner's approval of content/render/*

[Alexei Svitkine (slow), 2014-02-06 22:12:29]

https://codereview.chromium.org/140763008/diff/100002/content/renderer/render...
File content/renderer/renderer_main.cc (right):

https://codereview.chromium.org/140763008/diff/100002/content/renderer/render...
content/renderer/renderer_main.cc:248:
UMA_HISTOGRAM_BOOLEAN("Blacklist.PatchedInRenderer", true);
I would actually suggest:

UMA_HISTOGRAM_BOOLEAN("Blacklist.PatchedInRenderer",
blacklist::IsBlacklistInitialized());

That way, the histogram will tell you the ratio of the two, rather than the
absolute count which is hard to analyze without a baseline to compare to.

Unless of course your question is only "is this happening at all", in which case
the current way is fine.

[csharp, 2014-02-06 22:21:58]

https://codereview.chromium.org/140763008/diff/100002/content/renderer/render...
File content/renderer/renderer_main.cc (right):

https://codereview.chromium.org/140763008/diff/100002/content/renderer/render...
content/renderer/renderer_main.cc:248:
UMA_HISTOGRAM_BOOLEAN("Blacklist.PatchedInRenderer", true);
On 2014/02/06 22:12:29, Alexei Svitkine wrote:
> I would actually suggest:
> 
> UMA_HISTOGRAM_BOOLEAN("Blacklist.PatchedInRenderer",
> blacklist::IsBlacklistInitialized());
> 
> That way, the histogram will tell you the ratio of the two, rather than the
> absolute count which is hard to analyze without a baseline to compare to.
> 
> Unless of course your question is only "is this happening at all", in which
case
> the current way is fine.

We just want the raw numbers since ideally this should always be false (we only
want to blacklist the browser process).

[Alexei Svitkine (slow), 2014-02-06 22:26:04]

lgtm

https://codereview.chromium.org/140763008/diff/100002/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/140763008/diff/100002/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1281: +    blacklist patch.
Maybe expand comment to mention that the expectation is this is never hit.

[jamesr, 2014-02-07 05:39:48]

Why don't you histogram this in the blacklist init code?  It doesn't seem right
to have xxx/renderer/ code reaching in to this.  It's definitely wrong to have
_content_/renderer/ code reaching in to this - this is not a content-level
feature.

[jamesr, 2014-02-07 05:40:17]

https://codereview.chromium.org/140763008/diff/100002/content/renderer/DEPS
File content/renderer/DEPS (right):

https://codereview.chromium.org/140763008/diff/100002/content/renderer/DEPS#n...
content/renderer/DEPS:2: "+chrome_elf",
not lgtm on this part in particular. content/ shouldn't depend on any
chrome-level functionality

[csharp, 2014-02-07 14:56:25]

The blacklist setup code happens really early in the process setup (before
histograms and a lot of libraries are loaded) so we can't record any stats
there.

The histogram setup code doesn't seem to be run until renderer_main.cc:188 so
our code needs to run after that (I think).

I don't know enough about the renderer startup process to know where I can move
this to make it work. Any advice on where I should move it to? Thanks

https://codereview.chromium.org/140763008/diff/100002/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/140763008/diff/100002/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1281: +    blacklist patch.
On 2014/02/06 22:26:05, Alexei Svitkine wrote:
> Maybe expand comment to mention that the expectation is this is never hit.

Done.

[csharp, 2014-02-10 14:52:49]

On 2014/02/07 14:56:25, csharp wrote:
> The blacklist setup code happens really early in the process setup (before
> histograms and a lot of libraries are loaded) so we can't record any stats
> there.
> 
> The histogram setup code doesn't seem to be run until renderer_main.cc:188 so
> our code needs to run after that (I think).
> 
> I don't know enough about the renderer startup process to know where I can
move
> this to make it work. Any advice on where I should move it to? Thanks
> 
>
https://codereview.chromium.org/140763008/diff/100002/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/140763008/diff/100002/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:1281: +    blacklist patch.
> On 2014/02/06 22:26:05, Alexei Svitkine wrote:
> > Maybe expand comment to mention that the expectation is this is never hit.
> 
> Done.

Ping

[jamesr, 2014-02-10 22:47:54]

On 2014/02/10 14:52:49, csharp wrote:
> Ping

Who are you pinging specifically?  As I've said content/ should not be depending
on this code.

[robertshield, 2014-02-11 14:51:31]

On 2014/02/10 22:47:54, jamesr wrote:
> On 2014/02/10 14:52:49, csharp wrote:
> > Ping
> 
> Who are you pinging specifically?  As I've said content/ should not be
depending
> on this code.

It looks like Chris' earlier question might have been missed, the ping was re.
his question just above:

> I don't know enough about the renderer startup process to know where I can
move
> this to make it work. Any advice on where I should move it to? Thanks

He's since found ChromeContentRendererClient which looks more suitable.

[csharp, 2014-02-11 15:07:02]

Adding sky@ for owner's approval of chrome/renderer changes 

ChromeContentRendererClient seems to be a correct home for this code, but let me
know if I'm mistaken.

[sky, 2014-02-11 15:10:03]

LGTM

[sky, 2014-02-11 15:10:58]

https://codereview.chromium.org/140763008/diff/500001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/140763008/diff/500001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:371:
UMA_HISTOGRAM_BOOLEAN("Blacklist.PatchedInRenderer", true);
Actually, one comment. Don't you want to remove the if and always do the
histogram? Otherwise isn't the value always going to be true?

[csharp, 2014-02-11 15:17:01]

https://codereview.chromium.org/140763008/diff/500001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/140763008/diff/500001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:371:
UMA_HISTOGRAM_BOOLEAN("Blacklist.PatchedInRenderer", true);
On 2014/02/11 15:10:59, sky wrote:
> Actually, one comment. Don't you want to remove the if and always do the
> histogram? Otherwise isn't the value always going to be true?

Nope, the blacklist initialization code has a check that is suppose to stop the
setup if it isn't in the browser process, but we got some crashes showing that
it was getting loaded (which we couldn't repo locally).

So this patch should show us how often we are incorrectly patching the renderer
process, and then hopefully fall to zero as we fix our check to ensure it is
only setup for the browser process.

[sky, 2014-02-11 15:25:03]

LGTM

[csharp, 2014-02-11 15:45:46]

The CQ bit was checked by csharp@chromium.org

[commit-bot: I haz the power, 2014-02-11 15:45:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/140763008/660001

[commit-bot: I haz the power, 2014-02-11 16:44:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-11 16:44:56]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[csharp, 2014-02-11 19:37:58]

The CQ bit was checked by csharp@chromium.org

[commit-bot: I haz the power, 2014-02-11 19:39:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/140763008/660001

[commit-bot: I haz the power, 2014-02-11 21:29:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/140763008/660001

[commit-bot: I haz the power, 2014-02-11 22:06:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/140763008/660001

[csharp, 2014-02-11 23:54:39]

The CQ bit was unchecked by csharp@chromium.org

[csharp, 2014-02-12 15:03:58]

The CQ bit was checked by csharp@chromium.org

[commit-bot: I haz the power, 2014-02-12 15:04:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/140763008/660001

[csharp, 2014-02-12 15:04:54]

The CQ bit was unchecked by csharp@chromium.org

[csharp, 2014-02-12 17:40:10]

maruel@ for quick sanity check of .isolate changes

[M-A Ruel, 2014-02-12 17:44:26]

lgtm

https://codereview.chromium.org/140763008/diff/1010001/chrome/interactive_ui_...
File chrome/interactive_ui_tests.isolate (right):

https://codereview.chromium.org/140763008/diff/1010001/chrome/interactive_ui_...
chrome/interactive_ui_tests.isolate:79: '<(PRODUCT_DIR)/chrome_elf.dll',
Is it there always? It's currently not a big deal, just asking.

[csharp, 2014-02-12 17:51:31]

https://codereview.chromium.org/140763008/diff/1010001/chrome/interactive_ui_...
File chrome/interactive_ui_tests.isolate (right):

https://codereview.chromium.org/140763008/diff/1010001/chrome/interactive_ui_...
chrome/interactive_ui_tests.isolate:79: '<(PRODUCT_DIR)/chrome_elf.dll',
On 2014/02/12 17:44:28, M-A Ruel wrote:
> Is it there always? It's currently not a big deal, just asking.

It should be. It is a dependency of renderer, which is a dependency of
interactive_ui_tests.

[csharp, 2014-02-12 17:51:41]

The CQ bit was checked by csharp@chromium.org

[commit-bot: I haz the power, 2014-02-12 17:54:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/140763008/1010001

[commit-bot: I haz the power, 2014-02-12 19:09:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-12 19:09:16]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[csharp, 2014-02-12 19:14:53]

The CQ bit was checked by csharp@chromium.org

[commit-bot: I haz the power, 2014-02-12 19:16:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/140763008/1010001

[commit-bot: I haz the power, 2014-02-12 23:16:58]

Change committed as 250828