Closed Bug 371174 Opened 13 years ago Closed 7 years ago

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

Categories

(MailNews Core :: Composition, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 19.0

People

(Reporter: sgautherie, Assigned: mnyromyr)

References

(Blocks 1 open bug, )

Details

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

Attachments

(1 file, 1 obsolete file)

[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.
[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
Closed: 13 years ago
Resolution: --- → FIXED
[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 → ---
Assignee: general → nobody
Status: REOPENED → NEW
QA Contact: general → composition
[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).
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
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.
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.
(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.
Blocks: 303545
Whiteboard: [See comment 1 for remaining warnings]
Duplicate of this bug: 381410
[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
<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-
Duplicate of this bug: 506680
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
}
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...)
Depends on: 737022
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
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.
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
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
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)
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 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+
Assignee: acelists → mnyromyr
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 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+
> 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...
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
Closed: 13 years ago7 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.