1011059 - crash in java.lang.IllegalArgumentException: invalid selection notification range: 0 to #, length: # at org.mozilla.gecko.GeckoEditable.onSelectionChange(GeckoEditable.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-bf84fd03-bf33-43cf-a9d3-a3d9a2140515.
=============================================================

java.lang.IllegalArgumentException: invalid selection notification range: 0 to 116776, length: 310
	at org.mozilla.gecko.GeckoEditable.onSelectionChange(GeckoEditable.java:790)
	at org.mozilla.gecko.GeckoAppShell.notifyIMEChange(GeckoAppShell.java:466)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:345)
	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:178)

Reported by conversation at: https://twitter.com/robodesign/status/466977159710797824

Comment 1

[Aaron Train [:aaronmt]]

> "Firefox always crashes when I try to select all of the text in the editor at HTMLPad.org"

Comment 2

[Aaron Train [:aaronmt]]

I crashed too

E/GeckoAppShell(18495): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1780 ("Gecko")
E/GeckoAppShell(18495): java.lang.IllegalArgumentException: invalid selection notification range: 0 to 116776, length: 310
E/GeckoAppShell(18495): 	at org.mozilla.gecko.GeckoEditable.onSelectionChange(GeckoEditable.java:815)
E/GeckoAppShell(18495): 	at org.mozilla.gecko.GeckoAppShell.notifyIMEChange(GeckoAppShell.java:475)
E/GeckoAppShell(18495): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
E/GeckoAppShell(18495): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
E/GeckoAppShell(18495): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:354)
E/GeckoAppShell(18495): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:174)

Selected all text in the default EtherPad generated, and then I hit copy on the actionbar

Comment 3

[Mihai Sucan [:msucan]]

Aaron, thanks for reporting the bug for me. The provided steps for reproducing the crash seem to work for you. I thought it might be related to the SwiftKey keyboard i have installed. 

My device is an LG d686 with android 4.1. please let me know if any further information is needed.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Aaron - Is Fx29 unaffected? Do we have any ideas why htmlpad.org is crashing? Is this specific to htmlpad.org, or do we see this crash in other similar sites?


Jim - does this look similar to any other bugs you are tracking?

Comment 5

[Aaron Train [:aaronmt]]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Aaron - Is Fx29 unaffected? Do we have any ideas why htmlpad.org is
> crashing? Is this specific to htmlpad.org, or do we see this crash in other
> similar sites?

29 is affected.

> Selected all text in the default EtherPad generated, and then I hit copy on the actionbar

Correction, merely selecting all text in any EtherPad will crash.

E.g, http://etherpad.mozilla.org/fennec, create the document, select all of the default text

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Aaron - Is Fx29 unaffected? Do we have any ideas why htmlpad.org is
> crashing? Is this specific to htmlpad.org, or do we see this crash in other
> similar sites?
> 
> 
> Jim - does this look similar to any other bugs you are tracking?

Nope. Taking this one.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Use editor root as selection root (v1)

One of the problems is, for a designMode editor, the "selection root" is the whole document but the "editor root" is the body element. The code in ContentEventHandler assumes the selection is always under the editor root, but when we call nsISelectionController::SelectAll, the whole document is selected, outside of the body element.

I think we should always use the editor root as the selection root if we have an editor. It makes things more consistent, and makes nsISelectionController::SelectAll behave the same way as nsIEditor::SelectAll.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Only reset selection if there is selection, when setting ancestor limiter

When setting the selection ancestor limiter, we want any existing selection to relocate to within the limiter. However, if there is no existing selection, we should just skip this step. This patch fixes a reftest failure when applying the first patch.

Comment 9

[(no longer active)]

Comment on attachment 8431651 [details] [diff] [review]
Use editor root as selection root (v1)

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

I think this will affect event handling as well (I cannot convince myself whether this is correct...)

Comment 10

[(no longer active)]

Comment on attachment 8431654 [details] [diff] [review]
Only reset selection if there is selection, when setting ancestor limiter

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

This part seems less controversial.  ;-)

Comment 11

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

Comment on attachment 8431651 [details] [diff] [review]
Use editor root as selection root (v1)

Not sure yet whether this patch is ok, but it is certainly not ok for branches, 
so if we need to fix this bug there too, some safer approach needs to be found.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8431651 [details] [diff] [review]
Use editor root as selection root (v1)

