Crash in DirectX capturer

webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc

Index: webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc
diff --git a/webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc b/webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc
index b17c195340b0e495fa6e123a2b33c06141e9a2c5..27bb885793969ad8f01cd7bb9eba6fa57e0da71b 100644
--- a/webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc
+++ b/webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc
@@ -297,7 +297,7 @@ void DxgiOutputDuplicator::Unregister(const Context* const context) {
 
 void DxgiOutputDuplicator::SpreadContextChange(const Context* const source) {
   for (Context* dest : contexts_) {
-    if (dest != source) {
+    if (dest && dest != source) {

[Jamie, 2016/11/11 00:00:41]

Under what circumstances does it makes sense to allow nulls in contexts_? Could
we instead make sure that doesn't happen?

[Hzj_jie, 2016/11/11 00:10:51]

It's a performance trick, and totally expected. You may refer to line 290.

More background,
The Context is designed as a bridge between singleton (DXGI APIs) and multiton
(ScreenCapturerWinDirectx). Each ScreenCapturerWinDirectx has its own state,
since it's double-buffered, and we do not want to copy entire frame each time
for performance concern. But DxgiDuplicatorController is singleton because of
the restriction of DXGI APIs.
So once a ScreenCapturerWinDirectx is created, it needs to register its Context
object in DxgiDuplicatorController to get updated region information during each
capturer call. And once it's destroyed, its context needs to be removed. So in
line 290, there is a small trick to avoid removing element from a vector (O(1)
vs O(n)). So we need to have this check here.

[Jamie, 2016/11/11 00:15:21]

Thanks for the context. I think my question changes in that case to whether or
not that performance optimization is worthwhile. How large is the vector
expected to grow, and how often is Unregister called? If it's not on the
critical path, I would prefer we just clean up the vector properly, otherwise
we're leaving ourselves open to bugs like this in the future.

       dest->updated_region.AddRegion(source->updated_region);
     }
   }