1041428 - Fix query cancellation in UnifiedComplete.js.

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Asaf Romano (gone)]

***
|_addMatch| is used in three places: |_onResultRow|; "recursively" for frecnecy (see below); and at the end of |execute| to pick up fallback frecency results. However, it throws |StopIteration| (necessary for the |_onResultsRow| case) when there are enough item in places. It seems it's |_onResultRow| that should throw |StopIteration|.

***

    // We keep this array in reverse order, so we can walk it and remove stuff
    // from it in one pass.  Notice that for frecency reverse order means from
    // lower to higher.
    this._frecencyMatches.sort((a, b) => a.frecency - b.frecency);
...

    if (this._frecencyMatches) {
      for (let i = this._frecencyMatches.length - 1;  i >= 0 ; i--) {
        if (this._frecencyMatches[i].frecency > match.frecency) {
          this._addMatch(this._frecencyMatches.splice(i, 1)[0]);
        }
      }
    }

So:
1) What's done here isn't trivial ("Yes, here comes a result, but maybe better results came from some other place while we were waiting for this one, so let's do them first"). Please document the logic.
2) no real harm, but, |_addMatch| for a frecency match also iterates the frecnecy matches array.
3) It seems the only thing that stops this from generating too many results is |StopIteration|. Keep that in mind when addressing the previous issue.
4) I think that a while-loop + Array.pop usage would make this code much easier to follow.
5) The array is sorted, so the iteration-continuation can be conditioned.

***

    // If we didn't find enough matches and we have some frecency-driven
    // matches, add them.
    if (this._frecencyMatches) {
      this._frecencyMatches.forEach(this._addMatch, this);
    }

Hmm, |_frecencyMatches| is lower-to-higher, so this seems wrong, but it isn't breaking anything due to the "recursive" |_addMatch| for frecency matches.

***

With all the above in mind, |_addFrecencyMatch| needs a better name. Or maybe it's |_addMatch|.

***

  /**
   * Used to cancel this search, will stop providing results.
   */
  cancel: function () {
    if (this._sleepTimer)
      this._sleepTimer.cancel();
    if (this._sleepDeferred) {
      this._sleepDeferred.resolve();
      this._sleepDeferred = null;
    }
    delete this._pendingQuery;
  },

  /**
   * Whether this search is running.
   */
  get pending() !!this._pendingQuery,
...
    if (this._result.matchCount == Prefs.maxRichResults || !this.pending) {
      // We have enough results, so stop running our search.
      this.cancel();
      // This tells Sqlite.jsm to stop providing us results and cancel the
      // underlying query.
      throw StopIteration;
    }

1) |cancel| is misleading both by its name and by its documentation. 
2) I'm not sure I understand then "!this.pending" check. |!pending| is possible only once cancel is called.

Comment 1

[Marco Bonardo [:mak]]

(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #0)
> ***
> |_addMatch| is used in three places: |_onResultRow|; "recursively" for
> frecnecy (see below); and at the end of |execute| to pick up fallback
> frecency results. However, it throws |StopIteration| (necessary for the
> |_onResultsRow| case) when there are enough item in places. It seems it's
> |_onResultRow| that should throw |StopIteration|.

this must be addressed, it looks indeed wrong.

> ...
>     if (this._result.matchCount == Prefs.maxRichResults || !this.pending) {
>       // We have enough results, so stop running our search.
>       this.cancel();
>       // This tells Sqlite.jsm to stop providing us results and cancel the
>       // underlying query.
>       throw StopIteration;
>     }
> 
> 1) |cancel| is misleading both by its name and by its documentation. 
> 2) I'm not sure I understand then "!this.pending" check. |!pending| is
> possible only once cancel is called.

We have a pending check now at the beginning of the method so this can be removed.

The other comments are obsolete since _addFrecencyMatch has gone

Comment 2

[Marco Mucci [:MarcoM]]

Hi Marco, can you provide a point value.

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1

I didn't rename cancel cause I don't have better ideas... Though now it should be clearer and it is doing the expected thing.

Comment 4

[Asaf Romano (gone)]

Comment on attachment 8497128 [details] [diff] [review]
patch v1

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

::: toolkit/components/places/UnifiedComplete.js
@@ +635,2 @@
>     */
>    cancel: function () {

s/can/may/

@@ +1058,2 @@
>        // We have enough results, so stop running our search.
> +      // We don't need to notifyResults in this case, cause the main promise

to call notifyResult / to notify results

Comment 5

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/fx-team/rev/1f8b260d82da

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1f8b260d82da