> nsIContent*
> nsINode::GetSelectionRootContent(nsIPresShell* aPresShell)
> {
>   NS_ENSURE_TRUE(aPresShell, nullptr);
> 
>-  if (IsNodeOfType(eDOCUMENT))
>-    return static_cast<nsIDocument*>(this)->GetRootElement();
>+  // For document nodes, return the editor root if the document has an editor.
>+  // If it does not have an editor, return the frame selection root.
>+
>   if (!IsNodeOfType(eCONTENT))
>     return nullptr;

I don't understand this change because if the node is a document node, it always returns nullptr because it must not be a content node. (Or, document nodes have content node bit too?)

And I'm not sure this works fine with contenteditable editor case.

Anyway, you should add tests for:

* designMode document
* <html contenteditable>
* <body contenteditable>
* <div contenteditable>

At performing "Select All", check the selection range if each of them is expected.

I guess that the safest change here is,

if (IsNodeOfType(eDOCUMENT)) {
  if (IsEditable()) {
    return something; // in design mode
  }
  return static_cast<nsIDocument*>(this)->GetRootElement(); // in non-deisgn mode
}

How about you?

>@@ -382,17 +382,18 @@ nsHTMLEditor::FindSelectionRoot(nsINode *aNode)
> 
>   nsCOMPtr<nsIDocument> doc = aNode->GetCurrentDoc();
>   if (!doc) {
>     return nullptr;
>   }
> 
>   nsCOMPtr<nsIContent> content;
>   if (doc->HasFlag(NODE_IS_EDITABLE) || !aNode->IsContent()) {
>-    content = doc->GetRootElement();
>+    // Use the editor root element as the selection root
>+    content = do_QueryInterface(GetRoot());
>     return content.forget();

It seems that this is risky, but perhaps, we should try it with this approach.

Comment 13

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

Comment on attachment 8431651 [details] [diff] [review]
Use editor root as selection root (v1)

I'm possibly missing something here, but 
I think GetSelectionRootContent for a document node should return the root element (or even better would be document object itself).
In other words, I don't quite understand this change.

Shouldn't we change the other code which doesn't deal with the
current GetSelectionRootContent behavior.

Comment 14

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

Also, scripts may change selection, so why editor root should be the same as selection root?

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Actually only the nsHTMLEditor::FindSelectionRoot change is required to fix this bug, but I changed nsINode::GetSelectionRootContent for consistency.

Right now, for a design mode document, GetSelectionRootContent has the behavior that doc->GetSelectionRootContent() returns doc root, doc->GetBodyElement()->GetSelectionRootContent() returns the body element, and (any node under body)->GetSelectionRootContent() returns the body element. Essentially we have two selection roots (doc root and body) for a single document. It didn't feel quite right to me but maybe it's the intended behavior.

What do you think about only changing nsHTMLEditor::FindSelectionRoot? Right now nsISelectionController::SelectAll and nsIEditor::SelectAll behave differently for a design mode document (the former selects doc root and the latter selects only the body). Is that correct behavior?

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8431651 [details] [diff] [review]
Use editor root as selection root (v1)

I wait next patch or review request.

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Use editor rather than selection controller to select all (v1)

As a workaround, when selecting all text, we should use the editor to do the selection rather than the selection controller. The two have subtle differences between them and the editor's behavior is what we want here. The selection controller causes the selection to go beyond what the IME code expects and we are likely to crash.

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Don't remove all ranges when selecting (v1)

We shouldn't remove the selection when starting to select. If we're in an editor, the editor always expects to have a selection, so we shouldn't remove it. If we're outside of an editor, I think subsequent code will reset the selection anyways, so it's not necessary to remove the selection here.

The editor's select-all action fails without this patch.

Comment 19

[:Margaret Leibovic]

Comment on attachment 8438629 [details] [diff] [review]
Use editor rather than selection controller to select all (v1)

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

Looks reasonable to me. I trust you know more than me about the nuances of the editor code :)

Comment 20

[:Margaret Leibovic]

Comment on attachment 8438633 [details] [diff] [review]
Don't remove all ranges when selecting (v1)

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

I believe this was just copy/pasted out of old XUL Fennec code, so I don't even know if it was ever necessary.

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/fx-team/rev/9d24c582f836
https://hg.mozilla.org/integration/fx-team/rev/3cc0b4b7911d

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9d24c582f836
https://hg.mozilla.org/mozilla-central/rev/3cc0b4b7911d

Comment 24

[Aaron Train [:aaronmt]]

Beta uplift?

Comment 25

[(inactive) Jim Chen [:jchen] [:darchons]]

This bug caused some regressions, so it may be better to let it ride the train, unless the crash volume is high on Beta.