483643 - Strict JavaScript warnings "reference to undefined property" and "assignment to undeclared variable"

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090315 Sunbird/1.0pre [custom release build based on comm-central/mozilla-central]

Steps to Reproduce:
1. get a 1.9.2 based sunbird build
2. set advanced preference "javascript.options.strict" to true
3. restart sunbird and play around in views and dialogs

Actual Results:
The following warning is logged several thousand times:

Warning: reference to undefined property this.mData[aName]
Source file: file:///E:/sb/mozilla/dist/bin/calendar-js/calUtils.js
Line: 1668

Other warnings that are logged once or multiple times:

Warning: trailing comma is not legal in ECMA-262 object initializers
Source file: chrome://calendar/content/widgets/calendar-list-tree.xml
Line: 118, Column: 6
Source code:
      }) 

Warning: assignment to undeclared variable date
Source file: chrome://calendar/content/widgets/minimonth.xml
Line: 435

Warning: assignment to undeclared variable sp
Source file: file:///E:/sb/mozilla/dist/bin/calendar-js/calStorageCalendar.js
Line: 1848

Warning: assignment to undeclared variable ap
Source file: file:///E:/sb/mozilla/dist/bin/calendar-js/calStorageCalendar.js
Line: 2538

Warning: assignment to undeclared variable j
Source file: file:///E:/sb/mozilla/dist/bin/calendar-js/calStorageCalendar.js
Line: 2579

Warning: assignment to undeclared variable calendarEvent
Source file: chrome://calendar/content/calendar-unifinder.js
Line: 785

Warning: assignment to undeclared variable currentListLength
Source file: chrome://calendar/content/import-export.js
Line: 258

Warning: assignment to undeclared variable newcol
Source file: chrome://calendar/content/calendar-multiday-view.xml
Line: 1286

Warning: reference to undefined property this[varname]
Source file: file:///E:/sb/mozilla/dist/bin/calendar-js/calItemBase.js
Line: 1020

Warning: reference to undefined property this.eventPromotedProps[name]
Source file: file:///E:/sb/mozilla/dist/bin/calendar-js/calEvent.js
Line: 189

Warning: reference to undefined property this.mExceptionMap[getRidKey(aRecurrenceId)]
Source file: file:///E:/sb/mozilla/dist/bin/calendar-js/calRecurrenceInfo.js
Line: 744

Warning: reference to undefined property calendarOfflineManager.prototype
Source file: chrome://calendar/content/calendar-management.js
Line: 259

Warning: reference to undefined property label.localName
Source file: chrome://calendar/content/calendar-base-view.xml
Line: 598

Warning: reference to undefined property unifinderObserver.prototype
Source file: chrome://calendar/content/calendar-unifinder.js
Line: 102

Warning: reference to undefined property window.mAlarmService
Source file: chrome://calendar/content/calendar-alarm-dialog.js
Line: 47

Warning: reference to undefined property window.organizer
Source file: chrome://calendar/content/calendar-event-dialog.js
Line: 2105

Warning: reference to undefined property window.organizer
Source file: chrome://calendar/content/calendar-event-dialog.js 
Line: 1230

Warning: reference to undefined property parent.backupPrefList
Source file: chrome://calendar/content/preferences/categories.js
Line: 73

Comment 1

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090420 Calendar/1.0pre (BuildID: 20090420050308).

I now see many (all?) of the warnings in the official 1.9.1 builds too.

Comment 2

[Martin Schröder [:mschroeder]]

Attachment: [checked in] Fix for undefined property label.localName

Easy fix for "Warning: reference to undefined property label.localName
Source file: chrome://calendar/content/calendar-base-view.xml". Just a 'let' was missing. I do not if this patch has visible influence on the calendar views.

Comment 3

[Martin Schröder [:mschroeder]]

Stefan, is there any strict JavaScript warning left after my fix from comment#2?

Comment 4

[Stefan Sitter [:ssitter]]

I still see for example the following warnings:

