Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(5)

Issue 1317443002: Fix UAF in Origin Info Bubble and permission settings UI. (Closed)

Created:
4 years, 1 month ago by palmer
Modified:
4 years, 1 month ago
CC:
tfarina, raymes+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix UAF in Origin Info Bubble and permission settings UI. In addition to fixing the UAF, will this also fix the problem of the bubble showing over the previous tab (if the bubble is open when the tab it was opened for closes). BUG=490492 TBR=tedchoc Committed: https://crrev.com/f2cba0d13b3a6d76dedede66731e5ca253d3b2af Cr-Commit-Position: refs/heads/master@{#346023}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Give WebsiteSettings a WebContents instead of an InfoBarService; retrieve the IBS just before it's … #

Total comments: 2

Patch Set 3 : Avoid null deref. #

Patch Set 4 : Finish up the Cocoa side. #

Patch Set 5 : Removed some chaff that was part of a previous iteration. #

Patch Set 6 : Fix the unit test's call site for WebsiteSettings ctor. #

Patch Set 7 : Restore the Cocoa changes. #

Patch Set 8 : Oops, Android has a call-site that needs to be changed too. Fixed. #

Patch Set 9 : Fix unit_tests build on Mac. #

Patch Set 10 : Fix another Andorid call site. Checked all call sites. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -43 lines) Patch
M chrome/browser/ui/android/connection_info_popup_android.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/android/website_settings_popup_android.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.h View 1 2 3 6 5 chunks +12 lines, -4 lines 0 comments Download
M chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm View 1 2 3 4 6 4 chunks +16 lines, -11 lines 0 comments Download
M chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller_unittest.mm View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/website_settings/website_settings_popup_view.h View 3 chunks +11 lines, -7 lines 0 comments Download
M chrome/browser/ui/views/website_settings/website_settings_popup_view.cc View 1 2 chunks +11 lines, -5 lines 0 comments Download
M chrome/browser/ui/website_settings/website_settings.h View 1 3 chunks +7 lines, -7 lines 0 comments Download
M chrome/browser/ui/website_settings/website_settings.cc View 1 2 3 4 5 6 7 8 9 3 chunks +9 lines, -4 lines 0 comments Download
M chrome/browser/ui/website_settings/website_settings_ui.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/website_settings/website_settings_unittest.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 35 (13 generated)
palmer
felt and/or markusheintz for chrome/browser/ui/website_settings and chrome/browser/ui/views/website_settings. rsesek for the Cocoa version coming soon in ...
4 years, 1 month ago (2015-08-25 00:02:14 UTC) #2
felt
Peter mentioned in https://code.google.com/p/chromium/issues/detail?id=490492#c2 that the WebsiteSettings shouldn't be keeping an InfoBarService*. The only place ...
4 years, 1 month ago (2015-08-25 18:20:09 UTC) #3
palmer
> Peter mentioned in https://code.google.com/p/chromium/issues/detail?id=490492#c2 > that the WebsiteSettings shouldn't be keeping an InfoBarService*. The ...
4 years, 1 month ago (2015-08-25 21:46:41 UTC) #4
felt
https://codereview.chromium.org/1317443002/diff/20001/chrome/browser/ui/website_settings/website_settings.cc File chrome/browser/ui/website_settings/website_settings.cc (right): https://codereview.chromium.org/1317443002/diff/20001/chrome/browser/ui/website_settings/website_settings.cc#newcode327 chrome/browser/ui/website_settings/website_settings.cc:327: InfoBarService::FromWebContents(web_contents_); does this fail gracefully if web_contents is a ...
4 years, 1 month ago (2015-08-25 22:30:43 UTC) #5
felt
4 years, 1 month ago (2015-08-25 22:30:44 UTC) #6
palmer
https://codereview.chromium.org/1317443002/diff/20001/chrome/browser/ui/website_settings/website_settings.cc File chrome/browser/ui/website_settings/website_settings.cc (right): https://codereview.chromium.org/1317443002/diff/20001/chrome/browser/ui/website_settings/website_settings.cc#newcode327 chrome/browser/ui/website_settings/website_settings.cc:327: InfoBarService::FromWebContents(web_contents_); > does this fail gracefully if web_contents is ...
4 years, 1 month ago (2015-08-25 22:40:50 UTC) #7
felt
lgtm
4 years, 1 month ago (2015-08-26 00:06:14 UTC) #8
palmer
OK, I think we're set. PTAL, rsesek. Thanks!
4 years, 1 month ago (2015-08-26 00:20:26 UTC) #9
markusheintz_
On 2015/08/26 00:20:26, palmer wrote: > OK, I think we're set. PTAL, rsesek. Thanks! LGTM ...
4 years, 1 month ago (2015-08-26 07:51:59 UTC) #10
Robert Sesek
The cocoa/ changes were dropped from the latest patch set?
4 years, 1 month ago (2015-08-26 15:27:13 UTC) #11
palmer
> The cocoa/ changes were dropped from the latest patch set? Wow, I really hate ...
4 years, 1 month ago (2015-08-26 19:24:12 UTC) #12
Robert Sesek
cocoa/ LGTM
4 years, 1 month ago (2015-08-26 19:25:06 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1317443002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1317443002/120001
4 years, 1 month ago (2015-08-26 21:16:02 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile_dbg/builds/52801)
4 years, 1 month ago (2015-08-26 21:31:21 UTC) #18
palmer
tedchoc: TBRing you because the Android change is trivial.
4 years, 1 month ago (2015-08-26 22:13:55 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1317443002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1317443002/140001
4 years, 1 month ago (2015-08-26 22:14:55 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/90480)
4 years, 1 month ago (2015-08-26 22:32:55 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1317443002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1317443002/160001
4 years, 1 month ago (2015-08-27 00:35:38 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile_dbg/builds/52936)
4 years, 1 month ago (2015-08-27 00:55:33 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1317443002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1317443002/180001
4 years, 1 month ago (2015-08-27 22:27:39 UTC) #33
commit-bot: I haz the power
Committed patchset #10 (id:180001)
4 years, 1 month ago (2015-08-27 23:15:14 UTC) #34
commit-bot: I haz the power
4 years, 1 month ago (2015-08-27 23:15:54 UTC) #35
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/f2cba0d13b3a6d76dedede66731e5ca253d3b2af
Cr-Commit-Position: refs/heads/master@{#346023}

Powered by Google App Engine
This is Rietveld 408576698