1390976 - TEST-UNEXPECTED-FAIL | xpcshell-libical.ini:calendar/test/unit/test_gdata_provider.js | xpcshell return code: 0

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1390852 +++

While bug 1390852 was fixed by adding missing generator *'s, this bug is a lot weirder.

Same regression range
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9ab2470a3210324bc11320531b15d195aa&tochange=6ebc251bd288c268b020815025b05854cc

and most likely also due to bug 1390106.

Looking at a log:
https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1502889714/comm-central_win7_ix_test-xpcshell-bm109-tests1-windows-build6.txt.gz

I see 350 JavaScript errors, like this one:
"CONSOLE_MESSAGE: (info) [JavaScript Error: "[calGoogleSessionManager] Creating session xpcshell" {file: "resource://gdata-provider/modules/gdataSession.jsm" line: 67}]"
07:53:56     INFO -  "CONSOLE_MESSAGE: (info) [JavaScript Error: "[calTimezoneService] Loading resource://calendar/timezones/zones.json"

The corresponding line in gdataSession.jsm is:
cal.LOG("[calGoogleSessionManager] Creating session " + aSessionId);

There many other occurrences like this one, all showing errors from cal.LOG:
"CONSOLE_MESSAGE: (info) [JavaScript Error: "[calTimezoneService] Timezones version 2.2017b loaded" {file: "file:///C:/slave/test/build/application/thunderbird/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calTimezoneService.js" line: 126}]"

The corresponding line in calTimezoneService.js is:
cal.LOG("[calTimezoneService] Timezones version " + this.version + " loaded");

There is also:
"CONSOLE_MESSAGE: (error) [JavaScript Error: "Couldn't find [object Object]" {file: "file:///C:/slave/test/build/application/thunderbird/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calTimezoneService.js" line: 198}]"

from calTimezoneService.js line: 198
cal.ERROR("Couldn't find " + tzid);

So that one really failed.

Aceman, can you see what's going on?

Comment 1

[Jorg K (CEST = GMT+2)]

cal.LOG is here:
https://dxr.mozilla.org/comm-central/rev/89fef90ea52f78ac6e9a8d4b4fd8d157eefacaa4/calendar/base/src/calUtils.js#783

Needless to say that the tree is looking pretty bad due to this.

Comment 2

[Jorg K (CEST = GMT+2)]

Hmm, I ran the test locally by hand
mozilla/mach xpcshell-test calendar/test/unit/test_gdata_provider.js
on a previous version of M-C and it passes, but there are a lot messages like:
"CONSOLE_MESSAGE: (info) [JavaScript Error: "[calStorageCalendar] Timezones have been changed from null to 2.2017b, updating calendar data." {file: "resource://calendar/modules/calStorageUpgrade.jsm" line: 373}]"

So that looks like it's normal. I'll have to rebuild and run the test locally.

Comment 3

[Jorg K (CEST = GMT+2)]

Running it locally the failure that breaks it is:

"CONSOLE_MESSAGE: (error) [JavaScript Error: "[calGoogleCalendar] Adding Item New Event failed:2147500037: undefined" {file: "file:///c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/bin/extensions/%7Ba62ef8ec-5fdc-40c2-873c-223b8a6925cc%7D/components/calGoogleCalendar.js" line: 417}]"

That can also be seen in the logs. The source is:
        }.bind(this), function(e) {
            let code = e.result || Components.results.NS_ERROR_FAILURE;
417:        cal.ERROR("[calGoogleCalendar] Adding Item " + aItem.title +
                      " failed:" + code + ": " + e.message);

And that code is NS_ERROR_FAILURE, so not so useful and most likely originating from the line above.

Could the calendar people please take a look.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1390976.patch - temporarily disable test

Comment 5

[Jorg K (CEST = GMT+2)]

I found the problem:
https://hg.mozilla.org/mozilla-central/rev/92e4bdf39c1f#l5.94

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1390976-take2.patch

This fixes the test.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7711130b207e
Bug 1390852 follow-up: Adjust return values after bug 1390106. rs=bustage-fix

Comment 8

[:aceman]

Thanks, but I do not understand the fix. Hopefully Fallen can look at that.

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #8)
> Thanks, but I do not understand the fix. Hopefully Fallen can look at that.
Neither do I, but it fixes the test and follows: https://hg.mozilla.org/mozilla-central/rev/92e4bdf39c1f#l5.94

Comment 10

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

Comment on attachment 8898037 [details] [diff] [review]
1390976-take2.patch

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

If nobody understands the fix, then maybe it would have been a good idea to have it reviewed before pushing it in without review in the name of bustage fixes.

generator functions in Task.jsm had a syntax-backwards-compatible way of notifying what the result of the task is. This was back when there was not yet support for generator functions with a star and they were only marked as such using yield. Those old generator functions did not support returning a value. This made it work with Gecko 8/9, while still supporting latest central at the time. So to recap the following alerted the same result:

Task.spawn(function() {
  yield somePromise();
  throw new Task.Result("uhm, ok?");
}).then(function(res) { alert(res); });


Task.spawn(function*() {
  yield somePromise();
  return "uhm, ok?";
}).then(function(res) { alert(res); });

Comment 11

[Jorg K (CEST = GMT+2)]

Thanks the the review and explanation.

To answer the first paragraph: Sadly we have so much bustage (yesterday I pushed fix bustage-fix patches) that given uncertain review times, the tree would become unmanageable pretty soon if I waited for review. In many cases, we follow M-C changes, without always fully understanding them. This fix here fixed the test and followed M-C, so it had a 96% chance of being correct. But thanks for stopping by and confirming it. You can stick an r+ onto the missing stars in bug 1390852, too.

Comment 12

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

I totally understand your situation and how hard it is to keep up. If you ever need a quick review due to bustage, please do ping me on IRC and I'm happy to help out with reviews. This specific patch is correct and good job on investigating, let's see if we can find a better balance for future bugfixes.