SerializeHostDescriptions needs to deal with multiple same-named hosts |
|||||
Issue descriptionSerializeHostDescriptions takes a vector of DevToolsAgentHost descriptions. Each of them has a |name| string, which is an identifier of the node. It is possible to pass in multiple descriptions with the same |name|. Currently, this leads to the helper function CreateDictionaryForest populating a vector of root nodes with copies of the same pointers, which then leads the Serialize helper to repeatedly move out the same DictionaryValue. As a result, the ListValue returned by SerializeHostDescriptions contains DictionaryValues in undefined state. By lucky coincidence, this was caught by the test SerializeDictionaryForestTest.Stub when merged to M59, where DictionaryValue still has its dictionary storage held by a unique_ptr -- moving that out makes the pointer null and the test crashes when dereferencing it later. On trunk (M60) this issue goes unnoticed, because the dictionary storage is directly in DictionaryValue, and while undefined, holds rests of data good enough to make the test pass. This can be fixed either by requiring that SerializeHostDescriptions never gets name duplicates, or by requiring that it de-duplicates those (e.g., by ignoring all but the first node of a given name). The first option means to have a contract which is only checked by DCHECKS, and a test sending multiple same-named hosts would not be possible (breaks the contract). Hence this issue could go unnoticed in the user builds, in case it is or becomes possible to create hosts with identical names. Therefore I prefer the second option. (Happy to discuss this further with devtools OWNERS.) Because M59 needs to get r465948 back (it was merged and later reverted due to the above described issue), this (coming) fix will also need to be merged into M59, right after re-merging r465948. Once the fix lands on trunk and reaches Canary, I will request merge. I'm Cc-ing dgozman@, a devtools OWNER who reviewed the previous fix and who I will ask to review the new fix as well. I'm also Cc-ing jdoerrie@ who might enjoy reading another creative way in which I accidentally misused base::Value API and how pure luck with branch timing prevented that from going unnoticed. :)
,
Apr 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b95f5bc3e4433d6e28d44850506f47efcf8e4fff commit b95f5bc3e4433d6e28d44850506f47efcf8e4fff Author: vabr <vabr@chromium.org> Date: Tue Apr 25 07:17:03 2017 SerializeHostDescriptions ignores hosts with duplicated names |HostDescriptionNode.name| identifies the node and should not be shared by two different nodes. If multiple nodes are passed to SerializeHostDescriptions with the same name, currently SerializeHostDescriptions ends up using DictionaryValue after performing std::move on it, because of mixing nodes with the same name. This CL replaces some internal use of std::vector with std::unordered_set to discard duplicities. The CL also improves the unit test to be less dependent on the resulting order of the serialized descriptions. The CL also fixes the test name to match its filename and the name of the tested function. BUG= 714368 Review-Url: https://codereview.chromium.org/2835823002 Cr-Commit-Position: refs/heads/master@{#466913} [modify] https://crrev.com/b95f5bc3e4433d6e28d44850506f47efcf8e4fff/chrome/browser/devtools/serialize_host_descriptions.cc [modify] https://crrev.com/b95f5bc3e4433d6e28d44850506f47efcf8e4fff/chrome/browser/devtools/serialize_host_descriptions_unittest.cc
,
Apr 26 2017
r466913 is in 60.0.3081.0, which so far only reached Win ASAN canaries. I'll wait until it reaches non-ASAN canaries as well before requesting a merge.
,
Apr 27 2017
Requesting merge of r466913 to M59 (branch 3071). This, together with a separate merge mentioned in #3, are crucial to fix a use after free bug in M59. The patch is small and has been in Canary since yesterday.
,
Apr 27 2017
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 27 2017
Thanks, I'm executing a merge sequence as described in https://crbug.com/712119#c19 now.
,
Apr 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6152aef1f655ee4ff82bd7cbc9210e1857855343 commit 6152aef1f655ee4ff82bd7cbc9210e1857855343 Author: Vaclav Brozek <vabr@chromium.org> Date: Thu Apr 27 08:33:55 2017 SerializeHostDescriptions ignores hosts with duplicated names |HostDescriptionNode.name| identifies the node and should not be shared by two different nodes. If multiple nodes are passed to SerializeHostDescriptions with the same name, currently SerializeHostDescriptions ends up using DictionaryValue after performing std::move on it, because of mixing nodes with the same name. This CL replaces some internal use of std::vector with std::unordered_set to discard duplicities. The CL also improves the unit test to be less dependent on the resulting order of the serialized descriptions. The CL also fixes the test name to match its filename and the name of the tested function. BUG= 714368 Review-Url: https://codereview.chromium.org/2835823002 Cr-Commit-Position: refs/heads/master@{#466913} (cherry picked from commit b95f5bc3e4433d6e28d44850506f47efcf8e4fff) Review-Url: https://codereview.chromium.org/2843383002 . Cr-Commit-Position: refs/branch-heads/3071@{#260} Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641} [modify] https://crrev.com/6152aef1f655ee4ff82bd7cbc9210e1857855343/chrome/browser/devtools/serialize_host_descriptions.cc [modify] https://crrev.com/6152aef1f655ee4ff82bd7cbc9210e1857855343/chrome/browser/devtools/serialize_host_descriptions_unittest.cc
,
Apr 27 2017
I'll continue watching beta builders, but unless I see a breakage, this can be considered fixed. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by vabr@chromium.org
, Apr 22 2017