1430982 - Make EditorBase not treat nsTextServicesDocument for mInlineSpellChecker as nsIEditActionListener

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Comment 1

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

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

Comment 2

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

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

Comment 3

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

Hmm. nsEditorSpellChecker can be created by JS. Then, editor may be associated with 2 or more nsTextServicesDocument. However, this is really rare case. So, we should optimize performance for the case that there is only one nsTextServicesDocument is associated.

Comment 4

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

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

Comment 5

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

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

Comment 6

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

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

Comment 7

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

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

Comment 8

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

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

Comment 9

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

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

Comment 10

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

FYI:

nsTextServicesDocument is owned by mozSpellChecker, mozSpellChecker is owned by nsEditorSpellCheck, nsEditorSpellCheck is owned by mozInlineSpellChecker, mozInlineSpellChecker is owned by EditorBase in most cases.

Some exceptions are, some classes can be created from chrome JS. Actually, composer UI of comm-central creates nsEditorSpellCheck (although I'm not sure it's alive): https://searchfox.org/comm-central/source/editor/ui/dialogs/content/EdSpellCheck.js#26

Current goal of this bug is, making nsTextServicesDocument for EditorBase::mInlineSpellChecker is not added to the edit action listener array. Instead, called its methods directly.

Comment 11

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

And note that there is a bug of releasing above class instances when EditorBase becomes disabling spell check. When it becomes disabled, mozInlineSpellChecker releases nsEditorSpellCheck. However, if its refcount doesn't become 0, it keeps objects but if tried to clear them from mozInlineSpellChecker, that causes new crashes caused by nullptr access in some automated tests. I don't want to struggle with it in this bug.  Therefore, coming patches hack EditorBase::AddEditActionListener(). I think that this won't make damage to initializing performance because it's not called so many times.

Comment 12

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

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

Comment 13

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

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

Comment 14

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

Attachment: Bug 1430982 - part 1: Make nsTextServicesDocument store editor with RefPtr<TextEditor> rather than nsWeakPtr

nsTextServicesDocument stores editor with nsWeakPtr for avoiding circular
reference.  However, both TextEditor and nsTextServicesDocument are cycle
collectable. So, we don't need to worry about the issue and we can simply
make it store with RefPtr<TextEditor>.  Then, we can make EditorBase and
nsTextServicesDocument access each other directly.

Review commit: https://reviewboard.mozilla.org/r/214078/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214078/

Comment 15

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

Attachment: Bug 1430982 - part 2: Rename nsTextServicesDocument to mozilla::TextServicesDocument and expose its header

For making nsTextServicesDocument accessible from anywhere directly, we need to
expose its header.  Then, it should be in mozilla namespace.

This patch renames nsTextServicesDocument to mozilla::TextServicesDocument
and expose the header file as "mozilla/TextServicesDocument.h".

Review commit: https://reviewboard.mozilla.org/r/214080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214080/

Comment 16

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

Attachment: Bug 1430982 - part 3: Make mozSpellChecker and nsEditorSpellChecker treat TextServicesDocument directly rather than nsITextServicesDocument

Now, TextServicesDocument can be treated by anybody.  So, let's make
mozSpellChecker and nsEditorSpellChecker treat it directly instead of
nsITextServicesDocument.

Review commit: https://reviewboard.mozilla.org/r/214082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214082/

Comment 17

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

Attachment: Bug 1430982 - part 4: Get rid of nsITextServicesDocument because it used by nobody

nsITextServicesDocument isn't used by anybody now.  So, we can get rid of it.
Additionally, nsITextService and nsTextServicesFactory are not also used by
anybody.  Therefore, this patch removes all of them.

Review commit: https://reviewboard.mozilla.org/r/214084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214084/

Comment 18

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

Attachment: Bug 1430982 - part 5: Make nsEditorSpellCheck store mozSpellChecker directly rather than nsISpellChecker

nsEditorSpellCheck::mSpellChecker is always mozSpellChecker because it's
created only by nsEditorSpellCheck.  Additionally, mozSpellChecker.h is
already exposed.  So, nsEditorSpellCheck can store it as mozSpellChecker
directly.

Review commit: https://reviewboard.mozilla.org/r/214086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214086/

Comment 19

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

Attachment: Bug 1430982 - part 6: Rename nsEditorSpellCheck to mozilla::EditorSpellCheck and expose its header

