1053048 - Accessing selectionStart or selectionEnd from nsISelectionListener::NotifySelectionChanged() may cause cancelling the edit action

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Accessing selectionStart from mozilla::IMEContentObserver::NotifySelectionChanged()

spinning off from bug 1026997.

1. Load the URL with a build applied the attached patch.
2. Click blank area of the <textarea>
3. Press Enter key

Line break should be inserted but actually not inserted.

The stack trace of calling nsISelectionListener::NotifySelectionChanged():

>  	xul.dll!mozilla::IMEContentObserver::NotifySelectionChanged(nsIDOMDocument * aDOMDocument, nsISelection * aSelection, short aReason)  Line 375	C++
>  	xul.dll!nsFrameSelection::NotifySelectionListeners(short aType)  Line 2018 + 0xb3 bytes	C++
>  	xul.dll!nsFrameSelection::EndBatchChanges()  Line 2007 + 0xc bytes	C++
>  	xul.dll!mozilla::dom::Selection::EndBatchChanges()  Line 5621	C++
>  	xul.dll!nsEditor::EndUpdateViewBatch()  Line 4134	C++
>  	xul.dll!nsEditor::EndPlaceHolderTransaction()  Line 951	C++
>  	xul.dll!nsAutoPlaceHolderBatch::~nsAutoPlaceHolderBatch()  Line 36 + 0x16 bytes	C++
>  	xul.dll!nsPlaintextEditor::TypedText(const nsAString_internal & aString, nsPlaintextEditor::ETypingAction aAction)  Line 433 + 0x16 bytes	C++
>  	xul.dll!nsPlaintextEditor::HandleKeyPressEvent(nsIDOMKeyEvent * aKeyEvent)  Line 402 + 0x11 bytes	C++
>  	xul.dll!nsEditorEventListener::KeyPress(nsIDOMEvent * aKeyEvent)  Line 500	C++

Flushing layout is here:

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLElement.cpp?rev=a0778b3a1c04#1090

>>	xul.dll!nsGenericHTMLElement::GetFormControlFrame(bool aFlushFrames)  Line 1090	C++
>  	xul.dll!mozilla::dom::HTMLTextAreaElement::GetSelectionRange(int * aSelectionStart, int * aSelectionEnd)  Line 765	C++
>  	xul.dll!mozilla::dom::HTMLTextAreaElement::GetSelectionStart(mozilla::ErrorResult & aError)  Line 648 + 0xf bytes	C++
>  	xul.dll!mozilla::dom::HTMLTextAreaElement::GetSelectionStart(int * aSelectionStart)  Line 641	C++
>  	xul.dll!mozilla::IMEContentObserver::NotifySelectionChanged(nsIDOMDocument * aDOMDocument, nsISelection * aSelection, short aReason)  Line 375	C++

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Flusing frames at the start of nsEditor::EndPlaceHolderTransaction()

This is the minimum patch for reproducing this bug.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Okay, probably, I understand the cause. Currently, testing on tryserver:
https://tbpl.mozilla.org/?tree=Try&rev=9663b8fa1ccc

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: test of this bug

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Fix

In the normal path, methods related to this bug are called as following order:

1. nsEditor::EndPlaceHolderTransaction()
2. selection listeners
3. nsTextInputListener::EditAction()
4. flushing layout
5. nsTextEditorState::UnbindFromFrame(nsTextControlFrame* aFrame)

In the #3, the <textarea> or <input>'s value is modified and marked as "modified". However, at reproducing this bug, the order is:

1. nsEditor::EndPlaceHolderTransaction()
2. selection listeners
3. flushing layout
4. nsTextEditorState::UnbindFromFrame(nsTextControlFrame* aFrame)
5. nsTextInputListener::EditAction()

Therefore, at detaching the editor, the modified value is ignored since following mTextCtrlElement->ValueChanged() returns false.
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp?rev=a0778b3a1c04#1846

This is the reason why the old value is back at creating new editor for reframe.

This patch ensures to call nsTextInputListener::EditAction() when nsTextEditorState::UnbindFromFrame() is called before it.

Comment 5

[(no longer active)]

Comment on attachment 8472975 [details] [diff] [review]
Fix

Review of attachment 8472975 [details] [diff] [review]:
-----------------------------------------------------------------

I don't like this approach very much, but I can't think of a better way to fix this...

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> I don't like this approach very much, but I can't think of a better way to
> fix this...

If you don't like mInEditAction, nsIEditor can have similar method (e.g., nsIEditor::IsHandlingEditAction()). But nsEditor needs to manage it with similar flag but it can be shared with IMEContentObserver and some other addons. If you like this approach better, I'll post new patch.

Comment 7

[(no longer active)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #5)
> > I don't like this approach very much, but I can't think of a better way to
> > fix this...
> 
> If you don't like mInEditAction, nsIEditor can have similar method (e.g.,
> nsIEditor::IsHandlingEditAction()). But nsEditor needs to manage it with
> similar flag but it can be shared with IMEContentObserver and some other
> addons. If you like this approach better, I'll post new patch.

