[Extensions] Fix a data race in ChromeExtensionMessageFilter.

[Extensions] Fix a data race in ChromeExtensionMessageFilter.

There is a data race in OnGetExtMessageBundleOnBlockingPool method. It calls
InfoMap::extensions() on a blocking pool, but all InfoMap methods must be
called on the IO thread.

This CL does the following:
1. Add DCHECK_CURRENTRY_ON(..) in extensions::InfoMap.
   With those DCHECKs browser_tests.ExtensionApiTest.SharedModuleLocale starts
   crashing (because of a wrong thread).
2. Remove calls to IO-only methods from
   LoadMessageBundleSubstitutionMapWithImport and rename that function.
3. Use OnGetExtMessageBundle to collect all necessary information before
   posting a task to a blocking pool.
4. Remove NOTREACHED(), because an extension might be removed
   before OnGetExtMessageBundle message was received.

TEST=browser_tests.ExtensionApiTest.SharedModuleLocale
BUG=

Review-Url: https://codereview.chromium.org/2686463003
Cr-Commit-Position: refs/heads/master@{#451339}
Committed: https://chromium.googlesource.com/chromium/src/+/54e8d335c0e4bebe49fbb2cacc5e49c06f29dc6e

[atuchin, 2017-02-07 15:01:07]

Description was changed from

==========
[Extensions] Fix a data race in ChromeExtensionMessageFilter.

There is a data race in OnGetExtMessageBundleOnBlockingPool method. It calls
InfoMap::extensions() on a blocking pool, but all InfoMap methods must be
called on the IO thread.

This CL does the following:
1. Add DCHECK_CURRENTRY_ON(..) in extensions::InfoMap.
   With those DCHECKs browser_tests.ExtensionApiTest.SharedModuleLocale starts
   crashing (because of a wrong thread).
2. Remove calls to IO-only methods from
   LoadMessageBundleSubstitutionMapWithImport and rename that function.
3. Use OnGetExtMessageBundle to collect all necessary information before
   posting a task to a blocking pool.
4. Remove NOTREACHED(), because an extension might be removed
   before OnGetExtMessageBundle message was received.

TEST=browser_tests.ExtensionApiTest.SharedModuleLocale
BUG=
==========

to

==========
[Extensions] Fix a data race in ChromeExtensionMessageFilter.

There is a data race in OnGetExtMessageBundleOnBlockingPool method. It calls
InfoMap::extensions() on a blocking pool, but all InfoMap methods must be
called on the IO thread.

This CL does the following:
1. Add DCHECK_CURRENTRY_ON(..) in extensions::InfoMap.
   With those DCHECKs browser_tests.ExtensionApiTest.SharedModuleLocale starts
   crashing (because of a wrong thread).
2. Remove calls to IO-only methods from
   LoadMessageBundleSubstitutionMapWithImport and rename that function.
3. Use OnGetExtMessageBundle to collect all necessary information before
   posting a task to a blocking pool.
4. Remove NOTREACHED(), because an extension might be removed
   before OnGetExtMessageBundle message was received.

TEST=browser_tests.ExtensionApiTest.SharedModuleLocale
BUG=
==========

[atuchin, 2017-02-07 15:01:08]

atuchin@yandex-team.ru changed reviewers:
+ rdevlin.cronin@chromium.org

[atuchin, 2017-02-08 05:51:42]

https://codereview.chromium.org/2686463003/diff/20001/extensions/common/file_...
File extensions/common/file_util.cc (right):

https://codereview.chromium.org/2686463003/diff/20001/extensions/common/file_...
extensions/common/file_util.cc:536: MessageBundle::SubstitutionMap*
LoadMessageBundleSubstitutionMapFromPaths(
In addition: What do you think about implementing
LoadMessageBundleSubstitutionMap method via
LoadMessageBundleSubstitutionMapFromPaths?

[atuchin, 2017-02-08 05:52:42]

atuchin@yandex-team.ru changed reviewers:
+ finnur@chromium.org
- rdevlin.cronin@chromium.org

[atuchin, 2017-02-10 06:29:57]

On 2017/02/08 05:51:42, atuchin wrote:
>
https://codereview.chromium.org/2686463003/diff/20001/extensions/common/file_...
> File extensions/common/file_util.cc (right):
> 
>
https://codereview.chromium.org/2686463003/diff/20001/extensions/common/file_...
> extensions/common/file_util.cc:536: MessageBundle::SubstitutionMap*
> LoadMessageBundleSubstitutionMapFromPaths(
> In addition: What do you think about implementing
> LoadMessageBundleSubstitutionMap method via
> LoadMessageBundleSubstitutionMapFromPaths?

Finnur, please take a look.

[Finnur, 2017-02-10 14:59:55]

finnur@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org
- finnur@chromium.org

[Finnur, 2017-02-10 15:00:08]

Devlin is a better candidate, I think.

[atuchin, 2017-02-14 05:22:55]

On 2017/02/10 15:00:08, Finnur wrote:
> Devlin is a better candidate, I think.
Ping.

[Devlin, 2017-02-14 17:45:49]

Mostly good, with one suggestion for an optimization.  Thanks for the patch! :)

https://codereview.chromium.org/2686463003/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_extension_message_filter.cc (right):

https://codereview.chromium.org/2686463003/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_extension_message_filter.cc:239:
extensions::LocaleInfo::GetDefaultLocale(extension), reply_msg));
If the extension isn't localized, we only return the pair of the extension id,
which is information we'd have available before posting the task here.  I think
it would make sense to expose a
GetNonLocalizedExtensionMessageBundle(extension_id);
method (feel free to change the name) that we could call from here, to avoid the
extra thread hops when unnecessary.

