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

RESOLVED FIXED in Thunderbird 19.0

Status

MailNews Core
Composition
--
minor
RESOLVED FIXED
11 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: Karsten Düsterloh)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 19.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [See comment 33 if the need arises to use both the conflicting interfaces!], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
[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.
(Reporter)

Comment 1

11 years ago
[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
}}
This should be fixed now.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Reporter)

Comment 3

11 years ago
[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 ?
Status: RESOLVED → REOPENED
Component: General → MailNews: Composition
Product: Mozilla Application Suite → Core
Resolution: FIXED → ---
(Reporter)

Updated

11 years ago
Assignee: general → nobody
Status: REOPENED → NEW
QA Contact: general → composition
(Reporter)

Comment 4

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

Same Editor warnings in TB.
(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).
(Reporter)

Comment 6

11 years ago
Right, (Thanks)

(3 functions)
prompt: lines 1392 and 1445
promptUsernameAndPassword: lines 1427 and 1459
promptPassword: lines 1400 and 1467
Severity: trivial → minor
OS: Windows 2000 → All
Hardware: PC → All
Summary: Various strict "Warning: redeclaration of property". → In <ComposerCommands.js>, 3 strict "Warning: redeclaration of property", caused by function redeclarations

Comment 7

11 years ago
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.
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

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








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

Comment 11

11 years ago
(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?
It should be. It looks like they've been overwritten (and therefore unused) since 2002.
(Reporter)

Updated

11 years ago
Blocks: 303545
Whiteboard: [See comment 1 for remaining warnings]

Updated

10 years ago
Duplicate of this bug: 381410
(Reporter)

Comment 14

10 years ago
[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 ?)
Flags: blocking-thunderbird3?
Product: Core → MailNews Core
(Reporter)

Comment 15

9 years ago
<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
}}
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.
Flags: blocking-thunderbird3? → blocking-thunderbird3-

Updated

8 years ago
Duplicate of this bug: 506680
(Reporter)

Comment 18

8 years ago
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
Blocks: 28792
(Reporter)

Comment 19

7 years ago
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
}
status-thunderbird3.1: --- → ?
status-thunderbird3.1: ? → ---

Comment 20

5 years ago
Does this still happen? What needs to be done to see the warnings?
(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 :-(
(I filed bug 737022 on the lack of warnings...)
(Reporter)

Updated

5 years ago
Depends on: 737022

Comment 23

5 years ago
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.
Component: Composition → Editor
Flags: blocking-thunderbird3-
Product: MailNews Core → Core
QA Contact: composition → editor

Comment 24

5 years ago
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

5 years ago
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.
Assignee: nobody → acelists

Comment 26

5 years ago
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.
Not a core bug.
Component: Editor → Composition
Product: Core → MailNews Core
QA Contact: editor → composition

Comment 28

5 years ago
Created attachment 619400 [details] [diff] [review]
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).
Attachment #619400 - Flags: feedback?(neil)
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. ???
Attachment #619400 - Flags: feedback?(bzbarsky)
(Reporter)

Comment 30

5 years ago
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

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

Seems fine.
Attachment #619400 - Flags: feedback?(bzbarsky) → feedback+
(In reply to neil@parkwaycc.co.uk 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 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.
Attachment #619400 - Flags: feedback?(neil) → feedback+

Updated

5 years ago
Assignee: acelists → mnyromyr
(Assignee)

Comment 34

5 years ago
Created attachment 668558 [details] [diff] [review]
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.
Attachment #619400 - Attachment is obsolete: true
Attachment #668558 - Flags: superreview?(neil)
Attachment #668558 - Flags: review?(iann_bugzilla)
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 : ...
...
},
 ******************************************************************************/
Attachment #668558 - Flags: superreview?(neil) → superreview+

Comment 36

5 years ago
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?
Attachment #668558 - Flags: review?(iann_bugzilla) → review+

Comment 37

5 years ago
> 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...
(Assignee)

Comment 38

5 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [See comment 1 for remaining warnings] → [See comment 33 if the need arises to use both the conflicting interfaces!]
Target Milestone: --- → Thunderbird 19.0
You need to log in before you can comment on or make changes to this bug.