1465702 - EditorBase, TextEditor and HTMLEditor should use a stack class to store all information which are necessary to handle each EditAction like TextEditRules and HTMLEditRules

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Since fix of bug 1463327, EditorBase, TextEditor and HTMLEditor have public methods only for starting to handle each edit action. And like the fix of bug 1460509, those classes should initialize a stack class in each public methods to share edit action information with all protected/private methods of them (even though some of them may be called by other helper classes but they are called while one of the public methods is being called).

So, here we should do:

* Create new stack class to store EditAction, top-level EditSubAction, Selection (and each of them should be accessible via protected methods).
* Define EditAction for each public method.
* Maybe need to split some public methods, one is for called by outside and the other is for called by outside.

Additionally, we need to check if it's kicked by XUL command or web content. This is required when we implement beforeinput event. So, if it can fix easily and quickly, I'll include this fix into the patches.

Additionally, those patches can reduce redundant calls of EditorBase::GetSelection(). The method is not cheap and I see it a lot when I check profile. So, must be good fix for Quantum Flow too.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a35584ea9fd0c1fbcc86ca8652aa6414210066a9

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c80b80d46aa9edb94b366130a0fb3d8072da595

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be24cacb44b3f5705046db8b75c3e92443f445ef

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

There might be some public methods, which are not caught by my check, though, I finished filing bugs to split all remaining public methods which should be marked each edit action.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccc53c93b0f42c9c08338acdbfd823a6f0348a85

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f89cc3a2b178d659bf1eabf877278fea65460e96

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed445f3f2cd4bc3a70df644c9c8af1e570a22c25

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8412ce7183296a8e54137861362b368f73911462

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=819d38e6b82e2879ef3e25bb03f7027616a412fc

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d7d3494a9a2658d1fdeb904c993fbbc2c7cabae

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2df3d3322859ee0da29291d52c67e5262a0d1e7b

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fe5eac89a84eba559ec4fab9be2689ae9c01383

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=494ec3e111566ec1a12d9bbdb281a20d50910594

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ab8385ea4a51d81f7ed7f66b707cbc4459650e6

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=295ad0f31d46f8c82461abc76ddc95baeaa2ab20

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26f19695d34dfb69b720dac32b34bfd6ec278e73

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37ed0f82a8444044c1dc5c03aa90cc9834aefa2d

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5365b7e9f292c3f49061789b022d6d034806dac

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6a1e58b4974d70a2aa8332d23585b2241ee17c3

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ed75d62a74c7363a93efc3597b1fd1ee624fd56

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dafbb6a5fa6e31342c77bff45e50829b02d6021

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1f406ca2b5d746fe48a1344eb13693945af6ed1

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f904c26b511eb6d8b4910a3ee968a5cbb5469d60

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1465702 - part 1: Add EditorBase::AutoEditActionDataSetter to store edit action and grab Selection instance while editor classes handle an edit action

Like TextEditRules, EditorBase should have a stack class which cache necessary
objects and current handling edit action.  The edit action will be necessary
when we implement InputEvent.inputType.

Different from TextEditRules, this adds |const RefPtr<Selection>&| instead
of |Selection&|.  The reason is, when I add MOZ_CAN_RUN_SCRIPT to some methods,
it's not allowed like this:

> foo->CanRunScriptMethod(SelectionRef());

I'll update TextEditRules for consistency in the following patches.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1465702 - part 2: Make public methods of EditorBase create AutoEditActionDataSetter if necessary

This patch makes public methods of EditorBase create AutoEditActionDataSetter
as far as possible.  However, does not do so for some public methods if they are
not necessary to create it.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1465702 - part 3: Make public methods of TextEditor create AutoEditActionDataSetter if necessary

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1465702 - part 4: Make public methods of HTMLEditor create AutoEditActionDataSetter if necessary

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1465702 - part 5: Remove unnecessary Selection argument from editor module

EditorBase::SelectionRefPtr() is now safe to use in editor and really fast to
retrieve Selection than EditorBase::GetSelection().  Therefore, we can get rid of
all Selection pointer/reference argument of each method which always take
normal Selection.

Note that this changes nsIHTMLEditor.checkSelectionStateForAnonymousButtons()
because its argument is only Selection.  So, BlueGriffon should work even
though it calls the method with nsIEditor.selection.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1465702 - part 6: Remove unnecessary Selection argument from TextEditRules and HTMLEditRules and rename/reimplement TextEditRules::SelectionRef() as TextEditRules::SelectionRefPtr()

