Don't crash if DontProceed is chosen on the SSL interstitial page after Proceed is chosen.

Don't crash if DontProceed is chosen on the SSL interstitial page after Proceed is chosen.

A common place where this happens is when the user navigates to an https:// URL with a
bad certificate, and the content is a download. The user chooses to proceed, but the SSL interstitial
is never hidden because there is never a page to be committed. The user presses the back button, which
does a DontProceed, and this would crash. Note that "back to safety" and "proceed" within the SSL page
are filtered out after an action is taken, so these are no-ops currently.

This sequence will still treat the SSL certificate as accepted for future navigations.

BUG=90864, 93605
TEST=Start run_testserver --https-cert=expired. Navigate to a normal page like www.google.com. Navigate to the testserver at https://localhost:<port>/files/downloads/dangerous/dangerous.exe. See the interstitial and click proceed. Press the back button on the browser, and you should see the normal page again, instead of a crash.


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

[cbentzel, 2011-09-02 18:21:00]

wtc: Does the general SSL policy make sense?
jam: You touched this region last.

I'm working on an SSL browser test for this as well, although those seem to be
in a fairly bad state right now. I'd rather get this in soon and potentially
merge with M14.

[cbentzel, 2011-09-02 18:23:03]

Also - I think there's some other changes we might want to make here
[potentially hiding the interstitial right after "proceed" is chosen, hiding the
interstitial after discovering that the main resource is a download, letting
proceed/back to safety buttons work after one is clicked, etc]. but I just
wanted to get a small isolated fix in for now.

[cbentzel, 2011-09-02 19:41:16]

The newest patch adds a browser test. This test crashed with the old code, works
correctly with the new.

[cbentzel, 2011-09-02 20:41:19]

+jcivelli: Feel free to pass on this review, but you wrote most of the SSL
Interstitial code.

[wtc, 2011-09-02 20:43:15]

LGTM, but I have a question:

You said:
> A common place where this happens is when the user
> navigates to an https:// URL with a bad certificate, and
> the content is a download. The user chooses to proceed, but
> the SSL interstitial is never hidden because there is never
> a page to be committed.

I wonder if this is the real problem.  I expect the
SSL interstitial to go away after I click "Proceed".  If
it is a download, I expect the tab to show the original
page (where I clicked a link that resulted in the download).

So, perhaps this behavior is what should be fixed.

If this behavior is desired, then your fix should be
correct.  I am not familiar with this code though.

http://codereview.chromium.org/7826036/diff/2001/chrome/browser/ssl/ssl_block...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

http://codereview.chromium.org/7826036/diff/2001/chrome/browser/ssl/ssl_block...
chrome/browser/ssl/ssl_blocking_page.cc:161: DCHECK(callback_);
Does this DCHECK need the same treatment?

[cbentzel, 2011-09-02 20:50:17]

On Fri, Sep 2, 2011 at 4:43 PM,  <wtc@chromium.org> wrote:
> LGTM, but I have a question:
>
> You said:
>>
>> A common place where this happens is when the user
>> navigates to an https:// URL with a bad certificate, and
>> the content is a download. The user chooses to proceed, but
>> the SSL interstitial is never hidden because there is never
>> a page to be committed.
>
> I wonder if this is the real problem.  I expect the
> SSL interstitial to go away after I click "Proceed".  If
> it is a download, I expect the tab to show the original
> page (where I clicked a link that resulted in the download).
>

I agree that this may need to be the real fix, and wrote a separate CL
to do that. There are some complications though. Right now it waits
until the navigated-to-page commits [essentially gets to onload
portion] before hiding the interstitial - this way you don't see the
old page for a little bit before the next page comes in.

In the download case, there is never a page load commit, so this stays
up. Once we detect a download we should probably close the
interstitial - there's a separate bug for that.

I'd like to get the crash fix in for M15, and possibly backported to M14.

It looks like the SSL browser interstitial area needs an owner to
clean it up since there's been some bitrot. Hopefully the suggested
cleanups could be part of it.

> So, perhaps this behavior is what should be fixed.
>
> If this behavior is desired, then your fix should be
> correct.  I am not familiar with this code though.
>
>
>
http://codereview.chromium.org/7826036/diff/2001/chrome/browser/ssl/ssl_block...
> File chrome/browser/ssl/ssl_blocking_page.cc (right):
>
>
http://codereview.chromium.org/7826036/diff/2001/chrome/browser/ssl/ssl_block...
> chrome/browser/ssl/ssl_blocking_page.cc:161: DCHECK(callback_);
> Does this DCHECK need the same treatment?

No. If you click "Go back to safety" or do the dont proceed, the SSL
interstitial is immediately deleted, so this path is not possible.
>
> http://codereview.chromium.org/7826036/
>

[jam, 2011-09-03 00:00:36]

On 2011/09/02 18:21:00, cbentzel wrote:
> wtc: Does the general SSL policy make sense?
> jam: You touched this region last.

I'm not familiar with this code, I may have just been refactoring it.

> 
> I'm working on an SSL browser test for this as well, although those seem to be
> in a fairly bad state right now. I'd rather get this in soon and potentially
> merge with M14.

[cbentzel, 2011-09-03 00:27:17]

-jam+jcivelli

[commit-bot: I haz the power, 2011-09-03 05:03:28]

Change committed as 99542