Refactor frame navigation/detach state cleanup to be more sane.

Refactor frame navigation/detach state cleanup to be more sane.

Prior to this patch, cleaning up state for a navigating or detaching
frame was ad hoc: often times, code to do the same thing (e.g. detach
child frames) was present in multiple locations just to "be sure" that
the cleanup would happen no matter what code path was taken.

This logic has been reorganized so that the state cleanup always follows
a similar flow for both navigation and detach. There are now 4 common
steps:
  1. FrameLoader::dispatchUnloadEvent()
  2. Frame::detachChildren()
  3. Document::prepareForDestruction()
  4. FrameLoader::clear()

BUG=464764
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193157

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193227

[dcheng, 2015-04-03 08:16:30]

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

[dcheng, 2015-04-03 14:35:45]

So after trying to document how frame state changes across navigation/detach, I
came to the conclusion that it was a little too insane:
- Document::prepareForDestruction() can be called as a side effect of
LocalFrame::setView(). What?
- It's also called as a side effect of FrameLoader::clear().
- And just to be really sure, it's also called by
DocumentLoader::createWriterFor().

Similarly, detaching child frames:
- Can happen as a side effect of calling Document::prepareForDestruction().
- But sometimes happens explicitly by a call to Frame::detachChildren().

I've tried to consolidate the duplicated/redundant logic and add invariants to
assert about, e.g. Document::prepareForDestruction() can now only be called if
all the children were already detached. This revealed a few unit tests that
tested contrived situations that don't reflect how the code works in reality.

The remaining test crashes due to my change are due to AXObjectCache timer's
firing on AXObject's for detached frames. I'm still trying to figure out how we
avoided this before, but I think I will be able to resolve the crashes without
substantial changes to this patch.

https://codereview.chromium.org/1052993006/diff/60001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryEvaluatorTest.cpp (left):

https://codereview.chromium.org/1052993006/diff/60001/Source/core/css/MediaQu...
Source/core/css/MediaQueryEvaluatorTest.cpp:161:
pageHolder->frame().setView(view);
Since this test didn't need to flout the invariants about frame state, I
simplified it a bit. This meant I had to remove an ASSERT in DummyPageHolder's
dtor, but I think that's OK: holding references to keep objects alive to test
weird edge cases of frame detach is a legitimate need of unit test code, and
does not indicate a logic error.

https://codereview.chromium.org/1052993006/diff/60001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1052993006/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2164: ASSERT(!m_frame ||
m_frame->tree().childCount() == 0);
If we were active at some point, all children must be detached before calling
us. This eliminates the insanity of "sometimes detachChildren() detaches the sub
frames, but sometimes prepareForDisconnection() does it just to catch cases that
were missed".

https://codereview.chromium.org/1052993006/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2166: if (!isActive())
We still need to check this, due to the magic of nested synchronous navigations
to javascript: URLs in pagehide handlers.

https://codereview.chromium.org/1052993006/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2169: m_markers->prepareForDestruction();
Moving this here seemed to be a no-op, and makes the code a bit nicer in that
nothing happens if the Document is already inactive.

https://codereview.chromium.org/1052993006/diff/60001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (left):

https://codereview.chromium.org/1052993006/diff/60001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:314: if (m_document->isActive()) {
I moved this directly into the XSLT code so we don't need to special-case it
here.

I should add an assert that the document is inactive though (I'll do that in a
followup patch set).

https://codereview.chromium.org/1052993006/diff/60001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1052993006/diff/60001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:266: m_loader.clear();
Calling this multiple times appears to be safe, and let's us keep the general
pattern of:
dispatchUnloadEvent();
detachChildren();
document->prepareForDestruction();
m_loader.clear();

