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

RESOLVED FIXED in mozilla34

Status

()

Core
Editor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla34
x86_64
Windows 8.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 4 obsolete attachments)

Created attachment 8472116 [details] [diff] [review]
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++
Depends on: 1052343
Created attachment 8472133 [details] [diff] [review]
Flusing frames at the start of nsEditor::EndPlaceHolderTransaction()

This is the minimum patch for reproducing this bug.
Okay, probably, I understand the cause. Currently, testing on tryserver:
https://tbpl.mozilla.org/?tree=Try&rev=9663b8fa1ccc
Assignee: nobody → masayuki
Created attachment 8472970 [details] [diff] [review]
test of this bug
Attachment #8472116 - Attachment is obsolete: true
Attachment #8472133 - Attachment is obsolete: true
Attachment #8472970 - Flags: review?(ehsan)
Created attachment 8472975 [details] [diff] [review]
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.
Attachment #8472975 - Flags: review?(ehsan)
Status: NEW → ASSIGNED

Comment 5

4 years ago
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...
Attachment #8472975 - Flags: review?(ehsan) → review+

Updated

4 years ago
Attachment #8472970 - Flags: review?(ehsan) → review+
(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.
Flags: needinfo?(ehsan)

Comment 7

4 years ago
(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...
Flags: needinfo?(ehsan)
Okay, I'll recreate it with changing nsIEditor.
Created attachment 8474481 [details] [diff] [review]
Implement nsIEditor.isInEditAction readonly attribute

Let's create nsIEditor::isInEditAction for sharing the state between IMEContentObserver and nsTextEditorState.
Attachment #8474481 - Flags: superreview?(bugs)
Attachment #8474481 - Flags: review?(ehsan)
Comment on attachment 8474481 [details] [diff] [review]
Implement nsIEditor.isInEditAction readonly attribute

Could we make the attribute [infallible]
Attachment #8474481 - Flags: superreview?(bugs) → superreview+
(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].)
Flags: needinfo?(bugs)
Oh, nsIEditor should be builtinclass. That means C++ only implementations.
Flags: needinfo?(bugs)
Comment on attachment 8474481 [details] [diff] [review]
Implement nsIEditor.isInEditAction readonly attribute

Thanks. I'll post a new patch.
Attachment #8474481 - Flags: review?(ehsan)
Created attachment 8474879 [details] [diff] [review]
Implement nsIEditor.isInEditAction readonly attribute

I forgot to update uuid of nsIEditor derived classes (nsIPlaintextEditor and nIHTMLEditor).
Attachment #8474481 - Attachment is obsolete: true
Attachment #8474879 - Flags: superreview?(bugs)
Attachment #8474879 - Flags: review?(ehsan)

Comment 15

4 years ago
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.
Attachment #8474879 - Flags: review?(ehsan) → review+
(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?
Flags: needinfo?(ehsan)

Comment 17

4 years ago
(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.
Flags: needinfo?(ehsan)
(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.
Created attachment 8474911 [details] [diff] [review]
Implement nsIEditor.isInEditAction readonly attribute (r=ehsan)
Attachment #8474879 - Attachment is obsolete: true
Attachment #8474911 - Flags: superreview?(bugs)
Attachment #8474879 - Flags: superreview?(bugs)

Comment 20

4 years ago
BTW: thanks for figuring this bug out! :)
(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.

Updated

4 years ago
Attachment #8474911 - Flags: superreview?(bugs) → superreview+
https://hg.mozilla.org/mozilla-central/rev/e8101d5f9def
https://hg.mozilla.org/mozilla-central/rev/99f640f31359
https://hg.mozilla.org/mozilla-central/rev/77bf46f52906
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34

Comment 24

4 years ago
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.
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

4 years ago
: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.
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.
> If you think the actual regression by it,

If you know... I meant.
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.

Updated

4 years ago
Depends on: 1059705
Or I can make the change too.

Comment 31

4 years ago
Thanks.
You need to log in before you can comment on or make changes to this bug.