Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2884163002:
media: Use StrongBindingSet for InterfaceFactoryImpl in MediaService (Closed)
|
Created:
3 years, 7 months ago by xhwang Modified:
3 years, 7 months ago Reviewers:
alokp CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, alokp+watch_chromium.org, darin (slow to review) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Use StrongBindingSet for InterfaceFactoryImpl in MediaService
Previously InterfaceFactoryImpl instances are using SrongBinding such
that they will only be destroyed on connection error. However, we could
hit an issue when MediaService is being destructed, the MojoMediaClient
is already destroyed, and an InterfaceFactoryImpl still tries to access
the MojoMediaClient.
This CL ensures that when MediaService is being destructed, all
InterfaceFactoryImpl will also be destroyed to avoid the issue.
BUG=721965
Review-Url: https://codereview.chromium.org/2884163002
Cr-Commit-Position: refs/heads/master@{#472261}
Committed: https://chromium.googlesource.com/chromium/src/+/5223bd13f8a97e293361a3b9b2cc31101ba597da
Patch Set 1 #
Total comments: 2
Messages
Total messages: 22 (16 generated)
The CQ bit was checked by xhwang@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...
Description was changed from ========== media: Use StrongBindingSet for InterfaceFactoryImpl in MediaService Previously InterfaceFactoryImpl instances are using SrongBinding such that they will only be destroyed on connection error. However, we could hit an issue when MediaService is being destructed, the MojoMediaClient is already destroyed, and an InterfaceFactoryImpl still tries to access the MojoMediaClient. This CL ensures that when MediaService is being destructed, all InterfaceFactoryImpl will also be destroyed to avoid the issue. BUG=721965 TEST=Added new test. ========== to ========== media: Use StrongBindingSet for InterfaceFactoryImpl in MediaService Previously InterfaceFactoryImpl instances are using SrongBinding such that they will only be destroyed on connection error. However, we could hit an issue when MediaService is being destructed, the MojoMediaClient is already destroyed, and an InterfaceFactoryImpl still tries to access the MojoMediaClient. This CL ensures that when MediaService is being destructed, all InterfaceFactoryImpl will also be destroyed to avoid the issue. BUG=721965 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
xhwang@chromium.org changed reviewers: + alokp@chromium.org
PTAL
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xhwang@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...
lgtm https://codereview.chromium.org/2884163002/diff/1/media/mojo/services/media_s... File media/mojo/services/media_service.cc (right): https://codereview.chromium.org/2884163002/diff/1/media/mojo/services/media_s... media/mojo/services/media_service.cc:44: interface_factory_bindings_.CloseAllBindings(); Do we need to manually delete InterfaceFactory bindings ot |mojo_media_client_|? If they are declared in the correct order, the right thing should happen automatically in the destructor.
https://codereview.chromium.org/2884163002/diff/1/media/mojo/services/media_s... File media/mojo/services/media_service.cc (right): https://codereview.chromium.org/2884163002/diff/1/media/mojo/services/media_s... media/mojo/services/media_service.cc:44: interface_factory_bindings_.CloseAllBindings(); On 2017/05/16 16:54:44, alokp wrote: > Do we need to manually delete InterfaceFactory bindings ot |mojo_media_client_|? > If they are declared in the correct order, the right thing should happen > automatically in the destructor. That should work, but we need a comment to make sure the declaration order is correct. With that, I feel clearing everything here is cleaner.
The CQ bit was unchecked by xhwang@chromium.org
The CQ bit was checked by xhwang@chromium.org
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": 1, "attempt_start_ts": 1494957083981460, "parent_rev":
"40b5e68dcd6a93d50931898b2d180195966adac3", "commit_rev":
"5223bd13f8a97e293361a3b9b2cc31101ba597da"}
Message was sent while issue was closed.
Description was changed from ========== media: Use StrongBindingSet for InterfaceFactoryImpl in MediaService Previously InterfaceFactoryImpl instances are using SrongBinding such that they will only be destroyed on connection error. However, we could hit an issue when MediaService is being destructed, the MojoMediaClient is already destroyed, and an InterfaceFactoryImpl still tries to access the MojoMediaClient. This CL ensures that when MediaService is being destructed, all InterfaceFactoryImpl will also be destroyed to avoid the issue. BUG=721965 ========== to ========== media: Use StrongBindingSet for InterfaceFactoryImpl in MediaService Previously InterfaceFactoryImpl instances are using SrongBinding such that they will only be destroyed on connection error. However, we could hit an issue when MediaService is being destructed, the MojoMediaClient is already destroyed, and an InterfaceFactoryImpl still tries to access the MojoMediaClient. This CL ensures that when MediaService is being destructed, all InterfaceFactoryImpl will also be destroyed to avoid the issue. BUG=721965 Review-Url: https://codereview.chromium.org/2884163002 Cr-Commit-Position: refs/heads/master@{#472261} Committed: https://chromium.googlesource.com/chromium/src/+/5223bd13f8a97e293361a3b9b2cc... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/5223bd13f8a97e293361a3b9b2cc...
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2892563002/ by kolos@chromium.org. The reason for reverting is: Layout tests failure ; mojo/module-loading(-manual-deps-loading).html BUG=723461. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
