New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 610048 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Not currently working on Chromium
Closed: May 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Use explicit operator bool in scoped_refptr instead of "Safe Bool Idiom".

Project Member Reported by scheib@chromium.org, May 7 2016

Issue description

Previously the "Safe Bool Idiom" was implemented using a typedef ... Testable work around. The C++11 explicit conversion operator feature
is now available and this hack is no longer required.

 
I'd started low-priority work on this in https://codereview.chromium.org/1958823002.

When uploading I noticed  Issue 605794  and that other related work is also in progress. Let me know if I'm being redundant.

Comment 2 by w...@chromium.org, May 7 2016

I haven't seen anyone tackle scoped_refptr::operator bool() yet.

 Issue 605794  is marked blocked on an investigation of performance
regressions when making the switch for WTF::RefPtr, apparently because
precisely how you choose to coerce pointer->bool at call-sites that need it
affects whether the optimizer works effectively. :(
Blockedon: 607208
Project Member

Comment 4 by bugdroid1@chromium.org, May 12 2016

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

commit 4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733
Author: scheib <scheib@chromium.org>
Date: Thu May 12 00:55:03 2016

Fix implicit access to raw pointer of scoped_refptr.

In following patch https://codereview.chromium.org/1963323004/
scoped_refptr will have the previous flawed "Safe Bool Idiom"
implementation removed.

This feature was incorrectly used throughout the codebase
with code implicitly accessing the raw pointer from
scoped_refptr instances. A correct "Safe Bool Idiom" would
have prevented that, but the point is moot as we can now
move to Cxx11 "explicit operator bool()".

Usage has been corrected in two ways:
+ Logic tests no longer compare pointer values, but use a
  boolean expression (typically just the variable name).
+ Casts to boolean types are now explicit with stati_cast<bool>().

BUG= 610048 
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/base/threading/thread_unittest.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/blimp/common/blob_cache/in_memory_blob_cache_unittest.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/cc/test/fake_video_frame_provider.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/cc/trees/layer_tree_host_unittest_serialization.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/chrome/browser/banners/app_banner_manager.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/chrome/browser/chromeos/certificate_provider/certificate_provider_service_unittest.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/chrome/browser/themes/theme_service.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/chrome/browser/ui/views/certificate_selector_browsertest.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/chrome/browser/ui/webui/options/password_manager_handler.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/components/drive/drive_uploader.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/components/suggestions/image_manager_unittest.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/components/tracing/child_memory_dump_manager_delegate_impl.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/content/browser/cache_storage/cache_storage.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/content/browser/compositor/gpu_process_transport_factory.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/content/browser/media/webrtc/webrtc_internals.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/content/child/child_thread_impl.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/content/child/shared_memory_data_consumer_handle.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/content/renderer/media/android/webmediaplayer_android.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/content/renderer/media/webrtc/peer_connection_dependency_factory.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/device/bluetooth/bluetooth_adapter_win.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/device/bluetooth/bluez/bluetooth_advertisement_bluez_unittest.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/device/usb/usb_device_handle_impl.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/gpu/command_buffer/client/gles2_implementation_unittest.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/ipc/ipc_message_attachment_set_posix_unittest.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/media/blink/video_frame_compositor.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/media/gpu/h264_decoder.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/mojo/edk/system/shared_buffer_dispatcher.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/mojo/edk/system/wait_set_dispatcher_unittest.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/net/proxy/proxy_script_decider_unittest.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/net/quic/crypto/quic_crypto_server_config.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/ui/gfx/win/direct_write.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/ui/gl/gl_image_ozone_native_pixmap_unittest.cc
[modify] https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733/ui/ozone/platform/drm/host/drm_cursor.cc

Project Member

Comment 5 by bugdroid1@chromium.org, May 12 2016

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

commit 116b5a887f10705fdaf736b679f52cc84fd178d9
Author: scheib <scheib@chromium.org>
Date: Thu May 12 06:18:46 2016

Use explicit operator bool in scoped_refptr.

Previously the "Safe Bool Idiom" was incorrectly
implemented using a typedef ... Testable that leaked
the raw pointer of scoped_refptr.

Code relying on that side effect has been corrected
in https://codereview.chromium.org/1958823002/

The C++11 explicit conversion operator feature
is now available and this hack is no longer required,
and usage will be enforced to not inadvertently
use the raw pointer.

BUG= 610048 

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

[modify] https://crrev.com/116b5a887f10705fdaf736b679f52cc84fd178d9/base/memory/ref_counted.h

Comment 6 by scheib@chromium.org, May 12 2016

Blockedon: -607208
Status: Fixed (was: Started)
Related to, but not blocking on  issue 607208 .

Sign in to add a comment