Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2540253002:
Fix heap use-after-free in PrintingMessageFilter (Closed)
DescriptionFix heap use-after-free in PrintingMessageFilter
Change PrintingMessageFilter and PluginInfoMessageFilter to use
KeyedServiceShutdownNotifier. This prevents the warning on Chrome
shutdown about pref observers due to PluginInfoMessageFilter not
destroying the pref observers and prevents a possible use after free in
PrintingMessageFilter if the Profile and PrefMember were deleted before
the pointer to the PrefMember it was storing previously.
BUG=669289
Committed: https://crrev.com/654f42dca9af36fa50f097514a39e97682c27730
Cr-Commit-Position: refs/heads/master@{#436049}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix compile error #
Total comments: 4
Messages
Total messages: 31 (21 generated)
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/2540253002/diff/1/chrome/browser/plugins/plug... File chrome/browser/plugins/plugin_info_message_filter.cc (right): https://codereview.chromium.org/2540253002/diff/1/chrome/browser/plugins/plug... chrome/browser/plugins/plugin_info_message_filter.cc:243: // Destroy on the UI thread because we contain a |PrefMember|. Oh hey, turns out this isn't necessary anymore either :) https://codereview.chromium.org/2540253002/diff/1/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (right): https://codereview.chromium.org/2540253002/diff/1/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:119: BrowserThread::DeleteOnUIThread::Destruct(this); I don't think you even need this anymore – once we properly shut the PrefMember down on the UI thread, it should be possible to destroy this object on any thread :)
https://codereview.chromium.org/2540253002/diff/1/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (right): https://codereview.chromium.org/2540253002/diff/1/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:119: BrowserThread::DeleteOnUIThread::Destruct(this); On 2016/11/30 22:53:18, Bernhard Bauer wrote: > I don't think you even need this anymore – once we properly shut the PrefMember > down on the UI thread, it should be possible to destroy this object on any > thread :) I tried removing this locally (along with the DCHECK in the destructor), but am getting a crash when I close the browser. It seems like the behavior depends on what gets destroyed first - the profile or the message filter. If the profile is destroyed first the PrefMember gets destroyed via ShutdownOnUIThread (this is the case the bug mentioned), but if the message filter gets destroyed first the PrefMember is still around and the attempt to delete it in ~PrintingMessageFilter() fails since it isn't called on the UI thread. Did I set something up incorrectly?
Description was changed from ========== Fix heap use-after-free in PrintingMessageFilter Change PrintingMessageFilter and PluginInfoMessageFilter to use KeyedServiceShutdownNotifier. This prevents the warning on Chrome shutdown about pref observers due to PluginInfoMessageFilter not destroying the pref observers and prevents a possible use after free in PrintingMessageFilter if the Profile and PrefMember were deleted before the pointer to the PrefMember it was storing previously. BUG=669289 ========== to ========== Fix heap use-after-free in PrintingMessageFilter Change PrintingMessageFilter and PluginInfoMessageFilter to use KeyedServiceShutdownNotifier. This prevents the warning on Chrome shutdown about pref observers due to PluginInfoMessageFilter not destroying the pref observers and prevents a possible use after free in PrintingMessageFilter if the Profile and PrefMember were deleted before the pointer to the PrefMember it was storing previously. BUG=669289 ==========
rbpotter@chromium.org changed reviewers: + vitalybuka@chromium.org
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Initial version LGTM; sorry for the noise. https://codereview.chromium.org/2540253002/diff/1/chrome/browser/printing/pri... File chrome/browser/printing/printing_message_filter.cc (right): https://codereview.chromium.org/2540253002/diff/1/chrome/browser/printing/pri... chrome/browser/printing/printing_message_filter.cc:119: BrowserThread::DeleteOnUIThread::Destruct(this); On 2016/12/01 01:11:12, rbpotter wrote: > On 2016/11/30 22:53:18, Bernhard Bauer wrote: > > I don't think you even need this anymore – once we properly shut the > PrefMember > > down on the UI thread, it should be possible to destroy this object on any > > thread :) > > I tried removing this locally (along with the DCHECK in the destructor), but am > getting a crash when I close the browser. It seems like the behavior depends on > what gets destroyed first - the profile or the message filter. If the profile is > destroyed first the PrefMember gets destroyed via ShutdownOnUIThread (this is > the case the bug mentioned), but if the message filter gets destroyed first the > PrefMember is still around and the attempt to delete it in > ~PrintingMessageFilter() fails since it isn't called on the UI thread. Did I set > something up incorrectly? Oh, maybe the PrintingMessageFilter gets destroyed when the channel is closed, which is before the profile is shutdown… Okay, nevermind then.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Uploaded a new patch that returns to the original version and fixes a compilation error from the bots.
https://codereview.chromium.org/2540253002/diff/40001/chrome/browser/printing... File chrome/browser/printing/printing_message_filter.cc (right): https://codereview.chromium.org/2540253002/diff/40001/chrome/browser/printing... chrome/browser/printing/printing_message_filter.cc:95: is_printing_enabled_.MoveToThread( Thy it's moved to IO thread here, but destroyed on UI later? https://codereview.chromium.org/2540253002/diff/40001/chrome/browser/printing... chrome/browser/printing/printing_message_filter.cc:105: is_printing_enabled_.Destroy(); what is guarantee that OnMessageReceived is not going to be called after this?
https://codereview.chromium.org/2540253002/diff/40001/chrome/browser/printing... File chrome/browser/printing/printing_message_filter.cc (right): https://codereview.chromium.org/2540253002/diff/40001/chrome/browser/printing... chrome/browser/printing/printing_message_filter.cc:95: is_printing_enabled_.MoveToThread( On 2016/12/02 18:51:00, Vitaly Buka wrote: > Thy it's moved to IO thread here, but destroyed on UI later? It's moved to the IO thread so that GetValue can operate from the IO thread, but it has to be destroyed on the UI thread regardless of whether it has been moved, per the function description (line 187 of https://cs.chromium.org/chromium/src/components/prefs/pref_member.h): "Unsubscribes the PrefMember from the PrefService...This method should only be called on the UI thread." https://codereview.chromium.org/2540253002/diff/40001/chrome/browser/printing... chrome/browser/printing/printing_message_filter.cc:105: is_printing_enabled_.Destroy(); On 2016/12/02 18:51:00, Vitaly Buka wrote: > what is guarantee that OnMessageReceived is not going to be called after this? If it does get called I think it will be okay as the Destroy() function description states (line 187 of https://cs.chromium.org/chromium/src/components/prefs/pref_member.h): "...the PrefMember may not be used any more on the UI thread. Assuming |MoveToThread| was previously called, |GetValue|, |IsManaged|, and |IsUserModifiable| can still be called from the other thread but the results will no longer update from the PrefService." Since GetValue is called from the IO thread, it won't update any more after the profile is destroyed, but it will not cause a crash. The profile gets destroyed when the browser closes so the message filter should destruct shortly afterward anyway, so I don't expect that the value not updating would cause a problem. What do you think? I can add some changes to exit from OnMessageReceived early if this has already been called, just not sure if it is necessary or not.
> Since GetValue is called from the IO thread, it won't update any more after the > profile is destroyed, but it will not cause a crash. The profile gets destroyed > when the browser closes so the message filter should destruct shortly afterward > anyway, so I don't expect that the value not updating would cause a problem. > What do you think? I can add some changes to exit from OnMessageReceived early > if this has already been called, just not sure if it is necessary or not. No updates is OK. So chrome/browser/printing/ LGTM if you think OnMessageReceived will not crash
The CQ bit was checked by rbpotter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2540253002/#ps40001 (title: "Fix compile error")
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": 40001, "attempt_start_ts": 1480712934502190,
"parent_rev": "eae9d14a97a08345769f2c4dd1ca05a75545622f", "commit_rev":
"57a998f5db5b842cb388108c0f5c988e7f699f94"}
Message was sent while issue was closed.
Description was changed from ========== Fix heap use-after-free in PrintingMessageFilter Change PrintingMessageFilter and PluginInfoMessageFilter to use KeyedServiceShutdownNotifier. This prevents the warning on Chrome shutdown about pref observers due to PluginInfoMessageFilter not destroying the pref observers and prevents a possible use after free in PrintingMessageFilter if the Profile and PrefMember were deleted before the pointer to the PrefMember it was storing previously. BUG=669289 ========== to ========== Fix heap use-after-free in PrintingMessageFilter Change PrintingMessageFilter and PluginInfoMessageFilter to use KeyedServiceShutdownNotifier. This prevents the warning on Chrome shutdown about pref observers due to PluginInfoMessageFilter not destroying the pref observers and prevents a possible use after free in PrintingMessageFilter if the Profile and PrefMember were deleted before the pointer to the PrefMember it was storing previously. BUG=669289 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix heap use-after-free in PrintingMessageFilter Change PrintingMessageFilter and PluginInfoMessageFilter to use KeyedServiceShutdownNotifier. This prevents the warning on Chrome shutdown about pref observers due to PluginInfoMessageFilter not destroying the pref observers and prevents a possible use after free in PrintingMessageFilter if the Profile and PrefMember were deleted before the pointer to the PrefMember it was storing previously. BUG=669289 ========== to ========== Fix heap use-after-free in PrintingMessageFilter Change PrintingMessageFilter and PluginInfoMessageFilter to use KeyedServiceShutdownNotifier. This prevents the warning on Chrome shutdown about pref observers due to PluginInfoMessageFilter not destroying the pref observers and prevents a possible use after free in PrintingMessageFilter if the Profile and PrefMember were deleted before the pointer to the PrefMember it was storing previously. BUG=669289 Committed: https://crrev.com/654f42dca9af36fa50f097514a39e97682c27730 Cr-Commit-Position: refs/heads/master@{#436049} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/654f42dca9af36fa50f097514a39e97682c27730 Cr-Commit-Position: refs/heads/master@{#436049} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
