364580 - asynchronous provider -> fast traveling freezes the weekview

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Andreas Treumann]

REPRODUCTION:
=============

- switch to weekview
- use the arrow button for fast forward/reward traveling
- after some fast mouse clicks the calendar view hangs


RESULT:
=======

- after some fast mouse clicks the calendar view hangs
- selecting a day in the calender view unfreezes the view  

EXPECTED RESULT:
================

- no freeze 

REPRODUCIBLE:
=============

- always

Comment 1

[Andreas Treumann]

Andreas: Oh, I forgot that this happens only with asynchron calendars, like WCAP, sorry.

Comment 2

[Daniel Boelzle [:dbo]]

js console outputs:

Error: this.selectedDay has no properties
Source File: chrome://calendar/content/calendar-decorated-week-view.xml
Line: 183

Comment 4

[cmtalbert]

[e-mail] also discovered this problem when using only CalDav calendars too, which was reported in bug 365646

Comment 5

[Michael Büttner (no reviews TFN)]

taking this one.

Comment 6

[Michael Büttner (no reviews TFN)]

unfortunately this is a regression from bug #329035. traveling through different weeks results in calls to calendar-multiday-view::setDateRange() which calls calendar-multiday-view::refresh() which in turn rebuilds the internal list of dates that are requested to be displayed. bug #329035 introduced that rebuilding is delayed until any previously scheduled requests have been finished. thus findColumnForDate() can't find the requested date under these circumstances which leads to the above described issue.

Comment 7

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

This patch resolves the problem by calling relayout() whether or not there are pending refresh-requests instead of waiting for previous requests to finish. I also added the missing call to the proper location in calendar-month-view since it suffers from the same problem, even if it did not cause any trouble so far.

Comment 8

[Matthew (lilmatt) Willis]

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

Adding ui-review request to make sure this is indeed the behaviour we want in this situation.

Comment 9

[Joey Minta]

Why not just clear the mRefreshPending flag when setDateRange is called?  That seems to me like it would solve this problem and avoid the big performance hit with synchronous providers that this patch would require.

Comment 10

[Michael Büttner (no reviews TFN)]

(In reply to comment #9)
> Why not just clear the mRefreshPending flag when setDateRange is called?  That
> seems to me like it would solve this problem and avoid the big performance hit
> with synchronous providers that this patch would require.
This would re-introduce bug #329035 as it would completely circumvent the request queue. But you're right that the proposed patch involves a performance hit for synchronous providers. It would be better to call relayout() only if there are request jobs in the queue, something like this:

            if(this.mRefreshQueue.length > 0) {
              this.relayout();
            }

This would update the mDateColumns-array before any request to send down to the providers and ignore calling relayout in case the request queue has been finished. Any comments on this?

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

I'm quite confused here. It sound to me that the problem is that data for old date ranges is coming in after the daterange has been changed. I would that that this data should just be ignored. I don't see the need to force it to redraw.

Comment 12

[Michael Büttner (no reviews TFN)]

(In reply to comment #11)
> I'm quite confused here. It sound to me that the problem is that data for old
> date ranges is coming in after the daterange has been changed. I would that
> that this data should just be ignored. I don't see the need to force it to
> redraw.
Obviously I didn't make this clear enough ;-) The problem is not the old data is coming in after the daterange has been updated, but that the daterange hasn't been updated but new data is coming in expecting the daterange to be up to date. What actually went wrong is that the call to relayout() has been defered due to the fact that we need to take asynchronous providers into account.

Comment 13

[Michael Büttner (no reviews TFN)]

Attachment: patch v2

addresses previous comments.

Comment 14

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 251929 [details] [diff] [review]
patch v2

What are the implications of this patch?
It's quite hard to tell from the code what this patch will do to both the sync case and the async case, from a viewpoint of the user.

Comment 15

[Matthew (lilmatt) Willis]

Comment on attachment 251929 [details] [diff] [review]
patch v2

I'm letting jminta and mvl figure out the "if we want to take this patch part".

Only nit: add a space between the if and the (

r=lilmatt
</buck passed>

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

I really can't oversee the implications of this patch. So, i repeat my questions:
What does this do for the users of an sync calendar? Will it speed up the views or will it slow them down? And how about async calendars?

Comment 17

[Michael Büttner (no reviews TFN)]

(In reply to comment #16)
> What does this do for the users of an sync calendar? Will it speed up the views
> or will it slow them down? And how about async calendars?
Obviously I didn't do a good job making myself clear. To answer this specific question, this patch doesn't affect performance at all. Bug #329035 and this one take care about the issue that methods get called in the right order, even in case asynchronous providers take part in the game.

I'm going to explain what this is all about, but i'll restrict this discussion to the multiday view since all of this applies to the other views as well.

Views get refreshed by calling refresh(). this happens under various circumstances, e.g. moveView() -> goToDay() -> setDateRange() -> refresh().

refresh() calls relayout() which in turn updates the internal components of the view. an important role plays the mDateColumns array which contains the specific dates that the view displays. after calling relayout() getItems() on the composite calendar gets called, thus each calendar receives a getItems() call. thus for each calendar mOperationListener.onGetResult() gets called plus mOperationListener.onOperationComplete() after the operation has been completed.

all of this works great until asynchronous providers enter the game. in this case onGetResult() could be called with an unspecified delay. it could well be possible that refresh() gets called again before past getItems() calls are finished and are notified through onGetResult(). to prevent this issue i introduced a queue in bug #329035. in case previous requests didn't finished before relayout() gets called again the request is put into a queue. in case the previous request finishes the next job is fetched from the queue and executed.

with this queue in place the re-entrancy problems are solved. unfortunately one problem remains in case relayout() gets called before the queue is empty (previous requests have been finished) - the mDateColumns array is not up to date. and this exactly what this patch is about to address. in case a request is pending (result has not been received) and there's at least another job waiting we're calling relayout(). this ensures that the mDateColumns array is up to date. this scenario happens immediately in case we're traveling from week to week with an asynchronous provider in place. in case the user switches to another week before the current request has been finished we're calling relayout() in order to update the mDateColumns to reflect the dates of the week we're switching to. the previously submitted request (which we didn't receive the result for) will eventually be received later but won't do any harm since those items won't find the date columns (since the mDateColumns array already reflects the new dates).

the conclusion of what i tried to explain is that with synchronous providers we don't see any difference whatsoever. with asynchronous providers we're just calling the methods delayed, no performance penalty is involved.

Comment 18

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 251929 [details] [diff] [review]
patch v2

Thanks for your explenation. Patch looks good.
ui-review+r2=mvl

One thing i do wonder is if instead of a queue, we need to cancel previous requests (or just ignore the results) if the user has already switched to a different timerange. But that's for a future bug.

Comment 19

[Michael Büttner (no reviews TFN)]

Checked in on trunk and MOZILLA_1_8_BRANCH