300338 - debug build too chatty with ldap disabled (JS errors in MsgComposeCommands.js)

Status: VERIFIED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Andrew Schultz]

My debug build does not mind reminding me that I have LDAP disabled now and
then.  However when I type an address in mail composition, it complains with
each keystroke.

JavaScript strict warning:
chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 134:
reference to undefined property
Components.classes['@mozilla.org/ldapprefs-service;1']
ERROR: Cannot get the LDAP service
TypeError: Components.classes['@mozilla.org/ldapprefs-service;1'] has no properties
++WEBSHELL == 3
++DOMWINDOW == 3
JavaScript strict warning:
chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 1241:
reference to undefined property args.bodyislink
JavaScript strict warning:
chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 790:
reference to undefined property
Components.classes['@mozilla.org/autocompleteSession;1?type=ldap']
JavaScript strict warning:
chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 790:
reference to undefined property
Components.classes['@mozilla.org/autocompleteSession;1?type=ldap']
JavaScript strict warning:
chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 790:
reference to undefined property
Components.classes['@mozilla.org/autocompleteSession;1?type=ldap']
etc.

Comment 1

[Andrew Schultz]

Attachment: patch

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 188911 [details] [diff] [review]
patch

>Index: MsgComposeCommands.js
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/compose/resources/content/MsgComposeCommands.js,v
>retrieving revision 1.366
>diff -u -8 -p -r1.366 MsgComposeCommands.js
>--- MsgComposeCommands.js	1 Jun 2005 22:48:31 -0000	1.366
>+++ MsgComposeCommands.js	11 Jul 2005 07:19:48 -0000
>@@ -126,18 +126,21 @@ const kComposeAttachDirPrefName = "mail.
> function InitializeGlobalVariables()
> {
>   gAccountManager = Components.classes["@mozilla.org/messenger/account-manager;1"].getService(Components.interfaces.nsIMsgAccountManager);
>   gIOService = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);
>   gPromptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);
>   
>   //This migrates the LDAPServer Preferences from 4.x to mozilla format.
>   try {
>-      gLDAPPrefsService = Components.classes["@mozilla.org/ldapprefs-service;1"].getService();       
>-      gLDAPPrefsService = gLDAPPrefsService.QueryInterface( Components.interfaces.nsILDAPPrefsService);                  
>+      gLDAPPrefsService = Components.classes["@mozilla.org/ldapprefs-service;1"];
>+      if (gLDAPPrefsService) {
>+          gLDAPPrefsService = gLDAPPrefsService.getService()       
>+              .gLDAPPrefsService.QueryInterface( Components.interfaces.nsILDAPPrefsService);                  
>+      }
>   } catch (ex) {dump ("ERROR: Cannot get the LDAP service\n" + ex + "\n");}

Given this change, there's no longer any point in having a try/catch here at 
all.  Get rid of that, and r+sr=dmose.	Also, if you could add a comment in
both places explaining why an if is being used rather than the more usual
try/catch, that'd be great.

Comment 3

[Andrew Schultz]

Attachment: patch v2

review comments addressed

Comment 4

[Andrew Schultz]

Attachment: patch v3

ajschult: i think, really, instead of having the try/catch go 
ajschult: the try/catch should move inside the if

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 189578 [details] [diff] [review]
patch v3

r+sr=dmose

Comment 6

[Andrew Schultz]

Comment on attachment 189578 [details] [diff] [review]
patch v3

low risk change to silence warnings

Comment 7

[Andrew Schultz]

FIXED

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <MsgComposeCommands.js>

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050813
SeaMonkey/1.0a] (nightly) (W98SE)

Fixes regression from patch v3
{{
Warning: reference to undefined property
gLDAPPrefsService.getService().gLDAPPrefsService
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 136
}}
and a lot of space++ SM<->TB syncing (no feature change).

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> and a lot of space++ SM<->TB syncing (no feature change).

but porting patch v3 to TB !

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Serge: can you attach a "diff -uwp" here so that it's easy to tell what's
changed other than whitespace?

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <MsgComposeCommands.js> (-uwpB to help review)

Comment 12

[Mark Banner (:standard8)]

I'm requesting blocking seamonkey-1.0a on this as without the Bv1 patch our ldap
autocomplete won't work for compose (a regression caused by patch 3 on this
bug). We can easily seperate out the fix if necessary, but as the patch is
there, hopefully, it'll be in before 1.0a happens.

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #12)
> I'm requesting blocking seamonkey-1.0a on this as without the Bv1 patch our ldap
> autocomplete won't work for compose (a regression caused by patch 3 on this
> bug). We can easily seperate out the fix if necessary, but as the patch is
> there, hopefully, it'll be in before 1.0a happens.

