Iterate over listeners when sending process died notification.

Iterate over listeners when sending process died notification.

The refactor in http://crrev.com/207913 moved the RenderWidgetHost iterator outside of RenderProcessHost. Since it is now a vector of pointers, which is not updated when RenderWidgetHosts are removed, iterating through those causes us to access already freed objects.
We should be iterating over all listeners and notifying those. In addition, it will fix the case where we have non-RWH listeners, which also need to be cleaned up.

BUG=255524
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209772

[nasko, 2013-06-28 22:35:44]

Hey Charlie,
Can you review this change for me?
Thanks,
Nasko

[Charlie Reis, 2013-07-01 17:16:42]

This looks good, but the test is failing on Mac.  Any ideas?

And just to make sure I follow-- this will prevent the crash because the only
place we're deleting RenderWidgetHosts while iterating is in ProcessDied? 
Theoretically, it's still possible for other people to call
GetRenderWidgetHosts() and unintentionally delete elements of the list before
getting to them.  While it would be nice to have something a bit more robust, I
suppose we would consider that a bug at the call site.

A few nits below.

https://codereview.chromium.org/18167004/diff/1/chrome/browser/renderer_host/...
File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
(right):

https://codereview.chromium.org/18167004/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:358: 
nit: Remove extra blank line.

https://codereview.chromium.org/18167004/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:371:
LOG(ERROR) << "WD::RenderViewGone";
Remove log statements?

https://codereview.chromium.org/18167004/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:387: //
RenderProcessHost and they are invalidated, we remove them properly and don't
nit: and they are invalidated -> and invalidating them
(I think?)

https://codereview.chromium.org/18167004/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:388: //
access already freed objects. See http://crbug.com/255524
nit: End with a period.

[nasko, 2013-07-01 23:08:42]

Fixed the Mac failure. There is no WINDOW_CLOSED notification there :(. Changed
it to use BROWSER_CLOSED.

The issue of RWH vector being stale is there. We can file a separate bug for it,
as it is related, but not exactly the same problem.

https://codereview.chromium.org/18167004/diff/1/chrome/browser/renderer_host/...
File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
(right):

https://codereview.chromium.org/18167004/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:358: 
On 2013/07/01 17:16:42, creis wrote:
> nit: Remove extra blank line.

Done.

https://codereview.chromium.org/18167004/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:371:
LOG(ERROR) << "WD::RenderViewGone";
On 2013/07/01 17:16:42, creis wrote:
> Remove log statements?

Done.

https://codereview.chromium.org/18167004/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:387: //
RenderProcessHost and they are invalidated, we remove them properly and don't
On 2013/07/01 17:16:42, creis wrote:
> nit: and they are invalidated -> and invalidating them
> (I think?)

Done.

https://codereview.chromium.org/18167004/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:388: //
access already freed objects. See http://crbug.com/255524
On 2013/07/01 17:16:42, creis wrote:
> nit: End with a period.

Done.

[nasko, 2013-07-01 23:57:49]

Adding jochen@ for OWNERS review of chrome/.

[jochen (gone - plz use gerrit), 2013-07-02 07:56:46]

lgtm

https://codereview.chromium.org/18167004/diff/11001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
(right):

https://codereview.chromium.org/18167004/diff/11001/chrome/browser/renderer_h...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:382: };
nit. DISALLOW_COPY_AND_ASSIGN()

[commit-bot: I haz the power, 2013-07-02 14:38:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/18167004/23001

[nasko, 2013-07-02 14:57:29]

https://codereview.chromium.org/18167004/diff/11001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
(right):

https://codereview.chromium.org/18167004/diff/11001/chrome/browser/renderer_h...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:382: };
On 2013/07/02 07:56:46, jochen wrote:
> nit. DISALLOW_COPY_AND_ASSIGN()

Done.

[Charlie Reis, 2013-07-02 16:29:26]

LGTM.

[commit-bot: I haz the power, 2013-07-02 21:01:13]

Change committed as 209772