service worker: Fix valgrind errors on unit_tests

service worker: Fix valgrind errors on unit_tests

Followup to https://codereview.chromium.org/1771283002/
We must clear request_context_getter_ on the IO thread or the valgrind
bots cry. Also, call RemoveObserver in ShutdownOnIO in case OnContextIsShuttingDown
is never called, as is the case on content_shell.

BUG=587621, 593913
Committed: https://crrev.com/275701504450e46612ac5bc67c32a71838b6fdad
Cr-Commit-Position: refs/heads/master@{#380924}

[falken, 2016-03-11 04:29:48]

falken@chromium.org changed reviewers:
+ michaeln@chromium.org, mmenke@chromium.org

[falken, 2016-03-11 04:29:49]

This patch has dual yet related purposes:
1) Fix valgrind bots
2) Call RemoveObserver to balance out AddObserver even if OnContxtShuttingDown
is not called (as is the case on content_shell)

To do 1) it looks like clearing |request_context_getter_| on the IO thread in
ShutdownOnIO is needed, though I don't totally understand why.

2) doesn't look strictly needed to fix the bots or prevent a crash (because the
observer list does not have |check_empty|), but  it seems cleaner to remove
ourself as an observer when |request_context_getter_| is cleared.

I see there's controversy on calling RemoveObserver in ShutdownOnIO in
https://codereview.chromium.org/1771283002/ but it seems not something worth
spending too much time on.

[mmenke, 2016-03-11 16:30:40]

On 2016/03/11 04:29:49, falken wrote:
> This patch has dual yet related purposes:
> 1) Fix valgrind bots
> 2) Call RemoveObserver to balance out AddObserver even if OnContxtShuttingDown
> is not called (as is the case on content_shell)
> 
> To do 1) it looks like clearing |request_context_getter_| on the IO thread in
> ShutdownOnIO is needed, though I don't totally understand why.
> 
> 2) doesn't look strictly needed to fix the bots or prevent a crash (because
the
> observer list does not have |check_empty|), but  it seems cleaner to remove
> ourself as an observer when |request_context_getter_| is cleared.
> 
> I see there's controversy on calling RemoveObserver in ShutdownOnIO in
> https://codereview.chromium.org/1771283002/ but it seems not something worth
> spending too much time on.

Why is there a leak here?  The IOThread generally outlives the
ServiceWorkerContextWrapper, no?  Is this just an artifact of not using real
threads in test code?

[michaeln, 2016-03-11 23:33:13]

lgtm

> The IOThread generally outlives the ServiceWorkerContextWrapper, no

i don't believe there is a guarantee of that, nor does there need to be, we have
the shutdown[OnIO] methods

[mmenke, 2016-03-11 23:52:26]

On 2016/03/11 23:33:13, michaeln wrote:
> lgtm
> 
> > The IOThread generally outlives the ServiceWorkerContextWrapper, no
> 
> i don't believe there is a guarantee of that, nor does there need to be, we
have
> the shutdown[OnIO] methods

Oh...its refcounted, so it may be owned by the UI thread's message loop
itself...Or some other random thread.  :(

[falken, 2016-03-14 03:08:22]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2016-03-14 03:08:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1779353003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1779353003/1

[commit-bot: I haz the power, 2016-03-14 04:45:55]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-14 04:47:27]

Description was changed from

==========
service worker: Fix valgrind errors on unit_tests

Followup to https://codereview.chromium.org/1771283002/
We must clear request_context_getter_ on the IO thread or the valgrind
bots cry. Also, call RemoveObserver in ShutdownOnIO in case
OnContextIsShuttingDown
is never called, as is the case on content_shell.

BUG=587621,593913
==========

to

==========
service worker: Fix valgrind errors on unit_tests

Followup to https://codereview.chromium.org/1771283002/
We must clear request_context_getter_ on the IO thread or the valgrind
bots cry. Also, call RemoveObserver in ShutdownOnIO in case
OnContextIsShuttingDown
is never called, as is the case on content_shell.

BUG=587621,593913

Committed: https://crrev.com/275701504450e46612ac5bc67c32a71838b6fdad
Cr-Commit-Position: refs/heads/master@{#380924}
==========

[commit-bot: I haz the power, 2016-03-14 04:47:28]

Patchset 1 (id:??) landed as
https://crrev.com/275701504450e46612ac5bc67c32a71838b6fdad
Cr-Commit-Position: refs/heads/master@{#380924}