Last Comment Bug 371174 - In <ComposerCommands.js>, 3 strict "Warning: redeclaration of property", caused by function redeclarations
: In <ComposerCommands.js>, 3 strict "Warning: redeclaration of property", caus...
Status: RESOLVED FIXED
[See comment 33 if the need arises to...
:
Product: MailNews Core
Classification: Components
Component: Composition (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 19.0
Assigned To: Karsten Düsterloh
:
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
: 381410 506680 (view as bug list)
Depends on: 365869 737022
Blocks: 303545 28792
  Show dependency treegraph
 
Reported: 2007-02-21 14:58 PST by Serge Gautherie (:sgautherie)
Modified: 2012-11-29 02:16 PST (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
removal of the first function declarations (2.37 KB, patch)
2012-04-29 08:04 PDT, :aceman
neil: feedback+
bzbarsky: feedback+
Details | Diff | Splinter Review
comment out unneeded implementations to end the clutter (1.98 KB, patch)
2012-10-05 12:32 PDT, Karsten Düsterloh
iann_bugzilla: review+
neil: superreview+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2007-02-21 14:58:04 PST
[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) 2007-02-21 16:42:16 PST
[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) 2007-02-27 20:05:02 PST
This should be fixed now.
Comment 3 Serge Gautherie (:sgautherie) 2007-02-28 11:51:54 PST
[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) 2007-02-28 12:31:07 PST
[Mozilla Thunderbird, version 3 alpha 1 (20070228)] (nightly) (W2Ksp4)

Same Editor warnings in TB.
Comment 5 Blake Kaplan (:mrbkap) 2007-02-28 14:06:49 PST
(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) 2007-02-28 16:56:05 PST
Right, (Thanks)

(3 functions)
prompt: lines 1392 and 1445
promptUsernameAndPassword: lines 1427 and 1459
promptPassword: lines 1400 and 1467
Comment 7 neil@parkwaycc.co.uk 2007-03-01 02:23:13 PST
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) 2007-03-01 09:01:08 PST
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 neil@parkwaycc.co.uk 2007-03-01 16:05:56 PST
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) 2007-03-01 16:59:54 PST
It should still work. Only now, there's a strict warning for the duplicated property.
Comment 11 neil@parkwaycc.co.uk 2007-03-02 01:51:18 PST
(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) 2007-03-02 01:54:40 PST
It should be. It looks like they've been overwritten (and therefore unused) since 2002.
Comment 13 Adam Guthrie 2007-05-21 17:49:50 PDT
*** Bug 381410 has been marked as a duplicate of this bug. ***
Comment 14 Serge Gautherie (:sgautherie) 2007-10-16 18:19:14 PDT
[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) 2008-08-06 11:28:01 PDT
<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, limited time in Dec) 2008-08-14 02:07:33 PDT
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 17 Philip Chee 2009-07-28 03:50:50 PDT
*** Bug 506680 has been marked as a duplicate of this bug. ***
Comment 18 Serge Gautherie (:sgautherie) 2009-07-28 07:06:19 PDT
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) 2010-03-27 21:53:51 PDT
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 2012-03-19 04:53:53 PDT
Does this still happen? What needs to be done to see the warnings?
Comment 21 neil@parkwaycc.co.uk 2012-03-19 07:11:55 PDT
(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 neil@parkwaycc.co.uk 2012-03-19 10:50:09 PDT
(I filed bug 737022 on the lack of warnings...)
Comment 23 :aceman 2012-04-12 13:46:37 PDT
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 2012-04-12 19:25:52 PDT
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 2012-04-14 04:31:49 PDT
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 2012-04-14 07:12:48 PDT
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 :Ehsan Akhgari 2012-04-23 12:29:05 PDT
Not a core bug.
Comment 28 :aceman 2012-04-29 08:04:10 PDT
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).
Comment 29 neil@parkwaycc.co.uk 2012-04-29 12:02:05 PDT
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) 2012-05-01 01:49:42 PDT
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 [:bz] (still a bit busy) 2012-05-04 14:04:55 PDT
Comment on attachment 619400 [details] [diff] [review]
removal of the first function declarations

Seems fine.
Comment 32 Justin Dolske [:Dolske] 2012-05-04 18:40:12 PDT
(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 33 neil@parkwaycc.co.uk 2012-05-06 10:15:34 PDT
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 2012-10-05 12:32:10 PDT
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.
Comment 35 neil@parkwaycc.co.uk 2012-10-05 13:49:51 PDT
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 2012-10-07 09:00:17 PDT
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 2012-10-07 21:49:30 PDT
> 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 2012-10-08 14:03:14 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.