When each method of TextEditRules and HTMLEditRules should be called only by
EditorBase/TextEditor/HTMLEditor while the editor instance has
AutoEditActionDataSetter instance.  Therefore, we can get rid of all
pointers/references of Selection from each method's argument.

Additionally, this reimplements TextEditRules::SelectionRef() with
EditorBase::SelectionRefPtr() and make it return |const RefPtr<Selection>&|
rather than |Selection&| since RefPtr reference is required when we call
methods which are marked as MOZ_CAN_RUN_SCRIPT.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1465702 - part 7: Make protected/private methods of EditorBase/TextEditor/HTMLEditor use SelectionRefPtr() instead of GetSelection()

Now, any protected/private methods of editor classes can refer
SelectionRefPtr() safely.  So, we can cut the cost of calling GetSelection()
only once per edit action handling.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc8d04abd39b
part 1: Add EditorBase::AutoEditActionDataSetter to store edit action and grab Selection instance while editor classes handle an edit action r=m_kato
https://hg.mozilla.org/integration/autoland/rev/1c3326439c30
part 2: Make public methods of EditorBase create AutoEditActionDataSetter if necessary r=m_kato
https://hg.mozilla.org/integration/autoland/rev/27ca7b49a11e
part 3: Make public methods of TextEditor create AutoEditActionDataSetter if necessary r=m_kato
https://hg.mozilla.org/integration/autoland/rev/986f21f1f5d5
part 4: Make public methods of HTMLEditor create AutoEditActionDataSetter if necessary r=m_kato
https://hg.mozilla.org/integration/autoland/rev/1945bfc27c6d
part 5: Remove unnecessary Selection argument from editor module r=m_kato
https://hg.mozilla.org/integration/autoland/rev/258672244b3e
part 6: Remove unnecessary Selection argument from TextEditRules and HTMLEditRules and rename/reimplement TextEditRules::SelectionRef() as TextEditRules::SelectionRefPtr() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/bda6c79cf03c
part 7: Make protected/private methods of EditorBase/TextEditor/HTMLEditor use SelectionRefPtr() instead of GetSelection() r=m_kato

Comment 32

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/fc8d04abd39b
https://hg.mozilla.org/mozilla-central/rev/1c3326439c30
https://hg.mozilla.org/mozilla-central/rev/27ca7b49a11e
https://hg.mozilla.org/mozilla-central/rev/986f21f1f5d5
https://hg.mozilla.org/mozilla-central/rev/1945bfc27c6d
https://hg.mozilla.org/mozilla-central/rev/258672244b3e
https://hg.mozilla.org/mozilla-central/rev/bda6c79cf03c

Comment 33

[Julien G]

Hello there, I'm having reproducible crashes on Nightly when editing a table on MDN.
I was able to bisect to https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5cdef82c0fb01e17ab451c546f4bff1b90a1f7af&tochange=bda6c79cf03c921d16e4944fc200a424b4af78b0 using mozregression.

The STR for my crashes boil down to:
- Open an MDN page with tables (ex. https://developer.mozilla.org/en-US/docs/User:SphinxKnight/tabCrash )
- Edit it
- After a few strokes, the tab crashes

Please tell me if you prefer me to open another bug and/or provides more information.

Here is the relevant part of the log from mozregression:

54:08.08 INFO: Narrowed inbound regression window from [ba133f47, bda6c79c] (3 builds) to [5cdef82c, bda6c79c] (2 builds) (~1 steps left)
54:08.08 INFO: No more inbound revisions, bisection finished.
54:08.08 INFO: Last good revision: 5cdef82c0fb01e17ab451c546f4bff1b90a1f7af
54:08.08 INFO: First bad revision: bda6c79cf03c921d16e4944fc200a424b4af78b0
54:08.08 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5cdef82c0fb01e17ab451c546f4bff1b90a1f7af&tochange=bda6c79cf03c921d16e4944fc200a424b4af78b0

Comment 34

[Julien G]

Adding a crash report if that may help: https://crash-stats.mozilla.com/report/index/b57a2b4b-1b98-4d4b-82a6-647e00181103

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It's bug 1504093. Please file a new bug and add bug # which caused the regression into "blocks" when you find a new regression.