903871 - JS crash in SelectionHandler during longtap text select on target page

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Mark Capella [:capella]]

Attachment: bugtext

STR:

1) Open page http://www.businessinsider.com/4-billion-year-old-fossil-proteins-resurrected-2013-8
2) long tap word to select

expected: word is selected in orange, handles are displayed

actual  : logcat shows E/GeckoConsole(13463): [JavaScript Error: "[Exception... "Index or size is negative or greater than the allowed amount"  code: "1" nsresult: "0x80530001 (IndexSizeError)"  location: "chrome://browser/content/SelectionHandler.js Line: 576"]" {file: "chrome://browser/content/SelectionHandler.js" line: 524}]

This implicates this line http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#524, which fails since here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#284 we can return to it a value of "", which isn't expected nor coded for in this and several places.

The attached WIP looks to gracefully handle the issue. *the bad news* is I don't understand why the text isn't selected in the first, and I'm still looking at it.

Comment 1

[:Margaret Leibovic]

Comment on attachment 788708 [details] [diff] [review]
bugtext

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

I still need to think about this problem more, but here's a quick drive-by for now...

::: mobile/android/chrome/content/SelectionHandler.js
@@ +200,5 @@
>      }
>  
>      let selection = this._getSelection();
>      // If the range didn't have any text, let's bail
> +    if (!selection || selection == "") {

!"" returns true (you can test that out in the web console), so adding this selection == "" check is redundant.

Comment 2

[Mark Capella [:capella]]

Maybe I've mis-represented something? using this code after retrieving selection:

  _updateCacheForSelection: function sh_updateCacheForSelection(aIsStartHandle) {
    Cu.reportError("_updateCacheForSelection(" + aIsStartHandle + ")");

    let selection = this._getSelection();
    Cu.reportError("_updateCacheForSelection() selection[" + selection + "]");
    Cu.reportError("_updateCacheForSelection() !selection[" + !selection + "]");
    Cu.reportError("_updateCacheForSelection() selection == empty [" + (selection == "") + "]");

With the failing text selection I see:
    _updateCacheForSelection() selection[]
    _updateCacheForSelection() !selection[false]
    _updateCacheForSelection() selection == empty [true]

With a valid selection of the work |speaking| on a different page I see:
    _updateCacheForSelection() selection[speaking]
    _updateCacheForSelection() !selection[false]
    _updateCacheForSelection() selection == empty [false]

Comment 3

[Mark Capella [:capella]]

Ah ... forgot I was dealing with XPCOM and C++ again ... for this .rangeCount() == 0 might be the trick

Comment 4

[Mark Capella [:capella]]

It seems that for that particular website, nsDOMWindowUtils::SelectAtPoint() calls nsFrame::SelectByTypeAtPoint() which bails early, as it has nsISelectionController::SELECTION_OFF.

I guess this is design, (though I can mouse select text on that page on desktop).

Comment 5

[Mark Capella [:capella]]

Attachment: bug903871

I think this is all this needs ... you'll see one question I left in the patch re: error logging if no selection is available ... which seems to be a normal possibility.

Comment 6

[:Margaret Leibovic]

Comment on attachment 789060 [details] [diff] [review]
bug903871

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ +197,5 @@
>      }
>  
>      let selection = this._getSelection();
> +    // If the range didn't have any text, do we need to log?
> +    if (selection.rangeCount == 0) {

I think we still want to check !selection to make sure there isn't an empty selection. We definitely do want to always call this._onFail if we're bailing on the selection, since that cleans up various SelectionHandler stuff (like observers/event listeners).

However, it's bad if we have to bail here. Something is obviously going wrong somewhere to cause this problem, and I think we should include some platform people in this conversation to see if we can find the root cause of this.

@@ +347,5 @@
>        this._cache.end.y = aY;
>      }
>  
>      let selection = this._getSelection();
> +    if (selection.rangeCount == 0) {

If there isn't a valid selection here, then something is definitely wrong, and we should be catching that earlier (and shutting down the selection if that's the case). You check up above would do that, so you should be able to get rid of this check.

@@ +529,5 @@
>    // param to decide whether the selection has been reversed.
>    _updateCacheForSelection: function sh_updateCacheForSelection(aIsStartHandle) {
>      let selection = this._getSelection();
> +    if (selection.rangeCount == 0) {
> +      return false;

Same thing here - it seems like we shouldn't ever end up here with a selection with no ranges, so we should be doing something to catch this earlier.

Comment 7

[:Margaret Leibovic]

Jim, it looks like the Fennec code is having some problem with selectAtPoint on this one page, and we're ending up with a selection without any ranges. Have you ever seen anything like this?

I confirmed this happens on release, so it's not a regression.

Comment 8

[Mark Capella [:capella]]

fyi it seems to be any page on www.businessinsider.com, so I was assuming there was something like a -moz-user-select: -moz-none involved ...

Also, I think we always get a nsISeleection back ... so checking for rangeCount == 0 s/b valid (?)

Comment 9

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #8)
> fyi it seems to be any page on www.businessinsider.com, so I was assuming
> there was something like a -moz-user-select: -moz-none involved ...
> 
> Also, I think we always get a nsISeleection back ... so checking for
> rangeCount == 0 s/b valid (?)

You're totally correct! I just loaded the page on desktop with Fennec's UA, and they definitely set -moz-user-select: none;

So... let's update the patch to just bail in startSelection and see if that fixes things. I don't think we should end up in any of the other places if we don't have an active selection going on.

Comment 10

[Mark Capella [:capella]]

So if the patch is just to guard when selection.rangeCount == 0 in startSelection, a condition we consider normal, can we lose onFail() and just call _closeSelection in it's place for cleanup? (as _initTargetInfo has added observers/listeners ...)

Comment 11

[Mark Capella [:capella]]

Attachment: bug903871

That would be like this ....

Comment 12

[:Margaret Leibovic]

Comment on attachment 789349 [details] [diff] [review]
bug903871

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

r+ with comment addressed.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +197,5 @@
>      }
>  
>      let selection = this._getSelection();
>      // If the range didn't have any text, let's bail
> +    if (selection.rangeCount == 0) {

We should still watch out for empty selections, so this should be |if (!selection || selection.rangeCount == 0) {|.

An empty selection ("") will still have a rangeCount of 1.

@@ -459,5 @@
> -  _onFail: function sh_onFail(aDbgMessage) {
> -    if (aDbgMessage && aDbgMessage.length > 0)
> -      Cu.reportError("SelectionHandler - " + aDbgMessage);
> -    this._closeSelection();
> -  },

Note to self: I added this when I was trying to move our code towards sharing logic with metro's SelectionHandler. Since we decided it's not worth it to try to do that, it's fine to get rid of this.

Comment 13

[Mark Capella [:capella]]

Great!

I'll make the |if (!selection || selection.rangeCount == 0) {| change.

In my testing I did observe that empty selections returned rangeCount's of 0, despite my best attempt to trick it to think otherwise through manipulation of the selection, manual movement of the handles, and etc.

This solution wfm as it covers both possibilites, so yay ! Us ! \o/

Comment 14

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/09f56cdf27de

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/09f56cdf27de

Comment 16

[u421692]

I can still see this issue on latest Nightly 26.0a1 (2013-08-19).

Comment 17

[Mark Capella [:capella]]

If you're seeing it in your logcat, can you paste the relevent snip?

Comment 18

[u421692]

Attachment: long_tap_logs

Comment 19

[Mark Capella [:capella]]

Thanks! can you describe your STR?

(I think this is a slight variation of the original problem we didn't believe could happen)

Comment 20

[Mark Capella [:capella]]

fyi - we put another change in last night's m-c so todays build would generate that error but on different line numbers.

Co-incidentally, it may have also solved the reason why you're getting that error message in the logcat in the first place.

Comment 21

[u421692]

Adding STR to another scenario:
1. Open http://www.businessinsider.com/4-billion-year-old-fossil-proteins-resurrected-2013-8
2. Long tap on the article text
3. Open another site in a different tab without closing the current tab
4. Long tap on text or a link

Expected result:
The selection handler or the context menu are displayed

Actual result:
At step 2 you can see in logcat a JS error, and the selection handler is not displayed
At step 4 The selection handler or the context menu are not displayed when long taping

Comment 22

[Mark Capella [:capella]]

Ah ... this actually fell out of the solution in bug 864589, the change we implemented after this one.