390842 - Error: redeclaration of var Cc in console, kills Lightning

Status: VERIFIED FIXED

(Calendar :: Lightning Only, defect)

Description

[alta88]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6 XPCOMViewer/0.9.5
Build Identifier: 0.7pref (build 2007080305)

Error: redeclaration of var Cc
Source file: chrome://calendar/content/calUtils.js
Line: 42

any ext can declare const Cc and thus there will be collision. need to do something like:
if (!Cc) var Cc = Components.classes;

Reproducible: Always

Comment 1

[Stefan Sitter [:ssitter]]

I don't see this error using Lightning 0.7pre (2007080305) with Thunderbird 2.0.0.7pre (20070803) on Windows 2000. Please provide steps for reproduction.

Comment 2

[alta88]

if you install Remove Duplicate Messages (Alternate) or Remote Image Cache this will happen.  the latter ext does this check, but Remove Dupes also breaks with this error (author aware).  sorry, should have noted that before..  

Comment 3

[Michael Büttner (no reviews TFN)]

Confirming. I also stumbled across this issue once in a while but forgot to file a bug for this. It happens for example if you try to include calUtils.js in today-pane.xul. Upon startup this throws the above mentioned exception.

One particular annoyance is that this gets in the way for initializing top-level code. One prominent example is indeed the today pane, where the onLoad() function initializes an array that would well have been initialized top-level (outside of the onLoad() function), but this would require to include calUtils.js which has the above mentioned flaw.

The cure is simple (see above code snippet from alta88), which is also what I had suggested. The only downside would be that we lose the const qualifier. Does anybody know a better solution to this problem? JS-guru's one step forward please... :-)

Comment 4

[Stefan Sitter [:ssitter]]

(In reply to comment #0)
> if (!Cc) var Cc = Components.classes;

This solution has one drawback: As you said Cc could be declared by any extension. If there is one extension that happens to declare Cc = 456; this would break Lightning too.

The only save solution in my opinion is to get rid of Cc (and Ci and Cr) and use the full qualifier instead. (At least in the shared js files that are included in many locations.)

Comment 5

[alta88]

(In reply to comment #4)
> (In reply to comment #0)
> > if (!Cc) var Cc = Components.classes;
> 
> This solution has one drawback: As you said Cc could be declared by any
> extension. If there is one extension that happens to declare Cc = 456; this
> would break Lightning too.

True.

> 
> The only save solution in my opinion is to get rid of Cc (and Ci and Cr) and
> use the full qualifier instead. (At least in the shared js files that are
> included in many locations.)
> 

Yes, also true.

Comment 6

[alta88]

perhaps using something uncommon, like const ltnCc, would do the trick as well.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Similar issues have affected Firefox code as well. I think the ideal solution is to add some backend DOM/xpconnect support for these shorthands and have them work in chrome/JS component for all Mozilla products.

Comment 8

[Stefan Sitter [:ssitter]]

I just stumbled above this issue trying to test the TBLSummary extension for Lightning <https://addons.mozilla.org/thunderbird/addon/5534>.

  Error: redeclaration of const Cc
  Source File: chrome://calendar/content/calUtils.js Line: 43

  Error: redeclaration of const MODE_RDONLY
  Source File: chrome://calendar/content/import-export.js Line: 44

I think we should fix this for 0.7 as more and more extension are being developed for Sunbird/Lightning currently.

We could guard the declaration as proposed in Comment #0 or replace all occurrences of Cc/Ci/Cr with the full qualifier. Thoughts?

Comment 9

[Daniel Boelzle [:dbo]]

We should go with replacing all Cc/Ci/Cr once all 0.7+ patches have landed. Due to the fact that this change is trivial (but a patch would be large and potentially bitrotten instantly), I am fine with a review waiver for the change or a "over-the-shoulder-review (clint)" if possible.

Comment 10

[Stefan Sitter [:ssitter]]

Attachment: Part 1: clean the views, rev0

Comment 11

[Stefan Sitter [:ssitter]]

Attachment: Part 2: clean the dialogs, rev0

Comment 12

[Stefan Sitter [:ssitter]]

Attachment: Part 3: clean the gdata provider

Comment 13

[Stefan Sitter [:ssitter]]

Attachment: Part 4: clean the providers, rev0

Comment 14

[Stefan Sitter [:ssitter]]

Attachment: Part 5: clean base files, rev0

Comment 15

[Stefan Sitter [:ssitter]]

With the patches above hopefully all locations that use Cc, Ci or Cr are eliminated. I used the following query to search for the files:
http://mxr.mozilla.org/seamonkey/search?string=C%5Bcir%5D&regexp=on&case=on&find=calendar&findi=&filter=&tree=seamonkey

If everything is OK we finally can clean and remove the declaration in mozilla/calendar/base/src/calUtils.js.

Comment 16

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

Comment on attachment 281658 [details] [diff] [review]
Part 3: clean the gdata provider

I guess there is no way to enforce 80 chars/line using Components.* in its long form. I really don't think this is the way to fix this bug. I think its safe to assume, that if Cc/Cr/Ci is defined, then the extension author in question has defined it as we have. If not, then its rather his or her extension that should be fixed. I would also prefer Gavin's solution.

Since I don't want to stand in the way, r+ for this part.

As an added bonus, you could change to createAttendee() here:
>-            var attendee = Cc["@mozilla.org/calendar/attendee;1"]
>-                           .createInstance(Ci.calIAttendee);
>+            var attendee = Components.classes["@mozilla.org/calendar/attendee;1"]
>+                           .createInstance(Components.interfaces.calIAttendee);

Comment 17

[alta88]

(In reply to comment #16)
> I really don't think this is the way to fix this bug. I think its safe to
> assume, that if Cc/Cr/Ci is defined, then the extension author in question has
> defined it as we have. If not, then its rather his or her extension that should
> be fixed. I would also prefer Gavin's solution.
> 
> 
sorry, that's wrong.  bad sandbox sportsmanship.  the dom/xpcom or such utility solution is very nice, but not exactly on anyone's todo.  in the interim, this is the most appropriate fix.

Comment 18

[Daniel Boelzle [:dbo]]

(In reply to comment #7)
IMO that's unlikely to happen on branch and doesn't help us for current versions.

Comment 19

[Daniel Boelzle [:dbo]]

Thanks for your efforts Stefan!

Patches checked in on HEAD and MOZILLA_1_8_BRANCH.

calUtils.js to be done

Comment 20

[Stefan Sitter [:ssitter]]

Attachment: Part 6: clean calUtils.js

Comment 21

[Daniel Boelzle [:dbo]]

Patch checked in on HEAD and MOZILLA_1_8_BRANCH.

Comment 22

[Amol]

zop ky problem nahi hote