Closed Bug 903871 Opened 7 years ago Closed 7 years ago

JS crash in SelectionHandler during longtap text select on target page

Categories

(Firefox for Android :: Text Selection, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: capella, Assigned: capella)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch bugtext (obsolete) — Splinter Review
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.
Attachment #788708 - Flags: feedback?(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.
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]
Ah ... forgot I was dealing with XPCOM and C++ again ... for this .rangeCount() == 0 might be the trick
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).
Attached patch bug903871 (obsolete) — Splinter Review
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.
Assignee: nobody → markcapella
Attachment #788708 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #788708 - Flags: feedback?(margaret.leibovic)
Attachment #789060 - Flags: review?(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.
Attachment #789060 - Flags: review?(margaret.leibovic) → review-
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.
Flags: needinfo?(jmathies)
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 (?)
(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.
Flags: needinfo?(jmathies)
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 ...)
Attached patch bug903871Splinter Review
That would be like this ....
Attachment #789060 - Attachment is obsolete: true
Attachment #789349 - Flags: review?(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.
Attachment #789349 - Flags: review?(margaret.leibovic) → review+
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/
https://hg.mozilla.org/mozilla-central/rev/09f56cdf27de
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
I can still see this issue on latest Nightly 26.0a1 (2013-08-19).
If you're seeing it in your logcat, can you paste the relevent snip?
Attached file long_tap_logs
Thanks! can you describe your STR?

(I think this is a slight variation of the original problem we didn't believe could happen)
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.
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
Ah ... this actually fell out of the solution in bug 864589, the change we implemented after this one.
You need to log in before you can comment on or make changes to this bug.