For making mozInlineSpellChecker stores nsEditorSpellCheck directly, we need
to expose its header file.  For doing that this patch renames the class to
mozilla::EditorSpellCheck and expose it as "mozilla/EditorSpellCheck.h".

Review commit: https://reviewboard.mozilla.org/r/214088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214088/

Comment 20

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

Attachment: Bug 1430982 - part 7: Make mozInlineSpellChecker store EditorSpellCheck directly rather than nsIEditorSpellCheck

mozInlienSpellChecker::mSpellCheck and mozInlineSpellChecker::mPendingSpellCheck
are always instances of EditorSpellCheck because mozInlienSpellChecker creates
its instance.  Therefore, making mozInlineSpellChecker store EditorSpellCheck
directly makes sense.

Review commit: https://reviewboard.mozilla.org/r/214090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214090/

Comment 21

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

Attachment: Bug 1430982 - part 8: Make EditorBase store inline spell checker as mozInlineSpellChecker rather than nsIInlineSpellChecker

EditorBase::mInlineSpellChecker is always an instance of mozInlineSpellChecker.
Fortunately, it's easy to expose mozInlineSpellChecker.h.  So, making EditorBase
store it as mozInlineSpellChecker directly, EditorBase can access any of
mozInlineSpellChecker, EditorSpellCheck, mozSpellChecker and
TextServicesDocument with new accessors created by following patch.

Review commit: https://reviewboard.mozilla.org/r/214092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214092/

Comment 22

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

Attachment: Bug 1430982 - part 9: Create accessors for each member of mozSpellChecker, EditorSpellCheck, mozInlineSpellChecker

mozSpellChecker stores TextServicesDocument, EditorSpellCheck stores
mozSpellChecker and mozInlineSpellChecker stores EditorSpellCheck.
So, they should have accessors for their member.  Then, EditorBase can
access all of them.

Review commit: https://reviewboard.mozilla.org/r/214094/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214094/

Comment 23

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

Attachment: Bug 1430982 - part 10: Make EditorBase store TextServicesDocument for its mInlineSpellChecker for avoiding to run for loops to call nsIEditActionListener methods

Currently, first edit action listener is always TextServicesDocument for
EditorBase::mInlineSpellChecker if spell check is enabled and it requires
only DidDeleteNode() and DidJoinNodes().  So, in most cases, EditorBase
should call only them to avoid running redundant for loops for
nsIEditActionListener methods.

This patch makes that EditorBase::AddEditActionListener() and
RemoveEditActionListener() check whether coming nsIEditActionListener is
TextServicesDocument for mInlineSpellChecker.  If so, EditorBase should
store it as reference to TextServicesDocument.  And when edit action occurs,
its DidDeleteNode() and DidJoinNodes() should be called directly.

Unfortunately, this patch makes TextServiceDocument's maintaining rules
complicated.  However, it must be really rare case to add new edit action
listener because it's really old component.

Note that EditorSpellCheck may be created multiple instances for an editor
from chrome JS.  Therefore, we need to keep TextServicesDocument being
derived from nsIEditActionListener because in such case, TextServicesDocument
for other EditorSpellCheck instances should be supported via
nsIEditActionListener even though such case makes EditorBase slower.

Review commit: https://reviewboard.mozilla.org/r/214096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214096/

Comment 24

[Makoto Kato [:m_kato]]

Comment on attachment 8943794 [details]
Bug 1430982 - part 1: Make nsTextServicesDocument store editor with RefPtr<TextEditor> rather than nsWeakPtr

https://reviewboard.mozilla.org/r/214078/#review219930

Comment 25

[Makoto Kato [:m_kato]]

Comment on attachment 8943795 [details]
Bug 1430982 - part 2: Rename nsTextServicesDocument to mozilla::TextServicesDocument and expose its header

https://reviewboard.mozilla.org/r/214080/#review219932

Comment 26

[Makoto Kato [:m_kato]]

Comment on attachment 8943796 [details]
Bug 1430982 - part 3: Make mozSpellChecker and nsEditorSpellChecker treat TextServicesDocument directly rather than nsITextServicesDocument

https://reviewboard.mozilla.org/r/214082/#review219936

Comment 27

[Makoto Kato [:m_kato]]

Comment on attachment 8943797 [details]
Bug 1430982 - part 4: Get rid of nsITextServicesDocument because it used by nobody

https://reviewboard.mozilla.org/r/214084/#review219938

