Remove throttling code from the Browser process and...

Remove throttling code from the Browser process and implement throttling in the Renderer.

The previous way of doing throttling was just calling CloseContents() on a window to reject it. But since the Browser is notified about a window opening asynchronously, by the time the CloseContents() sends a message back to the Renderer, a bunch more windows have been opened, leading to memory exhaustion. 

Instead, make all RenderViews created from a parent RenderView share a counter and start refusing to create RenderViews if too many RV have been created. Every RenderView (except for the first one) is assumed to be an unrequested popup, until notified by the Browser process by either a ViewMsg_DisassociateFromPopupCount message (this RenderView is a new top level page) or a ViewMsg_DisassociatePopup message (this RenderView is a requested popup and therefore shouldn't count against the count.)

BUG=3382, 2632
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=3568

[Elliot Glaysher, 2008-10-16 17:28:26]

Darin: I'm looking more for a high level design review from you. Do you believe
that this is a good approach?

Brett: I'm not entirely sure about whether this is good stylistically. Do we
have some sort of shared counter class that I missed?

[brettw, 2008-10-16 18:33:49]

I don't know of a better way to implement this, assuming this is the best design
:)

http://codereview.chromium.org/7388/diff/1/5
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/7388/diff/1/5#newcode55
Line 55: // RenderView and all RenderViews created from the first RV. We do this
to
This sentence confuses me. Can you provide some more background and describe
what's happening in some more detail?

http://codereview.chromium.org/7388/diff/1/5#newcode86
Line 86: const scoped_refptr<SharedRenderViewCounter>& counter,
I think it's more common to pass this type of thing as a raw pointer, and
document that the function will take a reference to it. This will simplify the
calling code in WebContents as well.

[Elliot Glaysher, 2008-10-16 19:53:51]

Brett, I've updated the patch set.

[brettw, 2008-10-16 19:57:25]

Implementation LGTM

[Elliot Glaysher, 2008-10-16 20:29:28]

A unit test has been added so hopefully this doesn't regress.

[darin (slow to review), 2008-10-16 20:38:13]

http://codereview.chromium.org/7388/diff/1/5
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/7388/diff/1/5#newcode54
Line 54: // Shared instnace to count the total number of RenderViews created by
a
s/instnace/instance/

http://codereview.chromium.org/7388/diff/1/5#newcode56
Line 56: // constrain a page to creating no more than kMaximumNumberOfPopups
popups.
what happens if a user unconstrains a popup?  does that then no longer count
against the limit?

http://codereview.chromium.org/7388/diff/1/5#newcode64
Line 64: int view_count;
It seems like it would be handy to define in base/ref_counted.h a class like
RefCountedData<T>:

template <class T>
class RefCountedData : public RefCounted< RefCountedData<T> > {
 public:
  T value;
};

[Elliot Glaysher, 2008-10-16 20:51:57]

http://codereview.chromium.org/7388/diff/1/5
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/7388/diff/1/5#newcode54
Line 54: // Shared instnace to count the total number of RenderViews created by
a
On 2008/10/16 20:38:13, darin wrote:
> s/instnace/instance/

Comment was modified in the interim.

http://codereview.chromium.org/7388/diff/1/5#newcode56
Line 56: // constrain a page to creating no more than kMaximumNumberOfPopups
popups.
On 2008/10/16 20:38:13, darin wrote:
> what happens if a user unconstrains a popup?  does that then no longer count
> against the limit?

No; from the view point of the Renderer process, there's no difference between
constrained and unconstrained popups. It' s simply a UI detail. An unconstrained
popup will still count against this limit, as will all popups spawned.

Since the original problem was timing issues between the Renderer and Browser
process, I am hesitant to add some sort of message to decrement this counter. I
can look into it if you think it's an issue.

http://codereview.chromium.org/7388/diff/1/5#newcode64
Line 64: int view_count;
Sure. I'll move this around while waiting for your response to the above issue.

[Elliot Glaysher, 2008-10-16 23:39:05]

Please take another look; I've implemented the plan in that email response I
sent you.

[Elliot Glaysher, 2008-10-17 18:45:47]

Updated to only use one message.

[darin (slow to review), 2008-10-17 19:20:27]

LGTM modulo items below

http://codereview.chromium.org/7388/diff/260/263
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/7388/diff/260/263#newcode97
Line 97: static const int kMaximumNumberOfPopups = 25;
nit: might want to revise the wording here to be specific about the fact that we
are only limiting unwanted or suppressed popups.

http://codereview.chromium.org/7388/diff/260/263#newcode2587
Line 2587: if (decrement_shared_popup_at_destruction_)
elliot tells me that this line should be deleted ;)