Fix crash due to VisibilityTimerTabHelper not being cancelled

Fix crash due to VisibilityTimerTabHelper not being cancelled

BUG=586944
Committed: https://crrev.com/95c569111cd96fa581b86543a6e632b0ffa9cf86
Cr-Commit-Position: refs/heads/master@{#375928}

[johnme, 2016-02-16 14:35:23]

johnme@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2016-02-16 15:28:28]

Thanks for the prompt fix! :)

Is there any way to add a test that verifies that the crash is fixed? Removing
the fix but running the test only causes the EXPECTs to fail.

https://codereview.chromium.org/1702633002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_permission_context.cc (right):

https://codereview.chromium.org/1702633002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_permission_context.cc:45: void
CancelTask(const PermissionRequestID& id);
+docs

https://codereview.chromium.org/1702633002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_permission_context.cc:60: typedef
std::pair<PermissionRequestID, scoped_ptr<base::Timer>> TaskEntry;
Could you define a small structure instead? |task_queue_| is used a fair bit,
and the usage of {first, second} doesn't help readability.

https://codereview.chromium.org/1702633002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_permission_context.cc:96: false /*
is_repeating */))));
nit: readability

=====
  scoped_ptr<base::Timer> timer(new base::Timer(
      from_here, visible_delay,
      base::Bind(&VisibilityTimerTabHelper::RunTask,
                 base::Unretained(this), task),
      false /* is_repeating */));

  task_queue_.push_back(std::make_pair(id, std::move(timer)));
=====

https://codereview.chromium.org/1702633002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_permission_context.cc:114:
web_contents()->RemoveUserData(UserDataKey());
What is the significance of eagerly deleting |this| here? Is this just an
optimization?

https://codereview.chromium.org/1702633002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_permission_context_unittest.cc
(right):

https://codereview.chromium.org/1702633002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_permission_context_unittest.cc:203:
scoped_refptr<base::SingleThreadTaskRunner> old_task_runner(
The TestMockTimeTaskRunner is now being instantiated in three places (130-135,
203-208 and 239-244), and removed three times as well.

Let's add a helper method + automatic restore in TearDown() if the
|old_task_runner_| is not NULL?

[johnme, 2016-02-16 18:14:34]

Addressed review comments - thanks :)

> Is there any way to add a test that verifies that the
> crash is fixed? Removing the fix but running the test
> only causes the EXPECTs to fail.

Done - I now request and cancel via the PermissionManager. Now if the fix is
removed it crashes in PermissionManager::OnPermissionsRequestResponseStatus,
same as reported in https://crbug.com/586944.

https://codereview.chromium.org/1702633002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_permission_context.cc (right):

https://codereview.chromium.org/1702633002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_permission_context.cc:45: void
CancelTask(const PermissionRequestID& id);
On 2016/02/16 15:28:28, Peter Beverloo wrote:
> +docs

Done.

https://codereview.chromium.org/1702633002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_permission_context.cc:60: typedef
std::pair<PermissionRequestID, scoped_ptr<base::Timer>> TaskEntry;
On 2016/02/16 15:28:28, Peter Beverloo wrote:
> Could you define a small structure instead? |task_queue_| is used a fair bit,
> and the usage of {first, second} doesn't help readability.

Done.

https://codereview.chromium.org/1702633002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_permission_context.cc:96: false /*
is_repeating */))));
On 2016/02/16 15:28:28, Peter Beverloo wrote:
> nit: readability
> 
> =====
>   scoped_ptr<base::Timer> timer(new base::Timer(
>       from_here, visible_delay,
>       base::Bind(&VisibilityTimerTabHelper::RunTask,
>                  base::Unretained(this), task),
>       false /* is_repeating */));
> 
>   task_queue_.push_back(std::make_pair(id, std::move(timer)));
> =====

Done.

https://codereview.chromium.org/1702633002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_permission_context.cc:114:
web_contents()->RemoveUserData(UserDataKey());
On 2016/02/16 15:28:28, Peter Beverloo wrote:
> What is the significance of eagerly deleting |this| here? Is this just an
> optimization?

It seemed neater at the time, but it adds a bunch of complexity, so I've removed
the RemoveUserData calls now, and instead I just call task_queue_.clear() in
WebContentsDestroyed.

https://codereview.chromium.org/1702633002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_permission_context_unittest.cc
(right):

https://codereview.chromium.org/1702633002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_permission_context_unittest.cc:203:
scoped_refptr<base::SingleThreadTaskRunner> old_task_runner(
On 2016/02/16 15:28:28, Peter Beverloo wrote:
> The TestMockTimeTaskRunner is now being instantiated in three places (130-135,
> 203-208 and 239-244), and removed three times as well.
> 
> Let's add a helper method + automatic restore in TearDown() if the
> |old_task_runner_| is not NULL?

Done.

[Peter Beverloo, 2016-02-16 22:43:28]

LGTM!

https://codereview.chromium.org/1702633002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_permission_context.cc (right):

https://codereview.chromium.org/1702633002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:117: }
Personally I would add a bunch of blank lines to further help readability like
you do in the unit test (probably after lines 96, 102, 104, 110, 114), but I'll
leave that up to you.

[johnme, 2016-02-17 13:32:07]

https://codereview.chromium.org/1702633002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_permission_context.cc (right):

https://codereview.chromium.org/1702633002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_permission_context.cc:117: }
On 2016/02/16 22:43:28, Peter Beverloo wrote:
> Personally I would add a bunch of blank lines to further help readability like
> you do in the unit test (probably after lines 96, 102, 104, 110, 114), but
I'll
> leave that up to you.

Done.

[johnme, 2016-02-17 13:32:11]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-02-17 13:32:11]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1702633002/#ps60001
(title: "Add whitespace")

[commit-bot: I haz the power, 2016-02-17 13:33:26]

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

[commit-bot: I haz the power, 2016-02-17 14:04:51]

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

[commit-bot: I haz the power, 2016-02-17 14:04:51]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[johnme, 2016-02-17 17:28:43]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-02-17 17:28:44]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1702633002/#ps120001
(title: "Remove unclear std::move")

[commit-bot: I haz the power, 2016-02-17 17:30:11]

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

[commit-bot: I haz the power, 2016-02-17 18:42:41]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-17 18:44:33]

Description was changed from

==========
Fix crash due to VisibilityTimerTabHelper not being cancelled

BUG=586944
==========

to

==========
Fix crash due to VisibilityTimerTabHelper not being cancelled

BUG=586944

Committed: https://crrev.com/95c569111cd96fa581b86543a6e632b0ffa9cf86
Cr-Commit-Position: refs/heads/master@{#375928}
==========

[commit-bot: I haz the power, 2016-02-17 18:44:34]

Patchset 7 (id:??) landed as
https://crrev.com/95c569111cd96fa581b86543a6e632b0ffa9cf86
Cr-Commit-Position: refs/heads/master@{#375928}