https://codereview.chromium.org/2686463003/diff/20001/extensions/common/file_...
File extensions/common/file_util.cc (right):

https://codereview.chromium.org/2686463003/diff/20001/extensions/common/file_...
extensions/common/file_util.cc:536: MessageBundle::SubstitutionMap*
LoadMessageBundleSubstitutionMapFromPaths(
On 2017/02/08 05:51:42, atuchin wrote:
> In addition: What do you think about implementing
> LoadMessageBundleSubstitutionMap method via
> LoadMessageBundleSubstitutionMapFromPaths?

Sounds good to me, now that this no longer does anything with SharedModules.

https://codereview.chromium.org/2686463003/diff/20001/extensions/common/file_...
extensions/common/file_util.cc:557: if (bundle.get()) {
nit: s/bundle.get()/bundle

[atuchin, 2017-02-15 08:17:29]

Thanks for comments.
PTAL!

https://codereview.chromium.org/2686463003/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_extension_message_filter.cc (right):

https://codereview.chromium.org/2686463003/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_extension_message_filter.cc:239:
extensions::LocaleInfo::GetDefaultLocale(extension), reply_msg));
On 2017/02/14 17:45:49, Devlin wrote:
> If the extension isn't localized, we only return the pair of the extension id,
> which is information we'd have available before posting the task here.  I
think
> it would make sense to expose a
> GetNonLocalizedExtensionMessageBundle(extension_id);
> method (feel free to change the name) that we could call from here, to avoid
the
> extra thread hops when unnecessary.

Done, take a look.
I thought about that before. It should give a some boost, but adds more code and
make the control flow more complex. That's why I had chosen to keep the original
behavior (always hope).

https://codereview.chromium.org/2686463003/diff/20001/extensions/common/file_...
File extensions/common/file_util.cc (right):

