371174 - In <ComposerCommands.js>, 3 strict "Warning: redeclaration of property", caused by function redeclarations

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/20070221 SeaMonkey/1.5a] (nightly) (W2Ksp4)

After checkin of bug 365869 (1st) fix,

On SeaMonkey Browser and/or MailNews startup:

{{
Warning: redeclaration of property __proto__
Source File: chrome://global/content/nsDragAndDrop.js
Line: 240

Warning: redeclaration of property __proto__
Source File: chrome://communicator/content/bookmarks/bookmarks.js
Line: 1708
Warning: redeclaration of property doTransaction
Source File: chrome://communicator/content/bookmarks/bookmarks.js
Line: 1711
Warning: redeclaration of property undoTransaction
Source File: chrome://communicator/content/bookmarks/bookmarks.js
Line: 1723
...
Warning: redeclaration of property beginUpdateBatch
Source File: chrome://communicator/content/bookmarks/bookmarks.js
Line: 1792
Warning: redeclaration of property endUpdateBatch
Source File: chrome://communicator/content/bookmarks/bookmarks.js
Line: 1799
...
Warning: redeclaration of property redoTransaction
Source File: chrome://communicator/content/bookmarks/bookmarks.js
Line: 1813

Warning: redeclaration of property toString
Source File: chrome://communicator/content/nsContextMenu.js
Line: 918
}}
NB: I did not copy the other lines at which the same <bookmarks.js> errors occur too.

It seems that the "__proto__" errors will be fixed by an additional patch to bug 365869.
This leaves the other errors.

Comment 1

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/20070221 SeaMonkey/1.5a] (nightly) (W2Ksp4)

3 more, related to mail Composer:

{{
Warning: redeclaration of property prompt
Source File: chrome://editor/content/ComposerCommands.js
Line: 1446

Warning: redeclaration of property promptUsernameAndPassword
Source File: chrome://editor/content/ComposerCommands.js
Line: 1460

Warning: redeclaration of property promptPassword
Source File: chrome://editor/content/ComposerCommands.js
Line: 1468
}}

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

This should be fixed now.

Comment 3

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/20070228 SeaMonkey/1.5a] (nightly) (W2Ksp4)

Yesterday's bug 365869 patch did fix comment 0 Browser and MailNews warnings :-)

But ... comment 1 Editor warnings are still there :-| Hints ?

Comment 4

[Serge Gautherie (:sgautherie)]

[Mozilla Thunderbird, version 3 alpha 1 (20070228)] (nightly) (W2Ksp4)

Same Editor warnings in TB.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #3)
> But ... comment 1 Editor warnings are still there :-| Hints ?

It looks like those warnings are real. As far as I can tell, those properties are defined multiple times on gEditorOutputProgressListener (for nsIPrompt and nsIAuthPrompt).

Comment 6

[Serge Gautherie (:sgautherie)]

Right, (Thanks)

(3 functions)
prompt: lines 1392 and 1445
promptUsernameAndPassword: lines 1427 and 1459
promptPassword: lines 1400 and 1467

Comment 7

[[e-mail]]

OK, so it's difficult for JS to provide both nsIPrompt and nsIAuthPrompt. Do we actually need to provide both? Neither implementation appears completely correct, so probably only one call actually gets made, which presumably is one of the ones that happens to work.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

necko itself doesn't call the password methods of nsIPrompt, if that's your question. there are cases where it calls other methods on it. and of course there are cases where it calls the authentication methods on nsIAuthPrompt.

Comment 9

[[e-mail]]

So the reason this worked before is that the nsIAuthPrompt versions of the authentication methods fortuitously overrode the nsIPrompt versions?

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

It should still work. Only now, there's a strict warning for the duplicated property.

Comment 11

[[e-mail]]

