Fix UAF in Origin Info Bubble and permission settings UI.

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}

[palmer, 2015-08-25 00:02:13]

palmer@chromium.org changed reviewers:
+ felt@chromium.org, markusheintz@chromium.org, rsesek@chromium.org

[palmer, 2015-08-25 00:02:14]

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 another patchset (but you can
probably see what it's going to be like based on the Views version).

[felt, 2015-08-25 18:20:09]

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
where infobar_service_ is used is in WebsiteSettings::OnUIClosing. So perhaps
WebsiteSettings should take a WebContents* instead of an InfoBarService*?

https://codereview.chromium.org/1317443002/diff/1/chrome/browser/ui/views/web...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/1317443002/diff/1/chrome/browser/ui/views/web...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:390:
void WebsiteSettingsPopupView::RenderFrameDeleted(
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 during a nav)? (My guess is
that it does -- if I'm right, can you add that to the description?)

https://codereview.chromium.org/1317443002/diff/1/chrome/browser/ui/website_s...
File chrome/browser/ui/website_settings/website_settings.h (right):

https://codereview.chromium.org/1317443002/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/website_settings.h:139: void
DoNotShowInfoBar() { show_info_bar_ = false; }
nit: should this be a setter for show_info_bar_ instead?

[palmer, 2015-08-25 21:46:41]

> 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
> where infobar_service_ is used is in WebsiteSettings::OnUIClosing. So perhaps
> WebsiteSettings should take a WebContents* instead of an InfoBarService*?

Ah, yes. Thanks. Trying it out, it seems to obviate much of the rest of this CL
(!). So, simpler fix coming in the next patchset. I hope.

> 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 during a nav)? (My guess
is
> that it does -- if I'm right, can you add that to the description?)

Yes, and done.

> DoNotShowInfoBar() { show_info_bar_ = false; }
> nit: should this be a setter for show_info_bar_ instead?

Yes, if we end up needing to keep it. Not sure we will.

[felt, 2015-08-25 22:30:43]

https://codereview.chromium.org/1317443002/diff/20001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/1317443002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:327:
InfoBarService::FromWebContents(web_contents_);
does this fail gracefully if web_contents is a nullptr? or do you need
if(show_info_bar_ && web_contents) ?

[palmer, 2015-08-25 22:40:50]

https://codereview.chromium.org/1317443002/diff/20001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/1317443002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:327:
InfoBarService::FromWebContents(web_contents_);
> does this fail gracefully if web_contents is a nullptr? or do you need
> if(show_info_bar_ && web_contents) ?

We do need the check; it would deref the nullptr. Fixed.

[felt, 2015-08-26 00:06:14]

lgtm

[palmer, 2015-08-26 00:20:26]

OK, I think we're set. PTAL, rsesek. Thanks!

[markusheintz_, 2015-08-26 07:51:59]

On 2015/08/26 00:20:26, palmer wrote:
> OK, I think we're set. PTAL, rsesek. Thanks!

LGTM thanks!

[Robert Sesek, 2015-08-26 15:27:13]

The cocoa/ changes were dropped from the latest patch set?

[palmer, 2015-08-26 19:24:12]

> The cocoa/ changes were dropped from the latest patch set?

Wow, I really hate trying to coordinate 1 patch on 2 different machines.
Restored the Cocoa stuff now.

[Robert Sesek, 2015-08-26 19:25:06]

cocoa/ LGTM

[palmer, 2015-08-26 21:15:18]

The CQ bit was checked by palmer@chromium.org

[palmer, 2015-08-26 21:15:18]

The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org,
markusheintz@chromium.org
Link to the patchset: https://codereview.chromium.org/1317443002/#ps120001
(title: "Restore the Cocoa changes.")

[commit-bot: I haz the power, 2015-08-26 21:16:02]

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

[commit-bot: I haz the power, 2015-08-26 21:31:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-26 21:31:21]

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...)

[palmer, 2015-08-26 22:13:55]

palmer@chromium.org changed reviewers:
+ tedchoc@chromium.org

[palmer, 2015-08-26 22:13:55]

tedchoc: TBRing you because the Android change is trivial.

[palmer, 2015-08-26 22:14:01]

The CQ bit was checked by palmer@chromium.org

[palmer, 2015-08-26 22:14:01]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
markusheintz@chromium.org, felt@chromium.org
Link to the patchset: https://codereview.chromium.org/1317443002/#ps140001
(title: "Oops, Android has a call-site that needs to be changed too. Fixed.")

[commit-bot: I haz the power, 2015-08-26 22:14:55]

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

[commit-bot: I haz the power, 2015-08-26 22:32:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-26 22:32:55]

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_comp...)

[palmer, 2015-08-27 00:34:33]

The CQ bit was checked by palmer@chromium.org

[palmer, 2015-08-27 00:34:34]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
markusheintz@chromium.org, felt@chromium.org
Link to the patchset: https://codereview.chromium.org/1317443002/#ps160001
(title: "Fix unit_tests build on Mac.")

[commit-bot: I haz the power, 2015-08-27 00:35:38]

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

[commit-bot: I haz the power, 2015-08-27 00:55:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-27 00:55:33]

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...)

[palmer, 2015-08-27 22:26:57]

The CQ bit was checked by palmer@chromium.org

[palmer, 2015-08-27 22:26:57]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
markusheintz@chromium.org, felt@chromium.org
Link to the patchset: https://codereview.chromium.org/1317443002/#ps180001
(title: "Fix another Andorid call site. Checked all call sites.")

[commit-bot: I haz the power, 2015-08-27 22:27:39]

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

[commit-bot: I haz the power, 2015-08-27 23:15:14]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-08-27 23:15:54]

Patchset 10 (id:??) landed as
https://crrev.com/f2cba0d13b3a6d76dedede66731e5ca253d3b2af
Cr-Commit-Position: refs/heads/master@{#346023}