Comment 28

[Makoto Kato [:m_kato]]

Comment on attachment 8943798 [details]
Bug 1430982 - part 5: Make nsEditorSpellCheck store mozSpellChecker directly rather than nsISpellChecker

https://reviewboard.mozilla.org/r/214086/#review219940

Comment 29

[Makoto Kato [:m_kato]]

Comment on attachment 8943799 [details]
Bug 1430982 - part 6: Rename nsEditorSpellCheck to mozilla::EditorSpellCheck and expose its header

https://reviewboard.mozilla.org/r/214088/#review219946

Comment 30

[Makoto Kato [:m_kato]]

Comment on attachment 8943800 [details]
Bug 1430982 - part 7: Make mozInlineSpellChecker store EditorSpellCheck directly rather than nsIEditorSpellCheck

https://reviewboard.mozilla.org/r/214090/#review219948

Comment 31

[Makoto Kato [:m_kato]]

Comment on attachment 8943801 [details]
Bug 1430982 - part 8: Make EditorBase store inline spell checker as mozInlineSpellChecker rather than nsIInlineSpellChecker

https://reviewboard.mozilla.org/r/214092/#review219950

Comment 32

[Makoto Kato [:m_kato]]

Comment on attachment 8943803 [details]
Bug 1430982 - part 10: Make EditorBase store TextServicesDocument for its mInlineSpellChecker for avoiding to run for loops to call nsIEditActionListener methods

https://reviewboard.mozilla.org/r/214096/#review219966

Comment 33

[Makoto Kato [:m_kato]]

Comment on attachment 8943802 [details]
Bug 1430982 - part 9: Create accessors for each member of mozSpellChecker, EditorSpellCheck, mozInlineSpellChecker

https://reviewboard.mozilla.org/r/214094/#review219968

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44be45cf31b4
part 1: Make nsTextServicesDocument store editor with RefPtr<TextEditor> rather than nsWeakPtr r=m_kato
https://hg.mozilla.org/integration/autoland/rev/1e9838519ff1
part 2: Rename nsTextServicesDocument to mozilla::TextServicesDocument and expose its header r=m_kato
https://hg.mozilla.org/integration/autoland/rev/05408b857941
part 3: Make mozSpellChecker and nsEditorSpellChecker treat TextServicesDocument directly rather than nsITextServicesDocument r=m_kato
https://hg.mozilla.org/integration/autoland/rev/0901d6f8322f
part 4: Get rid of nsITextServicesDocument because it used by nobody r=m_kato
https://hg.mozilla.org/integration/autoland/rev/2e02cfe32f94
part 5: Make nsEditorSpellCheck store mozSpellChecker directly rather than nsISpellChecker r=m_kato
https://hg.mozilla.org/integration/autoland/rev/2398b267741b
part 6: Rename nsEditorSpellCheck to mozilla::EditorSpellCheck and expose its header r=m_kato
https://hg.mozilla.org/integration/autoland/rev/c27c6e2b0494
part 7: Make mozInlineSpellChecker store EditorSpellCheck directly rather than nsIEditorSpellCheck r=m_kato
https://hg.mozilla.org/integration/autoland/rev/d0270ad1df87
part 8: Make EditorBase store inline spell checker as mozInlineSpellChecker rather than nsIInlineSpellChecker r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4ef85ad5bf42
part 9: Create accessors for each member of mozSpellChecker, EditorSpellCheck, mozInlineSpellChecker r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ff3e5fd4c725
part 10: Make EditorBase store TextServicesDocument for its mInlineSpellChecker for avoiding to run for loops to call nsIEditActionListener methods r=m_kato

Comment 36

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/44be45cf31b4
https://hg.mozilla.org/mozilla-central/rev/1e9838519ff1
https://hg.mozilla.org/mozilla-central/rev/05408b857941
https://hg.mozilla.org/mozilla-central/rev/0901d6f8322f
https://hg.mozilla.org/mozilla-central/rev/2e02cfe32f94
https://hg.mozilla.org/mozilla-central/rev/2398b267741b
https://hg.mozilla.org/mozilla-central/rev/c27c6e2b0494
https://hg.mozilla.org/mozilla-central/rev/d0270ad1df87
https://hg.mozilla.org/mozilla-central/rev/4ef85ad5bf42
https://hg.mozilla.org/mozilla-central/rev/ff3e5fd4c725