Fix crash caused by middle clicking the privacy link in SSL interstitials

Fix crash caused by middle clicking the privacy link in SSL interstitials

Middle clicking the privacy link initiates a new navigation instead of an
in-page navigation which causes a crash. This CL changes the link to an
in-page link to prevent that.

BUG=682596
Review-Url: https://codereview.chromium.org/2673963004
Cr-Commit-Position: refs/heads/master@{#448753}
Committed: https://chromium.googlesource.com/chromium/src/+/5ecfaa1461f7ec8faf7bae9456cf2888a7608add

[meacer, 2017-02-06 20:21:13]

meacer@chromium.org changed reviewers:
+ estark@chromium.org

[meacer, 2017-02-06 20:21:14]

estark: PTAL? Also, do you know if we test interstitial UI? I searched around
but all the tests I found were simulating proceed or not proceed actions without
actually clicking the UI.

[estark, 2017-02-07 02:10:42]

lgtm to fix the crash, but two things:

1.) It looks like some ssl_browser_tests simulate clicks by dispatching a click
event ([1], [2]). Could you test this fix by executing a script in the
interstitial page that triggers a MouseEvent with the button set to 1 (wheel or
middle button)? http://www.w3schools.com/jsref/event_button.asp

2.) I don't really understand why this causes a crash and it seems like it could
regress easily, e.g. if someone comes along and adds a different link to the
interstitial. You could add a comment to controller_client.cc to explain why
it's important that the link is an in-page navigation. Or I don't suppose
there's some easy way to not crash when there's a full-blown navigation?

[1]
https://cs.chromium.org/chromium/src/chrome/test/data/ssl/top_frame.html?type...

[2]
https://cs.chromium.org/chromium/src/chrome/browser/ssl/ssl_browser_tests.cc?...

[meacer, 2017-02-07 02:25:19]

On 2017/02/07 02:10:42, estark wrote:
> lgtm to fix the crash, but two things:
> 
> 1.) It looks like some ssl_browser_tests simulate clicks by dispatching a
click
> event ([1], [2]). Could you test this fix by executing a script in the
> interstitial page that triggers a MouseEvent with the button set to 1 (wheel
or
> middle button)? http://www.w3schools.com/jsref/event_button.asp

Sorry, I should have given more info. I tried doing that, but it seems we can't
access the interstitial's DOM from ExecuteScriptAndExtract* methods so
simulating a mouse event didn't work. 

> 
> 2.) I don't really understand why this causes a crash and it seems like it
could
> regress easily, e.g. if someone comes along and adds a different link to the
> interstitial. You could add a comment to controller_client.cc to explain why
> it's important that the link is an in-page navigation. Or I don't suppose
> there's some easy way to not crash when there's a full-blown navigation?

The crash is caused when a new navigation on the interstitial tries to create
and attach another interstitial. Now that I said that, I think a better approach
could be to detach the interstitial and then try navigation before we try to
load the page? I'll take another look tomorrow.

> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/test/data/ssl/top_frame.html?type...
> 
> [2]
>
https://cs.chromium.org/chromium/src/chrome/browser/ssl/ssl_browser_tests.cc?...

[estark, 2017-02-07 02:33:43]