Ok, my fault, the error doesn't stop autocomplete working - clearing blocking
request.

Comment 14

[OstGote!]

For me Autocomplete stops working completely in compose msg with the error from
comment 8 in Seamonkey trunk build 2005081906 on WinXP (i.e. without Bv1 patch).

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 192858 [details] [diff] [review]
(Bv1) <MsgComposeCommands.js> (-uwpB to help review)

In general, this looks good.  Just a few nits to be addressed:

> @@ -191,10 +189,8 @@ function disableEditableFields()
>   gMsgCompose.editor.flags |= nsIPlaintextEditorMail.eEditorReadonlyMask;
>   var disableElements = document.getElementsByAttribute("disableonsend", "true");
>   for (i=0;i<disableElements.length;i++)
>-  {
>     disableElements[i].setAttribute('disabled', 'true');
>   }
>-}

Having braces around block clauses helps avoid the problem where somebody needs
to insert something in the future, forgets to add the braces, and then causes a
semantic error.  So Id prefer you leave the above clause (and other similar
clauses in this patch) the way they are.

>@@ -1217,20 +1211,17 @@ function ComposeStartup(recycled, aParam
>   else
>     if (window.arguments && window.arguments[0]) {
>       try {
>-      if (window.arguments[0] instanceof Components.interfaces.nsIMsgComposeParams)
>-        params = window.arguments[0];
>-      else
>-        params = handleMailtoArgs(window.arguments[0]);
>+        params = (window.arguments[0] instanceof Components.interfaces.nsIMsgComposeParams)
>+                 ? window.arguments[0]
>+                 : handleMailtoArgs(window.arguments[0]);
>       }

Because the subexpressions here are not entirely trivial, I think this change
makes the code significantly less readable.  So Id prefer it if you would leave
this code (and other instances of this pattern in the patch) the way it is.

>@@ -128,10 +129,13 @@ function InitializeGlobalVariables()
>   gPromptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);
>   
>   //This migrates the LDAPServer Preferences from 4.x to mozilla format.
>+  gLDAPPrefsService = Components.classes["@mozilla.org/ldapprefs-service;1"];
>+  if (gLDAPPrefsService) {
>   try {
>-      gLDAPPrefsService = Components.classes["@mozilla.org/ldapprefs-service;1"].getService();       
>-      gLDAPPrefsService = gLDAPPrefsService.QueryInterface( Components.interfaces.nsILDAPPrefsService);                  
>-  } catch (ex) {dump ("ERROR: Cannot get the LDAP service\n" + ex + "\n");}
>+      gLDAPPrefsService = gLDAPPrefsService
>+          .getService(Components.interfaces.nsILDAPPrefsService);
>+    } catch (ex) {dump ("ERROR: Cannot get the LDAP prefs service\n" + ex + "\n");}
>+  }

If the lookup in Components.classes fails, will an exception be thrown?  If so, 
the if would need to live inside the try.

Comment 16

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2) <MsgComposeCommands.js> (-Bpuw to help review)

Bv1, with comment 15 suggestion(s).

(In reply to comment #15)
> (From update of attachment 192858 [details] [diff] [review] [edit])
> If the lookup in Components.classes fails, will an exception be thrown?

No, |null| is returned is that case.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 193988 [details] [diff] [review]
(Bv2) <MsgComposeCommands.js> (-Bpuw to help review)

r=dmose

Comment 18

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2) <MsgComposeCommands.js> [Checkin: Comment 22]

Bv2 "full diff".

Keeping
{{
(Bv2) <MsgComposeCommands.js> (-Bpuw to help review)	 patch	       
2005-08-26 16:38 PDT	13.28 KB	dmose: review+ 
}}

Comment 19

[David :Bienvenu]

Comment on attachment 194219 [details] [diff] [review]
(Bv2) <MsgComposeCommands.js>
[Checkin: Comment 22]

sr=bienvenu, except:

-  else if (window.arguments && window.arguments[0]) 
-  {
-    try 
-    {
+  else
+    if (window.arguments && window.arguments[0]) {


else if ... on one line is fine - why change that?

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 194219 [details] [diff] [review]
(Bv2) <MsgComposeCommands.js>
[Checkin: Comment 22]

'approval1.8b4=?': (at least for the SM part)
I know it's only a SM warning fix, and some file sync...
Yet this regression was introduced by previous "benjamin: approval1.8b4+"
patch...

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 194219 [details] [diff] [review]
(Bv2) <MsgComposeCommands.js>
[Checkin: Comment 22]

please never, ever again nominate a warning fix. thanks.

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 194219 [details] [diff] [review]
(Bv2) <MsgComposeCommands.js>
[Checkin: Comment 22]

I just checked this in with bienvenu's comments addressed.

Comment 23

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050901 SeaMonkey/1.1a]
(nightly) (W98SE)

V.Fixed.

Comment 24

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> Created an attachment (id=192677) [edit]
> (Bv1) <MsgComposeCommands.js>
> 
> [Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050813
> SeaMonkey/1.0a] (nightly) (W98SE)
> 
> Fixes regression from patch v3
> {{
> Warning: reference to undefined property
> gLDAPPrefsService.getService().gLDAPPrefsService
> Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
> Line: 136
> }}
> and a lot of space++ SM<->TB syncing (no feature change).

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20051213 SeaMonkey/1.0b] (release) (W98SE)

This warning is in the SMv1.0 branch (too/still).

Mnyromyr,
I'd like you to (re)consider the MailNews part of this patch for a=SM10b...

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #24)
> This warning is in the SMv1.0 branch (too/still).
> 
> Mnyromyr,
> I'd like you to (re)consider the MailNews part of this patch for a=SM10b...

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.0.1) Gecko/20060130 SeaMonkey/1.0] (release) (W98SE)

Warning still there...

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 194219 [details] [diff] [review]
(Bv2) <MsgComposeCommands.js>
[Checkin: Comment 22]

'approval-branch-1.8.1=?':
Trivial code cleanup, no risk.

Comment 27

[Serge Gautherie (:sgautherie)]

Comment on attachment 194219 [details] [diff] [review]
(Bv2) <MsgComposeCommands.js>
[Checkin: Comment 22]

'approval1.8.0.2=?':
Trivial code fix (and cleanup), no risk.

Comment 28

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-18) <MsgComposeCommands.js> [Checkin: Comment 31]

Bv2, unbitrotted for MOZILLA_1_8_BRANCH:
due to Mail/326280/v1.72.2.9 and MailNews/320650/v1.369.2.2.

Keeping
{{
(Bv2) <MsgComposeCommands.js> [Checkin: Comment 22]   	 patch   	2005-08-29 12:39 PDT  	38.90 KB  	mscott: approval&#8209;branch&#8209;1.8.1+
}}

'approval1.8.0.3=?':
Trivial code fix (and cleanup), no risk.

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 217714 [details] [diff] [review]
(Cv1-18) <MsgComposeCommands.js>
[Checkin: Comment 31]

Cleanup patches are outside the scope of the 1.8.0 release. denying 1.8.0.3 request

Comment 30

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2-SMv10) <MsgComposeCommands.js>

Cv1-18, reduced to its (SeaMonkey specific) regression fix only,
after email talk with dveditz.

This fixes what looks like an overlooked copy&paste mistake,
which regressed code that had been working for a long time before,
and has not caused any regression after restoration on Trunk and Gv1.8 branch.

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 217714 [details] [diff] [review]
(Cv1-18) <MsgComposeCommands.js>
[Checkin: Comment 31]


Checkin: { 2006-04-14 05:48	bugzilla%standard8.demon.co.uk 	mozilla/mailnews/compose/resources/content/MsgComposeCommands.js 	1.369.2.3 	MOZILLA_1_8_BRANCH }

Comment 32

[Serge Gautherie (:sgautherie)]

(In reply to comment #31)
> (From update of attachment 217714 [details] [diff] [review] [edit])
> 
> Checkin: { 2006-04-14 05:48     bugzilla%standard8.demon.co.uk 
> mozilla/mailnews/compose/resources/content/MsgComposeCommands.js       
> 1.369.2.3       MOZILLA_1_8_BRANCH }

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20060415 SeaMonkey/1.1a] (nightly) (W98SE)

V.Fixed on SMv1.1a.

Comment 33

[Daniel Veditz [:dveditz]]

This is seamonkey-only code, should get seamonkey-council 1.8.0.3 approval. KaiRo? is this OK?

Comment 34

[Robert Kaiser]

Does this fix anything else than just a warning?
Warning fixes, even if trivial, are probably out of the focus of a frozen stablitity/security branch. If it fixes an actual regression and some functionality is broken without this fix, it could be worth to go in...

Comment 35

[Daniel Veditz [:dveditz]]

Comment on attachment 218282 [details] [diff] [review]
(Cv2-SMv10) <MsgComposeCommands.js>

Can't wait any longer for an answer, try for next release

Comment 36

[Serge Gautherie (:sgautherie)]

(In reply to comment #34)
> Does this fix anything else than just a warning?

See comments 13, 14 and 30 ... that's all I know about it.