Fix potential lifetime issue with render-frame-based media services hosted in the browser process |
|||||
Issue descriptionToday we are hosting render-frame-based media services in the browser process which will be used by MediaService running in either the browser process, or in other processes (e.g. gpu or utility), depending on the gn config. Here's a list of such services we have or plan to have: - OutputProtection: to check output protection status - PlatformVerification: to check whether the platform is secure - [In Progress] CdmFileIO: for the CDM to store persistent data - ProvisionFetcher: for Android MediaDrm device provisioning - [In Progress] MediaDrmStorage: To store persistent session data These services are all render-frame-based to enforce site-isolation and security. This is done by passing a RenderFrameHost into these services. The current model is that these services use a StrongBinding so that when MediaService is destroyed (typically as a result of the destruction of the media player or CDM in the render process), these services will also be destroyed. But given the nature of mojo IPC, the destruction order is hard to control and we "could" end up with timing issue where we try to use the RenderFrameHost after it's destructed. In https://chromiumcodereview.appspot.com/2765343003/, we have a solution to deal with this by using WebContentsObserver, and not using a StrongBinding. We should extract the WebContentsObserver logic to a common class which will be shared by all the render-frame-based media services.
,
Sep 22 2017
,
Sep 23 2017
engedy@ has an in-flight CL to simplify things somewhat in this space: https://chromium-review.googlesource.com/c/chromium/src/+/607667 Ideally, we shouldn't have to have every Mojo service observe navigations: what should happen is that when we bind the interface, we associate the origin strongly. Admittedly, it's a bit more fragile than if we were to have 1:1 frame to document lifetime (because the resulting classes will have to store a frame pointer and an origin member), but that's somewhat further down the road, and is blocked on several other work items.
,
Sep 23 2017
Let's discuss here in stead of in my CL: https://chromium-review.googlesource.com/c/chromium/src/+/678438 To be clear, these services is tied to RenderFrameHost's lifetime, but they are not used by clients in the renderer process. Rather, they are used by the mojo MediaService that runs in a separate process, e.g. utility, gpu or in the browser process itself. For example, in EME, we have a CDM in the render process, but the real CDM (a shared library) is running in the utility process (as part of MediaService). The CDM needs to be able to read/write to persistent storage, which isn't possible in the sandboxed utility process, so it needs a CdmFile service provided by the browser process. Conceptually, this service is tied to the CDM's lifetime, which is tied to the RenderFrames' lifetime, which should be tied to the RenderFrameHost's lifetime. It is also tied to the mojo connection with the MediaService obviously. This is why I am adding the "FrameServiceBase" to solve the potential lifetime issue. For more details, please see go/mojo-media-graph I skimmed engedy@'s CL, but it seems like it's more about the lifetime control on services provided by RFH to RF in the renderer process. I am not sure whether I can reuse it directly for the services I am working on. Please advise :)
,
Sep 26 2017
dcheng: kindly ping :) I think my CL that adds FrameServiceBase can wait, because the chance of this bug to happen should be very small. But then I don't think this should block jrummell@'s CLs. WDYT?
,
Sep 27 2017
Unfortunately, there are already race conditions like https://bugs.chromium.org/p/chromium/issues/detail?id=769189. The CL I previously saw was deriving an ID from the RFH, and could have this sort of race. So I think we need to resolve the browser side issues for sure. I'm sorry for the delay, but I'm currently travelling home. Would it be ok to chat on Thursday?
,
Sep 27 2017
Sure! Let's chat tomorrow :) Thanks!
,
Oct 5 2017
,
Oct 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/15303d0ccaaf531834cfce5e8b5ec51b7059fd44 commit 15303d0ccaaf531834cfce5e8b5ec51b7059fd44 Author: Xiaohan Wang <xhwang@chromium.org> Date: Fri Oct 06 05:22:44 2017 Add FrameServiceBase: base class for document bound mojo services This CL adds FrameServiceBase which is a base class for mojo interface implementations that is tied to the lifetime of a document. Also updates MediaDrmStorageImpl, OutputProtectionImpl and PlatformVerificationImpl to use the new FrameServiceBase class. TBR=halliwell@chromium.org,mnissler@chromium.org BUG= 707335 TEST=New unit tests for FrameServiceBase Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I7f69f70a3da24c784267ecc1d49fd681bbf1a71a Reviewed-on: https://chromium-review.googlesource.com/678438 Commit-Queue: Xiaohan Wang <xhwang@chromium.org> Reviewed-by: Luke Halliwell <halliwell@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Xiaohan Wang <xhwang@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: John Rummell <jrummell@chromium.org> Cr-Commit-Position: refs/heads/master@{#506979} [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/chrome/browser/BUILD.gn [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/chrome/browser/chromeos/attestation/platform_verification_impl.cc [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/chrome/browser/chromeos/attestation/platform_verification_impl.h [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/chrome/browser/media/android/cdm/media_drm_storage_factory.cc [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/chrome/browser/media/output_protection_impl.cc [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/chrome/browser/media/output_protection_impl.h [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/chromecast/browser/cast_content_browser_client.cc [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/components/cdm/browser/BUILD.gn [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/components/cdm/browser/DEPS [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/components/cdm/browser/media_drm_storage_impl.cc [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/components/cdm/browser/media_drm_storage_impl.h [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/components/cdm/browser/media_drm_storage_impl_unittest.cc [add] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/content/browser/frame_host/frame_service_base_unittest.cc [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/content/public/browser/BUILD.gn [add] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/content/public/browser/frame_service_base.h [modify] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/content/test/BUILD.gn [add] https://crrev.com/15303d0ccaaf531834cfce5e8b5ec51b7059fd44/content/test/echo.mojom
,
Oct 6 2017
CL landed. For anyone interested, there are some interesting discussion/context in the CL code review. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by xhw...@chromium.org
, Sep 22 2017