https://codereview.chromium.org/1052993006/diff/60001/Source/core/loader/Docu...
File Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/1052993006/diff/60001/Source/core/loader/Docu...
Source/core/loader/DocumentLoader.cpp:751: if (frame->document()) {
When creating the initial empty page, I believe this is null.

https://codereview.chromium.org/1052993006/diff/60001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1052993006/diff/60001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:289: clear();
Consistency!

https://codereview.chromium.org/1052993006/diff/60001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:975: if (m_frame->document())
Similarly, this has to handle the null case due to creating the initial empty
doc.

https://codereview.chromium.org/1052993006/diff/60001/Source/web/tests/PinchV...
File Source/web/tests/PinchViewportTest.cpp (right):

https://codereview.chromium.org/1052993006/diff/60001/Source/web/tests/PinchV...
Source/web/tests/PinchViewportTest.cpp:1552:
localFrame->frame()->document()->prepareForDestruction();
I couldn't think of a good way to adapt this test without having to understand
frame detach/navigation internals. So I just hacked around it for now. I plan on
revisiting this later.

[Nate Chapin, 2015-04-03 19:27:31]

This seems pretty reasonable, just a stylistic nit.

Let me know when you've got the AXObjectCache bug resolved.

https://codereview.chromium.org/1052993006/diff/60001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1052993006/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2166: if (!isActive())
On 2015/04/03 14:35:45, dcheng wrote:
> We still need to check this, due to the magic of nested synchronous
navigations
> to javascript: URLs in pagehide handlers.

D:

https://codereview.chromium.org/1052993006/diff/100001/Source/core/loader/Doc...
File Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/1052993006/diff/100001/Source/core/loader/Doc...
Source/core/loader/DocumentLoader.cpp:751: if (frame->document()) {
ASSERT(!frame->document() || !frame->document()->isActive()); ?

[dcheng, 2015-04-03 20:37:21]

https://codereview.chromium.org/1052993006/diff/100001/Source/core/loader/Doc...
File Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/1052993006/diff/100001/Source/core/loader/Doc...
Source/core/loader/DocumentLoader.cpp:751: if (frame->document()) {
On 2015/04/03 19:27:31, Nate Chapin wrote:
> ASSERT(!frame->document() || !frame->document()->isActive()); ?

Done.

https://codereview.chromium.org/1052993006/diff/160001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1052993006/diff/160001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:2174: // FIXME: We should revisit this.
I moved this logic over from LocalFrame::detachView, and it fixed the
AXObjectCache issues.

[Nate Chapin, 2015-04-03 20:42:33]

LGTM w/nits

https://codereview.chromium.org/1052993006/diff/160001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1052993006/diff/160001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:969: m_frame->detachChildren();
It's clear to me why detachChildren() is a noop if m_frame->document() is null,
but it might(?) be more clear if this is outside the if().

https://codereview.chromium.org/1052993006/diff/160001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:974: ASSERT(m_frame->tree().childCount() ==
0);
Maybe drop this else{} clause and ASSERT this unconditionally? It should always
be true at this point.

[dcheng, 2015-04-03 20:47:20]

https://codereview.chromium.org/1052993006/diff/160001/Source/core/loader/Fra...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1052993006/diff/160001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:969: m_frame->detachChildren();
On 2015/04/03 20:42:32, Nate Chapin wrote:
> It's clear to me why detachChildren() is a noop if m_frame->document() is
null,
> but it might(?) be more clear if this is outside the if().

The idea is if there's no document, there should be no children to detach either
(because it wouldn't be possible to attach them to the DOM/active Document yet).
That being said, your comment below makes sense (and I added a brief comment
here as well).

https://codereview.chromium.org/1052993006/diff/160001/Source/core/loader/Fra...
Source/core/loader/FrameLoader.cpp:974: ASSERT(m_frame->tree().childCount() ==
0);
On 2015/04/03 20:42:32, Nate Chapin wrote:
> Maybe drop this else{} clause and ASSERT this unconditionally? It should
always
> be true at this point.

Done.

[dcheng, 2015-04-03 20:47:23]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-04-03 20:47:23]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1052993006/#ps150016
(title: "More feedback")

[commit-bot: I haz the power, 2015-04-03 20:47:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1052993006/150016

[dcheng, 2015-04-03 21:16:58]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-04-03 21:16:59]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1052993006/#ps190001
(title: "Fix unit tests... again.")

[commit-bot: I haz the power, 2015-04-03 21:17:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1052993006/190001

[dcheng, 2015-04-04 01:00:26]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2015-04-04 02:26:33]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-04-04 02:26:33]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1052993006/#ps210001
(title: "Revert prepareForDestruction to be called after
DocumentLoader::detachFromFrame")

[commit-bot: I haz the power, 2015-04-04 02:26:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1052993006/210001

[dcheng, 2015-04-04 04:33:56]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-04-04 04:33:56]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1052993006/#ps230001
(title: "Add rebaseline expectation")

[commit-bot: I haz the power, 2015-04-04 04:34:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1052993006/230001

[commit-bot: I haz the power, 2015-04-04 06:01:59]

Committed patchset #13 (id:230001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193157

[dcheng, 2015-04-04 13:36:21]

A revert of this CL (patchset #13 id:230001) has been created in
https://codereview.chromium.org/1058403002/ by dcheng@chromium.org.

The reason for reverting is:
PasswordManagerBrowserTest.NoPromptForFailedLoginFromSubFrameWithMultiFramesInPage
is broken by this change..

[dcheng, 2015-04-06 22:11:26]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-04-06 22:11:26]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1052993006/#ps250001
(title: "Minor test cleanup")

[commit-bot: I haz the power, 2015-04-06 22:11:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1052993006/250001

[commit-bot: I haz the power, 2015-04-06 23:42:11]

Committed patchset #14 (id:250001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193227