https://codereview.chromium.org/2686463003/diff/20001/extensions/common/file_...
extensions/common/file_util.cc:536: MessageBundle::SubstitutionMap*
LoadMessageBundleSubstitutionMapFromPaths(
On 2017/02/14 17:45:49, Devlin wrote:
> On 2017/02/08 05:51:42, atuchin wrote:
> > In addition: What do you think about implementing
> > LoadMessageBundleSubstitutionMap method via
> > LoadMessageBundleSubstitutionMapFromPaths?
> 
> Sounds good to me, now that this no longer does anything with SharedModules.

Ok, done.

https://codereview.chromium.org/2686463003/diff/20001/extensions/common/file_...
extensions/common/file_util.cc:557: if (bundle.get()) {
On 2017/02/14 17:45:49, Devlin wrote:
> nit: s/bundle.get()/bundle

Done.

[Devlin, 2017-02-16 16:20:56]

Nice!  Just a few last nits.

https://codereview.chromium.org/2686463003/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_extension_message_filter.cc (right):

https://codereview.chromium.org/2686463003/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_extension_message_filter.cc:234: const
std::string default_locale =
nit: const std::string&

https://codereview.chromium.org/2686463003/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_extension_message_filter.cc:238: // hope.
typo: s/hope/hop.

https://codereview.chromium.org/2686463003/diff/40001/extensions/common/file_...
File extensions/common/file_util.cc (right):

https://codereview.chromium.org/2686463003/diff/40001/extensions/common/file_...
extensions/common/file_util.cc:549: for (auto iter : *bundle->dictionary()) {
nit: const auto&

https://codereview.chromium.org/2686463003/diff/40001/extensions/common/file_...
File extensions/common/file_util.h (right):

https://codereview.chromium.org/2686463003/diff/40001/extensions/common/file_...
extensions/common/file_util.h:138: // extension. Contains only the extension_id
item.
nit: let's expand this a little to include:
// This doesn't require hitting disk, so it's safe to call on any thread.

[atuchin, 2017-02-17 06:22:02]

Ок, all nits have fixed.
Take a look.

https://codereview.chromium.org/2686463003/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_extension_message_filter.cc (right):

https://codereview.chromium.org/2686463003/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_extension_message_filter.cc:234: const
std::string default_locale =
On 2017/02/16 16:20:56, Devlin wrote:
> nit: const std::string&

Done.

https://codereview.chromium.org/2686463003/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_extension_message_filter.cc:238: // hope.
On 2017/02/16 16:20:56, Devlin wrote:
> typo: s/hope/hop.

Done.

https://codereview.chromium.org/2686463003/diff/40001/extensions/common/file_...
File extensions/common/file_util.cc (right):

https://codereview.chromium.org/2686463003/diff/40001/extensions/common/file_...
extensions/common/file_util.cc:549: for (auto iter : *bundle->dictionary()) {
On 2017/02/16 16:20:56, Devlin wrote:
> nit: const auto&

Done.

https://codereview.chromium.org/2686463003/diff/40001/extensions/common/file_...
File extensions/common/file_util.h (right):

https://codereview.chromium.org/2686463003/diff/40001/extensions/common/file_...
extensions/common/file_util.h:138: // extension. Contains only the extension_id
item.
On 2017/02/16 16:20:56, Devlin wrote:
> nit: let's expand this a little to include:
> // This doesn't require hitting disk, so it's safe to call on any thread.

Done.

[Devlin, 2017-02-17 15:57:39]

lgtm; thanks for your work here! :)

https://codereview.chromium.org/2686463003/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_extension_message_filter.cc (right):

https://codereview.chromium.org/2686463003/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_extension_message_filter.cc:236: if
(default_locale.empty()) {
nit: Let's actually move this above finding the paths to load, i.e. right below
the block around line 216.

https://codereview.chromium.org/2686463003/diff/60001/extensions/browser/info...
File extensions/browser/info_map.cc (right):

https://codereview.chromium.org/2686463003/diff/60001/extensions/browser/info...
extensions/browser/info_map.cc:55: }
nit: \n

[atuchin, 2017-02-17 16:22:47]

Thanks for the review.

https://codereview.chromium.org/2686463003/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_extension_message_filter.cc (right):

https://codereview.chromium.org/2686463003/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_extension_message_filter.cc:236: if
(default_locale.empty()) {
On 2017/02/17 15:57:38, Devlin wrote:
> nit: Let's actually move this above finding the paths to load, i.e. right
below
> the block around line 216.

Done.

https://codereview.chromium.org/2686463003/diff/60001/extensions/browser/info...
File extensions/browser/info_map.cc (right):

https://codereview.chromium.org/2686463003/diff/60001/extensions/browser/info...
extensions/browser/info_map.cc:55: }
On 2017/02/17 15:57:38, Devlin wrote:
> nit: \n

Done.

[atuchin, 2017-02-17 16:22:58]

The CQ bit was checked by atuchin@yandex-team.ru

[atuchin, 2017-02-17 16:22:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2686463003/#ps80001
(title: "[Extensions] Fix a data race in ChromeExtensionMessageFilter.")

[commit-bot: I haz the power, 2017-02-17 16:23:23]

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, 2017-02-17 18:12:25]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487348578270980,
"parent_rev": "a8109d958f144e541a204da6debd820d936d6108", "commit_rev":
"54e8d335c0e4bebe49fbb2cacc5e49c06f29dc6e"}

[commit-bot: I haz the power, 2017-02-17 18:13:06]

Description was changed from

==========
[Extensions] Fix a data race in ChromeExtensionMessageFilter.

There is a data race in OnGetExtMessageBundleOnBlockingPool method. It calls
InfoMap::extensions() on a blocking pool, but all InfoMap methods must be
called on the IO thread.

This CL does the following:
1. Add DCHECK_CURRENTRY_ON(..) in extensions::InfoMap.
   With those DCHECKs browser_tests.ExtensionApiTest.SharedModuleLocale starts
   crashing (because of a wrong thread).
2. Remove calls to IO-only methods from
   LoadMessageBundleSubstitutionMapWithImport and rename that function.
3. Use OnGetExtMessageBundle to collect all necessary information before
   posting a task to a blocking pool.
4. Remove NOTREACHED(), because an extension might be removed
   before OnGetExtMessageBundle message was received.

TEST=browser_tests.ExtensionApiTest.SharedModuleLocale
BUG=
==========

to

==========
[Extensions] Fix a data race in ChromeExtensionMessageFilter.

There is a data race in OnGetExtMessageBundleOnBlockingPool method. It calls
InfoMap::extensions() on a blocking pool, but all InfoMap methods must be
called on the IO thread.

This CL does the following:
1. Add DCHECK_CURRENTRY_ON(..) in extensions::InfoMap.
   With those DCHECKs browser_tests.ExtensionApiTest.SharedModuleLocale starts
   crashing (because of a wrong thread).
2. Remove calls to IO-only methods from
   LoadMessageBundleSubstitutionMapWithImport and rename that function.
3. Use OnGetExtMessageBundle to collect all necessary information before
   posting a task to a blocking pool.
4. Remove NOTREACHED(), because an extension might be removed
   before OnGetExtMessageBundle message was received.

TEST=browser_tests.ExtensionApiTest.SharedModuleLocale
BUG=

Review-Url: https://codereview.chromium.org/2686463003
Cr-Commit-Position: refs/heads/master@{#451339}
Committed:
https://chromium.googlesource.com/chromium/src/+/54e8d335c0e4bebe49fbb2cacc5e...
==========

[commit-bot: I haz the power, 2017-02-17 18:13:07]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/54e8d335c0e4bebe49fbb2cacc5e...