Introduce SerializeDictionaryForest for devtools_target_ui

Desktop / Chromium - vabr [chromium.org] - 20 April 2017 04:27 EDT

Currently, LocalTargetsUIHandler::SendTargets contains a very succinct code to create a base::Value description of a forest of DevToolsAgentHost instances. One property of base::ListValue which made this code possible was that Values were stored in unique_ptrs inside ListValue. In particular, passing a value to ListValue::Append left the previously owning pointer still useful as a weak pointer.

That property no longer holds true, because ListValue has been converted to store Values directly in https://crrev.com/a5676c607e107238b2d0cdd55e626c93669a92f1. This results in use of values which have been std::moved, i.e., a kind of use-after-free. What's worse -- this code is not tested enough for sanitizers to have caught this, and was already seen in the wild by users of Chrome 59 and 60 as crashes.

This CL wants to achieve these goals:
- Fix the code to not to rely on implementation details of ListValue.
- Increase unit test coverage of the new code.
- Make the code more explicit and clearer.

The CL does the following to achieve the above goals:
- It introduces an explicit topological sort of the forest of DevToolsAgentHosts. This allows to do changes to base::Values before passing them inside a ListValue, so after calling ListValue::Append it does not matter what happens to the passed Values.
- The CL also separates the topological sort into a helper function, and adds a unit test for that.

The touched code also seemed to create too many scoped_refptr instances: in cases where there is already a scoped_refptr holding an object alive outside of the current context, it is better to use just raw pointers in for loops and argument passing, to avoid unnecessary churn with AddRef/Release calls.

Therefore this CL also changed for loops inside of LocalTargetsUIHandler::SendTargets to use const scoped_refptr& (still avoids the AddRef/Release churn when raw pointer is not an option) as well as the argument of DevToolsTargetsUIHandler::Serialize to just a raw pointer.

BUG=712119

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

6681597 Introduce SerializeDictionaryForest for devtools_target_ui
chrome/browser/devtools/BUILD.gn | 2 +
chrome/browser/devtools/devtools_targets_ui.cc | 37 ++-----
chrome/browser/devtools/devtools_targets_ui.h | 2 +-
.../devtools/serialize_host_descriptions.cc | 102 ++++++++++++++++++
.../browser/devtools/serialize_host_descriptions.h | 29 ++++++
.../serialize_host_descriptions_unittest.cc | 116 +++++++++++++++++++++
chrome/test/BUILD.gn | 1 +
7 files changed, 262 insertions(+), 27 deletions(-)

Upstream: git.chromium.org


  • Share