Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2529853002:
Fix shutdown crash in AudioOutputAuthorizationHandler. (Closed)
|
Created:
4 years ago by Max Morin Modified:
4 years ago CC:
audio-mojo-cl_google.com, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, miu+watch_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix shutdown crash in AudioOutputAuthorizationHandler.
If fetching device parameters is scheduled during shutdown, it is possible that the UI thread starts deleting the AudioManager, causing a crash.
The problem arises like this:
1. AudioOutputAuthorizationHandler posts GetAudioParametersOnDeviceThread to the audio thread.
2. Shutdown is started, browser threads other than UI are stopped and audio_manager_ in BrowserMainLoop is reset. The causes g_last_created in AudioManager to be nulled and a task to delete the pointer is posted to the audio thread.
3. GetAudioParametersOnDeviceThread runs and calls AudioManager::Get(), which returns g_last_created (i.e. null), and dereferences it, causing a crash.
On the contrary, if the AudioManager pointer was stored in AudioOutputAuthorizationhandler and passed into GetAudioParametersOnDeviceThread, we wouldn't have a problem with the UI thread nulling the global pointer. We know that the AudioManager won't be destructed before it fires because the task to destroy the AudioManager is posted after the IO thread is shutdown, and
GetAudioParametersOnDeviceThread is posted before/during the shutdown of the IO thread.
The AudioManager* is now passed in to the constructor of AudioOutputAuthorizationhandler.
Drive-by fix: Remove MakeAuthorizationData, since it's only called form a single place now. The string argument was irrelevant (and even the wrong kind of id).
Also a drive-by fix: elimination of bare new in initialization of AudioOutputAuthorizationhandler.
BUG=656923, 667981
Committed: https://crrev.com/070d5b996806694c9303f75d9a419392041dec9c
Cr-Commit-Position: refs/heads/master@{#434460}
Patch Set 1 #Patch Set 2 : New fix. #Patch Set 3 : Add mac comment. #
Total comments: 11
Patch Set 4 : . #
Total comments: 6
Messages
Total messages: 28 (13 generated)
Description was changed from ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread deletes the AudioManager, causing a crash. The code before https://codereview.chromium.org/2424163004/ avoided this by delaying shutdown until AudioRendererHost was deleted (I think, how this works is unclear to me). The fix is to make the AudioRendererHost stay alive while an authorization is in progress. Drive-by fix: simplify signature of MakeAuthorizationData. The string argument was irrelevant (and even the wrong kind of id). BUG=656923,667981 ========== to ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread deletes the AudioManager, causing a crash. The code before https://codereview.chromium.org/2424163004/ avoided this by delaying shutdown until AudioRendererHost was deleted (I think, how this works is unclear to me). The fix is to make the AudioRendererHost stay alive while an authorization is in progress. Drive-by fix: simplify signature of MakeAuthorizationData. The string argument was irrelevant (and even the wrong kind of id). BUG=656923,667981 ==========
maxmorin@chromium.org changed reviewers: + olka@chromium.org
Olga: WDYT about this approach? I would add comments explaining how the AudioManager lifetime works, but I don't know for sure.
Description was changed from ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread deletes the AudioManager, causing a crash. The code before https://codereview.chromium.org/2424163004/ avoided this by delaying shutdown until AudioRendererHost was deleted (I think, how this works is unclear to me). The fix is to make the AudioRendererHost stay alive while an authorization is in progress. Drive-by fix: simplify signature of MakeAuthorizationData. The string argument was irrelevant (and even the wrong kind of id). BUG=656923,667981 ========== to ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread deletes the AudioManager, causing a crash. The code before https://codereview.chromium.org/2424163004/ avoided this by delaying shutdown until AudioRendererHost was deleted (I think, how this works is unclear to me). The fix is to make the AudioRendererHost stay alive while an authorization is in progress. The AudioManager* is now passed in to the AudioOutputAuthorizationhandler constructor to clarify this dependency. Drive-by fix: simplify signature of MakeAuthorizationData. The string argument was irrelevant (and even the wrong kind of id). BUG=656923,667981 ==========
On 2016/11/24 08:26:32, Max Morin Chromium wrote: > Olga: WDYT about this approach? I would add comments explaining how the > AudioManager lifetime works, but I don't know for sure. Another possible reason that the old code worked is that the old code stored a pointer to the audio manager while the new used AudioManager::Get. When the audio manager is reset on the UI thread, the pointer given by AudioManager::Get is nulled, but it's actually deleted in a task posted to the audio thread (https://cs.chromium.org/chromium/src/media/audio/audio_manager.cc?sq=package:...). Maybe GetAudioParametersOnDeviceThread would always be scheduled before the deletion task, and thus be ok (for a generous definition of ok :)).
Description was changed from ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread deletes the AudioManager, causing a crash. The code before https://codereview.chromium.org/2424163004/ avoided this by delaying shutdown until AudioRendererHost was deleted (I think, how this works is unclear to me). The fix is to make the AudioRendererHost stay alive while an authorization is in progress. The AudioManager* is now passed in to the AudioOutputAuthorizationhandler constructor to clarify this dependency. Drive-by fix: simplify signature of MakeAuthorizationData. The string argument was irrelevant (and even the wrong kind of id). BUG=656923,667981 ========== to ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread starts deleting the AudioManager, causing a crash. The problem arises like this: 1. AudioOutputAuthorizationHandler posts GetAudioParametersOnDeviceThread to the audio thread. 2. Shutdown is started, browser threads other than UI are stopped and audio_manager_ in BrowserMainLoop is reset. The causes g_last_created in AudioManager to be nulled and a task to delete the pointer is posted to the audio thread. 3. GetAudioParametersOnDeviceThread runs and calls AudioManager::Get(), which returns g_last_created which is null, and dereferences it, causing a crash. On the contrary, if the AudioManager pointer was stored in AudioOutputAuthorizationhandler and passed into GetAudioParametersOnDeviceThread, we wouldn't have a problem with the UI thread nulling the global pointer. We know that the AudioManager won't be destructed before it fires because the task to destroy the AudioManager is posted after the IO thread is shutdown, and GetAudioParametersOnDeviceThread is posted before/during the shutdown of the IO thread. The AudioManager* is now passed in to the constructor of AudioOutputAuthorizationhandler. Drive-by fix: simplify signature of MakeAuthorizationData. The string argument was irrelevant (and even the wrong kind of id). BUG=656923,667981 ==========
Description was changed from ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread starts deleting the AudioManager, causing a crash. The problem arises like this: 1. AudioOutputAuthorizationHandler posts GetAudioParametersOnDeviceThread to the audio thread. 2. Shutdown is started, browser threads other than UI are stopped and audio_manager_ in BrowserMainLoop is reset. The causes g_last_created in AudioManager to be nulled and a task to delete the pointer is posted to the audio thread. 3. GetAudioParametersOnDeviceThread runs and calls AudioManager::Get(), which returns g_last_created which is null, and dereferences it, causing a crash. On the contrary, if the AudioManager pointer was stored in AudioOutputAuthorizationhandler and passed into GetAudioParametersOnDeviceThread, we wouldn't have a problem with the UI thread nulling the global pointer. We know that the AudioManager won't be destructed before it fires because the task to destroy the AudioManager is posted after the IO thread is shutdown, and GetAudioParametersOnDeviceThread is posted before/during the shutdown of the IO thread. The AudioManager* is now passed in to the constructor of AudioOutputAuthorizationhandler. Drive-by fix: simplify signature of MakeAuthorizationData. The string argument was irrelevant (and even the wrong kind of id). BUG=656923,667981 ========== to ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread starts deleting the AudioManager, causing a crash. The problem arises like this: 1. AudioOutputAuthorizationHandler posts GetAudioParametersOnDeviceThread to the audio thread. 2. Shutdown is started, browser threads other than UI are stopped and audio_manager_ in BrowserMainLoop is reset. The causes g_last_created in AudioManager to be nulled and a task to delete the pointer is posted to the audio thread. 3. GetAudioParametersOnDeviceThread runs and calls AudioManager::Get(), which returns g_last_created (i.e. null), and dereferences it, causing a crash. On the contrary, if the AudioManager pointer was stored in AudioOutputAuthorizationhandler and passed into GetAudioParametersOnDeviceThread, we wouldn't have a problem with the UI thread nulling the global pointer. We know that the AudioManager won't be destructed before it fires because the task to destroy the AudioManager is posted after the IO thread is shutdown, and GetAudioParametersOnDeviceThread is posted before/during the shutdown of the IO thread. The AudioManager* is now passed in to the constructor of AudioOutputAuthorizationhandler. Drive-by fix: simplify signature of MakeAuthorizationData. The string argument was irrelevant (and even the wrong kind of id). BUG=656923,667981 ==========
PTAL. I'm not sure what the best place to put comments about the audio managers lifetime it, but i put one in audio_output_authorization_handler.cc when posting to the audio thread.
Nice CL description! lgtm % minor comment clarification. https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:194: // Note: |*audio_manager_| outlives the IO thread, so unretained is safe. |audio_manager_| is deleted on audio manager thread after IO message loop destruction. https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:195: // Mac is a special case. On Mac, this task is posted to the UI thread, because audio managers tuns on UI thread there. https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:44: // AudioOutputAuthorizationHandler. This is kind of obvious: you pass pointers to them in the constructor. https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:49: return std::make_pair(stream_id, std::make_pair(false, std::string())); call it directly?
Description was changed from ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread starts deleting the AudioManager, causing a crash. The problem arises like this: 1. AudioOutputAuthorizationHandler posts GetAudioParametersOnDeviceThread to the audio thread. 2. Shutdown is started, browser threads other than UI are stopped and audio_manager_ in BrowserMainLoop is reset. The causes g_last_created in AudioManager to be nulled and a task to delete the pointer is posted to the audio thread. 3. GetAudioParametersOnDeviceThread runs and calls AudioManager::Get(), which returns g_last_created (i.e. null), and dereferences it, causing a crash. On the contrary, if the AudioManager pointer was stored in AudioOutputAuthorizationhandler and passed into GetAudioParametersOnDeviceThread, we wouldn't have a problem with the UI thread nulling the global pointer. We know that the AudioManager won't be destructed before it fires because the task to destroy the AudioManager is posted after the IO thread is shutdown, and GetAudioParametersOnDeviceThread is posted before/during the shutdown of the IO thread. The AudioManager* is now passed in to the constructor of AudioOutputAuthorizationhandler. Drive-by fix: simplify signature of MakeAuthorizationData. The string argument was irrelevant (and even the wrong kind of id). BUG=656923,667981 ========== to ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread starts deleting the AudioManager, causing a crash. The problem arises like this: 1. AudioOutputAuthorizationHandler posts GetAudioParametersOnDeviceThread to the audio thread. 2. Shutdown is started, browser threads other than UI are stopped and audio_manager_ in BrowserMainLoop is reset. The causes g_last_created in AudioManager to be nulled and a task to delete the pointer is posted to the audio thread. 3. GetAudioParametersOnDeviceThread runs and calls AudioManager::Get(), which returns g_last_created (i.e. null), and dereferences it, causing a crash. On the contrary, if the AudioManager pointer was stored in AudioOutputAuthorizationhandler and passed into GetAudioParametersOnDeviceThread, we wouldn't have a problem with the UI thread nulling the global pointer. We know that the AudioManager won't be destructed before it fires because the task to destroy the AudioManager is posted after the IO thread is shutdown, and GetAudioParametersOnDeviceThread is posted before/during the shutdown of the IO thread. The AudioManager* is now passed in to the constructor of AudioOutputAuthorizationhandler. Drive-by fix: Remove MakeAuthorizationData, since it's only called form a single place now. The string argument was irrelevant (and even the wrong kind of id). BUG=656923,667981 ==========
https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:194: // Note: |*audio_manager_| outlives the IO thread, so unretained is safe. On 2016/11/24 13:17:35, o1ka wrote: > |audio_manager_| is deleted on audio manager thread after IO message loop > destruction. Well, it's a bit more complicated since audio manager must not only outlive posting the task, but also executing the task, which is does since the task to delete it is posted after the IO thread is stopped. https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:195: // Mac is a special case. On Mac, this task is posted to the UI thread, On 2016/11/24 13:17:35, o1ka wrote: > because audio managers tuns on UI thread there. Done. https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.h (right): https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.h:44: // AudioOutputAuthorizationHandler. On 2016/11/24 13:17:36, o1ka wrote: > This is kind of obvious: you pass pointers to them in the constructor. Done. https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_renderer_host.cc:49: return std::make_pair(stream_id, std::make_pair(false, std::string())); On 2016/11/24 13:17:36, o1ka wrote: > call it directly? Done.
https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:194: // Note: |*audio_manager_| outlives the IO thread, so unretained is safe. On 2016/11/24 14:12:20, Max Morin Chromium wrote: > On 2016/11/24 13:17:35, o1ka wrote: > > |audio_manager_| is deleted on audio manager thread after IO message loop > > destruction. > > Well, it's a bit more complicated since audio manager must not only outlive > posting the task, but also executing the task, which is does since the task to > delete it is posted after the IO thread is stopped. Yes, it's exactly what I'm pointing to. It's not enough for audio_manager to outlive the IO thread. (and remove *) https://codereview.chromium.org/2529853002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2529853002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:57: permission_checker_(base::MakeUnique<MediaDevicesPermissionChecker>()), Why? https://codereview.chromium.org/2529853002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:196: // to delete the audio manager hasn't been posted yet. This means that task to delete -> destructor?
Description was changed from ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread starts deleting the AudioManager, causing a crash. The problem arises like this: 1. AudioOutputAuthorizationHandler posts GetAudioParametersOnDeviceThread to the audio thread. 2. Shutdown is started, browser threads other than UI are stopped and audio_manager_ in BrowserMainLoop is reset. The causes g_last_created in AudioManager to be nulled and a task to delete the pointer is posted to the audio thread. 3. GetAudioParametersOnDeviceThread runs and calls AudioManager::Get(), which returns g_last_created (i.e. null), and dereferences it, causing a crash. On the contrary, if the AudioManager pointer was stored in AudioOutputAuthorizationhandler and passed into GetAudioParametersOnDeviceThread, we wouldn't have a problem with the UI thread nulling the global pointer. We know that the AudioManager won't be destructed before it fires because the task to destroy the AudioManager is posted after the IO thread is shutdown, and GetAudioParametersOnDeviceThread is posted before/during the shutdown of the IO thread. The AudioManager* is now passed in to the constructor of AudioOutputAuthorizationhandler. Drive-by fix: Remove MakeAuthorizationData, since it's only called form a single place now. The string argument was irrelevant (and even the wrong kind of id). BUG=656923,667981 ========== to ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread starts deleting the AudioManager, causing a crash. The problem arises like this: 1. AudioOutputAuthorizationHandler posts GetAudioParametersOnDeviceThread to the audio thread. 2. Shutdown is started, browser threads other than UI are stopped and audio_manager_ in BrowserMainLoop is reset. The causes g_last_created in AudioManager to be nulled and a task to delete the pointer is posted to the audio thread. 3. GetAudioParametersOnDeviceThread runs and calls AudioManager::Get(), which returns g_last_created (i.e. null), and dereferences it, causing a crash. On the contrary, if the AudioManager pointer was stored in AudioOutputAuthorizationhandler and passed into GetAudioParametersOnDeviceThread, we wouldn't have a problem with the UI thread nulling the global pointer. We know that the AudioManager won't be destructed before it fires because the task to destroy the AudioManager is posted after the IO thread is shutdown, and GetAudioParametersOnDeviceThread is posted before/during the shutdown of the IO thread. The AudioManager* is now passed in to the constructor of AudioOutputAuthorizationhandler. Drive-by fix: Remove MakeAuthorizationData, since it's only called form a single place now. The string argument was irrelevant (and even the wrong kind of id). Also a drive-by fix: elimination of bare new in initialization of AudioOutputAuthorizationhandler. BUG=656923,667981 ==========
https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:194: // Note: |*audio_manager_| outlives the IO thread, so unretained is safe. On 2016/11/24 14:34:01, o1ka wrote: > On 2016/11/24 14:12:20, Max Morin Chromium wrote: > > On 2016/11/24 13:17:35, o1ka wrote: > > > |audio_manager_| is deleted on audio manager thread after IO message loop > > > destruction. > > > > Well, it's a bit more complicated since audio manager must not only outlive > > posting the task, but also executing the task, which is does since the task to > > delete it is posted after the IO thread is stopped. > > Yes, it's exactly what I'm pointing to. It's not enough for audio_manager to > outlive the IO thread. > (and remove *) Done in the latest patch set. https://codereview.chromium.org/2529853002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2529853002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:57: permission_checker_(base::MakeUnique<MediaDevicesPermissionChecker>()), On 2016/11/24 14:34:01, o1ka wrote: > Why? This was also a drive-by fix :). Reading on chromium-dev, I see many are quite hostile to bare new, see also comment at https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?q=makeunique&sq=p.... In this particular case, it wasn't clear in the old code that permission_checker_ wasn't a raw pointer, so you'd have to check the h file to see if the code was correct. https://codereview.chromium.org/2529853002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:196: // to delete the audio manager hasn't been posted yet. This means that On 2016/11/24 14:34:01, o1ka wrote: > task to delete -> destructor? Not really? The task will delete the pointer (if you follow the template nonsense in code search, you end up at https://cs.chromium.org/chromium/src/base/sequenced_task_runner_helpers.h?sq=...). This will also run the destructor, but that's not all the task does.
https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2529853002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:194: // Note: |*audio_manager_| outlives the IO thread, so unretained is safe. On 2016/11/24 14:49:03, Max Morin Chromium wrote: > On 2016/11/24 14:34:01, o1ka wrote: > > On 2016/11/24 14:12:20, Max Morin Chromium wrote: > > > On 2016/11/24 13:17:35, o1ka wrote: > > > > |audio_manager_| is deleted on audio manager thread after IO message loop > > > > destruction. > > > > > > Well, it's a bit more complicated since audio manager must not only outlive > > > posting the task, but also executing the task, which is does since the task > to > > > delete it is posted after the IO thread is stopped. > > > > Yes, it's exactly what I'm pointing to. It's not enough for audio_manager to > > outlive the IO thread. > > (and remove *) > > Done in the latest patch set. Acknowledged. https://codereview.chromium.org/2529853002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_output_authorization_handler.cc (right): https://codereview.chromium.org/2529853002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:57: permission_checker_(base::MakeUnique<MediaDevicesPermissionChecker>()), On 2016/11/24 14:49:03, Max Morin Chromium wrote: > On 2016/11/24 14:34:01, o1ka wrote: > > Why? > > This was also a drive-by fix :). Reading on chromium-dev, I see many are quite > hostile to bare new, see also comment at > https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?q=makeunique&sq=p.... > In this particular case, it wasn't clear in the old code that > permission_checker_ wasn't a raw pointer, so you'd have to check the h file to > see if the code was correct. Ok. Though I have not really seen a problem with the code. https://codereview.chromium.org/2529853002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_output_authorization_handler.cc:196: // to delete the audio manager hasn't been posted yet. This means that On 2016/11/24 14:49:03, Max Morin Chromium wrote: > On 2016/11/24 14:34:01, o1ka wrote: > > task to delete -> destructor? > > Not really? The task will delete the pointer (if you follow the template > nonsense in code search, you end up at > https://cs.chromium.org/chromium/src/base/sequenced_task_runner_helpers.h?sq=...). > This will also run the destructor, but that's not all the task does. :) Acknowledged.
maxmorin@chromium.org changed reviewers: + dalecurtis@chromium.org
Dale: PTAL at this followup CL. I introduced a subtle bug in the last one. :)
Seems simpler to just null check and abort processing since we're already in shutdown at this point, but this lgtm as well.
Thanks for fixing!
On 2016/11/24 18:11:42, DaleCurtis wrote: > Seems simpler to just null check and abort processing since we're already in > shutdown at this point, but this lgtm as well. Thanks Dale. Null-checking AudioManager::Get() in GetAudioParametersOnDeviceThread is racy. If we knew we would either get the original pointer or null, it would be fine, but access to the global pointer is not atomic, so it's technically not guaranteed.
The CQ bit was checked by maxmorin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from olka@chromium.org Link to the patchset: https://codereview.chromium.org/2529853002/#ps60001 (title: ".")
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": 60001, "attempt_start_ts": 1480061268953740,
"parent_rev": "bbfe28df3498f054f2264f352fd7cbd9eb1abc2c", "commit_rev":
"121bd8019cff69614248f7789bcb830ba5f77429"}
Message was sent while issue was closed.
Description was changed from ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread starts deleting the AudioManager, causing a crash. The problem arises like this: 1. AudioOutputAuthorizationHandler posts GetAudioParametersOnDeviceThread to the audio thread. 2. Shutdown is started, browser threads other than UI are stopped and audio_manager_ in BrowserMainLoop is reset. The causes g_last_created in AudioManager to be nulled and a task to delete the pointer is posted to the audio thread. 3. GetAudioParametersOnDeviceThread runs and calls AudioManager::Get(), which returns g_last_created (i.e. null), and dereferences it, causing a crash. On the contrary, if the AudioManager pointer was stored in AudioOutputAuthorizationhandler and passed into GetAudioParametersOnDeviceThread, we wouldn't have a problem with the UI thread nulling the global pointer. We know that the AudioManager won't be destructed before it fires because the task to destroy the AudioManager is posted after the IO thread is shutdown, and GetAudioParametersOnDeviceThread is posted before/during the shutdown of the IO thread. The AudioManager* is now passed in to the constructor of AudioOutputAuthorizationhandler. Drive-by fix: Remove MakeAuthorizationData, since it's only called form a single place now. The string argument was irrelevant (and even the wrong kind of id). Also a drive-by fix: elimination of bare new in initialization of AudioOutputAuthorizationhandler. BUG=656923,667981 ========== to ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread starts deleting the AudioManager, causing a crash. The problem arises like this: 1. AudioOutputAuthorizationHandler posts GetAudioParametersOnDeviceThread to the audio thread. 2. Shutdown is started, browser threads other than UI are stopped and audio_manager_ in BrowserMainLoop is reset. The causes g_last_created in AudioManager to be nulled and a task to delete the pointer is posted to the audio thread. 3. GetAudioParametersOnDeviceThread runs and calls AudioManager::Get(), which returns g_last_created (i.e. null), and dereferences it, causing a crash. On the contrary, if the AudioManager pointer was stored in AudioOutputAuthorizationhandler and passed into GetAudioParametersOnDeviceThread, we wouldn't have a problem with the UI thread nulling the global pointer. We know that the AudioManager won't be destructed before it fires because the task to destroy the AudioManager is posted after the IO thread is shutdown, and GetAudioParametersOnDeviceThread is posted before/during the shutdown of the IO thread. The AudioManager* is now passed in to the constructor of AudioOutputAuthorizationhandler. Drive-by fix: Remove MakeAuthorizationData, since it's only called form a single place now. The string argument was irrelevant (and even the wrong kind of id). Also a drive-by fix: elimination of bare new in initialization of AudioOutputAuthorizationhandler. BUG=656923,667981 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread starts deleting the AudioManager, causing a crash. The problem arises like this: 1. AudioOutputAuthorizationHandler posts GetAudioParametersOnDeviceThread to the audio thread. 2. Shutdown is started, browser threads other than UI are stopped and audio_manager_ in BrowserMainLoop is reset. The causes g_last_created in AudioManager to be nulled and a task to delete the pointer is posted to the audio thread. 3. GetAudioParametersOnDeviceThread runs and calls AudioManager::Get(), which returns g_last_created (i.e. null), and dereferences it, causing a crash. On the contrary, if the AudioManager pointer was stored in AudioOutputAuthorizationhandler and passed into GetAudioParametersOnDeviceThread, we wouldn't have a problem with the UI thread nulling the global pointer. We know that the AudioManager won't be destructed before it fires because the task to destroy the AudioManager is posted after the IO thread is shutdown, and GetAudioParametersOnDeviceThread is posted before/during the shutdown of the IO thread. The AudioManager* is now passed in to the constructor of AudioOutputAuthorizationhandler. Drive-by fix: Remove MakeAuthorizationData, since it's only called form a single place now. The string argument was irrelevant (and even the wrong kind of id). Also a drive-by fix: elimination of bare new in initialization of AudioOutputAuthorizationhandler. BUG=656923,667981 ========== to ========== Fix shutdown crash in AudioOutputAuthorizationHandler. If fetching device parameters is scheduled during shutdown, it is possible that the UI thread starts deleting the AudioManager, causing a crash. The problem arises like this: 1. AudioOutputAuthorizationHandler posts GetAudioParametersOnDeviceThread to the audio thread. 2. Shutdown is started, browser threads other than UI are stopped and audio_manager_ in BrowserMainLoop is reset. The causes g_last_created in AudioManager to be nulled and a task to delete the pointer is posted to the audio thread. 3. GetAudioParametersOnDeviceThread runs and calls AudioManager::Get(), which returns g_last_created (i.e. null), and dereferences it, causing a crash. On the contrary, if the AudioManager pointer was stored in AudioOutputAuthorizationhandler and passed into GetAudioParametersOnDeviceThread, we wouldn't have a problem with the UI thread nulling the global pointer. We know that the AudioManager won't be destructed before it fires because the task to destroy the AudioManager is posted after the IO thread is shutdown, and GetAudioParametersOnDeviceThread is posted before/during the shutdown of the IO thread. The AudioManager* is now passed in to the constructor of AudioOutputAuthorizationhandler. Drive-by fix: Remove MakeAuthorizationData, since it's only called form a single place now. The string argument was irrelevant (and even the wrong kind of id). Also a drive-by fix: elimination of bare new in initialization of AudioOutputAuthorizationhandler. BUG=656923,667981 Committed: https://crrev.com/070d5b996806694c9303f75d9a419392041dec9c Cr-Commit-Position: refs/heads/master@{#434460} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/070d5b996806694c9303f75d9a419392041dec9c Cr-Commit-Position: refs/heads/master@{#434460} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
