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

Issue 742306 link

Starred by 1 user
Status: Fixed
Owner:
jdoerrie@chromium.org
Closed: Sep 2017
Cc:
█ brettw@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Disallow copy and assign for base::ListValue and base::DictionaryValue

Project Member Reported by battre@chromium.org, Jul 13 2017 
I have reviewed a CL today and was surprised to see ListValues passed by by value. I am concerned that this introduces patterns where people copy (!) large parts of the lists and dictionaries out of the pref service.

What do you think of the following proposal to make copying an explicit activity?

disallow
explicit ListValue(const ListStorage&);
ListStorage& operator=(const ListStorage&);

but provide
explicit ListValue(ListStorage&&);
ListStorage& operator=(ListStorage&&);
ListStorage DeepClone();

Comment 1 by jdoerrie@chromium.org, Jul 13 2017 

Unfortunately your proposal wouldn't be enough to disallow all copying of lists and dicts. In the Refactor Proposal (http://docs/document/d/1uDLu5uTRlCWePxQUEHc8yNQdEoE1BDISYdpggWEABnw/edit#heading=h.icznl3bfmf6v) it is mentioned as a goal to get rid of all subclasses and move to value semantics with easy copying and moving.

While we could disallow the copy constructors taking ListStorage and DictStorage, the general copy constructor taking another Value still does a deep copy.

E.g. the following code is possible:

