New issue
Advanced search Search tips

Issue 678592 link

Starred by 1 user

Issue metadata

Status: Duplicate
Owner:
Closed: Jan 2017
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Use-after-free for AddToHomescreenDataFetcherTestCommon.ManifestShort* tests

Project Member Reported by mattcary@chromium.org, Jan 5 2017

Issue description

Mergedinto: 677339
Status: Duplicate (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4a286ed6b9406edfb1031ae1d48766f79652a6ca

commit 4a286ed6b9406edfb1031ae1d48766f79652a6ca
Author: pkotwicz <pkotwicz@chromium.org>
Date: Tue Jan 17 20:45:26 2017

Fix use-after-free in base::Timer::StopAndAbandon()

A timer may be owned by a ref counted class. If |Timer::user_task_| is a method
in the ref counted class, it is possible for |Timer::user_task_| to hold the
only reference to the ref counted class. This CL changes
Timer::StopAndAbandon() so that Timer::Stop() is called after
Timer::AbandonScheduledTask(). If |Timer::user_task_| holds the only reference
to ref counted class, Timer::Stop() destroys the timer object.

Timer::StopAndAbandon() can be called while |Timer::user_task_| holds the only
reference to the ref counted class if the message loop is shut down.

BUG= 678592 
TEST=MessageLoopShutdownSelfOwningTimer

Review-Url: https://codereview.chromium.org/2624133004
Cr-Commit-Position: refs/heads/master@{#444132}

[modify] https://crrev.com/4a286ed6b9406edfb1031ae1d48766f79652a6ca/base/timer/timer.h
[modify] https://crrev.com/4a286ed6b9406edfb1031ae1d48766f79652a6ca/base/timer/timer_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a

commit 4e844bb5f686dbd3a062c6aaf83c8a1df171a08a
Author: gab <gab@chromium.org>
Date: Thu Jun 01 16:23:36 2017

Make base::Timer sequence-friendly.

This CL was also going to fix a race condition in the existing implementation when SetTaskRunner() was used but the test-only failures caused by tests having long-lived
with the status quo are proving hard than expected to fix. The race is no worse than
before by making this code sequence-friendly (and generally healthier). Will fix
race in a follow-up CL.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - (not true anymore, postponed to race fix CL:)
   The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG= 587199 ,  552633 ,  678592 , 684640,  675631 

Review-Url: https://codereview.chromium.org/2491613004
Cr-Commit-Position: refs/heads/master@{#476317}

[modify] https://crrev.com/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a/base/timer/timer.cc
[modify] https://crrev.com/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a/base/timer/timer.h
[modify] https://crrev.com/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a/base/timer/timer_unittest.cc
[modify] https://crrev.com/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a/build/sanitizers/tsan_suppressions.cc
[modify] https://crrev.com/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a/content/browser/loader/upload_progress_tracker.cc
[modify] https://crrev.com/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a/content/browser/loader/upload_progress_tracker.h
[modify] https://crrev.com/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a/content/browser/loader/upload_progress_tracker_unittest.cc
[modify] https://crrev.com/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a/content/browser/service_worker/service_worker_job_unittest.cc
[modify] https://crrev.com/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a/media/base/android/media_codec_loop.cc
[modify] https://crrev.com/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a/media/base/android/media_codec_loop.h
[modify] https://crrev.com/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a/media/base/android/media_codec_loop_unittest.cc
[modify] https://crrev.com/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a/media/filters/android/media_codec_audio_decoder.cc

Sign in to add a comment