890130 - Background thumbnails should timeout based on when item is processed rather than when queued.

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

The background thumbnail service currently starts the timeout for an item when it is added to the queue rather than when it gets to the head of the queue and starts being processed.  This means if a large number of items are queued, there is a real risk items will timeout before they even start being captured.  The timeout should be applied from the time the item starts being processed.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Start timeout when capture starts

A fairly straight forward patch, although the test timeoutQueueing() might need some tweaking (and hence only the f+ request).  That test required a fairly trivial change, but now seems overly complex for the simple semantics that are now in place (ie, only the "current" capture can currently timeout as only one capture can be in progress).  So maybe this test should be gutted?  Or not? :)

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 771946 [details] [diff] [review]
Start timeout when capture starts

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

You also need to update BackgroundPageThumbs._onCaptureOrTimeout so that it dequeues the queue (and throws an error if the dequeued capture is not the capture passed to the method).

::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ +241,5 @@
>     * The only intended external use of this method is by the service when it's
>     * uninitializing and doing things like destroying the thumbnail browser.  In
>     * that case the consumer's completion callback will never be called.
>     *
>     * This method is not idempotent.  It's an error to call it more than once on

This paragraph in the comment isn't true anymore, so it can be removed.

@@ +253,2 @@
>      if (this._msgMan) {
>        // The capture may have never started, so _msgMan may be undefined.

A similar comment now applies to _timeoutTimer, so add a comment to the top of destroy's body along the lines of, "This method can be called on captures that haven't started yet."?

::: toolkit/components/thumbnails/test/browser_thumbnails_background.js
@@ +91,1 @@
>    function timeoutQueueing() {

Yeah, timeoutQueueing can be removed altogether.  You might add a timed out capture to the middle of the queueing test's queue though, just to verify that the queue is correctly processed after a timeout.  You'd have to tweak that test a little to support timeouts, but it shouldn't be hard.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated based on feedback

> You also need to update BackgroundPageThumbs._onCaptureOrTimeout so that it
> dequeues the queue (and throws an error if the dequeued capture is not the
> capture passed to the method).

I'm afraid I didn't quite understand this comment, but did notice that this will now only ever be called when |capture| is at the head of the queue - which is probably what you meant.  So please see new changes here.

All other comments addressed.

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 773784 [details] [diff] [review]
Updated based on feedback

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

(In reply to Mark Hammond (:markh) from comment #3)
> I'm afraid I didn't quite understand this comment, but did notice that this
> will now only ever be called when |capture| is at the head of the queue -
> which is probably what you meant.

Yeah.

::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ +179,4 @@
>     */
>    _onCaptureOrTimeout: function (capture) {
> +    // Since timeouts start as an item is being processed, only the first
> +    // item in the queue can be cancelled via this method.

I wouldn't use the word "cancelled" here since canceling implies intervention to stop an ongoing task, and that's not what's happening here, which could confuse readers.  "... will be passed to this method"?  "will have completed"?

@@ +179,5 @@
>     */
>    _onCaptureOrTimeout: function (capture) {
> +    // Since timeouts start as an item is being processed, only the first
> +    // item in the queue can be cancelled via this method.
> +    if (capture !== this._captureQueue[0]) 

Nit: please remove the trailing space on this line.

@@ +181,5 @@
> +    // Since timeouts start as an item is being processed, only the first
> +    // item in the queue can be cancelled via this method.
> +    if (capture !== this._captureQueue[0]) 
> +      throw new Error("The capture should be at the head of the queue.");
> +    this._captureQueue.splice(0, 1);

this._captureQueue.shift();

@@ +242,4 @@
>     */
>    destroy: function () {
> +    // This method may be called for captures that haven't started yet, so
> +    // guard against not yet having _timeoutTimer, _msgMan etc attributes...

Nit: attributes -> properties

Comment 5

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be6ee216cb17

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/be6ee216cb17