951943 - SelectionHandler._updateCacheForSelection() Error: "TypeError: rects[0] is undefined" at line: 726

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

Attachment: BUG

This may have been Bug 766651 ... in my current test it's at line 726

testcase: |data:text/plain,This is a test|

Long tap word to select it, open action bar
Tap SelectAll in action bar
   Observe left handle visible, right handle way off screen to the right
   (different ... bug 947284)

Drag left handle to middle of empty screen and release

Observe logcat error message, and left handle now floating on screen

Comment 1

[Wesley Johnston (:wesj)]

Comment on attachment 8349786 [details] [diff] [review]
BUG

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

Still have some questions before I r+.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +141,5 @@
>              selection.extend(anchorNode, anchorOffset);
>            }
>            // Act on selectionChange notifications after handle movement ends
>            this._ignoreSelectionChanges = false;
> +          if (this._getSelectionStatus() != this.TYPE_NONE) {

This seems to imply that _activeType is some how out of sync with the selection? I don't quite follow why that happened.

@@ +194,5 @@
> +   * Returns a determination for what our internal _activeSelection status should be
> +   * After we've observed a final "TextSelection:Position", "TextSelection:Action",
> +   * Or appropriate notifySelectionChanged() event
> +   */
> +  _getSelectionStatus: function sh_getSelectionStatus() {

This again, implies that _activeType is wrong. Do we need this? Can we just keep _activeType better updated?

@@ +761,5 @@
>    // Returns true if the selection has been reversed. Takes optional aIsStartHandle
>    // param to decide whether the selection has been reversed.
>    _updateCacheForSelection: function sh_updateCacheForSelection(aIsStartHandle) {
> +    let rects = this._getSelection().getRangeAt(0).getClientRects(); // Isn't this expensive?
> +    if (!rects[0]) {

I think you're better to just test rects.length > 0 here

@@ +763,5 @@
>    _updateCacheForSelection: function sh_updateCacheForSelection(aIsStartHandle) {
> +    let rects = this._getSelection().getRangeAt(0).getClientRects(); // Isn't this expensive?
> +    if (!rects[0]) {
> +      // nsISelection object exists, but there's no user visible "orange stuff"
> +      Cu.reportError("SelectionHandler: _updateCacheForSelection() Warning: nsISelection object exists, but there's no user visible \"orange stuff\"");

"selection" instead of "orange stuff" :)

Comment 2

[Mark Capella [:capella]]

re: |This again, implies that _activeType is wrong. Do we need this? Can we just keep _activeType better updated?|

I'm confused by this question. The _activeType can change from between the moment the user starts dragging a handle and the moment they let it go.

If the let the handle go, while draggin into a location such that the real-time selection no longer exists, then releases the handle, we have an invalid selection at the end.

Now you might actually wonder, should we be closing it, or simply re-presenting the original selection in that case.

I chose the simple, "user asked for it - user got it" solution, but this can change.

Checking chrome behaviour with the "data:text/plain, this is a test" example, I see they're different in that the handle is never allow into an invalid position in the first place, so the selection is always maintained in one way or another.

Comment 3

[:Margaret Leibovic]

If the _activeType changes, that means the handle type would need to change, and I don't think that should ever happen while the user is dragging a handle.

I'm not really a fan of this _getSelectionStatus method, since it adds another layer of tricky logic to our text selection code (I'm sorry I didn't push back harder in previous patches, I've grown to feel more strongly about this over the past week).

It seems like the real problem here is that we're in a broken state with the right handle way off to the right, and that's how we end up in this bad state with an orphaned left handle. What we really just need here is more graceful error handling.

A solution I would prefer here is to add this check in _updateCacheForSelection like you're currently doing, but just report the error and call _closeSelection in the event that we got into a borked state.

(I was able to reproduce this on all channels, so I'm setting the flags here appropriately. I'm having a hard time keeping track of the state of all these different text selection bugs, let's make sure we're diligent about keeping track of which release version are affected by them.)

Comment 4

[:Margaret Leibovic]

Comment on attachment 8349786 [details] [diff] [review]
BUG

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

This is too complicated. Let's come up with a more straightforward solution.

Comment 5

[Mark Capella [:capella]]

Yes, this is a demonstrable issue on Release.

I'm not sure how it could be fixed more easily, so I wait further review.

Comment 6

[Mark Capella [:capella]]

Attachment: bug951943

This revision fixes the |left handle now floating on screen| issue from the original comment, in a more streamlined manner, (closer to what you suggest in comment #3).

Comment 7

[:Margaret Leibovic]

Comment on attachment 8356639 [details] [diff] [review]
bug951943

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ -149,2 @@
>          }
> -        this._positionHandles();

You should just leave this as-is, since this change doesn't change the behavior here.

@@ +755,5 @@
>    _updateCacheForSelection: function sh_updateCacheForSelection(aIsStartHandle) {
> +    let rects = this._getSelection().getRangeAt(0).getClientRects();
> +    if (!rects[0]) {
> +      // nsISelection object exists, but there's nothing actually selected
> +      return null;

Returning null like this feels fishy, especially since startSelection isn't the only place that _updateCacheForSelection is called, and those consumers don't check the return value, but expect the cache to get updated properly. I think it would be better to just call closeSelection in here and throw an exception. We can catch that exception in startSelection and return early like this patch is doing, but this solution will also make sure we don't get into a busted state from the other _updateCacheForSeleciton calls.

Comment 8

[Mark Capella [:capella]]

Attachment: bug951943 (v2)

re: |Returning null like this feels fishy|

   :D I don't like throwing exceptions, but I agree returning null didn't feel right either, so I've changed it.

re: |You should just leave this as-is, since this change doesn't change the behavior here.|

   Given the recent small edge-case bugs, I've been trying to bullet-proof as much as possible. Currently, if  receives an errant event while not in a valid selection state, the code will call this._positionHandles(). I wanted to close that hole. I can leave that change out if you don't think it's worth considering.

This patch catches the handleEvent("TextSelection:Position") exception now thrown from _updateCacheForSelection().

The three other consumers of _updateCacheForSelection() are:
   observe("after-viewport-change");
   startSelection();
   subdocumentScrolled();

I don't think a non-existent selection could exist in any of these situations ... nothing could be thrown or be caught in those cases, (If so, I'd believe we'd be seeing it already), so I didn't try/catch the code around them. If the new thrown exception starts showing up in logcat we can fix it then on a separate bug / avoid scope creep (?)

Comment 9

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #8)

> re: |You should just leave this as-is, since this change doesn't change the
> behavior here.|
> 
>    Given the recent small edge-case bugs, I've been trying to bullet-proof
> as much as possible. Currently, if  receives an errant event while not in a
> valid selection state, the code will call this._positionHandles(). I wanted
> to close that hole. I can leave that change out if you don't think it's
> worth considering.

So you're saying we receive "TextSelection:Position" events when this._activeType is TYPE_NONE? If that's the case, that's a bug, and I'd rather we notice it than silently eat it. If this is something you've been noticing, perhaps we should add another else case that reports this invalid state.

> This patch catches the handleEvent("TextSelection:Position") exception now
> thrown from _updateCacheForSelection().
> 
> The three other consumers of _updateCacheForSelection() are:
>    observe("after-viewport-change");
>    startSelection();
>    subdocumentScrolled();
> 
> I don't think a non-existent selection could exist in any of these
> situations ... nothing could be thrown or be caught in those cases, (If so,
> I'd believe we'd be seeing it already), so I didn't try/catch the code
> around them. If the new thrown exception starts showing up in logcat we can
> fix it then on a separate bug / avoid scope creep (?)

Sounds like a good plan.

Comment 10

[:Margaret Leibovic]

Comment on attachment 8356849 [details] [diff] [review]
bug951943 (v2)

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

r+ with comments addressed.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +145,4 @@
>            // Act on selectionChange notifications after handle movement ends
>            this._ignoreSelectionChanges = false;
> +          this._positionHandles();
> +          break;

Moving these _positionHandles calls is fine if you add an else statement down below to report an invalid state, but you don't need to move these break statements as well, since we'll always just fall down to the break below.

@@ +753,5 @@
>    _updateCacheForSelection: function sh_updateCacheForSelection(aIsStartHandle) {
> +    let rects = this._getSelection().getRangeAt(0).getClientRects();
> +    if (!rects[0]) {
> +      // nsISelection object exists, but there's nothing actually selected
> +      throw "SelectionHandler: TextSelection:Position completes with no valid selection";

Let's make this exception message generic to _updateCacheForSelection, since in theory there could be a future consumer of this that runs into this exception. Maybe something like "Failed to update cache for invalid selection".

Comment 11

[Mark Capella [:capella]]

Nice!

re: |So you're saying we receive "TextSelection:Position" events when this._activeType is TYPE_NONE|.

I'm not sure I've explained it right, but, we receive "TextSelection:Move" messages while |(this._activeType == this.TYPE_SELECTION)| and the user is dragging a left/right handle around. The selection dynamically changes as this occurs.

When she releases her finger, a final mouseup is generated that triggers a "TextSelection:Position" to us. We're still in |(this._activeType == this.TYPE_SELECTION)| status when we receive it.

Then, at the end of the processing that event, we need to look at the final selection status which may now be invalid, based on where the user positioned the handle when it was released.

In the example here, there's a single line of text. If you highlight the entire selection, then drag the left handle around, (generating a stream of "TextSelection:Move" messages) the selection changes onscreen as the handle moves. If you move that left handle right, past the end of the right handle, then release it, we get the final "TextSelection:position" message.

During this._updateCacheForSelection(), we fail doing |let rects = selection.getRangeAt(0).getClientRects();| because we're received the event while |(this._activeType == this.TYPE_SELECTION)|, but user placement of the handle now invalidates that, as no selection/.clientRect now exists.

Comment 12

[Mark Capella [:capella]]

Attachment: Screenshot bug generated by STR

Comment 13

[Mark Capella [:capella]]

With changes requested for final approval ...

https://tbpl.mozilla.org/?tree=Try&rev=128924c361e4

Comment 14

[Mark Capella [:capella]]

and in to fx-team for the final bugsquish

https://hg.mozilla.org/integration/fx-team/rev/69a6c6c99c85

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/69a6c6c99c85