Skip to content

Commit 7b70db8

Browse files
oldnewthingoldnewthing
authored
Fix holes in IObservableVector<IInspectable> (#544)
1 parent e347c60 commit 7b70db8

File tree

3 files changed

+97
-23
lines changed

3 files changed

+97
-23
lines changed

‎strings/base_collections_vector.h‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,15 @@ namespace winrt::impl
100100

101101
bool IndexOf(Windows::Foundation::IInspectable const& value, uint32_t& index) const
102102
{
103-
return IndexOf(unbox_value<T>(value), index);
103+
try
104+
{
105+
return IndexOf(unbox_value<T>(value), index);
106+
}
107+
catch (hresult_no_interface const&)
108+
{
109+
index = 0;
110+
return false;
111+
}
104112
}
105113

106114
using base_type::GetMany;
@@ -167,16 +175,15 @@ namespace winrt::impl
167175

168176
void ReplaceAll(array_view<Windows::Foundation::IInspectable const> values)
169177
{
170-
this->increment_version();
171-
m_values.clear();
172-
m_values.reserve(values.size());
178+
Container new_values;
179+
new_values.reserve(values.size());
173180

174-
std::transform(values.begin(), values.end(), std::back_inserter(m_values), [&](auto && value)
181+
std::transform(values.begin(), values.end(), std::back_inserter(new_values), [&](auto && value)
175182
{
176183
return unbox_value<T>(value);
177184
});
178185

179-
this->call_changed(Windows::Foundation::Collections::CollectionChange::Reset, 0);
186+
base_type::ReplaceAll(std::move(new_values));
180187
}
181188

182189
using base_type::VectorChanged;

‎strings/base_reference_produce.h‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -288,26 +288,33 @@ WINRT_EXPORT namespace winrt
288288
{
289289
return value.as<T>();
290290
}
291-
else if constexpr (std::is_enum_v<T>)
291+
else
292292
{
293-
if (auto temp = value.try_as<Windows::Foundation::IReference<T>>())
293+
if (!value)
294294
{
295-
return temp.Value();
295+
throw hresult_no_interface();
296296
}
297-
else
297+
if constexpr (std::is_enum_v<T>)
298298
{
299-
return static_cast<T>(value.as<Windows::Foundation::IReference<std::underlying_type_t<T>>>().Value());
299+
if (auto temp = value.try_as<Windows::Foundation::IReference<T>>())
300+
{
301+
return temp.Value();
302+
}
303+
else
304+
{
305+
return static_cast<T>(value.as<Windows::Foundation::IReference<std::underlying_type_t<T>>>().Value());
306+
}
300307
}
301-
}
302308
#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
303-
else if constexpr (std::is_same_v<T, GUID>)
304-
{
305-
return value.as<Windows::Foundation::IReference<guid>>().Value();
306-
}
309+
else if constexpr (std::is_same_v<T, GUID>)
310+
{
311+
return value.as<Windows::Foundation::IReference<guid>>().Value();
312+
}
307313
#endif
308-
else
309-
{
310-
return value.as<Windows::Foundation::IReference<T>>().Value();
314+
else
315+
{
316+
return value.as<Windows::Foundation::IReference<T>>().Value();
317+
}
311318
}
312319
}
313320

‎test/test/single_threaded_observable_vector.cpp‎

Lines changed: 64 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,10 @@ TEST_CASE("single_threaded_observable_vector")
217217
REQUIRE((vector_i.IndexOf(2, index) && index == 1));
218218
index = 0;
219219
REQUIRE((vector_o.IndexOf(box_value(2), index) && index == 1));
220+
221+
// A vector of integers never contains non-integers.
222+
REQUIRE(!vector_o.IndexOf(nullptr, index));
223+
REQUIRE(!vector_o.IndexOf(box_value(L"Not-an-integer"), index));
220224
}
221225
{
222226
// GetView forwarding.
@@ -241,6 +245,10 @@ TEST_CASE("single_threaded_observable_vector")
241245

242246
REQUIRE(vector_i.GetAt(0) == 10);
243247
REQUIRE(vector_i.GetAt(1) == 20);
248+
249+
// Can't put non-integers in a vector of integers.
250+
REQUIRE_THROWS_AS(vector_o.SetAt(0, nullptr), hresult_no_interface);
251+
REQUIRE_THROWS_AS(vector_o.SetAt(0, box_value(L"Not-an-integer")), hresult_no_interface);
244252
}
245253
{
246254
// InsertAt forwarding.
@@ -258,6 +266,10 @@ TEST_CASE("single_threaded_observable_vector")
258266
REQUIRE(vector_i.GetAt(1) == 2);
259267
REQUIRE(vector_i.GetAt(2) == 3);
260268
REQUIRE(vector_i.GetAt(3) == 4);
269+
270+
// Can't put non-integers in a vector of integers.
271+
REQUIRE_THROWS_AS(vector_o.InsertAt(0, nullptr), hresult_no_interface);
272+
REQUIRE_THROWS_AS(vector_o.InsertAt(0, box_value(L"Not-an-integer")), hresult_no_interface);
261273
}
262274
{
263275
// Append forwarding.
@@ -273,6 +285,10 @@ TEST_CASE("single_threaded_observable_vector")
273285
REQUIRE(vector_i.Size() == 2);
274286
REQUIRE(vector_i.GetAt(0) == 1);
275287
REQUIRE(vector_i.GetAt(1) == 2);
288+
289+
// Can't put non-integers in a vector of integers.
290+
REQUIRE_THROWS_AS(vector_o.Append(nullptr), hresult_no_interface);
291+
REQUIRE_THROWS_AS(vector_o.Append(box_value(L"Not-an-integer")), hresult_no_interface);
276292
}
277293
{
278294
// GetMany boxing.
@@ -334,13 +350,13 @@ TEST_CASE("single_threaded_observable_vector")
334350
bool changed_i{};
335351
bool changed_o{};
336352

337-
vector_i.VectorChanged([&](IObservableVector<int> const& sender, IVectorChangedEventArgs const&)
353+
auto token_i = vector_i.VectorChanged(auto_revoke, [&](IObservableVector<int> const& sender, IVectorChangedEventArgs const&)
338354
{
339355
changed_i = sender.GetAt(0) == 123;
340356
REQUIRE(sender == vector_i);
341357
});
342358

343-
vector_o.VectorChanged([&](IObservableVector<IInspectable> const& sender, IVectorChangedEventArgs const&)
359+
auto token_o = vector_o.VectorChanged(auto_revoke, [&](IObservableVector<IInspectable> const& sender, IVectorChangedEventArgs const&)
344360
{
345361
changed_o = unbox_value<int>(sender.GetAt(0)) == 123;
346362
REQUIRE(sender == vector_o);
@@ -357,6 +373,35 @@ TEST_CASE("single_threaded_observable_vector")
357373
REQUIRE(vector_i.Size() == 1);
358374
REQUIRE(vector_i.GetAt(0) == 123);
359375
}
376+
{
377+
bool changed_i{};
378+
bool changed_o{};
379+
380+
auto token_i = vector_i.VectorChanged(auto_revoke, [&](IObservableVector<int> const& sender, IVectorChangedEventArgs const&)
381+
{
382+
changed_i = true;
383+
REQUIRE(sender == vector_i);
384+
});
385+
386+
auto token_o = vector_o.VectorChanged(auto_revoke, [&](IObservableVector<IInspectable> const& sender, IVectorChangedEventArgs const&)
387+
{
388+
changed_i = true;
389+
REQUIRE(sender == vector_o);
390+
});
391+
392+
// Verify strong exception guarantee when replacing with non-integers.
393+
REQUIRE_THROWS_AS(vector_o.ReplaceAll({ box_value(42), nullptr }), hresult_no_interface);
394+
REQUIRE(!changed_i); // unchanged on failure
395+
REQUIRE(!changed_o);
396+
REQUIRE(vector_i.Size() == 1);
397+
REQUIRE(vector_i.GetAt(0) == 123);
398+
399+
REQUIRE_THROWS_AS(vector_o.ReplaceAll({ box_value(L"Not-an-integer") }), hresult_no_interface);
400+
REQUIRE(!changed_i); // unchanged on failure
401+
REQUIRE(!changed_o);
402+
REQUIRE(vector_i.Size() == 1);
403+
REQUIRE(vector_i.GetAt(0) == 123);
404+
}
360405

361406
{
362407
IIterator<int> iterator_i = vector_i.First();
@@ -368,13 +413,13 @@ TEST_CASE("single_threaded_observable_vector")
368413
bool changed_i{};
369414
bool changed_o{};
370415

371-
vector_i.VectorChanged([&](IObservableVector<int> const& sender, IVectorChangedEventArgs const&)
416+
auto token_i = vector_i.VectorChanged(auto_revoke, [&](IObservableVector<int> const& sender, IVectorChangedEventArgs const&)
372417
{
373418
changed_i = sender.GetAt(0) == 123;
374419
REQUIRE(sender == vector_i);
375420
});
376421

377-
vector_o.VectorChanged([&](IObservableVector<IInspectable> const& sender, IVectorChangedEventArgs const&)
422+
auto token_o = vector_o.VectorChanged(auto_revoke, [&](IObservableVector<IInspectable> const& sender, IVectorChangedEventArgs const&)
378423
{
379424
changed_o = unbox_value<int>(sender.GetAt(0)) == 123;
380425
REQUIRE(sender == vector_o);
@@ -392,4 +437,19 @@ TEST_CASE("single_threaded_observable_vector")
392437
REQUIRE(vector_i.GetAt(0) == 123);
393438
}
394439
}
440+
441+
{
442+
IObservableVector<IVector<int>> vector_i = single_threaded_observable_vector<IVector<int>>({ });
443+
IObservableVector<IInspectable> vector_o = vector_i.as<IObservableVector<IInspectable>>();
444+
445+
// Verify that nulls are legal if the underlying type derives from IInspectable.
446+
REQUIRE_NOTHROW(vector_o.Append(nullptr));
447+
uint32_t index = 99;
448+
REQUIRE(vector_o.IndexOf(nullptr, index));
449+
REQUIRE(index == 0);
450+
451+
// Verify that type mismatch should report "not found" rather than
452+
// finding the null entry.
453+
REQUIRE(!vector_o.IndexOf(box_value(L"not-a-vector"), index));
454+
}
395455
}

0 commit comments

Comments
 (0)