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

VERIFIED FIXED in mozilla1.8beta4

Status

defect
VERIFIED FIXED
14 years ago
11 years ago

People

(Reporter: ajschult784, Assigned: ajschult784)

Tracking

({fixed-seamonkey1.1a, fixed1.8.1})

Bug Flags:
blocking1.8b5 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Gv1.8.1/SMv1.1a flags apply to Cv1-18 patch] [verified-seamonkey1.1a])

Attachments

(2 attachments, 7 obsolete attachments)

Assignee

Description

14 years ago
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.
Assignee

Comment 1

14 years ago
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → ajschult
Status: NEW → ASSIGNED
Attachment #188911 - Flags: superreview?(dmose)
Attachment #188911 - Flags: review?(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.
Attachment #188911 - Flags: superreview?(dmose)
Attachment #188911 - Flags: review?(dmose)
Attachment #188911 - Flags: review-
Assignee

Comment 3

14 years ago
Posted patch patch v2 (obsolete) — Splinter Review
review comments addressed
Assignee

Comment 4

14 years ago
Posted patch patch v3Splinter Review
ajschult: i think, really, instead of having the try/catch go 
ajschult: the try/catch should move inside the if
Attachment #188911 - Attachment is obsolete: true
Attachment #189396 - Attachment is obsolete: true
Attachment #189578 - Flags: superreview?(dmose)
Attachment #189578 - Flags: review?(dmose)
Comment on attachment 189578 [details] [diff] [review]
patch v3

r+sr=dmose
Attachment #189578 - Flags: superreview?(dmose)
Attachment #189578 - Flags: superreview+
Attachment #189578 - Flags: review?(dmose)
Attachment #189578 - Flags: review+
Assignee

Comment 6

14 years ago
Comment on attachment 189578 [details] [diff] [review]
patch v3

low risk change to silence warnings
Attachment #189578 - Flags: approval1.8b4?

Updated

14 years ago
Attachment #189578 - Flags: approval1.8b4? → approval1.8b4+
Assignee

Comment 7

14 years ago
FIXED
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Posted patch (Bv1) <MsgComposeCommands.js> (obsolete) — Splinter Review
[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).
Attachment #192677 - Flags: superreview?(mscott)
Attachment #192677 - Flags: review?(mscott)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: --- → mozilla1.8beta4
(In reply to comment #8)
> and a lot of space++ SM<->TB syncing (no feature change).

but porting patch v3 to TB !
Flags: blocking1.8b4?

Updated

14 years ago
Assignee: ajschult → dmose
Status: REOPENED → NEW
Flags: blocking1.8b4? → blocking1.8b4-
Serge: can you attach a "diff -uwp" here so that it's easy to tell what's
changed other than whitespace?
OS: Linux → All
Hardware: PC → All
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.
Flags: blocking-seamonkey1.0a?
Assignee: dmose → gautheri
(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.
Flags: blocking-seamonkey1.0a?

Comment 14

14 years ago
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).
Summary: debug build too chatty with ldap disabled → debug build too chatty with ldap disabled (JS errors in MsgComposeCommands.js)
Attachment #192677 - Flags: superreview?(mscott)
Attachment #192677 - Flags: superreview?(dmose)
Attachment #192677 - Flags: review?(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.
Attachment #192677 - Flags: superreview?(dmose)
Attachment #192677 - Flags: review?(dmose)
Attachment #192677 - Flags: review-
Attachment #192677 - Flags: review?(mscott)
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.
Attachment #192858 - Attachment is obsolete: true
Attachment #193988 - Flags: review?(dmose)
Comment on attachment 193988 [details] [diff] [review]
(Bv2) <MsgComposeCommands.js> (-Bpuw to help review)

r=dmose
Attachment #193988 - Flags: review?(dmose) → review+
Bv2 "full diff".

Keeping
{{
(Bv2) <MsgComposeCommands.js> (-Bpuw to help review)	 patch	       
2005-08-26 16:38 PDT	13.28 KB	dmose: review+ 
}}
Attachment #192677 - Attachment is obsolete: true
Attachment #194219 - Flags: superreview?(bienvenu)
Attachment #194219 - Flags: review+

Comment 19

14 years ago
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?
Attachment #194219 - Flags: superreview?(bienvenu) → superreview+
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...
Attachment #194219 - Flags: approval1.8b4?
Attachment #193988 - Attachment is obsolete: true
Comment on attachment 194219 [details] [diff] [review]
(Bv2) <MsgComposeCommands.js>
[Checkin: Comment 22]

please never, ever again nominate a warning fix. thanks.
Attachment #194219 - Flags: approval1.8b4? → approval1.8b4-
Status: NEW → ASSIGNED
Comment on attachment 194219 [details] [diff] [review]
(Bv2) <MsgComposeCommands.js>
[Checkin: Comment 22]

I just checked this in with bienvenu's comments addressed.
Attachment #194219 - Attachment description: (Bv2) <MsgComposeCommands.js> → (Bv2) <MsgComposeCommands.js> (checked in)
Assignee: gautheri → ajschult
Status: ASSIGNED → NEW
Status: NEW → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050901 SeaMonkey/1.1a]
(nightly) (W98SE)

V.Fixed.
Status: RESOLVED → VERIFIED
(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...
(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 on attachment 194219 [details] [diff] [review]
(Bv2) <MsgComposeCommands.js>
[Checkin: Comment 22]

'approval-branch-1.8.1=?':
Trivial code cleanup, no risk.
Attachment #194219 - Flags: approval-branch-1.8.1?(mscott)

Updated

13 years ago
Attachment #194219 - Flags: approval-branch-1.8.1?(mscott) → approval-branch-1.8.1+
Comment on attachment 194219 [details] [diff] [review]
(Bv2) <MsgComposeCommands.js>
[Checkin: Comment 22]

'approval1.8.0.2=?':
Trivial code fix (and cleanup), no risk.
Attachment #194219 - Flags: approval1.8.0.2?
Attachment #194219 - Attachment description: (Bv2) <MsgComposeCommands.js> (checked in) → (Bv2) <MsgComposeCommands.js> [Checkin: Comment 22]
Attachment #194219 - Attachment is obsolete: true
Attachment #194219 - Flags: approval1.8.0.2?
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.
Attachment #217714 - Flags: approval1.8.0.3?
Attachment #217714 - Flags: approval-branch-1.8.1+
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
Attachment #217714 - Flags: approval1.8.0.3? → approval1.8.0.3-
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.
Attachment #217714 - Attachment is obsolete: true
Attachment #218282 - Flags: approval1.8.0.3?
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 }
Attachment #217714 - Attachment description: (Cv1-18) <MsgComposeCommands.js> → (Cv1-18) <MsgComposeCommands.js> [Checkin: Comment 31]
(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.
Whiteboard: [Gv1.8.1/SMv1.1a flags apply to Cv1-18 patch] [verified-seamonkey1.1a]
This is seamonkey-only code, should get seamonkey-council 1.8.0.3 approval. KaiRo? is this OK?

Comment 34

13 years ago
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 on attachment 218282 [details] [diff] [review]
(Cv2-SMv10) <MsgComposeCommands.js>

Can't wait any longer for an answer, try for next release
Attachment #218282 - Flags: approval1.8.0.4? → approval1.8.0.4-
(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.
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.