Made setting lowbox token a warning.

Made setting lowbox token a warning.
This patch makes setting of the lowbox toke a warning rather than a hard
failure. Some configurations cause the setting of the token to fail, this
is most likely issues with third party software. Worst case this will mean
that the lowbox token is not set, but we'll continue with the original
restricted token sandbox.

BUG=501975
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng

Committed: https://crrev.com/351566a4e91d7f5c204c6c0881986f6bb385a04b
Cr-Commit-Position: refs/heads/master@{#405173}

[forshaw, 2016-07-07 10:10:39]

Description was changed from

==========
Made setting lowbox token a warning.
This patch makes setting of the lowbox toke a warning rather than a hard
failure. Some configurations cause the setting of the token to fail, this
is most likely issues with third party software. Worst case this will mean
that the lowbox token is not set, but we'll continue with the original
restricted token sandbox.

BUG=501975
==========

to

==========
Made setting lowbox token a warning.
This patch makes setting of the lowbox toke a warning rather than a hard
failure. Some configurations cause the setting of the token to fail, this
is most likely issues with third party software. Worst case this will mean
that the lowbox token is not set, but we'll continue with the original
restricted token sandbox.

BUG=501975
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[forshaw, 2016-07-12 11:08:35]

forshaw@chromium.org changed reviewers:
+ isherman@chromium.org, wfh@chromium.org

[forshaw, 2016-07-12 11:08:35]

Can you take a look at this patch to make failing to apply the lowbox token a
warning.
wfh@ obviously the main body of the patch to look at.
isherman@ could you check the histogram additions.

Thanks.

[Will Harris, 2016-07-12 16:10:59]

looks good, few suggestions.

https://codereview.chromium.org/2130753002/diff/1/content/common/sandbox_win.cc
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/2130753002/diff/1/content/common/sandbox_win....
content/common/sandbox_win.cc:805: DWORD last_error = ::GetLastError();
Seems anachronistic to pass the last_error by SetLastError() and GetLastError()
but pass last_warning via param, I wonder if this code could be changed in a
future CL.

https://codereview.chromium.org/2130753002/diff/1/content/common/sandbox_win....
content/common/sandbox_win.cc:821:
UMA_HISTOGRAM_SPARSE_SLOWLY("Process.Sandbox.Launch.WarningResultCode",
it might be an idea to always log this first histogram even when last_warning is
SBOX_ALL_OK then we get an accurate bucket percentage for each type.

https://codereview.chromium.org/2130753002/diff/1/sandbox/win/src/broker_serv...
File sandbox/win/src/broker_services.cc (right):

https://codereview.chromium.org/2130753002/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:428: if (lowbox_token.IsValid()) {
comment here on why this does not abort

https://codereview.chromium.org/2130753002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2130753002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:43761: +    error code please refer to
http://goo.gl/fJJiAv.
explain exactly when these are both logged

[forshaw, 2016-07-12 21:01:44]

On 2016/07/12 16:10:59, Will Harris wrote:
> looks good, few suggestions.
> 
>
https://codereview.chromium.org/2130753002/diff/1/content/common/sandbox_win.cc
> File content/common/sandbox_win.cc (right):
> 
>
https://codereview.chromium.org/2130753002/diff/1/content/common/sandbox_win....
> content/common/sandbox_win.cc:805: DWORD last_error = ::GetLastError();
> Seems anachronistic to pass the last_error by SetLastError() and
GetLastError()
> but pass last_warning via param, I wonder if this code could be changed in a
> future CL.
> 
>
https://codereview.chromium.org/2130753002/diff/1/content/common/sandbox_win....
> content/common/sandbox_win.cc:821:
> UMA_HISTOGRAM_SPARSE_SLOWLY("Process.Sandbox.Launch.WarningResultCode",
> it might be an idea to always log this first histogram even when last_warning
is
> SBOX_ALL_OK then we get an accurate bucket percentage for each type.
> 
>
https://codereview.chromium.org/2130753002/diff/1/sandbox/win/src/broker_serv...
> File sandbox/win/src/broker_services.cc (right):
> 
>
https://codereview.chromium.org/2130753002/diff/1/sandbox/win/src/broker_serv...
> sandbox/win/src/broker_services.cc:428: if (lowbox_token.IsValid()) {
> comment here on why this does not abort
> 
>
https://codereview.chromium.org/2130753002/diff/1/tools/metrics/histograms/hi...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2130753002/diff/1/tools/metrics/histograms/hi...
> tools/metrics/histograms/histograms.xml:43761: +    error code please refer to
> http://goo.gl/fJJiAv.
> explain exactly when these are both logged

Made the changes, PTAL.

[Ilya Sherman, 2016-07-12 21:17:24]

Metrics lgtm.  How much overlap is there between the warning and error codes,
though?  Will both histograms provide useful data?

[forshaw, 2016-07-13 10:43:27]

On 2016/07/12 21:17:24, Ilya Sherman wrote:
> Metrics lgtm.  How much overlap is there between the warning and error codes,
> though?  Will both histograms provide useful data?

There shouldn't be much overlap between the two. If an error occurs then only
the error histogram gets logged. Only if the creation nominally succeeds but has
a warning will the other be logged. In theory this could be all logged in one
histogram and change the error codes to reflect something being a warning versus
an error but this way it might be easier to see if we're getting substantial
failures.

[forshaw, 2016-07-13 10:44:23]

wfh@ PTAL, made final changes you requested.

[Will Harris, 2016-07-13 14:28:34]

On 2016/07/13 10:44:23, forshaw wrote:
> wfh@ PTAL, made final changes you requested.

lgtm

[forshaw, 2016-07-13 14:39:01]

The CQ bit was checked by forshaw@chromium.org

[forshaw, 2016-07-13 14:39:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2130753002/#ps40001
(title: "Reverted change to logging")

[commit-bot: I haz the power, 2016-07-13 14:39:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 16:07:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-13 16:07:42]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 16:11:11]

Description was changed from

==========
Made setting lowbox token a warning.
This patch makes setting of the lowbox toke a warning rather than a hard
failure. Some configurations cause the setting of the token to fail, this
is most likely issues with third party software. Worst case this will mean
that the lowbox token is not set, but we'll continue with the original
restricted token sandbox.

BUG=501975
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Made setting lowbox token a warning.
This patch makes setting of the lowbox toke a warning rather than a hard
failure. Some configurations cause the setting of the token to fail, this
is most likely issues with third party software. Worst case this will mean
that the lowbox token is not set, but we'll continue with the original
restricted token sandbox.

BUG=501975
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng

Committed: https://crrev.com/351566a4e91d7f5c204c6c0881986f6bb385a04b
Cr-Commit-Position: refs/heads/master@{#405173}
==========

[commit-bot: I haz the power, 2016-07-13 16:11:12]

Patchset 3 (id:??) landed as
https://crrev.com/351566a4e91d7f5c204c6c0881986f6bb385a04b
Cr-Commit-Position: refs/heads/master@{#405173}