Extensions: Make ExtensionWebRequestEventRouter leaky.

Extensions: Make ExtensionWebRequestEventRouter leaky.

ExtensionWebRequestEventRouter is a Singleton primarily used on the IO thread.
However, being a Singleton it is destroyed on the main/UI thread. This is
potentially not thread-safe.

To fix, make ExtensionWebRequestEventRouter a leaky singleton. This also removes
the need for ExtensionWebRequestEventRouter to support weak pointers (which
didn't add any safety in the first place, since invalidating weak pointers on
one thread while accessing them on another is not thread-safe).

BUG=695198
Review-Url: https://codereview.chromium.org/2721793002
Cr-Commit-Position: refs/heads/master@{#453806}
Committed: https://chromium.googlesource.com/chromium/src/+/c75f6f0f5286abfacf96b9ef7ac088a9692ed897

[karandeepb, 2017-02-28 01:22:08]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 01:22:38]

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

[karandeepb, 2017-02-28 01:39:14]

Description was changed from

==========
Extensions: Make ExtensionWebRequestEventRouter leaky.
==========

to

==========
Extensions: Make ExtensionWebRequestEventRouter leaky.

ExtensionWebRequestEventRouter is a Singleton primarily used on the IO thread.
However, being a Singleton it is destroyed on the main/UI thread. This is
potentially not thread-safe.

To fix, make ExtensionWebRequestEventRouter a leaky singleton. This also removes
the need for ExtensionWebRequestEventRouter to support weak pointers (which
didn't add any safety in the first place, since invalidating weak pointers on
one thread while accessing them on another is not thread-safe).

BUG=695198
==========

[commit-bot: I haz the power, 2017-02-28 02:06:22]

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

[commit-bot: I haz the power, 2017-02-28 02:06:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[karandeepb, 2017-02-28 20:08:49]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 20:09:38]

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

[karandeepb, 2017-02-28 20:11:04]

karandeepb@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[karandeepb, 2017-02-28 20:11:05]

PTAL Devlin. Please also check the CL description.

[commit-bot: I haz the power, 2017-02-28 21:02:22]

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

[commit-bot: I haz the power, 2017-02-28 21:02:22]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-02-28 22:10:04]

lgtm

https://codereview.chromium.org/2721793002/diff/60001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2721793002/diff/60001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:566: static
ExtensionWebRequestEventRouter* instance =
nit: Let's use CR_DEFINE_STATIC_LOCAL() just so these are easy to trace.

https://codereview.chromium.org/2721793002/diff/60001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1112:
base::Unretained(this), browser_context,
nit: when using Unretained, it's good practice to include a comment why it's
safe, e.g.
// This Unretained() is safe because this object is leaked.

[karandeepb, 2017-02-28 23:01:14]

https://codereview.chromium.org/2721793002/diff/60001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2721793002/diff/60001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:566: static
ExtensionWebRequestEventRouter* instance =
On 2017/02/28 22:10:04, Devlin wrote:
> nit: Let's use CR_DEFINE_STATIC_LOCAL() just so these are easy to trace.

Done.

https://codereview.chromium.org/2721793002/diff/60001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1112:
base::Unretained(this), browser_context,
On 2017/02/28 22:10:04, Devlin wrote:
> nit: when using Unretained, it's good practice to include a comment why it's
> safe, e.g.
> // This Unretained() is safe because this object is leaked.

Done.

[karandeepb, 2017-02-28 23:01:35]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2017-02-28 23:01:39]

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/2721793002/#ps80001
(title: "Address review.")

[commit-bot: I haz the power, 2017-02-28 23:02:59]

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-03-01 01:49:56]

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

[commit-bot: I haz the power, 2017-03-01 01:49:56]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[karandeepb, 2017-03-01 01:52:40]

The CQ bit was checked by karandeepb@chromium.org

[commit-bot: I haz the power, 2017-03-01 01:53:08]

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-03-01 03:01:14]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488333160147520,
"parent_rev": "17624e38d397ab67a97f0fbc798c53618988a2f1", "commit_rev":
"c75f6f0f5286abfacf96b9ef7ac088a9692ed897"}

[commit-bot: I haz the power, 2017-03-01 03:01:55]

Description was changed from

==========
Extensions: Make ExtensionWebRequestEventRouter leaky.

ExtensionWebRequestEventRouter is a Singleton primarily used on the IO thread.
However, being a Singleton it is destroyed on the main/UI thread. This is
potentially not thread-safe.

To fix, make ExtensionWebRequestEventRouter a leaky singleton. This also removes
the need for ExtensionWebRequestEventRouter to support weak pointers (which
didn't add any safety in the first place, since invalidating weak pointers on
one thread while accessing them on another is not thread-safe).

BUG=695198
==========

to

==========
Extensions: Make ExtensionWebRequestEventRouter leaky.

ExtensionWebRequestEventRouter is a Singleton primarily used on the IO thread.
However, being a Singleton it is destroyed on the main/UI thread. This is
potentially not thread-safe.

To fix, make ExtensionWebRequestEventRouter a leaky singleton. This also removes
the need for ExtensionWebRequestEventRouter to support weak pointers (which
didn't add any safety in the first place, since invalidating weak pointers on
one thread while accessing them on another is not thread-safe).

BUG=695198

