New issue
Advanced search Search tips

Issue 900010 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Add a ScopedRefPtr specific version of ReleaseSoon

Project Member Reported by lethalantidote@chromium.org, Oct 29

Issue description

The current implementation of ReleaseSoon ( https://cs.chromium.org/chromium/src/base/sequenced_task_runner.h?rcl=2f39d1bc5bb2f57e1cb60d9366729a59f6ce6514&l=138 ) takes in a bare pointer object. It would make sense to add a version that takes in a scoped_refptr, as it is a common use case. This version would encapsulate some of the work that needs to be done before calling ReleaseSoon to make it safe. This work is non-obvious and easy to mess up, so it would be best to hide it as an implementation detail of this version. (Example of such work here https://cs.chromium.org/chromium/src/storage/browser/blob/blob_data_handle.cc?rcl=b75e4f1d8151eb16a067e1d9c9e995bef1bf2aba&l=73 ).
 
Cc: dcheng@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 21

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

commit 56f1380d111c7c78fc8e505e7eaa8e1e57eee22a
Author: CJ DiMeglio <lethalantidote@chromium.org>
Date: Wed Nov 21 21:59:53 2018

Adds scoped_refptr specific ReleaseSoon.

This CL makes using ReleaseSoon with scpoed_refptr easier. It
encapsulates the call to AddRef, and forces the uses to use std::move
when calling ReleaseSoon with a scoped_refptr. Both are required to make
ReleaseSoon run safely (and correctly), but it isn't obvious that they
are needed.

An example of its use is included with this CL.

Bug:  900010 
Change-Id: Ic1de31d0433ffeee52d98b630cafe643c223ed18
Reviewed-on: https://chromium-review.googlesource.com/c/1306639
Commit-Queue: CJ DiMeglio <lethalantidote@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610231}
[modify] https://crrev.com/56f1380d111c7c78fc8e505e7eaa8e1e57eee22a/base/sequenced_task_runner.h
[modify] https://crrev.com/56f1380d111c7c78fc8e505e7eaa8e1e57eee22a/content/renderer/render_thread_impl.cc

Note to not forget: Add CL that removes other uses of ReleaseSoon. 
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 8

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

commit 638cf54d0d3dd79abd4a1d7aa5c1b8357055a800
Author: CJ DiMeglio <lethalantidote@chromium.org>
Date: Sat Dec 08 02:22:14 2018

Removes rawptr uses of ReleaseSoon.

This CL removes the deprecated overload of ReleaseSoon that takes in a
rawptr, and replaces it with the safer overload that takes in a
scoped_refptr.

Bug:  900010 
Change-Id: Ica88ec1151bc71f1f4bb107bbfeb71288031c02c
Reviewed-on: https://chromium-review.googlesource.com/c/1346999
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Yuwei Huang <yuweih@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Cait Phillips <caitkp@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: CJ DiMeglio <lethalantidote@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614918}
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/base/memory/scoped_refptr.h
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/base/message_loop/message_loop_task_runner_unittest.cc
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/base/sequenced_task_runner.h
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/chrome/browser/devtools/device/android_device_manager.cc
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/chrome/browser/ui/login/login_handler.cc
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/chrome/service/cloud_print/printer_job_handler.cc
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/components/history/core/browser/history_service.cc
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/components/omnibox/browser/shortcuts_backend.cc
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/content/browser/browser_thread_unittest.cc
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/content/browser/indexed_db/indexed_db_backing_store.cc
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/content/browser/renderer_host/pepper/pepper_truetype_font_host.cc
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/content/public/browser/browser_thread.h
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/content/renderer/pepper/video_encoder_shim.cc
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/device/usb/usb_device_handle_impl.cc
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/remoting/protocol/webrtc_data_stream_adapter.cc
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/storage/browser/blob/blob_data_handle.cc
[modify] https://crrev.com/638cf54d0d3dd79abd4a1d7aa5c1b8357055a800/storage/browser/database/database_quota_client.cc

Status: Fixed (was: Assigned)

Sign in to add a comment