ListStorage big_list(1000);
Value list_value(std::move(big_list);
Value list_copy = list_value; // Expensive Deep Copy Here

Disallowing the regular Value copy constructor is not a desirable option, as Values frequently contain PODs like booleans, integers and doubles, and clients would expect the following code to work:

Value val(123);
Value copy = val;

The only options to completely disallow copying of Lists and Dicts are thus either adding runtime CHECKs in the regular copy constructor to crash code that attempts to do so, or making Value a move-only type by disabling the copy constructor and copy assignment operator all together.

Both of these options seem not desirable to me. I'm curious to here brettw@'s opinion, though.

Comment 2 by battre@chromium.org, Jul 13 2017 

I would propose that we sacrifice copy-assignment for POD wrappers as well.

Comment 3 by jdoerrie@chromium.org, Jul 18 2017 

I left a suggestion for an API and a comment on the proposal doc (https://docs.google.com/document/d/1uDLu5uTRlCWePxQUEHc8yNQdEoE1BDISYdpggWEABnw/edit#heading=h.icznl3bfmf6v, near the top of the second page). Further comments are welcome.
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 23 2017 

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

commit cc9f5730ee747c92fb8340818ee185d36bf8a89a
Author: jdoerrie <jdoerrie@chromium.org>
Date: Wed Aug 23 14:12:30 2017

Allow only explicit copies of base::Value

This change deletes base::Values's copy constructor and copy assignment
operator and introduces base::Value::Clone. It is the purpose of this
change to disallow implicit copies while still keeping the possibility
to make explicit copies.

Bug: 646113,  742306 
Change-Id: Idf91382a8dfad1a2b398958e5107b67686696475
Reviewed-on: https://chromium-review.googlesource.com/574715
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496678}
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/base/value_iterators_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/base/values.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/base/values.h
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/base/values_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/devtools/devtools_targets_ui.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/devtools/serialize_host_descriptions_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/extensions/api/tabs/tabs_event_router.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/notifications/notification_channels_provider_android.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/policy/policy_browsertest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/profiles/profile_attributes_entry.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/signin/easy_unlock_service_regular.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/supervised_user/supervised_user_pref_store_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/supervised_user/supervised_user_settings_service.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/ui/app_list/search/common/webservice_cache.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/ui/webui/chromeos/first_run/first_run_actor.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/ui/webui/chromeos/first_run/first_run_actor.h
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/ui/webui/chromeos/first_run/first_run_handler.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/ui/webui/chromeos/login/base_webui_handler.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/ui/webui/ntp/app_launcher_handler.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/ui/webui/policy_ui_browsertest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/ui/webui/policy_ui_handler.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/common/extensions/api/storage/storage_schema_manifest_handler_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/common/extensions/manifest_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/profiling/json_exporter_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/test/chromedriver/capabilities_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/test/chromedriver/chrome/devtools_client_impl.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/test/chromedriver/chrome/devtools_client_impl_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/test/chromedriver/chrome/web_view_impl.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/test/chromedriver/chrome_launcher.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/test/chromedriver/commands.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/test/chromedriver/devtools_events_logger.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/test/chromedriver/performance_logger.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chrome/test/chromedriver/performance_logger_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chromeos/components/tether/wifi_hotspot_connector_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chromeos/dbus/fake_shill_device_client.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chromeos/dbus/fake_shill_ipconfig_client.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chromeos/dbus/fake_shill_manager_client.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chromeos/dbus/fake_shill_profile_client.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chromeos/dbus/fake_shill_service_client.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chromeos/network/device_state.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chromeos/network/managed_network_configuration_handler_impl.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chromeos/network/managed_network_configuration_handler_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chromeos/network/network_ui_data.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chromeos/network/onc/onc_merger.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chromeos/network/onc/onc_translator_onc_to_shill.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/chromeos/network/onc/onc_translator_shill_to_onc.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/content_settings/core/browser/host_content_settings_map.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/content_settings/core/common/content_settings.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/login/screens/screen_context.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/offline_pages/content/renovations/render_frame_script_injector.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/policy/core/common/config_dir_policy_loader_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/policy/core/common/configuration_policy_provider_test.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/policy/core/common/mac_util_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/policy/core/common/policy_loader_win_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/policy/core/common/policy_test_utils.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/policy/core/common/registry_dict_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/policy/core/common/schema_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/safe_browsing/web_ui/safe_browsing_ui.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/search_engines/default_search_manager_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/search_engines/default_search_policy_handler.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/sync/js/js_event_details_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/sync_preferences/pref_model_associator.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/components/sync_preferences/pref_model_associator.h
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/content/browser/media/media_internals.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/content/browser/renderer_host/input/mouse_latency_browsertest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/content/browser/tracing/tracing_controller_impl.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/content/browser/webrtc/webrtc_internals.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/extensions/browser/api/networking_private/networking_private_chromeos_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/extensions/browser/api/web_request/web_request_event_details.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/extensions/browser/extension_prefs.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/extensions/browser/guest_view/web_view/web_view_permission_helper.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/extensions/browser/value_store/value_store_change.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/extensions/renderer/api_activity_logger_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/ios/chrome/browser/browser_state/browser_state_info_cache.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/ios/chrome/browser/browser_state/browser_state_info_cache.h
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/ios/chrome/browser/notification_promo.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/ios/chrome/browser/payments/ios_payment_instrument_launcher.mm
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/ios/chrome/browser/payments/ios_payment_instrument_launcher_unittest.mm
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/net/http/http_server_properties_manager.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/net/http/http_server_properties_manager_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/net/reporting/reporting_delivery_agent.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/remoting/host/it2me/it2me_host_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/remoting/host/it2me/it2me_native_messaging_host.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/remoting/host/setup/me2me_native_messaging_host.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/services/preferences/pref_service_factory_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/services/preferences/pref_store_consistency_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/services/preferences/public/cpp/dictionary_value_update.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/services/preferences/public/cpp/dictionary_value_update.h
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/services/preferences/tracked/dictionary_hash_store_contents.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/services/preferences/tracked/pref_hash_calculator_unittest.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/services/preferences/tracked/tracked_preferences_migration.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/services/resource_coordinator/tracing/test_util.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/services/service_manager/embedder/manifest_utils.cc
[modify] https://crrev.com/cc9f5730ee747c92fb8340818ee185d36bf8a89a/tools/json_schema_compiler/test/functions_as_parameters_unittest.cc

Comment 5 by jdoerrie@chromium.org, Sep 5 2017

Status: Fixed (was: Untriaged)
►

Sign in to add a comment