Don't reset LoadEventProgress if frame unload has already started.

Disable sub frame loads more reliably in frame detach.

Previously, guarding against attaching a new frame in frame detach
was done by inspecting LoadEventProgress. There are certain
operations, such as document.open(), that can cause LoadEventProgress
to never advance, causing the guard to get skipped.

BUG=577105
Committed: https://crrev.com/a4bcdcb1f8df4f7427e208201501a9d8e41e386b
Cr-Commit-Position: refs/heads/master@{#373581}

[dcheng, 2016-02-03 03:06:31]

Description was changed from

==========
Fix

BUG=577105
==========

to

==========
Don't reset LoadEventProgress if frame unload has already started.

LoadEventProgress is used to guard against creation of additional
subframes during frame detach. Calling document.open() without ever
calling document.close() results in a situation where
LoadEventProgress is never advanced during frame detach, causing
the logic to guard against subframe loads in detach to be skipped.

BUG=577105
==========

[dcheng, 2016-02-03 03:06:31]

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

[dcheng, 2016-02-03 03:13:29]

https://codereview.chromium.org/1659013003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/frames/open-then-unload.html (right):

https://codereview.chromium.org/1659013003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/frames/open-then-unload.html:10:
window[0].document.open();
At this point, LoadEventProgress gets reset to LoadEventNotRun without my patch.

https://codereview.chromium.org/1659013003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/frames/open-then-unload.html:11:
window[0].document.appendChild(document.createElement('iframe'));
Right now, our tree looks like this:

[Main frame]
     |
  [Child A] (m_loadEventProgress == LoadEventNotRun)

Since this child frame won't run unload events, we nest another frame under
Child A:

[Main frame]
     |
  [Child A] (m_loadEventProgress == LoadEventNotRun)
     |
  [Child B] (m_loadEventProgress == LoadEventCompleted)

https://codereview.chromium.org/1659013003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/frames/open-then-unload.html:13:
window[0].document.documentElement.appendChild(document.createElement('iframe'));
Since Child B's load event progress is normal, we can use an unload handler to
try to attach a frame to [Child A] as it is navigating.

https://codereview.chromium.org/1659013003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/frames/open-then-unload.html:14:
shouldBe('1', 'window[0].length');
If the iframe load is blocked, then window[0].length should remain at 1.

https://codereview.chromium.org/1659013003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1659013003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2776: m_loadEventProgress =
UnloadEventHandled;
This was the original fix proposed in the bug. I believe it is insufficient.

1. Page starts XHR that never completes.
2. Navigation starts.
3. Provisional navigation prepares to commit.
4. Unload handlers run. At this point, m_loadEventProgres == UnloadEventHandled.
5. Now XHR abort handlers run.
6. XHR abort handler calls document.open(), which resets m_loadEventProgress to
LoadEventNotRun.
7. Now, XHR abort handler can attach a child frame during frame detach again.

[dcheng, 2016-02-03 03:13:35]

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

[commit-bot: I haz the power, 2016-02-03 03:13:54]

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

[commit-bot: I haz the power, 2016-02-03 04:55:28]

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

[commit-bot: I haz the power, 2016-02-03 04:55:29]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-02-03 08:41:23]

Description was changed from

==========
Don't reset LoadEventProgress if frame unload has already started.

LoadEventProgress is used to guard against creation of additional
subframes during frame detach. Calling document.open() without ever
calling document.close() results in a situation where
LoadEventProgress is never advanced during frame detach, causing
the logic to guard against subframe loads in detach to be skipped.

BUG=577105
==========

to

==========
Disable sub frame loads more reliably in frame detach.

Previously, guarding against attaching a new frame in frame detach
was done by inspecting LoadEventProgress. There are certain
operations, such as document.open(), that can cause LoadEventProgress
to never advance, causing the guard to get skipped.

BUG=577105
==========

[dcheng, 2016-02-03 08:42:17]

I found a better (and more reliable way) to do this. We should still probably
make the document.open() fixes, but that can be done in a separate bug:
https://crbug.com/583586

https://codereview.chromium.org/1659013003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1659013003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1047:
SubframeLoadingDisabler disabler(m_frame->document());
Here, m_frame->document() might be null during the commit of the initial empty
page.

The other alternative was to make SubframeLoadingDisabler not stack allocated,
but that gets complicated as well.

[dcheng, 2016-02-03 08:42:34]

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

[commit-bot: I haz the power, 2016-02-03 08:42:50]

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

[commit-bot: I haz the power, 2016-02-03 09:42:47]

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

[commit-bot: I haz the power, 2016-02-03 09:42:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mariusz Mlynski, 2016-02-03 10:21:35]

