Add retry logic to V8Initializer snapshot loading files for suspected A/V problem.

Add retry logic to V8Initializer snapshot loading files for suspected A/V problem.

- Histograms in V8Initializer now collect data as we're unable to repro issue locally.

- Call base::StatisticsRecorder::Initialize() before calling into gin::V8Initializer.

BUG=479537
Committed: https://crrev.com/575f7fb5bbd8a5e20431f5dabb4cffd2f8bcf545
Cr-Commit-Position: refs/heads/master@{#328978}

[oth, 2015-05-07 09:53:46]

oth@chromium.org changed reviewers:
+ asvitkine@chromium.org, jochen@chromium.org

[oth, 2015-05-07 09:53:46]

asvitkine@chromium.org: Please review changes in tools/metrics/histograms.

jochen@chromium.org: Please review changes in content and gin.

[jochen (gone - plz use gerrit), 2015-05-07 15:03:18]

jochen@chromium.org changed reviewers:
+ cpu@chromium.org

[jochen (gone - plz use gerrit), 2015-05-07 15:03:19]

+cpu

Carlos, is it indeed the case that everything that opens files on windows has to
manually retry in case some AV thinks it should first scan the file?

https://codereview.chromium.org/1127183004/diff/1/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1127183004/diff/1/content/app/content_main_ru...
content/app/content_main_runner.cc:452: 
revert?

[jochen (gone - plz use gerrit), 2015-05-07 15:03:43]

or phrased differently, why is there nothing in base/ that does this?

[oth, 2015-05-07 15:54:54]

On 2015/05/07 15:03:43, jochen wrote:
> or phrased differently, why is there nothing in base/ that does this?

There was push back on doing this in base. In fairness, I don't think we
understand the problem very well, particularly as we can't repro locally.

The current code paths end up opening the snapshot files with
FILE_SHARE_READ|FILE_SHARE_WRITE. The KB article -
https://support.microsoft.com/en-us/kb/316609 - identifies this as problematic
with A/V.

Two alternatives might be:

1) The V8 snapshot code opens with File::FILE_WRITE_EXCLUSIVE as this would
avoid setting FILE_SHARE_WRITE flag in base's call to CreateFile. There are no
writes to the file anyway (it's just with read permission).

2) base only sets FILE_SHARE_WRITE if write access is requested without
File::FILE_WRITE_EXCLUSIVE.

[jochen (gone - plz use gerrit), 2015-05-07 15:59:51]

OK, if that's a problem unique to this use case, lgtm

[Alexei Svitkine (slow), 2015-05-07 17:09:09]

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

https://codereview.chromium.org/1127183004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:11410: +<histogram
name="gin.V8Initializer.LoadV8Snapshot.Result"
This seems to be adding a new top-level prefix for histograms - is it possible
to re-use an existing prefix instead, for e.g. V8.*?

[cpu_(ooo_6.6-7.5), 2015-05-08 02:55:34]

My guess is that the AV is actually blocking the file. I am ok to try this so
lgtm but I strongly urge you to remove it if the field issue does not go away.

This is one of the cases where the cure can be worse than the disease.

And for the record almost all file opens in chrome should be done with
share-for-read. Because of AVs.  I don't think share-for-write is something that
we need to handle.

[oth, 2015-05-08 09:21:53]

Thanks all, this change incorporates the feedback.

https://codereview.chromium.org/1127183004/diff/1/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1127183004/diff/1/content/app/content_main_ru...
content/app/content_main_runner.cc:452: 
On 2015/05/07 15:03:19, jochen wrote:
> revert?

Done.

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

https://codereview.chromium.org/1127183004/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:11410: +<histogram
name="gin.V8Initializer.LoadV8Snapshot.Result"
On 2015/05/07 17:09:09, Alexei Svitkine wrote:
> This seems to be adding a new top-level prefix for histograms - is it possible
> to re-use an existing prefix instead, for e.g. V8.*?

Done.

[Alexei Svitkine (slow), 2015-05-08 14:56:34]

histograms lgtm

[oth, 2015-05-08 16:07:19]

On 2015/05/08 02:55:34, cpu wrote:
> My guess is that the AV is actually blocking the file. I am ok to try this so
> lgtm but I strongly urge you to remove it if the field issue does not go away.
> 
> This is one of the cases where the cure can be worse than the disease.
> 
> And for the record almost all file opens in chrome should be done with
> share-for-read. Because of AVs.  I don't think share-for-write is something
that
> we need to handle.

Agree totally here and don't want this to be a long term fix.

It looks like FILE_SHARE_WRITE is set in file_win.cc unless FILE_EXCLUSIVE_WRITE
is set:

 
https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_wi...

Tried to finesse this a bit today, but there are some test failures as a result:

  https://codereview.chromium.org/1133753002/diff/60001/base/files/file_win.cc

[oth, 2015-05-08 16:10:15]

The CQ bit was checked by oth@chromium.org

[oth, 2015-05-08 16:10:16]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
cpu@chromium.org
Link to the patchset: https://codereview.chromium.org/1127183004/#ps20001
(title: "Incorporate review comments on patch set 1.")

[commit-bot: I haz the power, 2015-05-08 16:11:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1127183004/20001

[commit-bot: I haz the power, 2015-05-08 17:35:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-08 17:35:55]

Patchset 2 (id:??) landed as
https://crrev.com/575f7fb5bbd8a5e20431f5dabb4cffd2f8bcf545
Cr-Commit-Position: refs/heads/master@{#328978}