Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2125143003:
Add back RenderFrameHost sanity-check to AudioRendererHost. (Closed)
|
Created:
4 years, 5 months ago by miu Modified:
4 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, miu+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd back RenderFrameHost sanity-check to AudioRendererHost.
A render frame should always provide a valid render frame routing ID
whenever creating any audio outputs. There was once a simple DCHECK to
confirm a valid routing ID was specified in the CreateStream message;
but, this got dropped accidentally at some point.
This change adds back the sanity-checking and takes it one step further:
Now, the routing ID is validated by using it to do a look-up for a
RenderFrameHost instance. Only when the look-up succeeds is stream
creation allowed to proceed. This is designed to be a more-robust
protection for several critical browser features such as: tab OOM
killer, tab audio indicator, tab muting, tab audio capture, etc.
Committed: https://crrev.com/b2e49f2820c5696bb52118e1be84fdaf1049d2ed
Cr-Commit-Position: refs/heads/master@{#406499}
Patch Set 1 #Patch Set 2 : Only when DCHECKs are on. #
Total comments: 11
Patch Set 3 : Conditionally compile on DCHECK_IS_ON(). #
Total comments: 4
Patch Set 4 : REBASE #Patch Set 5 : REBASE again #
Messages
Total messages: 32 (11 generated)
miu@chromium.org changed reviewers: + olka@chromium.org
olka: PTAL. Given recent discussion, I'd like to make sure there's a robust check in the browser process to ensure render_frame_id is always being provided alongside the audio streams. Note that, with this change, unit tests on the trybots are currently failing. This indicates there has already been "code rot" because we did not have this check. I'll hold-off on committing this until you and guidou@ fix the PPAPI and WebRTC render_frame_id plumbing.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Do we want this validation in the general case? We're now blocking audio creation on UI thread contention. I think this is worth having as a DCHECK, but I'm less certain that it's worth having on the critical path.
The CQ bit was checked by miu@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...
On 2016/07/07 22:03:56, DaleCurtis wrote: > Do we want this validation in the general case? > > We're now blocking audio creation on UI thread contention. I think this is worth > having as a DCHECK, but I'm less certain that it's worth having on the critical > path. Normally I'd agree, but notice that the [expected] trybot failures above only triggered on the release buildbots (mac_chromium_rel_ng and linux_chromium_rel_ng)? I'm currently doing a dry-run CQ to see whether the CQ bots catch anything by running tests on debug builds... How about I just disable this for Official builds?
release build bots run with dchecks enabled last I heard.
Yup, "dcheck_always_on=1" https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...
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_...)
On 2016/07/07 23:01:14, DaleCurtis wrote: > Yup, "dcheck_always_on=1" > > https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel... So it is! Done (Patch Set 2).
https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:554: if (DCHECK_IS_ON()) { #if DCHECK_IS_ON() ? https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:558: validate_render_frame_id_function_, render_process_id_, What if validate_render_frame_id_function_ is nullptr? (set for testing, or just because of future code modifications). Add DCHECK? https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:593: DCHECK_GT(render_frame_id, 0) Do not quite understand why this DCHECK? why not DCHECK(render_frame_is_valid)?
https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:554: if (DCHECK_IS_ON()) { Hmm, I think we want #if syntax here instead, there are issues with this syntax and linking if functions are excluded.
Patchset #3 (id:40001) has been deleted
Thanks Dale and Olga for taking a look. I've basically made everything conditionally compile behind "#if DCHECK_IS_ON()". I built and run chrome and the unittests both with DCHECKs on and off to confirm there are no compile/link/runtime issues. PTAL. https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:554: if (DCHECK_IS_ON()) { On 2016/07/08 14:09:38, o1ka wrote: > #if DCHECK_IS_ON() ? Done. https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:554: if (DCHECK_IS_ON()) { On 2016/07/08 18:44:33, DaleCurtis wrote: > Hmm, I think we want #if syntax here instead, there are issues with this syntax > and linking if functions are excluded. Done. https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:558: validate_render_frame_id_function_, render_process_id_, On 2016/07/08 14:09:38, o1ka wrote: > What if validate_render_frame_id_function_ is nullptr? (set for testing, or just > because of future code modifications). Add DCHECK? It can't be. The ARH ctor sets it, so a test would have to explicitly set it to nullptr. If any future code change should set it to nullptr, then the developer will immediately know because the process will crash when attempting to run any tests (or commit the change in the CQ). https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:593: DCHECK_GT(render_frame_id, 0) On 2016/07/08 14:09:38, o1ka wrote: > Do not quite understand why this DCHECK? why not DCHECK(render_frame_is_valid)? My point was to further check whether the ID was invalid because it was <= 0, which means someone purposely hacked-in a bogus value (such as MSG_ROUTING_NONE). I removed this since, on second glace, I think I was just being too excessive here. Better to keep things simple.
https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:558: validate_render_frame_id_function_, render_process_id_, On 2016/07/08 21:21:37, miu wrote: > On 2016/07/08 14:09:38, o1ka wrote: > > What if validate_render_frame_id_function_ is nullptr? (set for testing, or > just > > because of future code modifications). Add DCHECK? > > It can't be. The ARH ctor sets it, so a test would have to explicitly set it to > nullptr. If any future code change should set it to nullptr, then the developer > will immediately know because the process will crash when attempting to run any > tests (or commit the change in the CQ). I would say it's still more human-friendly to add a DCHECK here: it's much more explicit than a crash in a callback and requires no debugging to understand what the problem is. https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:593: DCHECK_GT(render_frame_id, 0) On 2016/07/08 21:21:37, miu wrote: > On 2016/07/08 14:09:38, o1ka wrote: > > Do not quite understand why this DCHECK? why not > DCHECK(render_frame_is_valid)? > > My point was to further check whether the ID was invalid because it was <= 0, > which means someone purposely hacked-in a bogus value (such as > MSG_ROUTING_NONE). > > I removed this since, on second glace, I think I was just being too excessive > here. Better to keep things simple. Acknowledged. https://codereview.chromium.org/2125143003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/2125143003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.h:136: using ValidateRenderFrameIdFunction = probably #ifdef this as well? https://codereview.chromium.org/2125143003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.h:289: ValidateRenderFrameIdFunction validate_render_frame_id_function_; AudioRendererHost is exported from content. Is a situation possible that ARH user which is built with DCHECKs off is linked with ARH built with DCHECKs on, or vise verse? (I've seen problems like that in other projects, they are not easy to debug.)
lgtm
On 2016/07/11 10:35:50, o1ka wrote: > https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... > File content/browser/renderer_host/media/audio_renderer_host.cc (right): > > https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... > content/browser/renderer_host/media/audio_renderer_host.cc:558: > validate_render_frame_id_function_, render_process_id_, > On 2016/07/08 21:21:37, miu wrote: > > On 2016/07/08 14:09:38, o1ka wrote: > > > What if validate_render_frame_id_function_ is nullptr? (set for testing, or > > just > > > because of future code modifications). Add DCHECK? > > > > It can't be. The ARH ctor sets it, so a test would have to explicitly set it > to > > nullptr. If any future code change should set it to nullptr, then the > developer > > will immediately know because the process will crash when attempting to run > any > > tests (or commit the change in the CQ). > > I would say it's still more human-friendly to add a DCHECK here: it's much more > explicit than a crash in a callback and requires no debugging to understand what > the problem is. > > https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... > content/browser/renderer_host/media/audio_renderer_host.cc:593: > DCHECK_GT(render_frame_id, 0) > On 2016/07/08 21:21:37, miu wrote: > > On 2016/07/08 14:09:38, o1ka wrote: > > > Do not quite understand why this DCHECK? why not > > DCHECK(render_frame_is_valid)? > > > > My point was to further check whether the ID was invalid because it was <= 0, > > which means someone purposely hacked-in a bogus value (such as > > MSG_ROUTING_NONE). > > > > I removed this since, on second glace, I think I was just being too excessive > > here. Better to keep things simple. > > Acknowledged. > > https://codereview.chromium.org/2125143003/diff/60001/content/browser/rendere... > File content/browser/renderer_host/media/audio_renderer_host.h (right): > > https://codereview.chromium.org/2125143003/diff/60001/content/browser/rendere... > content/browser/renderer_host/media/audio_renderer_host.h:136: using > ValidateRenderFrameIdFunction = > probably #ifdef this as well? > > https://codereview.chromium.org/2125143003/diff/60001/content/browser/rendere... > content/browser/renderer_host/media/audio_renderer_host.h:289: > ValidateRenderFrameIdFunction validate_render_frame_id_function_; > AudioRendererHost is exported from content. > Is a situation possible that ARH user which is built with DCHECKs off is linked > with ARH built with DCHECKs on, or vise verse? (I've seen problems like that in > other projects, they are not easy to debug.) Other than that - lgtm. (And no need to wait for our changes to land this, we decided to skip passing fake framed ids)
Thanks for the code review. Replied to your last round of comments (did not result in any code changes). https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2125143003/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:558: validate_render_frame_id_function_, render_process_id_, On 2016/07/11 10:35:50, o1ka wrote: > I would say it's still more human-friendly to add a DCHECK here: it's much more Yep. The DCHECK() is already being done here: https://cs.chromium.org/chromium/src/base/bind_internal.h?rcl=1468509561&l=385 https://codereview.chromium.org/2125143003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/2125143003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.h:136: using ValidateRenderFrameIdFunction = On 2016/07/11 10:35:50, o1ka wrote: > probably #ifdef this as well? Based on past experience, we generally don't conditionally compile typedefs in Chromium when they do not contain platform-specific types. There should be no additional code generation resulting from this. https://codereview.chromium.org/2125143003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.h:289: ValidateRenderFrameIdFunction validate_render_frame_id_function_; On 2016/07/11 10:35:50, o1ka wrote: > AudioRendererHost is exported from content. > Is a situation possible that ARH user which is built with DCHECKs off is linked > with ARH built with DCHECKs on, or vise verse? (I've seen problems like that in > other projects, they are not easy to debug.) This shouldn't be an issue in the Chromium project. I'm not aware of any support for linking code built with different build_config.
The CQ bit was checked by miu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, olka@chromium.org Link to the patchset: https://codereview.chromium.org/2125143003/#ps80001 (title: "REBASE")
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
FYI--Waiting for https://codereview.chromium.org/2168463003/ to land (to disable flaky tests). Then, will retry landing this patch.
The CQ bit was checked by miu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, olka@chromium.org Link to the patchset: https://codereview.chromium.org/2125143003/#ps100001 (title: "REBASE again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add back RenderFrameHost sanity-check to AudioRendererHost. A render frame should always provide a valid render frame routing ID whenever creating any audio outputs. There was once a simple DCHECK to confirm a valid routing ID was specified in the CreateStream message; but, this got dropped accidentally at some point. This change adds back the sanity-checking and takes it one step further: Now, the routing ID is validated by using it to do a look-up for a RenderFrameHost instance. Only when the look-up succeeds is stream creation allowed to proceed. This is designed to be a more-robust protection for several critical browser features such as: tab OOM killer, tab audio indicator, tab muting, tab audio capture, etc. ========== to ========== Add back RenderFrameHost sanity-check to AudioRendererHost. A render frame should always provide a valid render frame routing ID whenever creating any audio outputs. There was once a simple DCHECK to confirm a valid routing ID was specified in the CreateStream message; but, this got dropped accidentally at some point. This change adds back the sanity-checking and takes it one step further: Now, the routing ID is validated by using it to do a look-up for a RenderFrameHost instance. Only when the look-up succeeds is stream creation allowed to proceed. This is designed to be a more-robust protection for several critical browser features such as: tab OOM killer, tab audio indicator, tab muting, tab audio capture, etc. Committed: https://crrev.com/b2e49f2820c5696bb52118e1be84fdaf1049d2ed Cr-Commit-Position: refs/heads/master@{#406499} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b2e49f2820c5696bb52118e1be84fdaf1049d2ed Cr-Commit-Position: refs/heads/master@{#406499} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