(In reply to comment #10)
>It should still work. Only now, there's a strict warning for the duplicated property.
The point is, it's safe to remove the nsIPrompt definitions?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

It should be. It looks like they've been overwritten (and therefore unused) since 2002.

Comment 14

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007101611 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

(Composer bug still there.)

'blocking‑thunderbird3=?':
Could be worth to sort this out ? ("security"-wise ?)

Comment 15

[Serge Gautherie (:sgautherie)]

<http://mxr.mozilla.org/comm-central/find?string=%2FnsI.*Prompt.%3F%5C.idl%24>
{{
/mozilla/netwerk/base/public/nsIPrompt.idl
/mozilla/netwerk/base/public/nsIAuthPrompt2.idl
/mozilla/netwerk/base/public/nsIAuthPrompt.idl
}}

Comment 16

[Mark Banner (:standard8)]

I don't see any serious issue here apart from some warnings on the console. If there is an actual problem with authenticating that hasn't been mentioned, then it may be worth fixing - but that's not here.

Comment 18

[Serge Gautherie (:sgautherie)]

Code was added "as is" by
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/editor/ui/composer/content/ComposerCommands.js&mark=1.92#1.93

Comment 19

[Serge Gautherie (:sgautherie)]

Bug still there:

http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird3.0/1269738362.1269739789.17439.gz
MacOSX 10.5 comm-1.9.1 bloat on 2010/03/27 18:06:02
http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1269724624.1269725222.19960.gz&fulltext=1
Linux comm-central bloat on 2010/03/27 14:17:04
{
JavaScript strict warning: chrome://editor/content/ComposerCommands.js, line 1445: redeclaration of property prompt
JavaScript strict warning: chrome://editor/content/ComposerCommands.js, line 1459: redeclaration of property promptUsernameAndPassword
JavaScript strict warning: chrome://editor/content/ComposerCommands.js, line 1467: redeclaration of property promptPassword
}

Comment 20

[:aceman]

Does this still happen? What needs to be done to see the warnings?

Comment 21

[[e-mail]]

(In reply to aceman from comment #20)
> Does this still happen? What needs to be done to see the warnings?
Well, the bogus code still exists, although I can't seem to work out how to trigger the warnings :-(

Comment 22

[[e-mail]]

(I filed bug 737022 on the lack of warnings...)

Comment 23

[:aceman]

It is true that the file contains multiple declarations of those functions, but sometimes they differ in number of arguments. Also, there are some comments indicating this could be intentional. Maybe Ehsan could know more.

Comment 24

[Philip Chee]

Whoever wrote the code was a C++ coder and didn't realize that Javascript doesn't do function overloading, so only the last method defined is called.

Comment 25

[:aceman]

I can confirm the warnings still exists in TB14 (after the string warnings got enabled). It is after opening the compose window.

I could just remove the first occurences of the functions but it is in core so I am not sure I could test it. Would working compose window in TB be enough of a test? I'd like confirmation from Ehsan that this is the right to do.

Comment 26

[Philip Chee]

You might also want to ask Neil as well. Just because the code "accidentally" works doesn't mean we can rely on it forever. We probably need to find all the callers to see what they actually expect to happen.

Comment 27

[(no longer active)]

Not a core bug.

Comment 28

[:aceman]

Attachment: removal of the first function declarations

Anybody eager to test it? :)
The warnings appear when TB compose window is opened.

After the patch they vanish. Xpcshell test pass. The mozmill ones are still doubtful for me, there are many fails even without the patch (but the number of fails does not seem to change after the patch).

Comment 29

[[e-mail]]

Comment on attachment 619400 [details] [diff] [review]
removal of the first function declarations

If only I could decide on the correct approach here...
1. Remove the duplication between the interfaces.
2. Switch from nsIAuthPrompt to nsIAuthPrompt2 (looks hard to implement)
3. Don't bother implementing nsIPrompt, instead return a stock prompt object
4. Don't bother implementing nsIAuthPrompt either, instead do something with the login manager to associate usernames and passwords with publish data.
5. ???

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 619400 [details] [diff] [review]
removal of the first function declarations

Fwiw, we should probably add a comment to document the actual "override" of these 3 methods and why it's not an issue.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 619400 [details] [diff] [review]
removal of the first function declarations

Seems fine.

Comment 32

[Justin Dolske [:Dolske]]

(In reply to [e-mail] from comment #29)
> Comment on attachment 619400 [details] [diff] [review]
> removal of the first function declarations

Removal? Sounds good to me! (I have no idea what's going on here ;)
 
> If only I could decide on the correct approach here...
> 1. Remove the duplication between the interfaces.

Too late for that, but you might take a look at how nsPrompter.js works around the problem...

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/nsPrompter.js#514

> 4. Don't bother implementing nsIAuthPrompt either, instead do something with
> the login manager to associate usernames and passwords with publish data.

That also sounds like a promising avenue.

Comment 33

[[e-mail]]

Comment on attachment 619400 [details] [diff] [review]
removal of the first function declarations

OK, so this needs a comment explaining that because the interface needs* to implement both nsIPrompt (so that people can call alert) and nsIAuthPrompt (so that it can save the password), there is a conflict between the method signatures and only the nsIAuthPrompt ones are provided since those are the ones that are actually needed, although we should really be saving the username and password in the login manager in the first place.

*We could in theory implement nsIInterfaceRequestor, and then implement getInterface to return Services.ww.getNewPrompter(gProgressDialog || window); as we don't actually do anything interesting with the nsIPrompt interface.

Comment 34

[Karsten Düsterloh]

Attachment: comment out unneeded implementations to end the clutter

Band-aid patch to end the clutter by commenting out the unusable functions, guarded by an explanatory comment.

Comment 35

[[e-mail]]

Comment on attachment 668558 [details] [diff] [review]
comment out unneeded implementations to end the clutter

Maybe this scheme would be more obvious:
/******************************************************************************
 * gEditorOutputProgressListener needs to implement both nsIPrompt (providing *
...
 * See bug 371174 for more information.                                       *
 ******************************************************************************
prompt : ...
...
},
 ******************************************************************************/

Comment 36

[Ian Neal]

Comment on attachment 668558 [details] [diff] [review]
comment out unneeded implementations to end the clutter

What he said...
Should there be a follow-up bug to remove/rework the code?

Comment 37

[Philip Chee]

> Should there be a follow-up bug to remove/rework the code?

Comment 32 has a hint on what needs to be done:

> but you might take a look at how nsPrompter.js works around the problem...

Comment 38

[Karsten Düsterloh]

Pushed <http://hg.mozilla.org/comm-central/rev/7758230a3ffc>, reformatted as per Neil's suggestion.

(In reply to Ian Neal from comment #36)
> Should there be a follow-up bug to remove/rework the code?

For the time being, I don't think so. Should we actually need to use both interfaces, we could always file a bug then; I also put a note onto the whiteboard here.