Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2606323002:
Add null check for MediaDevicesManager in MDDH destructor. (Closed)
|
Created:
3 years, 11 months ago by Guido Urdaneta Modified:
3 years, 11 months ago Reviewers:
tommi (sloooow) - chröme CC:
chromium-reviews, posciak+watch_chromium.org, jam, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, feature-media-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd null check for MediaDevicesManager in MDDH destructor.
Under certain scenarios (e.g., shutdown) it is possible that
the MediaDevicesManager object has been destroyed before a
MediaDevicesDispatcherHost tries to access it.
BUG=676919
Committed: https://crrev.com/9a54c7ed21d7dd59213d4ac6c74f3500884de656
Cr-Commit-Position: refs/heads/master@{#441136}
Patch Set 1 #
Total comments: 2
Patch Set 2 : tommi's comment #Patch Set 3 : remove invalid DCHECK #
Total comments: 2
Patch Set 4 : Add comment #
Messages
Total messages: 32 (20 generated)
Description was changed from ========== Add null check for MediaDevicesManager in MDDH destructor. During certain scenarios (e.g., shutdown) it is possible that the MediaDevicesManager object has been destroyed before a MediaDevicesDispatcherHost tries to access it. BUG=676919 ========== to ========== Add null check for MediaDevicesManager in MDDH destructor. It seems that in certain scenarios (e.g., shutdown) it is possible that the MediaDevicesManager object has been destroyed before a MediaDevicesDispatcherHost tries to access it. BUG=676919 ==========
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add null check for MediaDevicesManager in MDDH destructor. It seems that in certain scenarios (e.g., shutdown) it is possible that the MediaDevicesManager object has been destroyed before a MediaDevicesDispatcherHost tries to access it. BUG=676919 ========== to ========== Add null check for MediaDevicesManager in MDDH destructor. Under certain scenarios (e.g., shutdown) it is possible that the MediaDevicesManager object has been destroyed before a MediaDevicesDispatcherHost tries to access it. BUG=676919 ==========
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
guidou@chromium.org changed reviewers: + tommi@chromium.org
Hi, PTAL
https://codereview.chromium.org/2606323002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/media_devices_dispatcher_host.cc (right): https://codereview.chromium.org/2606323002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/media_devices_dispatcher_host.cc:99: if (media_stream_manager_->media_devices_manager() && would it make sense to do this check outside the for loop?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2606323002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/media_devices_dispatcher_host.cc (right): https://codereview.chromium.org/2606323002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/media_devices_dispatcher_host.cc:99: if (media_stream_manager_->media_devices_manager() && On 2017/01/03 13:13:20, tommi (chrömium) wrote: > would it make sense to do this check outside the for loop? Done.
lgtm
The CQ bit was checked by guidou@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tommi@: I had to remove a DCHECK that is now invalid. still lgtm?
lgtm with the below comment added https://codereview.chromium.org/2606323002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/media_stream_manager.cc (left): https://codereview.chromium.org/2606323002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/media_stream_manager.cc:460: DCHECK(media_devices_manager_.get()); can you add a comment here instead that explains that nullptr may be returned during shutdown?
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org Link to the patchset: https://codereview.chromium.org/2606323002/#ps100001 (title: "Add comment")
https://codereview.chromium.org/2606323002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/media_stream_manager.cc (left): https://codereview.chromium.org/2606323002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/media_stream_manager.cc:460: DCHECK(media_devices_manager_.get()); On 2017/01/03 15:58:27, tommi (chrömium) wrote: > can you add a comment here instead that explains that nullptr may be returned > during shutdown? Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483459359977750,
"parent_rev": "d80b09e17afb3b3ccc621d3f6d916007b2eb9916", "commit_rev":
"5969e3f8d5d730ce497e932485e654c7cab90d13"}
Message was sent while issue was closed.
Description was changed from ========== Add null check for MediaDevicesManager in MDDH destructor. Under certain scenarios (e.g., shutdown) it is possible that the MediaDevicesManager object has been destroyed before a MediaDevicesDispatcherHost tries to access it. BUG=676919 ========== to ========== Add null check for MediaDevicesManager in MDDH destructor. Under certain scenarios (e.g., shutdown) it is possible that the MediaDevicesManager object has been destroyed before a MediaDevicesDispatcherHost tries to access it. BUG=676919 Review-Url: https://codereview.chromium.org/2606323002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add null check for MediaDevicesManager in MDDH destructor. Under certain scenarios (e.g., shutdown) it is possible that the MediaDevicesManager object has been destroyed before a MediaDevicesDispatcherHost tries to access it. BUG=676919 Review-Url: https://codereview.chromium.org/2606323002 ========== to ========== Add null check for MediaDevicesManager in MDDH destructor. Under certain scenarios (e.g., shutdown) it is possible that the MediaDevicesManager object has been destroyed before a MediaDevicesDispatcherHost tries to access it. BUG=676919 Committed: https://crrev.com/9a54c7ed21d7dd59213d4ac6c74f3500884de656 Cr-Commit-Position: refs/heads/master@{#441136} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9a54c7ed21d7dd59213d4ac6c74f3500884de656 Cr-Commit-Position: refs/heads/master@{#441136} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
