Move UndoStack from Page to Editor

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 patch also removes |UndoStack::didUnloadFrame()| since its only purpose
is to filter out the undo steps for a particular frame, which no longer makes
sense after undo stacks are made per-frame.

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

Committed: https://crrev.com/6447f384cf6d95f70475798e3fd45b689316ce50
Cr-Commit-Position: refs/heads/master@{#403653}

[Xiaocheng, 2016-07-01 08:48:05]

Description was changed from

==========
Move UndoStack from Page to Editor

BUG=
==========

to

==========
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.

BUG=349272,549334
==========

[Xiaocheng, 2016-07-01 08:49:35]

xiaochengh@chromium.org changed reviewers:
+ yosin@chromium.org

[Xiaocheng, 2016-07-01 08:49:36]

PTAL. Thanks.

I also have some questions about the usage of the undo stack after its frame is
unloaded. Please refer to the TODOs I added in the CL.

[dcheng, 2016-07-01 09:09:24]

Description was changed from

==========
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.

BUG=349272,549334
==========

to

==========
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.

BUG=349272,549334
==========

[yosin_UTC9, 2016-07-01 09:21:22]

https://codereview.chromium.org/2110543008/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/undo/undo-iframe-location-change.html
(right):

https://codereview.chromium.org/2110543008/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/undo/undo-iframe-location-change.html:12:
function part1() {
Optional: Could you convert this test to use w3c test harness to get rid of
undo-iframe-location-change-expected.html and upstream to w3c test repository.
Example: http://crrev.com/2010333003

https://codereview.chromium.org/2110543008/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/undo/undo-iframe-location-change.html:15:
// Hack to perform the editing command.  Should not be able to
How about just say "Call execCommand for subframe document rather than main
window"?
Or better English statement. ;-)

https://codereview.chromium.org/2110543008/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/UndoStack.cpp (right):

https://codereview.chromium.org/2110543008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/UndoStack.cpp:68: //
TODO(xiaochengh): What's the use of this scope? Can we remove it?
This event scope to prevent JavaScript event handlers to destruct |frame|.
Since JavaScript is never executed during |m_{undo,redo}Stack.clear()|, we can
remove this |EventDispatchForbiddenScope|.

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

https://codereview.chromium.org/2110543008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:273: // TODO(xiaochengh):
can the undo stack be used again after frame unloading?
TL;DR: No, we don't use undo stack after frame unloading.

In theory, following scenario should work:

1. <body contenteditable>foo<iframe srcdoc="<body
contenteditable>bar</body>"></iframe>baz</body>
2. Type "quux" after "bar" in IFRAME's BODY
3. Select "foo" to "baz" in main frame
4. Hit Delete key in main frame
4. Undo in main frame
 => IFRAME should be shown with "barbaz"
5. Undo in iframe
 => IFRAME should show "bar"

Chrome, Edge and Firefox doesn't show "barbaz". They show "bar" only.
So, we don't use UndoStack after frame destruction.

This means we can get rid of |UndoStack::didUnloadFrame()|.
WDYT?

[Xiaocheng, 2016-07-04 04:55:51]

Description was changed from

==========
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.

BUG=349272,549334
==========

to

==========
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 patch also removes |UndoStack::didUnloadFrame()| since its only purpose
is to filter out the undo steps for a particular frame, which no longer makes
sense after undo stacks are made per-frame.

BUG=349272,549334
==========

[Xiaocheng, 2016-07-04 04:57:13]

Description was changed from

==========
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 patch also removes |UndoStack::didUnloadFrame()| since its only purpose
is to filter out the undo steps for a particular frame, which no longer makes
sense after undo stacks are made per-frame.

BUG=349272,549334
==========

to

==========
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 patch also removes |UndoStack::didUnloadFrame()| since its only purpose
is to filter out the undo steps for a particular frame, which no longer makes
sense after undo stacks are made per-frame.

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

[Xiaocheng, 2016-07-04 04:58:59]

xiaochengh@chromium.org changed reviewers:
+ tkent@chromium.org

[Xiaocheng, 2016-07-04 06:29:23]

PTAL at Patch 3.

|UndoStack::didUnloadFrame()| is completely removed since it doesn't do any
useful work now.

|Editor::undoStack()| is removed since Editor is now the only client class of
UndoStack.

The layout test is also rewritten using w3c test harness.

https://codereview.chromium.org/2110543008/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/undo/undo-iframe-location-change.html
(right):

https://codereview.chromium.org/2110543008/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/undo/undo-iframe-location-change.html:12:
function part1() {
On 2016/07/01 at 09:21:21, Yosi_UTC9 wrote:
> Optional: Could you convert this test to use w3c test harness to get rid of
undo-iframe-location-change-expected.html and upstream to w3c test repository.
> Example: http://crrev.com/2010333003

Done.

https://codereview.chromium.org/2110543008/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/undo/undo-iframe-location-change.html:15:
// Hack to perform the editing command.  Should not be able to
On 2016/07/01 at 09:21:21, Yosi_UTC9 wrote:
> How about just say "Call execCommand for subframe document rather than main
window"?
> Or better English statement. ;-)

Done (kind of).

https://codereview.chromium.org/2110543008/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/UndoStack.cpp (right):

https://codereview.chromium.org/2110543008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/UndoStack.cpp:68: //
TODO(xiaochengh): What's the use of this scope? Can we remove it?
On 2016/07/01 at 09:21:21, Yosi_UTC9 wrote:
> This event scope to prevent JavaScript event handlers to destruct |frame|.
> Since JavaScript is never executed during |m_{undo,redo}Stack.clear()|, we can
remove this |EventDispatchForbiddenScope|.

Thanks. This method is removed.

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

https://codereview.chromium.org/2110543008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:273: // TODO(xiaochengh):
can the undo stack be used again after frame unloading?
On 2016/07/01 at 09:21:21, Yosi_UTC9 wrote:
> TL;DR: No, we don't use undo stack after frame unloading.
> 
> In theory, following scenario should work:
> 
> 1. <body contenteditable>foo<iframe srcdoc="<body
contenteditable>bar</body>"></iframe>baz</body>
> 2. Type "quux" after "bar" in IFRAME's BODY
> 3. Select "foo" to "baz" in main frame
> 4. Hit Delete key in main frame
> 4. Undo in main frame
>  => IFRAME should be shown with "barbaz"
> 5. Undo in iframe
>  => IFRAME should show "bar"
> 
> Chrome, Edge and Firefox doesn't show "barbaz". They show "bar" only.
> So, we don't use UndoStack after frame destruction.
> 
> This means we can get rid of |UndoStack::didUnloadFrame()|.
> WDYT?

I agree. Removed.

[yosin_UTC9, 2016-07-04 07:27:41]

lgtm w/ small nit

Thanks for converting test!
Good job! (^_^)b

https://codereview.chromium.org/2110543008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Editor.h (right):

https://codereview.chromium.org/2110543008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.h:256: Member<UndoStack>
m_undoStack;
nit: Let's use|const Member<UndoStack>|, since it is read-only member variable.

[tkent, 2016-07-04 07:44:36]

lgtm

https://codereview.chromium.org/2110543008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/UndoStack.h (right):

https://codereview.chromium.org/2110543008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/UndoStack.h:43: class UndoStack
final : public GarbageCollected<UndoStack> {
Please add a comment that UndoStack is owned by Editor, i.e. per-Frame.

[Xiaocheng, 2016-07-04 08:03:37]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2016-07-04 08:03:37]

Thanks for the review!

I'm going ahead to commit since all comments are addressed.

https://codereview.chromium.org/2110543008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Editor.h (right):

https://codereview.chromium.org/2110543008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.h:256: Member<UndoStack>
m_undoStack;
On 2016/07/04 at 07:27:41, Yosi_UTC9 wrote:
> nit: Let's use|const Member<UndoStack>|, since it is read-only member
variable.

Done.

https://codereview.chromium.org/2110543008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/UndoStack.h (right):

https://codereview.chromium.org/2110543008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/UndoStack.h:43: class UndoStack
final : public GarbageCollected<UndoStack> {
On 2016/07/04 at 07:44:36, tkent wrote:
> Please add a comment that UndoStack is owned by Editor, i.e. per-Frame.

Done.

[Xiaocheng, 2016-07-04 08:03:37]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2110543008/#ps60001
(title: "Use |const Member| and add notes to UndoStack class")

[commit-bot: I haz the power, 2016-07-04 08:03:47]

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-04 09:16:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-04 09:17:04]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-04 09:18:24]

Description was changed from

==========
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 patch also removes |UndoStack::didUnloadFrame()| since its only purpose
is to filter out the undo steps for a particular frame, which no longer makes
sense after undo stacks are made per-frame.

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

to

==========
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 patch also removes |UndoStack::didUnloadFrame()| since its only purpose
is to filter out the undo steps for a particular frame, which no longer makes
sense after undo stacks are made per-frame.

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

Committed: https://crrev.com/6447f384cf6d95f70475798e3fd45b689316ce50
Cr-Commit-Position: refs/heads/master@{#403653}
==========

[commit-bot: I haz the power, 2016-07-04 09:18:25]

Patchset 4 (id:??) landed as
https://crrev.com/6447f384cf6d95f70475798e3fd45b689316ce50
Cr-Commit-Position: refs/heads/master@{#403653}

[Kunihiko Sakamoto, 2016-07-05 05:01:01]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2127463002/ by ksakamoto@chromium.org.

The reason for reverting is: speculative revert to see whether this caused
crbug.com/625736.

[tkent, 2016-07-05 08:17:55]

Some tests were flaky during Patch Set 4 was checked in.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=edi...
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=edi...
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=edi...