872195 - Freeze (hang) after opening ics file which has BYMONTHDAY and BYDAY in an RRULE

Status: RESOLVED FIXED

(Calendar :: ICAL.js Integration, defect)

Description

[Stefan Sitter [:ssitter]]

Attachment: testcase

The testcase file from Bug 356207 causes Thunderbird to freeze (hang) using 100% cpu cycles.

STR:
1. Set calendar.icaljs to true, restart Thunderbird
2. Use menu File > Open > Calendar File to subscribe to the attached ics calendar file

Error:
Thunderbird freezes (hangs) using 100% cpu cycles, must be closed forcefully.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Yeah, known issue. I've tried to apply the patch from back then to ical.js, but that didn't fix the bug, or rather it caused other bugs. I kind of hope that James' recurrence parser changes will fix this bug...

Comment 2

[Markus Adrario [:Taraman]]

Attachment: Patch V0.9

This was a hard one since it's two factors coming together.

1.
If BYMONTHDAY=1 then decrementing this.last.day in case of isInit causes the normalization function to set it to lastDayOfMonth
Therefore I introduced an integer copy of it.

2.
Since the date is incremented at the start, the nextMonth() function needs to set it to 0

The patch as is works on the testcase but needs further testing and maybe some comments.

this also fixes two calls to .length on objects.

Comment 3

[Markus Adrario [:Taraman]]

Attachment: Patch V1

I added some comments and did some more testing.
It works in nearly all cases I could imagine.

one known problem:
On rules with more than 2 BYMONTHDAYs, only the first and the last are working.

Since this patch fixes the hang as is, I will file the above problem as a spinoff bug.

Comment 4

[Markus Adrario [:Taraman]]

filed Bug 899770

Comment 5

[James Lal [:lightsofapollo]]

Comment on attachment 783335 [details] [diff] [review]
Patch V1

Particularly since we have a very nice ical sample file here we should write tests covering this failure.

Comment 6

[Markus Adrario [:Taraman]]

For this issue, there already is a test originating from bug 356207 where we had the same problem in libical. 

Do you think that is still sufficient?


[1] http://mxr.mozilla.org/comm-central/source/calendar/test/unit/test_bug356207.js

Comment 7

[Markus Adrario [:Taraman]]

Comment on attachment 783335 [details] [diff] [review]
Patch V1

I verified that the test mentioned in comment #6 fails without the patch an passes with it.

Therefore re-asking for review.

Here also the patch enabling icaljs in xpcshell [1] helps.

[1]: http://hg.mozilla.org/users/Mozilla_Adrario.de/mq-patches/file/efbad47506ee/enable-icaljs-testing.diff

Comment 8

[James Lal [:lightsofapollo]]

Its pretty difficult for me to review patches that are tested on comm-central. Those tests are never run in the ical.js project and we rely on them in gaia.

I don't feel comfortable reviewing a patch that is only tested externally and still think we should have test coverage for all incoming patches to ICAL.js. What do you think @Fallen?

Comment 9

[Markus Adrario [:Taraman]]

I'll have a look into the github ical.js repo an try to make a patch there.

Comment 10

[James Lal [:lightsofapollo]]

Please correct me if I am wrong but this landed right (in the joint pull request + tests) ?

Comment 11

[Markus Adrario [:Taraman]]

you are right.
I left this open.because I plan to add the tests to lightning.

that's the same with the other 2 bugs.

Comment 12

[Markus Adrario [:Taraman]]

This bug is fixed upstream in ical.js.

The tests will go into calendar with Bug 908745

Comment 13

[Stefan Sitter [:ssitter]]

This bug is not fixed in Lightning as long as Lightnings copy of ical.js is not fixed too. Maybe keep the bugs open until the fix is in Lightning too? Same for bug 899326 and bug 899770.

Comment 14

[Markus Adrario [:Taraman]]

The question is, what the exact procedure is, to update ical.js here. If it is done "automatically" somehow, we should keep the bugs open.

If it is done manually, then maybe another bug to track this would be a good thing.

Comment 15

[Martin Schröder [:mschroeder]]

Comment on attachment 783335 [details] [diff] [review]
Patch V1

Landed upstream, removing review request.

Comment 16

[Stefan Sitter [:ssitter]]

Should be fixed by Bug 932217 in Lightning 3.2.