Reland of "Move UndoStack from Page to Editor"

Reland of "Move UndoStack from Page to Editor"

Blink currently maintains per-page undo stacks, leading to:
- Security risks. A frame can directly manipulate content of another frame
by running document.execCommand('undo'), allowing Javascript to bypass
frame and even origin boundaries.
- Inconsistent behaviors. Without OOPIF, all changes in a page can be undone
by repeatedly invoking keyboard undo (CTRL+Z); With OOPIF, only those changes
in the focused frame and its same-origin frames can be undone.

Redos have analogous defects.

This patch changes UndoStack from per-page to per-frame, so that undos and
redos are consistently resolved by the frame where script is run or which
gets focused.

This CL is a relanding of https://codereview.chromium.org/2110543008 with
the following change:

A new function |UndoStack::clear| is introduced and called in |Editor::clear|.
In this way the leak reported by crbug.com/625736 is not reintroduced.

BUG=349272, 549334
TEST=editing/undo/undo-iframe-location-change.html

Committed: https://crrev.com/512508f0d652a006407ce66aafcd339b296a5276
Cr-Commit-Position: refs/heads/master@{#403871}

[Xiaocheng, 2016-07-05 08:35:23]

Description was changed from

==========
Reland of "Move UndoStack from Page to Editor"

Blink currently maintains per-page undo stacks, leading to:
- Security risks. A frame can directly manipulate content of another frame
by running document.execCommand('undo'), allowing Javascript to bypass
frame and even origin boundaries.
- Inconsistent behaviors. Without OOPIF, all changes in a page can be undone
by repeatedly invoking keyboard undo (CTRL+Z); With OOPIF, only those changes
in the focused frame and its same-origin frames can be undone.

Redos have analogous defects.

This patch changes UndoStack from per-page to per-frame, so that undos and
redos are consistently resolved by the frame where script is run or which
gets focused.

Changes in relanding: Keep the call of |UndoStack::didUnloadFrame()|, which is
now renamed to |UndoStack::clear()|, at the end of
|FrameLoader::dispatchUnloadEvent()|.
In this way the leak suggested by crbug.com/625736 is not reintroduced.

BUG=349272,549334
TEST=editing/undo/undo-iframe-location-change.html
==========

to

==========
Reland of "Move UndoStack from Page to Editor"

Blink currently maintains per-page undo stacks, leading to:
- Security risks. A frame can directly manipulate content of another frame
by running document.execCommand('undo'), allowing Javascript to bypass
frame and even origin boundaries.
- Inconsistent behaviors. Without OOPIF, all changes in a page can be undone
by repeatedly invoking keyboard undo (CTRL+Z); With OOPIF, only those changes
in the focused frame and its same-origin frames can be undone.

Redos have analogous defects.

This patch changes UndoStack from per-page to per-frame, so that undos and
redos are consistently resolved by the frame where script is run or which
gets focused.

Changes in relanding: Keep the call of |UndoStack::didUnloadFrame()|, which is
now renamed to |UndoStack::clear()|, at the end of
|FrameLoader::dispatchUnloadEvent()|.
In this way the leak reported by crbug.com/625736 is not reintroduced.

BUG=349272,549334
TEST=editing/undo/undo-iframe-location-change.html
==========

[Xiaocheng, 2016-07-05 08:38:03]

xiaochengh@chromium.org changed reviewers:
+ ksakamoto@chromium.org, tkent@chromium.org, yosin@chromium.org

[Xiaocheng, 2016-07-05 09:28:30]

PTAL.

The memory leak is fixed after adding back the manual cleanup of the undo stack.
I don't have an idea of why we have to manually cleanup an object that's already
on heap and should go away along with  the unloaded frame, though.

[yosin_UTC9, 2016-07-06 01:04:51]

https://codereview.chromium.org/2124723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/2124723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/Editor.cpp:1080: return
m_undoStack->clear();
How about doing |m_undoStack->clear()| to move |Editor::clear()|, which is
called by |FrameLoader::clear()|?

[tkent, 2016-07-06 01:15:25]

Did you resolve the flaky tests issue?

[Xiaocheng, 2016-07-06 04:18:33]

PTAL at Patch 2.

I didn't observe any crashes when running layout tests locally, so the flakiness
should have been fixed.

https://codereview.chromium.org/2124723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/2124723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/Editor.cpp:1080: return
m_undoStack->clear();
On 2016/07/06 at 01:04:51, Yosi_UTC9 wrote:
> How about doing |m_undoStack->clear()| to move |Editor::clear()|, which is
called by |FrameLoader::clear()|?

It seems a saner idea as now UndoStack is just an implementation details of
Editor.

Done.

[tkent, 2016-07-06 04:21:00]

lgtm

[Xiaocheng, 2016-07-06 05:19:52]

Description was changed from

==========
Reland of "Move UndoStack from Page to Editor"

Blink currently maintains per-page undo stacks, leading to:
- Security risks. A frame can directly manipulate content of another frame
by running document.execCommand('undo'), allowing Javascript to bypass
frame and even origin boundaries.
- Inconsistent behaviors. Without OOPIF, all changes in a page can be undone
by repeatedly invoking keyboard undo (CTRL+Z); With OOPIF, only those changes
in the focused frame and its same-origin frames can be undone.

Redos have analogous defects.

This patch changes UndoStack from per-page to per-frame, so that undos and
redos are consistently resolved by the frame where script is run or which
gets focused.

Changes in relanding: Keep the call of |UndoStack::didUnloadFrame()|, which is
now renamed to |UndoStack::clear()|, at the end of
|FrameLoader::dispatchUnloadEvent()|.
In this way the leak reported by crbug.com/625736 is not reintroduced.

BUG=349272,549334
TEST=editing/undo/undo-iframe-location-change.html
==========

to

==========
Reland of "Move UndoStack from Page to Editor"

Blink currently maintains per-page undo stacks, leading to:
- Security risks. A frame can directly manipulate content of another frame
by running document.execCommand('undo'), allowing Javascript to bypass
frame and even origin boundaries.
- Inconsistent behaviors. Without OOPIF, all changes in a page can be undone
by repeatedly invoking keyboard undo (CTRL+Z); With OOPIF, only those changes
in the focused frame and its same-origin frames can be undone.

Redos have analogous defects.

This patch changes UndoStack from per-page to per-frame, so that undos and
redos are consistently resolved by the frame where script is run or which
gets focused.

This CL is a relanding of https://codereview.chromium.org/2110543008 with
the following change:

A new function |UndoStack::clear| is introduced and called in |Editor::clear|.
In this way the leak reported by crbug.com/625736 is not reintroduced.

BUG=349272,549334
TEST=editing/undo/undo-iframe-location-change.html
==========

[yosin_UTC9, 2016-07-06 08:21:28]

lgtm

Sorry for late response. m(_ _)m

[Xiaocheng, 2016-07-06 08:22:11]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2016-07-06 08:22:12]

