Fix crasher with SSL interstitial

The URL in the bug triggers an SSL interstitial for a redirect that redirects to a bad SSL page.

This happens because:
- when proceeding on an interstitial, we wait for the navigation to commit before hiding the current interstitial
- when showing an interstitial, if an interstitial is already showing we call DontProceed on it

In the case of this bug, the interstitial has already been approved when the showing of the 2nd interstitial calls DontProceed on it, causing bad things to happen.

The fix is easy: we shouldn't call DontProceed on an interstitial already "Proceeded".


BUG=9286
TEST=See bug.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12727

[Charlie Reis, 2009-03-27 22:47:04]

LGTM, unless you want to add an accessor method.  Nice test.

http://codereview.chromium.org/56019/diff/1/2
File chrome/browser/tab_contents/interstitial_page.cc (right):

http://codereview.chromium.org/56019/diff/1/2#newcode135
Line 135: // If an interstitial is already showing, close it before showing the
new one.
Perhaps add to this comment?  (Something like "Be careful not to take an action
on the old interstitial more than once.")

http://codereview.chromium.org/56019/diff/1/2#newcode141
Line 141: }
Yep, seems like the right thing to do, since it's important that
DontProceed/Proceed gets called exactly once.  Maybe it's worth giving
action_taken_ an accessor method, if other objects need to check whether it's
safe to call DontProceed/Proceed or not.

[jcampan, 2009-03-27 23:06:10]

http://codereview.chromium.org/56019/diff/1/2
File chrome/browser/tab_contents/interstitial_page.cc (right):

http://codereview.chromium.org/56019/diff/1/2#newcode135
Line 135: // If an interstitial is already showing, close it before showing the
new one.
On 2009/03/27 22:47:04, creis wrote:
> Perhaps add to this comment?  (Something like "Be careful not to take an
action
> on the old interstitial more than once.")

Done.

http://codereview.chromium.org/56019/diff/1/2#newcode141
Line 141: }
On 2009/03/27 22:47:04, creis wrote:
> Yep, seems like the right thing to do, since it's important that
> DontProceed/Proceed gets called exactly once.  Maybe it's worth giving
> action_taken_ an accessor method, if other objects need to check whether it's
> safe to call DontProceed/Proceed or not.

I don't think any other object (other than derived classes) need access to that
yet, but I'll add an accessor anyway as it looks cleaner.

[Charlie Reis, 2009-03-27 23:13:27]

Great.  LGTM.