Do not allow GuestViewManager to (re)use an instance ID that was already removed.

Do not allow GuestViewManager to (re)use an instance ID that was already removed.

Guests can exhibit un-intended behavior otherwise.

BUG=None
Test=Internal

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

[Fady Samuel, 2014-05-22 15:02:12]

https://codereview.chromium.org/298913003/diff/1/chrome/browser/guest_view/gu...
File chrome/browser/guest_view/guest_view_manager.cc (left):

https://codereview.chromium.org/298913003/diff/1/chrome/browser/guest_view/gu...
chrome/browser/guest_view/guest_view_manager.cc:194:
guest_web_contents_by_instance_id_.end());
Make this a CHECK instead of a DCHECK because it has security implications.

https://codereview.chromium.org/298913003/diff/1/chrome/browser/guest_view/gu...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/298913003/diff/1/chrome/browser/guest_view/gu...
chrome/browser/guest_view/guest_view_manager.cc:209: if (guest_instance_id ==
last_instance_id_removed_ + 1) {
This code is hard to read. Could you please comment it. It's not clear to me
what last_instance_id_removed_ actually represents. I worry that this code might
have a bug in it if I don't understand the intent.

[lazyboy, 2014-05-22 15:59:30]

https://codereview.chromium.org/298913003/diff/1/chrome/browser/guest_view/gu...
File chrome/browser/guest_view/guest_view_manager.cc (left):

https://codereview.chromium.org/298913003/diff/1/chrome/browser/guest_view/gu...
chrome/browser/guest_view/guest_view_manager.cc:194:
guest_web_contents_by_instance_id_.end());
On 2014/05/22 15:02:13, Fady Samuel wrote:
> Make this a CHECK instead of a DCHECK because it has security implications.

Done.

https://codereview.chromium.org/298913003/diff/1/chrome/browser/guest_view/gu...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/298913003/diff/1/chrome/browser/guest_view/gu...
chrome/browser/guest_view/guest_view_manager.cc:209: if (guest_instance_id ==
last_instance_id_removed_ + 1) {
On 2014/05/22 15:02:13, Fady Samuel wrote:
> This code is hard to read. Could you please comment it. It's not clear to me
> what last_instance_id_removed_ actually represents.
There's a note in last_instance_id_removed_ in .h file, I've added more comments
here.

I worry that this code might
> have a bug in it if I don't understand the intent.

Yes, I've added unit test for this purpose.

https://codereview.chromium.org/298913003/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/298913003/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.cc:55:
DCHECK(GuestViewManager::FromBrowserContext(browser_context_)
Q: Do we need a CHECK here too?

[Fady Samuel, 2014-05-22 17:41:53]

https://codereview.chromium.org/298913003/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/298913003/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.cc:55:
DCHECK(GuestViewManager::FromBrowserContext(browser_context_)
On 2014/05/22 15:59:31, lazyboy wrote:
> Q: Do we need a CHECK here too?

If we have a CHECK inside AddGuest then why do we need a return value?

https://codereview.chromium.org/298913003/diff/40001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/298913003/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:212: // Following code compacts
the set by incrementing
The following code...

https://codereview.chromium.org/298913003/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:222:
++last_instance_id_removed_;
This line really confuses me. Let's chat about this algorithm.

[lazyboy, 2014-05-22 18:34:44]

https://codereview.chromium.org/298913003/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/298913003/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.cc:55:
DCHECK(GuestViewManager::FromBrowserContext(browser_context_)
On 2014/05/22 17:41:53, Fady Samuel wrote:
> On 2014/05/22 15:59:31, lazyboy wrote:
> > Q: Do we need a CHECK here too?
> 
> If we have a CHECK inside AddGuest then why do we need a return value?

The return value was for test, I've moved the logic to a separate function and
testing that instead.
Changed back to return void.

https://codereview.chromium.org/298913003/diff/40001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/298913003/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:212: // Following code compacts
the set by incrementing
On 2014/05/22 17:41:53, Fady Samuel wrote:
> The following code...

Done.

https://codereview.chromium.org/298913003/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:222:
++last_instance_id_removed_;
On 2014/05/22 17:41:53, Fady Samuel wrote:
> This line really confuses me. Let's chat about this algorithm.

Hmmm, seems I uploaded the half of it, the check in AddGuest wasn't uploaded.

Sure let's chat.

[Fady Samuel, 2014-05-22 19:10:59]

lgtm with nit.

https://codereview.chromium.org/298913003/diff/60001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/298913003/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:219: if (instance_id !=
last_instance_id_removed_ + 1)
Add a DCHECK and comment how instance_id > last_instance_id removed.

[lazyboy, 2014-05-22 21:14:25]

https://chromiumcodereview.appspot.com/298913003/diff/60001/chrome/browser/gu...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://chromiumcodereview.appspot.com/298913003/diff/60001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_manager.cc:219: if (instance_id !=
last_instance_id_removed_ + 1)
On 2014/05/22 19:11:00, Fady Samuel wrote:
> Add a DCHECK and comment how instance_id > last_instance_id removed.

Done.

[lazyboy, 2014-05-22 21:14:44]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-05-22 21:16:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lazyboy@chromium.org/298913003/80001

[commit-bot: I haz the power, 2014-05-23 09:41:25]

Change committed as 272454