Thanks for your review!

[commit-bot: I haz the power, 2016-07-06 08:22:29]

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, 2016-07-06 09:10:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-06 09:10:57]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-06 09:13:40]

Description was changed from

==========
Reland of "Move UndoStack from Page to Editor"

Blink currently maintains per-page undo stacks, leading to:
- Security risks. A frame can directly manipulate content of another frame
by running document.execCommand('undo'), allowing Javascript to bypass
frame and even origin boundaries.
- Inconsistent behaviors. Without OOPIF, all changes in a page can be undone
by repeatedly invoking keyboard undo (CTRL+Z); With OOPIF, only those changes
in the focused frame and its same-origin frames can be undone.

Redos have analogous defects.

This patch changes UndoStack from per-page to per-frame, so that undos and
redos are consistently resolved by the frame where script is run or which
gets focused.

This CL is a relanding of https://codereview.chromium.org/2110543008 with
the following change:

A new function |UndoStack::clear| is introduced and called in |Editor::clear|.
In this way the leak reported by crbug.com/625736 is not reintroduced.

BUG=349272,549334
TEST=editing/undo/undo-iframe-location-change.html
==========

to

==========
Reland of "Move UndoStack from Page to Editor"

Blink currently maintains per-page undo stacks, leading to:
- Security risks. A frame can directly manipulate content of another frame
by running document.execCommand('undo'), allowing Javascript to bypass
frame and even origin boundaries.
- Inconsistent behaviors. Without OOPIF, all changes in a page can be undone
by repeatedly invoking keyboard undo (CTRL+Z); With OOPIF, only those changes
in the focused frame and its same-origin frames can be undone.

Redos have analogous defects.

This patch changes UndoStack from per-page to per-frame, so that undos and
redos are consistently resolved by the frame where script is run or which
gets focused.

This CL is a relanding of https://codereview.chromium.org/2110543008 with
the following change:

A new function |UndoStack::clear| is introduced and called in |Editor::clear|.
In this way the leak reported by crbug.com/625736 is not reintroduced.

BUG=349272,549334
TEST=editing/undo/undo-iframe-location-change.html

Committed: https://crrev.com/512508f0d652a006407ce66aafcd339b296a5276
Cr-Commit-Position: refs/heads/master@{#403871}
==========

[commit-bot: I haz the power, 2016-07-06 09:13:41]

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