Hmm, yes, I think that would be a bit better...

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Okay, I'll recreate it with changing nsIEditor.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Implement nsIEditor.isInEditAction readonly attribute

Let's create nsIEditor::isInEditAction for sharing the state between IMEContentObserver and nsTextEditorState.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8474481 [details] [diff] [review]
Implement nsIEditor.isInEditAction readonly attribute

Could we make the attribute [infallible]

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8474481 [details] [diff] [review]
> Implement nsIEditor.isInEditAction readonly attribute
> 
> Could we make the attribute [infallible]

Adding [infallible] cases bustage:

> 0:17.14 xpidl.IDLError: error: [infallible] attributes are only allowed on [builtinclass] interfaces, ../../../dist/idl\nsIEditor.idl line 557:24
> 0:17.14   [infallible] readonly attribute boolean isInEditAction;

Should I make nsIEditor as [builtinclass]? (I don't know the meaning of [buildinclass].)

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Oh, nsIEditor should be builtinclass. That means C++ only implementations.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8474481 [details] [diff] [review]
Implement nsIEditor.isInEditAction readonly attribute

Thanks. I'll post a new patch.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Implement nsIEditor.isInEditAction readonly attribute

I forgot to update uuid of nsIEditor derived classes (nsIPlaintextEditor and nIHTMLEditor).

Comment 15

[(no longer active)]

Comment on attachment 8474879 [details] [diff] [review]
Implement nsIEditor.isInEditAction readonly attribute

Review of attachment 8474879 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsEditor.cpp
@@ +5341,5 @@
> +nsEditor::GetIsInEditAction(bool* aIsInEditAction)
> +{
> +  if (NS_WARN_IF(!aIsInEditAction)) {
> +    return NS_ERROR_INVALID_ARG;
> +  }

Once you've marked this as [noscript] you can drop this check, since then only our internal code can call this function.

::: editor/nsIEditor.idl
@@ +554,5 @@
> +   * True if an edit action is being handled (in other words, between calls of
> +   * nsIEditorObserver::BeforeEditAction() and nsIEditorObserver::EditAction()
> +   * or nsIEditorObserver::CancelEditAction().  Otherwise, false.
> +   */
> +  [infallible] readonly attribute boolean isInEditAction;

Please mark this [noscript] as well.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #15)
> Comment on attachment 8474879 [details] [diff] [review]
> Implement nsIEditor.isInEditAction readonly attribute
> 
> Review of attachment 8474879 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: editor/libeditor/nsEditor.cpp
> @@ +5341,5 @@
> > +nsEditor::GetIsInEditAction(bool* aIsInEditAction)
> > +{
> > +  if (NS_WARN_IF(!aIsInEditAction)) {
> > +    return NS_ERROR_INVALID_ARG;
> > +  }
> 
> Once you've marked this as [noscript] you can drop this check, since then
> only our internal code can call this function.

Sounds strange. Isn't this check needed for invalid C++ call?

> ::: editor/nsIEditor.idl
> @@ +554,5 @@
> > +   * True if an edit action is being handled (in other words, between calls of
> > +   * nsIEditorObserver::BeforeEditAction() and nsIEditorObserver::EditAction()
> > +   * or nsIEditorObserver::CancelEditAction().  Otherwise, false.
> > +   */
> > +  [infallible] readonly attribute boolean isInEditAction;
> 
> Please mark this [noscript] as well.

Hmm, I think that this may be useful attribute for some addons, dom/inputmetnods (B2G IME) and Gaia. Do you really think that this should be noscript?

Comment 17

[(no longer active)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #15)
> > Comment on attachment 8474879 [details] [diff] [review]
> > Implement nsIEditor.isInEditAction readonly attribute
> > 
> > Review of attachment 8474879 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: editor/libeditor/nsEditor.cpp
> > @@ +5341,5 @@
> > > +nsEditor::GetIsInEditAction(bool* aIsInEditAction)
> > > +{
> > > +  if (NS_WARN_IF(!aIsInEditAction)) {
> > > +    return NS_ERROR_INVALID_ARG;
> > > +  }
> > 
> > Once you've marked this as [noscript] you can drop this check, since then
> > only our internal code can call this function.
> 
> Sounds strange. Isn't this check needed for invalid C++ call?

Well, invalid C++ doing this will just crash, which we will hopefully catch during testing.  :-)  But if you feel uncomfortable feel free to ignore that part of my comment, I don't feel very strongly about it.

> > ::: editor/nsIEditor.idl
> > @@ +554,5 @@
> > > +   * True if an edit action is being handled (in other words, between calls of
> > > +   * nsIEditorObserver::BeforeEditAction() and nsIEditorObserver::EditAction()
> > > +   * or nsIEditorObserver::CancelEditAction().  Otherwise, false.
> > > +   */
> > > +  [infallible] readonly attribute boolean isInEditAction;
> > 
> > Please mark this [noscript] as well.
> 
> Hmm, I think that this may be useful attribute for some addons,
> dom/inputmetnods (B2G IME) and Gaia. Do you really think that this should be
> noscript?

Yes.  Any add-on which relies on this is relying on our implementation detail.  Also, I really think we should not expose more scriptable editor APIs unless there is a great reason to do so.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #17)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #15)
> > > Comment on attachment 8474879 [details] [diff] [review]
> > > Implement nsIEditor.isInEditAction readonly attribute
> > > 
> > > Review of attachment 8474879 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: editor/libeditor/nsEditor.cpp
> > > @@ +5341,5 @@
> > > > +nsEditor::GetIsInEditAction(bool* aIsInEditAction)
> > > > +{
> > > > +  if (NS_WARN_IF(!aIsInEditAction)) {
> > > > +    return NS_ERROR_INVALID_ARG;
> > > > +  }
> > > 
> > > Once you've marked this as [noscript] you can drop this check, since then
> > > only our internal code can call this function.
> > 
> > Sounds strange. Isn't this check needed for invalid C++ call?
> 
> Well, invalid C++ doing this will just crash, which we will hopefully catch
> during testing.  :-)  But if you feel uncomfortable feel free to ignore that
> part of my comment, I don't feel very strongly about it.

Okay, then, I'll use MOZ_ASSERT().

> > > ::: editor/nsIEditor.idl
> > > @@ +554,5 @@
> > > > +   * True if an edit action is being handled (in other words, between calls of
> > > > +   * nsIEditorObserver::BeforeEditAction() and nsIEditorObserver::EditAction()
> > > > +   * or nsIEditorObserver::CancelEditAction().  Otherwise, false.
> > > > +   */
> > > > +  [infallible] readonly attribute boolean isInEditAction;
> > > 
> > > Please mark this [noscript] as well.
> > 
> > Hmm, I think that this may be useful attribute for some addons,
> > dom/inputmetnods (B2G IME) and Gaia. Do you really think that this should be
> > noscript?
> 
> Yes.  Any add-on which relies on this is relying on our implementation
> detail.  Also, I really think we should not expose more scriptable editor
> APIs unless there is a great reason to do so.

Okay.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Implement nsIEditor.isInEditAction readonly attribute (r=ehsan)

Comment 20

[(no longer active)]

BTW: thanks for figuring this bug out! :)

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #20)
> BTW: thanks for figuring this bug out! :)

You're welcome ;-) This makes TSF confused due to the different result from the text and selection change notification.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77bf46f52906
https://hg.mozilla.org/integration/mozilla-inbound/rev/99f640f31359
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8101d5f9def

Comment 23

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/e8101d5f9def
https://hg.mozilla.org/mozilla-central/rev/99f640f31359
https://hg.mozilla.org/mozilla-central/rev/77bf46f52906

Comment 24

[Kent James (:rkent)]

There have been some comments on the tb-planning list concerning the change making nsIEditor [builtinclass].

I don't really know if that is a good idea or not, but because this has some major impact on  certain Thunderbird addons, we deserve more discussion of rationale beyond "Should I make nsIEditor as [builtinclass]? (I don't know the meaning of [buildinclass]) ... Oh, nsIEditor should be builtinclass."

Yes, I know, you'll ask "What are your use cases?" But the whole point of extendable code is that it allows people to do use cases that were not considered when the original code was written. It seems to me that the onus is on the person who removes a capability that others have relied on to justify the change.

Again, I am not saying it is a good or bad idea, I just would like for addon authors to understand what problem is being solved that justifies the pain they are being caused. Hopefully the discussion will be open-minded enough that the option of backing out the change would also be considered seriously.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

rkent:

See comment 12. buldinclass meants it's only implemented by C++. Anyway, it's almost impossible to implement editor with JS. So, there must be no use case.

Comment 26

[Kent James (:rkent)]

:masayuki

"buildinclass meants it's only implemented by C++" is a definition of what that does, not an explanation of why it is needed in this case.

"Anyway, it's almost impossible to implement editor with JS. So, there must be no use case."

Now that you know that one of the most important Thunderbird extensions, written by an official Mozilla Mailnews peer, has a use case, I would think you would start to question your assumption "there must be no use case". "it's almost impossible to implement (an interface) with JS" still permits the use where a js front end to a C++ implementation is written. Obviously there is a use case or we would not be complaining.

All that I am asking for is that you state the reasons why this change had to be made, and be open minded about possibly reverting that change if that reason turns out to be unnecessary. I don't think that is too much to ask.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

rkent:

It's possible to revert the change. If you think the actual regression by it, please file a bug. Here is not a good place for discussing it.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

> If you think the actual regression by it,

If you know... I meant.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

nsIEditor implemented in JS? That is very surprising to me, and obviously didn't know that
when reviewing.
We can revert the change pretty easily though. Just tiny bit uglier C++ code, but that is fine.

Masayuki, do you think you could make the change. And add a comment to nsIEditor that there
are actually addons implementing it in JS.
rkent, thanks for bringing this up.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Or I can make the change too.

Comment 31

[Kent James (:rkent)]

Thanks.