Review-Url: https://codereview.chromium.org/2721793002
Cr-Commit-Position: refs/heads/master@{#453806}
Committed:
https://chromium.googlesource.com/chromium/src/+/c75f6f0f5286abfacf96b9ef7ac0...
==========

[commit-bot: I haz the power, 2017-03-01 03:01:56]

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

[Wez, 2017-03-01 17:28:15]

wez@chromium.org changed reviewers:
+ wez@chromium.org

[Wez, 2017-03-01 17:28:16]

https://codereview.chromium.org/2721793002/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2721793002/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:569:
CR_DEFINE_STATIC_LOCAL(ExtensionWebRequestEventRouter, instance, ());
Will this have the correct behaviour with respect to instance state across
tests?

[Wez, 2017-03-01 17:29:18]

On 2017/03/01 17:28:16, Wez wrote:
>
https://codereview.chromium.org/2721793002/diff/80001/extensions/browser/api/...
> File extensions/browser/api/web_request/web_request_api.cc (right):
> 
>
https://codereview.chromium.org/2721793002/diff/80001/extensions/browser/api/...
> extensions/browser/api/web_request/web_request_api.cc:569:
> CR_DEFINE_STATIC_LOCAL(ExtensionWebRequestEventRouter, instance, ());
> Will this have the correct behaviour with respect to instance state across
> tests?

Also, the description says you're making this a leaky singleton, whereas you're
actually making it a static global.

[Devlin, 2017-03-01 17:35:07]

On 2017/03/01 17:29:18, Wez wrote:
> On 2017/03/01 17:28:16, Wez wrote:
> >
>
https://codereview.chromium.org/2721793002/diff/80001/extensions/browser/api/...
> > File extensions/browser/api/web_request/web_request_api.cc (right):
> > 
> >
>
https://codereview.chromium.org/2721793002/diff/80001/extensions/browser/api/...
> > extensions/browser/api/web_request/web_request_api.cc:569:
> > CR_DEFINE_STATIC_LOCAL(ExtensionWebRequestEventRouter, instance, ());
> > Will this have the correct behaviour with respect to instance state across
> > tests?
> 
> Also, the description says you're making this a leaky singleton, whereas
you're
> actually making it a static global.

I noticed that, but it's a small-s singleton, which I interpreted as the design
pattern of a singleton, rather than the Chromium class Singleton.  Since the
ctor and dtor are private and hidden behind a GetInstance() method, this is
ensured to be a (small-s) singleton, so I think the description is actually
correct (though perhaps subtle :)).

[karandeepb, 2017-03-01 18:07:21]

https://codereview.chromium.org/2721793002/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2721793002/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:569:
CR_DEFINE_STATIC_LOCAL(ExtensionWebRequestEventRouter, instance, ());
On 2017/03/01 17:28:16, Wez wrote:
> Will this have the correct behaviour with respect to instance state across
> tests?

Yeah state will be carried over for unit tests run in the same process. Isn't it
the same for base::Singleton though?

[karandeepb, 2017-03-01 18:08:50]

On 2017/03/01 17:29:18, Wez wrote:
> On 2017/03/01 17:28:16, Wez wrote:
> >
>
https://codereview.chromium.org/2721793002/diff/80001/extensions/browser/api/...
> > File extensions/browser/api/web_request/web_request_api.cc (right):
> > 
> >
>
https://codereview.chromium.org/2721793002/diff/80001/extensions/browser/api/...
> > extensions/browser/api/web_request/web_request_api.cc:569:
> > CR_DEFINE_STATIC_LOCAL(ExtensionWebRequestEventRouter, instance, ());
> > Will this have the correct behaviour with respect to instance state across
> > tests?
> 
> Also, the description says you're making this a leaky singleton, whereas
you're
> actually making it a static global.

[Wez, 2017-03-01 18:09:47]

https://codereview.chromium.org/2721793002/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2721793002/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:569:
CR_DEFINE_STATIC_LOCAL(ExtensionWebRequestEventRouter, instance, ());
On 2017/03/01 18:07:21, karandeepb wrote:
> On 2017/03/01 17:28:16, Wez wrote:
> > Will this have the correct behaviour with respect to instance state across
> > tests?
> 
> Yeah state will be carried over for unit tests run in the same process. Isn't
it
> the same for base::Singleton though? 

My recollection was that Singleton and LazyInstance *will* be torn-down by the
test-specific AtExitManager (maybe ShadowAtExitManager, but I may be thinking of
something else)?

[karandeepb, 2017-03-01 19:17:52]

https://codereview.chromium.org/2721793002/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2721793002/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:569:
CR_DEFINE_STATIC_LOCAL(ExtensionWebRequestEventRouter, instance, ());
On 2017/03/01 18:09:47, Wez wrote:
> On 2017/03/01 18:07:21, karandeepb wrote:
> > On 2017/03/01 17:28:16, Wez wrote:
> > > Will this have the correct behaviour with respect to instance state across
> > > tests?
> > 
> > Yeah state will be carried over for unit tests run in the same process.
Isn't
> it
> > the same for base::Singleton though? 
> 
> My recollection was that Singleton and LazyInstance *will* be torn-down by the
> test-specific AtExitManager (maybe ShadowAtExitManager, but I may be thinking
of
> something else)?

Yeah so there's the ShadowingAtExitManager which you can add to your tests/test
suite, which will call the registered AtExit callbacks whenever it is itself
destroyed, hence destroying any Singletons. At least for the tests in
web_request_api_unittest.cc, this wasn't the case. (There was no
ShadowingAtExitManager). 

Also, even if I were to use a leaky base::Singleton, I think the problem will
still be there since it won't register an AtExit callback for its destruction.
(Maybe it should when running tests?) I guess this is a problem for most/all
leaky singletons and requires explicit handling in tests itself. In case we do
start seeing some flakes, this can probably be revisited.