On 2017/02/07 02:25:19, Mustafa Emre Acer wrote:
> On 2017/02/07 02:10:42, estark wrote:
> > lgtm to fix the crash, but two things:
> > 
> > 1.) It looks like some ssl_browser_tests simulate clicks by dispatching a
> click
> > event ([1], [2]). Could you test this fix by executing a script in the
> > interstitial page that triggers a MouseEvent with the button set to 1 (wheel
> or
> > middle button)? http://www.w3schools.com/jsref/event_button.asp
> 
> Sorry, I should have given more info. I tried doing that, but it seems we
can't
> access the interstitial's DOM from ExecuteScriptAndExtract* methods so
> simulating a mouse event didn't work. 
> 
> > 
> > 2.) I don't really understand why this causes a crash and it seems like it
> could
> > regress easily, e.g. if someone comes along and adds a different link to the
> > interstitial. You could add a comment to controller_client.cc to explain why
> > it's important that the link is an in-page navigation. Or I don't suppose
> > there's some easy way to not crash when there's a full-blown navigation?
> 
> The crash is caused when a new navigation on the interstitial tries to create
> and attach another interstitial. Now that I said that, I think a better
approach
> could be to detach the interstitial and then try navigation before we try to
> load the page? I'll take another look tomorrow.
> 
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/chrome/test/data/ssl/top_frame.html?type...
> > 
> > [2]
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ssl/ssl_browser_tests.cc?...

Ok. Still lgtm if you want to land this as a band-aid to fix the crash while you
investigate more.

[meacer, 2017-02-07 21:36:08]

On 2017/02/07 02:33:43, estark wrote:
> On 2017/02/07 02:25:19, Mustafa Emre Acer wrote:
> > On 2017/02/07 02:10:42, estark wrote:
> > > lgtm to fix the crash, but two things:
> > > 
> > > 1.) It looks like some ssl_browser_tests simulate clicks by dispatching a
> > click
> > > event ([1], [2]). Could you test this fix by executing a script in the
> > > interstitial page that triggers a MouseEvent with the button set to 1
(wheel
> > or
> > > middle button)? http://www.w3schools.com/jsref/event_button.asp
> > 
> > Sorry, I should have given more info. I tried doing that, but it seems we
> can't
> > access the interstitial's DOM from ExecuteScriptAndExtract* methods so
> > simulating a mouse event didn't work. 
> > 
> > > 
> > > 2.) I don't really understand why this causes a crash and it seems like it
> > could
> > > regress easily, e.g. if someone comes along and adds a different link to
the
> > > interstitial. You could add a comment to controller_client.cc to explain
why
> > > it's important that the link is an in-page navigation. Or I don't suppose
> > > there's some easy way to not crash when there's a full-blown navigation?
> > 
> > The crash is caused when a new navigation on the interstitial tries to
create
> > and attach another interstitial. Now that I said that, I think a better
> approach
> > could be to detach the interstitial and then try navigation before we try to
> > load the page? I'll take another look tomorrow.
> > 
> > > 
> > > [1]
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/test/data/ssl/top_frame.html?type...
> > > 
> > > [2]
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ssl/ssl_browser_tests.cc?...
> 
> Ok. Still lgtm if you want to land this as a band-aid to fix the crash while
you
> investigate more.

Okay, I'll land this and ask for a merge and then investigate further.

[meacer, 2017-02-07 21:36:12]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2017-02-07 21:37:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 22:41:54]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486503372672870, "parent_rev":
"af5bd1316ab3fd3054d08471c0a25b4b7c66cf9c", "commit_rev":
"5ecfaa1461f7ec8faf7bae9456cf2888a7608add"}

[commit-bot: I haz the power, 2017-02-07 22:42:27]

Description was changed from

==========
Fix crash caused by middle clicking the privacy link in SSL interstitials

Middle clicking the privacy link initiates a new navigation instead of an
in-page navigation which causes a crash. This CL changes the link to an
in-page link to prevent that.

BUG=682596
==========

to

==========
Fix crash caused by middle clicking the privacy link in SSL interstitials

Middle clicking the privacy link initiates a new navigation instead of an
in-page navigation which causes a crash. This CL changes the link to an
in-page link to prevent that.

BUG=682596

Review-Url: https://codereview.chromium.org/2673963004
Cr-Commit-Position: refs/heads/master@{#448753}
Committed:
https://chromium.googlesource.com/chromium/src/+/5ecfaa1461f7ec8faf7bae9456cf...
==========

[commit-bot: I haz the power, 2017-02-07 22:42:28]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/5ecfaa1461f7ec8faf7bae9456cf...