Block javascript: document navigations during page dismissal events.

Block javascript: document navigations during page dismissal events.

This basically reflects the logic from FrameLoader::startLoad. Before this patch, javascript: document navigations could be performed during page dismissal events. This could be problematic, especially that dismissal events prevent loaders from being stopped or detached.

This patch adds a bail-out condition to FrameLoader::replaceDocumentWhileExecutingJavaScriptURL.

BUG=556724
Committed: https://crrev.com/e6fabc430b0f6a52dacc47860d66da09c79c310b
Cr-Commit-Position: refs/heads/master@{#360242}

[dcheng, 2015-11-17 22:15:25]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org, japhet@chromium.org

[dcheng, 2015-11-17 22:40:24]

The fix seems reasonable, but this area appears to be wildly underspecified. I
tried to see how IE/Firefox handle this.

IE: I'm not able to get the unload handler in this test case to trigger at all.
I tried navigating to a srcdoc and javascript:'moo' as well. Maybe I'm doing
something wrong?

Firefox: the navigation to the javascript: URL is scheduled, but the script
never executes (and thus, the result is never loaded).
    i.src = "javascript:'moo'";  // unload handler runs *twice*.
    i.src = 'data:text/html,foo';  // unload handler runs once.
    i.srcdoc = 'Hello world';  // unload handler runs once.

Chrome:
    i.src = "javascript:'moo'";  // unload handler doesn't run at all (?!)
    i.src = 'data:text/html,foo';  // unload handler runs, JS url is executed
and navigated to
    i.srcdoc = 'hello world';  // unload handler runs once, JS url is scheduled
for navigation but never executes or navigates.

Nate, any idea why we have three different behaviors in Blink here? I'm guessing
this is partially related to
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....

[Nate Chapin, 2015-11-17 22:53:35]

On 2015/11/17 22:40:24, dcheng wrote:
> The fix seems reasonable, but this area appears to be wildly underspecified. I
> tried to see how IE/Firefox handle this.
> 
> IE: I'm not able to get the unload handler in this test case to trigger at
all.
> I tried navigating to a srcdoc and javascript:'moo' as well. Maybe I'm doing
> something wrong?
> 
> Firefox: the navigation to the javascript: URL is scheduled, but the script
> never executes (and thus, the result is never loaded).
>     i.src = "javascript:'moo'";  // unload handler runs *twice*.
>     i.src = 'data:text/html,foo';  // unload handler runs once.
>     i.srcdoc = 'Hello world';  // unload handler runs once.
> 
> Chrome:
>     i.src = "javascript:'moo'";  // unload handler doesn't run at all (?!)
>     i.src = 'data:text/html,foo';  // unload handler runs, JS url is executed
> and navigated to
>     i.srcdoc = 'hello world';  // unload handler runs once, JS url is
scheduled
> for navigation but never executes or navigates.
> 
> Nate, any idea why we have three different behaviors in Blink here? I'm
guessing
> this is partially related to
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....

The javascript url case is easy to explain. We check whether the navigation is a
javascript: url really early in the process. Early enough that it short circuits
all of FrameLoader::startLoad, including the firing of unload events. I'm pretty
sure there's no good reason for that.

I'm unclear why there's a different between a data url and a srcdoc url.

[dcheng, 2015-11-17 23:09:17]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-17 23:11:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1451123002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1451123002/1

[dcheng, 2015-11-17 23:22:33]

lgtm

Marius, if you're around, can you upload a new version of the patch but add
yourself to src/AUTHORS? Otherwise, I'll patch this in locally and land on your
behalf.

[Mariusz Mlynski, 2015-11-17 23:25:59]

On 2015/11/17 22:40:24, dcheng wrote:
> IE: I'm not able to get the unload handler in this test case to trigger at
all.
> I tried navigating to a srcdoc and javascript:'moo' as well. Maybe I'm doing
> something wrong?

It looks like IE doesn't fire unload events for subframe navigations before the
main document is loaded. It works if |i.src = 'data:text/html,test'| is wrapped
in setTimeout.

No wonder about the discrepancies between browsers. I imagine that
javascript:"string" navigations are not that common, and loading them from an
unload event is definitely an edge case :-) The current state of matters is that
Chrome blocks all navigations that occur during page dismissal events, except
for the javascript:"" case. If you don't get too many complaints about the
onunload behavior then I guess it's safe to go ahead with this.

[Mariusz Mlynski, 2015-11-17 23:27:06]

On 2015/11/17 23:22:33, dcheng wrote:
> lgtm
> 
> Marius, if you're around, can you upload a new version of the patch but add
> yourself to src/AUTHORS? Otherwise, I'll patch this in locally and land on
your
> behalf.

Sure, will do so in a few minutes.

[commit-bot: I haz the power, 2015-11-17 23:39:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-17 23:39:29]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL)

[Mariusz Mlynski, 2015-11-17 23:55:57]

The CQ bit was checked by marius.mlynski@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-17 23:57:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1451123002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1451123002/20001

[dcheng, 2015-11-17 23:58:42]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2015-11-17 23:58:44]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-11-17 23:58:44]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1451123002/#ps20001
(title: "added e-mail to AUTHORS")

[commit-bot: I haz the power, 2015-11-18 00:00:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1451123002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1451123002/20001

[commit-bot: I haz the power, 2015-11-18 01:01:07]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-18 01:01:56]

Patchset 2 (id:??) landed as
https://crrev.com/e6fabc430b0f6a52dacc47860d66da09c79c310b
Cr-Commit-Position: refs/heads/master@{#360242}