490240 - reference to undefined property this.mData[aName], calUtils.js Line 1668

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[gekacheka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b5pre) Gecko/20090425 Shiretoko/3.5b5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b5pre) Gecko/20090426 Calendar/1.0pre

With pref javascript.options.strict true,
get PAGES of warnings:
  reference to undefined property this.mData[aName]
  calendar-js/calUtils.js Line 1668 



Reproducible: Always

Steps to Reproduce:
1. Create new profile.
2. Set pref javascript.options.strict true
(set via Tools > Options > Advanced > General > Config Editor
 search for "strict", double click it to toggle)
3. create 1 new event (in default home calendar) via event dialog
4. restart
5. open error console, view all

Actual Results:  
the warning
  reference to undefined property this.mData[aName]
  calendar-js/calUtils.js Line 1668 
appears 61 times (for 1 event)


Expected Results:  
the warning does not appear.


this can make startup very slow.

workaround is to turn off the preference.

Comment 1

[gekacheka]

Attachment: v1 patch: check if name is present before returning value

(patch -p 1 -l -i file.patch)

Warning occurs at
calPropertyBag.getProperty_(aName)
as called by
calItemBase.getProperty(aName)

Simple fix is to check if the property is in the bag before accessing
it.  (Since this path calls 'toUpperCase' on the property name, it is
not highly optimized, so it should be ok to check aName in this.mData
before accessing this.mData[aName]; js compiler may be able to
optimize any redundancy in key lookup.)

Comment 2

[Daniel Boelzle [:dbo]]

Comment on attachment 374696 [details] [diff] [review]
v1 patch: check if name is present before returning value

>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
>@@ -395,16 +395,18 @@
>     // nsIVariant getProperty(in AString name);
>     getProperty: function cIB_getProperty(aName) {
>         aName = aName.toUpperCase();
>-        var aValue = this.mProperties.getProperty_(aName);
>-        if (aValue === undefined) {
>-            aValue = (this.mIsProxy ? this.mParentItem.getProperty(aName) : null);
>+        if (this.mProperties.hasProperty(aName)) {
>+          return this.mProperties.getProperty(aName);
>+        } else if (this.mIsProxy) {
>+          return this.mParentItem.getProperty(aName);
>+        } else {
>+          return null;
>         }
>-        return aValue;
>     },
Do we really need to change the above code? Does |aValue === undefined| warn in case aValue is undefined?

>     // boolean hasProperty(in AString name);
>     hasProperty: function cIB_hasProperty(aName) {
>-        return (this.getProperty(aName.toUpperCase()) != null);
>+        return this.mProperties.hasProperty(aName.toUpperCase());
>     },
This looks wrong, you only check the direct property bag.

>     // void setProperty(in AString name, in nsIVariant value);
>diff --git a/calendar/base/src/calUtils.js b/calendar/base/src/calUtils.js
>--- a/calendar/base/src/calUtils.js
>+++ b/calendar/base/src/calUtils.js
>@@ -1664,15 +1664,16 @@
>     setProperty: function cpb_setProperty(aName, aValue) {
>         this.mData[aName] = aValue;
>     },
>+    hasProperty: function cpb_hasProperty(aName) {
>+      return aName in this.mData;
>+    },
>     getProperty_: function cpb_getProperty(aName) {
>-        return this.mData[aName];
>+      // avoid strict undefined property warning, can cause slowness
>+      return (aName in this.mData? this.mData[aName] : undefined);
>     },
>     getProperty: function cpb_getProperty(aName) {
>-        var aValue = this.mData[aName];
>-        if (aValue === undefined) {
>-            aValue = null;
>-        }
>-        return aValue;
>+      // avoid strict undefined property warning, can cause slowness
>+      return (aName in this.mData? this.mData[aName] : null);      
please spend a space here          ^ 

Please use 4 spaces indentation instead of 2.

Anybody knows whether the js engine has a setting to specify a warning level or the like for a script? Adding all those extra checks only makes the code slower, but has no benefit.

Comment 3

[gekacheka]

Attachment: v2 patch: check if name is present before returning value

(patch -l -p 1 -i file.patch)

removed unnecessary changes to calItemBase.
(thanks for catching the bug!)

Comment 4

[Daniel Boelzle [:dbo]]

Comment on attachment 374854 [details] [diff] [review]
v2 patch: check if name is present before returning value

>     getProperty_: function cpb_getProperty(aName) {
>-        return this.mData[aName];
>+        // avoid strict undefined property warning
>+        return (aName in this.mData ? this.mData[aName] : undefined);
>     },

I saw you fixed the warning like

(obj[prop] || undefined)

in the other warning patch. If that is equally good to avoid the warning, then we should use it (instead of the ternary operator above). I suspect it's faster.

>     getProperty: function cpb_getProperty(aName) {
>-        var aValue = this.mData[aName];
>-        if (aValue === undefined) {
>-            aValue = null;
>-        }
>-        return aValue;
>+        // avoid strict undefined property warning
>+        return (aName in this.mData ? this.mData[aName] : null);      

dto.

r=dbo anyway

Comment 5

[gekacheka]

preserving javascript false values (such as false, null, "", 0)

Property bag values may be set to javascript false values such as false, null, "", or 0.  Using (obj[prop] || undefined) would convert all these values to undefined, which would be wrong; ditto for any other javascript false value after the ||.  The ternary operator expression in the patch produces the saved value even if it is a javascript false value.

Comment 6

[Martin Schröder [:mschroeder]]

gekacheka, is your patch ready for checkin?

Comment 7

[Stefan Sitter [:ssitter]]

Pushed to comm-central <https://hg.mozilla.org/comm-central/rev/40378d7f3b5a>

--> FIXED