[JavaScript Warning: "assignment to undeclared variable cachedCalendar" {file:///e:/obj/sb-dbg/mozilla/dist/bin/calendar-js/calCachedCalendar.js" line: 192}]

[JavaScript Warning: "assignment to undeclared variable calendarEvent" {file: "chrome://calendar/content/calendar-unifinder.js" line: 785}]

[JavaScript Warning: "reference to undefined property Components.interfaces.calICachedCalendar" {file:///e:/obj/sb-dbg/mozilla/dist/bin/calendar-js/calCachedCalendar.js" line: 143}]

[JavaScript Warning: "reference to undefined property calendarOfflineManager.prototype" {file: "chrome://calendar/content/calendar-management.js" line: 256}]

[JavaScript Warning: "reference to undefined property label.localName" {file: "chrome://calendar/content/calendar-base-view.xml" line: 602}]

[JavaScript Warning: "reference to undefined property this.eventPromotedProps[name]" {file:///e:/obj/sb-dbg/mozilla/dist/bin/calendar-js/calEvent.js" line: 189}]

[JavaScript Warning: "reference to undefined property this.mTimerMap[calendar.id]" {file: "file:///e:/obj/sb-dbg/mozilla/dist/bin/calendar-js/calAlarmService.js" line: 512}]

[JavaScript Warning: "reference to undefined property this.todoPromotedProps[name]" {file:///e:/obj/sb-dbg/mozilla/dist/bin/calendar-js/calTodo.js" line: 218}]

[JavaScript Warning: "reference to undefined property this[varname]" {file:///e:/obj/sb-dbg/mozilla/dist/bin/calendar-js/calItemBase.js" line: 1021}]

[JavaScript Warning: "reference to undefined property window.mAlarmService" {file: "chrome://calendar/content/calendar-alarm-dialog.js" line: 47}]

[JavaScript Warning: "reference to undefined property window.organizer" {file: "chrome://calendar/content/calendar-event-dialog.js" line: 1239}]

Comment 5

[Stefan Sitter [:ssitter]]

Comment on attachment 380126 [details] [diff] [review]
[checked in] Fix for undefined property label.localName

r=ssitter

Comment 6

[Martin Schröder [:mschroeder]]

Comment on attachment 380126 [details] [diff] [review]
[checked in] Fix for undefined property label.localName

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/e2d3efb4e7df>

Comment 7

[Martin Schröder [:mschroeder]]

Attachment: Fix a bunch of strict warnings

Here is a patch for many warnings that you can easily encounter when having javascript.options.strict enabled. Most of the time, there is just a 'let' missing. But I also fixed some indentation and whitespace issues. A lot of the 'reference to undefined property' warnings could be solved when checking '"x" in y' to see if y[x] or y.x exists. When there is a return value needed I use the ternary operator, and I am not always sure if I choose the correct return value in case the property does not exist for an object, so please have a deeper look there.

Some remarks:

> [JavaScript Warning: "reference to undefined property
> Components.interfaces.calICachedCalendar"
> {file:///e:/obj/sb-dbg/mozilla/dist/bin/calendar-js/calCachedCalendar.js" line:
> 143}]

We have no interface for cached calendars. Removing "Components.interfaces.calICachedCalendar" from the array in QueryInterface would fix this. Or do we need to define a interface for other implementations of cached calendars?


> [JavaScript Warning: "reference to undefined property
> calendarOfflineManager.prototype" {file:
> "chrome://calendar/content/calendar-management.js" line: 256}]

Is singleton, and therefore has to be removed!


> [JavaScript Warning: "reference to undefined property
> this.eventPromotedProps[name]"
> {file:///e:/obj/sb-dbg/mozilla/dist/bin/calendar-js/calEvent.js" line: 189}]

Returns false using ternary operator now!


> [JavaScript Warning: "reference to undefined property
> this.todoPromotedProps[name]"
> {file:///e:/obj/sb-dbg/mozilla/dist/bin/calendar-js/calTodo.js" line: 218}]

Returns false using ternary operator now!


> [JavaScript Warning: "reference to undefined property this[varname]"
> {file:///e:/obj/sb-dbg/mozilla/dist/bin/calendar-js/calItemBase.js" line:
> 1021}]

Returns null using ternary operator now!

Comment 8

[Daniel Boelzle [:dbo]]

Comment on attachment 380760 [details] [diff] [review]
Fix a bunch of strict warnings

>diff --git a/calendar/base/content/calendar-alarm-> function getAlarmService() {
>-    if (!window.mAlarmService) {
>+    if (!(mAlarmService in window) || !window.mAlarmService) {
I think this is slightly wrong; |if (!("mAlarmService" in window))| should do it.

>     if (ok) {
>-        var calMgr = getCalendarManager();
>+        let calMgr = getCalendarManager();
cal.getCalendarManager()?

> function onChangeCalendar(calendar) {
>     var args = window.arguments[0];
>-    var organizer = args.organizer;
>+    var organizer = ("organizer" in args ? args.organizer: null);
|let organizer = (args.organizer || null)| IMO reads better.

>-    args.organizer = window.organizer && window.organizer.clone();
>+    args.organizer = (("organizer" in window && window.organizer) ? window.organizer.clone() : null);
dto here, e.g.
|args.organizer = (window.organizer || null) && window.organizer.clone()|

>     if (ok) {
>-        var child;  
>+        var child;
let?

>-    item.organizer = window.organizer;
>+    item.organizer = ("organizer" in window ? window.organizer : null);
dto

>     isPropertyPromoted: function (name) {
>-        return (this.eventPromotedProps[name]);
>+        // avoid strict undefined property warning
>+        return (name in this.eventPromotedProps ? this.eventPromotedProps[name] : false);
|return (this.eventPromotedProps[name] || false);|

> function makeMemberAttr(ctor, varname, dflt, attr, asProperty) {
>     // XXX handle defaults!
>     var getter = function () {
>         if (asProperty)
>             return this.getProperty(varname);
>         else
>-            return this[varname];
>+            return (varname in this ? this[varname] : null);
dto

>     isPropertyPromoted: function (name) {
>-        return (this.todoPromotedProps[name]);
>+        // avoid strict undefined property warning
>+        return (name in this.todoPromotedProps ? this.todoPromotedProps[name] : false);
dto

apart from that (though without testing the changes): r=dbo

Comment 9

[Martin Schröder [:mschroeder]]

Attachment: [checked in] Fix warning for undeclared variable gCalItemsArrayFound

Warning: assignment to undeclared variable gCalItemsArrayFound
Source File: chrome://lightning/content/imip-bar.js
Line: 233

The declaration is the only use of the variable, so we can remove it.db

Comment 10

[Daniel Boelzle [:dbo]]

Comment on attachment 381344 [details] [diff] [review]
[checked in] Fix warning for undeclared variable gCalItemsArrayFound

oh, leftovers from the pre-overhaul code..
thx, r=dbo

Comment 11

[Martin Schröder [:mschroeder]]

Comment on attachment 381344 [details] [diff] [review]
[checked in] Fix warning for undeclared variable gCalItemsArrayFound

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/e70ca8d00475>

Comment 12

[Martin Schröder [:mschroeder]]

Attachment: [checked in] Fix a bunch of strict warnings v2

(In reply to comment #8)
> (From update of attachment 380760 [details] [diff] [review])

> > function onChangeCalendar(calendar) {
> >     var args = window.arguments[0];
> >-    var organizer = args.organizer;
> >+    var organizer = ("organizer" in args ? args.organizer: null);
> |let organizer = (args.organizer || null)| IMO reads better.

> >-    args.organizer = window.organizer && window.organizer.clone();
> >+    args.organizer = (("organizer" in window && window.organizer) ? window.organizer.clone() : null);
> dto here, e.g.
> |args.organizer = (window.organizer || null) && window.organizer.clone()|

> 
> >-    item.organizer = window.organizer;
> >+    item.organizer = ("organizer" in window ? window.organizer : null);
> dto

This doesn't work, so I decided to add |window.organizer = null;| in 
onLoad(), and leave everything else as is.

Daniel, can you have another quick look at this?

Comment 13

[gekacheka]

> diff --git a/calendar/base/src/calItemBase.js b/calendar/base/src/calItemBase.js
> --- a/calendar/base/src/calItemBase.js
> +++ b/calendar/base/src/calItemBase.js
> @@ -1013,17 +1013,17 @@
>   *                        member. 
>   */
>  function makeMemberAttr(ctor, varname, dflt, attr, asProperty) {
>      // XXX handle defaults!
>      var getter = function () {
>          if (asProperty)
>              return this.getProperty(varname);
>          else
> -            return this[varname];
> +            return (this[varname] || null);
>      };

This seems wrong: it will turn values like 0 (e.g., PRIORITY) or ""
into null.  Also, shouldn't it be using the default 'dflt' value?
               return (varname in this ? this[varname] : dflt);

Comment 14

[Daniel Boelzle [:dbo]]

(In reply to comment #12)
> This doesn't work, so I decided to add |window.organizer = null;| in 
> onLoad(), and leave everything else as is.
which is OK, but being curious: why? We've fixed similar lookups e.g. calRecurrenceInfo.js this way. Does |let organizer = (args["organizer"] || null);| work?

(In reply to comment #13)
> >  function makeMemberAttr(ctor, varname, dflt, attr, asProperty) {
> >      // XXX handle defaults!
> >      var getter = function () {
> >          if (asProperty)
> >              return this.getProperty(varname);
> >          else
> > -            return this[varname];
> > +            return (this[varname] || null);
> >      };
> 
> This seems wrong: it will turn values like 0 (e.g., PRIORITY) or ""
> into null.  Also, shouldn't it be using the default 'dflt' value?
>                return (varname in this ? this[varname] : dflt);
Right. Though I am wondering why we run into that warning at all, because we handle all attributes as property (with the exception of attribute properties which seems to be correctly initialized).

Comment 15

[gekacheka]

> (In reply to comment #13)
> > >  function makeMemberAttr(ctor, varname, dflt, attr, asProperty) {
> > >      // XXX handle defaults!
> > >      var getter = function () {
> > >          if (asProperty)
> > >              return this.getProperty(varname);
> > >          else
> > > -            return this[varname];
> > > +            return (this[varname] || null);
> > >      };
> > 
> > This seems wrong: it will turn values like 0 (e.g., PRIORITY) or ""
> > into null.  Also, shouldn't it be using the default 'dflt' value?
> >                return (varname in this ? this[varname] : dflt);
> Right. Though I am wondering why we run into that warning at all, because we
> handle all attributes as property (with the exception of attribute properties
> which seems to be correctly initialized).

Not all calls to makeMemberAttr pass true for asProperty.
http://mxr.mozilla.org/comm-central/search?string=makeMemberAttr&find=calendar
shows calls in calAttendee.js

[efficiency nit at this code: wouldn't it be faster to check the asProperty outside the function, and return one of two functions, so the 'if' doesn't have to be executed every call?  Or is the compiler smart enough to detect this is a constant and omit the branch?]

Comment 16

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 381509 [details] [diff] [review]
[checked in] Fix a bunch of strict warnings v2

Looks good, r=philipp

Comment 17

[Martin Schröder [:mschroeder]]

(In reply to comment #14)
> (In reply to comment #12)
> > This doesn't work, so I decided to add |window.organizer = null;| in 
> > onLoad(), and leave everything else as is.
> which is OK, but being curious: why? We've fixed similar lookups e.g.
> calRecurrenceInfo.js this way. Does |let organizer = (args["organizer"] ||
> null);| work?

No, it doesn't.

Comment 18

[Martin Schröder [:mschroeder]]

Comment on attachment 381509 [details] [diff] [review]
[checked in] Fix a bunch of strict warnings v2

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/61c62b2d7ca7> without changes to calItemBase.js

Comment 19

[Martin Schröder [:mschroeder]]

Daniel, what solution for 'makeMemberAttr' do you prefer?

(a) return (varname in this ? this[varname] : null);

(b) return (varname in this ? this[varname] : dflt);

What else needs to be taken care of if we start using the default value?

Comment 20

[Daniel Boelzle [:dbo]]

I am not sure whether we should use the default value, but leave it to client code to fall back as in the current code. See also a discussion in bug 367229 about this (maybe we could close it if we come to a consensus). Thus I'd vote to just return null and remove the |dflt| parameter.

Comment 21

[Martin Schröder [:mschroeder]]

Attachment: makeMemberAttr without default value

Here is a patch for removing the default value.

Comment 22

[gekacheka]

Attachment: [checked in] patch v1: avoid warning in makeMemberAttr getter

Removing the default values seems outside of the scope of this bug, maybe that part can go in bug 367229, where discussion proposes an alternative place for them.
There should be some way to get defaulted values so that people writing extensions don't all have to provide their own default values (opportunity for inconsistency).

For the purpose of this bug (avoiding warning), this patch avoids the warning (on New Event > Invite Attendees) but does not change the current behavior (returning undefined).

Comment 23

[Martin Schröder [:mschroeder]]

Comment on attachment 385559 [details] [diff] [review]
[checked in] patch v1: avoid warning in makeMemberAttr getter

Daniel is the appropriate reviewer.

Comment 24

[Daniel Boelzle [:dbo]]

Comment on attachment 385559 [details] [diff] [review]
[checked in] patch v1: avoid warning in makeMemberAttr getter

I've asked before, but why do we need to use the ternary operator instead of |return (this[varname] || undefined)|?

You've fixed a warning in calRecurrenceInfo.js that way (http://mxr.mozilla.org/comm-central/source/calendar/base/src/calRecurrenceInfo.js#748) and I like it.

r=dbo anyway

Comment 25

[gekacheka]

(In reply to comment #24)
> (From update of attachment 385559 [details] [diff] [review])
> I've asked before, but why do we need to use the ternary operator instead of
> |return (this[varname] || undefined)|?

That would return undefined for properties whose value is 0, which seems wrong, as I wrote in comment 13.  It might cause round-trip dataloss.  For example. undefined percent complete (no percent-complete property in the ICS) could mean progress info is not available; 0 percent complete could mean progress info is available and the task has not been started.

Comment 26

[Daniel Boelzle [:dbo]]

Yes, right. I forgot about that.

Comment 27

[Martin Schröder [:mschroeder]]

Comment on attachment 385559 [details] [diff] [review]
[checked in] patch v1: avoid warning in makeMemberAttr getter

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/035fc7c4ed38>

Comment 28

[Stefan Sitter [:ssitter]]

I think this bug can be resolved as FIXED. Further strict warnings can be reported with new bugs.