Fix query cancellation in UnifiedComplete.js.

RESOLVED FIXED in mozilla35

Status

()

Toolkit
Places
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mano, Assigned: mak)

Tracking

unspecified
mozilla35
x86
Mac OS X
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

***
|_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.
Blocks: 995091
Flags: firefox-backlog+
(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
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Hi Marco, can you provide a point value.
Flags: qe-verify?
Flags: needinfo?(mak77)
Points: --- → 2
Flags: qe-verify?
Flags: qe-verify-
Flags: needinfo?(mak77)
Created attachment 8497128 [details] [diff] [review]
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.
Attachment #8497128 - Flags: review?(mano)
Summary: _addMatch general issues → Clarify query cancellation in UnifiedComplete.js.
Blocks: 1074457
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
Attachment #8497128 - Flags: review?(mano) → review+
Summary: Clarify query cancellation in UnifiedComplete.js. → Fix query cancellation in UnifiedComplete.js.
https://hg.mozilla.org/integration/fx-team/rev/1f8b260d82da
Target Milestone: --- → mozilla35

Updated

3 years ago
Iteration: --- → 35.3
https://hg.mozilla.org/mozilla-central/rev/1f8b260d82da
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.