On 2016/02/03 03:13:29, dcheng wrote:
>
https://codereview.chromium.org/1659013003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:2776: m_loadEventProgress =
> UnloadEventHandled;
> This was the original fix proposed in the bug. I believe it is insufficient.
> 
> 1. Page starts XHR that never completes.
> 2. Navigation starts.
> 3. Provisional navigation prepares to commit.
> 4. Unload handlers run. At this point, m_loadEventProgres ==
UnloadEventHandled.
> 5. Now XHR abort handlers run.
> 6. XHR abort handler calls document.open(), which resets m_loadEventProgress
to
> LoadEventNotRun.
> 7. Now, XHR abort handler can attach a child frame during frame detach again.

FWIW, calling document.open() in step #6 would cause the provisional document
loader to be detached (Document::open does
|m_frame->loader().stopAllLoaders()|), so the navigation started in step #2
would be safely aborted (when DocumentLoader::processData null-checks the
frameLoader later on). It'd assert in Document::detach, though, so I agree it's
suboptimal.

[kinuko, 2016-02-03 12:23:33]

This looks reasonable to me, but it looks it breaks some test like
WebViewTest.AddFrameInCloseURLUnload... where we directly call
dispatchUnloadEvent().  Should the test be updated?

[dcheng, 2016-02-03 22:56:19]

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

[dcheng, 2016-02-03 22:56:52]

On 2016/02/03 at 12:23:33, kinuko wrote:
> This looks reasonable to me, but it looks it breaks some test like
WebViewTest.AddFrameInCloseURLUnload... where we directly call
dispatchUnloadEvent().  Should the test be updated?

I updated dispatchUnloadEvent() to also instantiate a SubframeLoadingDisabler,
to keep things consistent between the non-process-swap and the process-swap
case. PTAL.

[commit-bot: I haz the power, 2016-02-03 22:57:13]

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

[commit-bot: I haz the power, 2016-02-03 23:14:56]

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

[commit-bot: I haz the power, 2016-02-03 23:14:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Nate Chapin, 2016-02-03 23:25:14]

https://codereview.chromium.org/1659013003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/1659013003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:116: explicit
SubframeLoadingDisabler(Node* root)
Do we have to have constructors for both a pointer and a reference? That seems
kind of lame.

[dcheng, 2016-02-03 23:33:31]

Also uploading a fix for the component build.

https://codereview.chromium.org/1659013003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/1659013003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:116: explicit
SubframeLoadingDisabler(Node* root)
On 2016/02/03 at 23:25:14, Nate Chapin wrote:
> Do we have to have constructors for both a pointer and a reference? That seems
kind of lame.

A pre-Oilpan alternative is to just keep the reference constructor. Then we
could do something like:

OwnPtr<SubframeLoadingDisabler> disabler;
if (m_frame->document())
  disabler.reset(new SubframeLoadingDisabler(*m_frame->document()));

But this doesn't work with Oilpan: SubframeLoadingDisabler would have to become
a garbage collected class, but then it references a garbage collected member in
the destructor, and all sorts of complexity.

I just added the pointer version because it is still nice to assert that the
passed in Node* is not null (which is true 99% of the time).

An alternative (which I want to try following up on) is to rearrange
prepareForCommit() to make it so we only take this branch if document is not
null, but that's a bigger change and harder to merge.

[dcheng, 2016-02-03 23:37:11]

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

[commit-bot: I haz the power, 2016-02-03 23:37:59]

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

[commit-bot: I haz the power, 2016-02-04 02:36:57]

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

[commit-bot: I haz the power, 2016-02-04 02:36:58]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-02-04 08:53:58]

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

[commit-bot: I haz the power, 2016-02-04 08:54:07]

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

[commit-bot: I haz the power, 2016-02-04 10:24:05]

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

[commit-bot: I haz the power, 2016-02-04 10:24:06]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-02-04 15:11:35]

lgtm, though you'd probably want to wait for Nate's stamp :)

[Nate Chapin, 2016-02-04 19:08:27]

lgtm

[dcheng, 2016-02-04 19:09:59]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-02-04 19:11:17]

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

[commit-bot: I haz the power, 2016-02-04 19:17:31]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-04 19:18:33]

Description was changed from

==========
Disable sub frame loads more reliably in frame detach.

Previously, guarding against attaching a new frame in frame detach
was done by inspecting LoadEventProgress. There are certain
operations, such as document.open(), that can cause LoadEventProgress
to never advance, causing the guard to get skipped.

BUG=577105
==========

to

==========
Disable sub frame loads more reliably in frame detach.

Previously, guarding against attaching a new frame in frame detach
was done by inspecting LoadEventProgress. There are certain
operations, such as document.open(), that can cause LoadEventProgress
to never advance, causing the guard to get skipped.

BUG=577105

Committed: https://crrev.com/a4bcdcb1f8df4f7427e208201501a9d8e41e386b
Cr-Commit-Position: refs/heads/master@{#373581}
==========

[commit-bot: I haz the power, 2016-02-04 19:18:35]

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