112908 - Error in walletOverlay.js on startup [redeclaration of const hide]

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Samir Gehani]

Using a new profile with today's (2001-11-30-09-trunk) mozilla build I see the
following error reported twice on startup in the JS console (this probably
happens twice because of the walletOverlay loading twice problem):

Error: redeclaration of const hide
Source File: chrome://wallet/content/walletOverlay.js
Line: 1

Comment 1

[Stephen P. Morse]

Yes, I see it too.

Comment 2

[Stephen P. Morse]

This is caused by the part of the patch for bug 77073 where

    var hide = -1;
    var disable = 0;
    var enable = 1;

were changed to

    const hide = -1;
    const disable = 0;
    const enable = 1;

I don't know why that would cause a redeclaration error since I never assign to 
these items anywhere else.  But changing it back to var eliminates the error 
message.

sgehani and alecf, please review the change to use var instead of const above.  
Unless you can see a better way to get rid of the error message.

Comment 3

[Alec Flett]

but that means there is already a global hide, disable, and enable. I suggest
you rename these to gMenuitemHide, etc...(or something similar) - generally its
not good to have simple global names like this, for precisely this reason.

Comment 4

[Stephen P. Morse]

I can rename them, but that's not the problem.  I know because I already tried 
that.

Comment 5

[Samir Gehani]

Before filing this bug I did an lxr search and found only one instance of a
|const hide| declaration.  I think the problem is that walletOverlay.js gets
called twice given the same js context and so it barfs on the first
redeclaration it sees.  I think the root of this problem is figuring out why
this js file is loaded twice.  (I think because walletOverlay.xul is called
twice but then why is that xul file overlaid twice?)

Is this error causing broken functionality?  If that is the case and changing
back to |var| declarations fixes this I'd be happy to review this as a stop-gap
measure.  Else, we should figure out the root problem.  

Comment 6

[Samir Gehani]

Ah, so I just lxr queried some more and found that there is no
walletOverlay.xul.  What an idiot for assuming so and spreading misinformation.
 Anyways, the js file is being included by multiple overlays.  Is it the case
that more than one overlay is being loaded at startup and so this file is being
loaded twice in the same js context?  How does js deal with reentrant code?  Do
we have to fix this manually?

Comment 7

[Stephen P. Morse]

Samir, you are right about a xul file being overlayed twice.  There are two xul 
files that use walletOverlay.js -- namely walletNavigatorOverlay.xul and 
walletContextOverlay.xul.

A long time ago I ran into the problem of walletContextOverlay.xul being 
overlayed twice and never could explain it.  See my comment in that file (search 
for the word "twice").

Comment 8

[Colin Phillips]

Stephan is right, if you load up venkman first, 

mozilla -chrome chrome://venkman/content

followed by going to tasks -> navigator, you'll notice that in the listing of
files that venkman generates, both walletContextOverlay.xul and
walletNavigatorOverlay.xul are being included in navigator.xul.

if you then search lxr for walletOverlay.js, 

http://lxr.mozilla.org/seamonkey/search?string=walletOverlay.js

you will find that both walletContextOverlay.xul and walletNavigatorOverlay.xul
both include walletOverlay.js.  Therefore, by transitivity, navigator.xul
includes walletOverlay.js twice.  Wow, I actually used something from school!

Comment 9

[Alec Flett]

now that we've found the source of the problem, lets get the overlays clenaed up
correctly

Comment 10

[Stephen P. Morse]

Alec, how are you suggesting we clean it up?

Comment 11

[Alec Flett]

by fixing the overlays and toplevel xul to avoid pulling in overlays and JS
multiple times. Colin has already done the analysis for you. This is not an
intractable problem. 

Besides, fixing this can only improve window opening time (and thus startup
time) and reduce bloat.

Fixing this by changing "const" to "var" fixes the symptoms, not the problem.

Comment 12

[Stephen P. Morse]

From what I've heard, the current plan is to remove wallet from the context 
menu.  In that case, walletContextOverlay.xul will vanish and so will this 
problem.

In the short term we can either change the const back to var (as it initially 
was) or we will be forced to live with this error message.

Comment 13

[Alec Flett]

I'm against the const -> var because const is perfectly good code, and we know
the source of the problem. Why do we need a "short term" solution? we're
spending more time debating whether or not we'll fix the overlays than it would
take to actually fix the overlays.

Comment 14

[timeless]

Attachment: stop using one of them

http://lxr.mozilla.org/seamonkey/search?string=wallet.*Overlay.xul&regexp=on

/extensions/wallet/resources/content/contents.rdf, line 45 --
<RDF:li>chrome://wallet/content/walletContextOverlay.xul</RDF:li>

is the only real use of that file... so looking ...
<RDF:Seq about="chrome://communicator/content/contentAreaContextOverlay.xul">
  <RDF:li>chrome://wallet/content/walletContextOverlay.xul</RDF:li>
</RDF:Seq>

http://lxr.mozilla.org/seamonkey/search?string=contentAreaContextOverlay.xul
/xpfe/components/sidebar/resources/sidebarOverlay.xul, line 23 -- <?xul-overlay

href="chrome://communicator/content/contentAreaContextOverlay.xul"?>

so there are two ways to look at this (assuming lxr is right, and it might not
be).. 1. if sidebar didn't exist this overlay wouldn't be pulled in, so remove
it. 2. sidebar is everywhere, navigator is only in one place, so we could go
back and remove the js include from the navigator overlay (assuming it is only
used in one place).

This patch is for 1.

Comment 15

[Doron Rosenberg (IBM)]

trying the patch, it removed the wallet context menuitems..

Comment 16

[Alex Vincent [:WeirdAl]]

Just out of curiousity, couldn't you just do:

if (!(hide in window)) {
    const hide = -1
    }

or

if (typeof hide == undefined) {
    const hide = -1
    }

Comment 17

[Stephen P. Morse]

*** Bug 119383 has been marked as a duplicate of this bug. ***

Comment 18

[Stephen P. Morse]

*** Bug 119469 has been marked as a duplicate of this bug. ***

Comment 19

[rossi]

comment #16 : that would remove the symptom, but not remove the problem which is
that this file is loaded twice...

Comment 20

[timeless]

actually comment 16 doens't work, because const is evaluated at compile time not at runtime logic.

Comment 21

[John Morrison]

*** Bug 120851 has been marked as a duplicate of this bug. ***

Comment 22

[Stephen Walker]

*** Bug 121676 has been marked as a duplicate of this bug. ***

Comment 23

[Simon Fraser [no longer active]]

Can we get some action here? This JS warning has been showing up for ages.

Comment 24

[Stephen P. Morse]

Simon, see comment #12.

Comment 25

[Andreas Franke (gone)]

I'm seeing this with Mozilla 0.9.8. Should this depend on bug 54300 ("Remove
auto-fill (form) items in document context menu") or is there another bug?

Comment 26

[Stephen P. Morse]

Yes, this should either be a dependency on bug 54300, or it should be fixed by 
backing out the regression that introduced it (comment #2).

Comment 27

[Alfonso Martinez]

*** Bug 123551 has been marked as a duplicate of this bug. ***

Comment 28

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

changing summary for easier searching

Comment 29

[John Morrison]

So, I was doing a little DOM scripting that was opening some windows, and 
I was trying to use the JS console to debug what I had done wrong, and ...
the console is really _unusable_ with all these errors overwriting the truly
exceptional conditions that a normal user would be interested in reviewing.

-> nsbeta1, and unsetting milestone for retriage

Comment 30

[Peter Trudelle]

nsbeta1- per Nav triage team, since this should be going away (comment#12)

Comment 31

[Stephen P. Morse]

Marking won't fix per comment #30

Comment 32

[Pierre Chanial]

*** Bug 125512 has been marked as a duplicate of this bug. ***

Comment 33

[Tuukka Tolvanen (sp3000)]

*** Bug 126777 has been marked as a duplicate of this bug. ***

Comment 34

[Stephen P. Morse]

*** Bug 126853 has been marked as a duplicate of this bug. ***

Comment 35

[Andreas Franke (gone)]

Reopening to avoid duplicate reports being filed.

Comment 36

[Stephen P. Morse]

Assigning this to [e-mail] to indicate that it's not going to be 
worked on but at the same time keeping it open as per Franke's request.

Comment 37

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

couldn't, as a short time fix, the const be changed back to var?
This would get rid of the JS error.

Comment 38

[Alec Flett]

but it's still an error, and indicates a huge problem with the wallet overlays -
we're pulling in and evaluating the same JS file three times. we are NOT going
to hide this problem just to make people who poke at the JS console happy. This
IS an error, this should be fixed.

adding at least the perf keyword, and cc'ing dp for bloat issues.

Comment 39

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

alecf, sure, that's true. however, this was decided to not be fixed (comment 30
and others), because the file would go away anyway. Now, if the real problem
cannot be fixed, maybe the symptoms can. For a reason to do this read comment
29; also, iirc printing something to the console is rather slow, which would be
another reason to avoid this message.

Comment 40

[Alec Flett]

the way I look at it is this: we fix the problem, not the symptoms. If that fix
involves waiting for another fix, that's fine. But if people are upset to see
this error message, then they should fix the real problem and stop waiting for
the other fix.

Here's what someone really needs to do, and this will solve everything: combine
walletTasksOverlay and walletNavigatorOverlay, and walletContextOverlay into the
same overlay file, creating a new walletOverlay.xul. That .xul should bring in
the one and only walletOverlay.js

That's the real fix for this bug. I'll super-review.

Comment 41

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

*sigh* ok, taking bug

Comment 42

[Samir Gehani]

Regarding comment 39: is it actually slow to simply *create* a JS error (which I
assume would just stay somewhere in memory) or is it slow to *bring up* the JS
console?  Those are two separate issues.

Comment 43

[timeless]

yes, jserrors are expensive (bz et al have a few nice bugs showing how fixing 
js warnings makes a very large difference for dom 'testcases').  and nothing is 
free, especially not double loading js or xul files.

Comment 44

[Alec Flett]

I hadn't thought about the slowness issue, but this actually brings up a VERY
good point: leaving this as an error is actually FASTER than changing this to a
"var" - the reason is that if we change it to a var, we get a JS warning, and
the rest of the file is interpreted. If we leave it as an error, then the
interpret is interrupted as soon as the error is encountered - yet another
reason not to change the const to var.

Regarding printing stuff to the console/etc - in the grand scheme of things:
- printing stuff to the screen is slow, but not if we don't print it (which I
thought we avoided in release builds)
- adding things to the JS console is actually VERY fast (just adding items to an
array) but this is slowed down if the JS console is on the screen. However, 99%
of users to NOT have the JS console up, so there should be no major slowdown for
code that's not in a loop (like this one)

christian, thanks for taking this on!

Comment 45

[Stephen P. Morse]

To set the record straight, what alec said was not quite right.  If we change it 
to a var, there is no warning -- it is accepted as correct.

Comment 46

[Alec Flett]

yes, if by "correct" you mean to reevaluate the file three times. Syntactically
correct yes, but the correct solution, no.

Comment 47

[Dan Tobias]

When this was marked "WontFix" it was on the grounds that the problem was going
away due to other planned changes anyway.  Can anybody report on just when those
other changes were planned to be made?  These errors in the JavaScript console
are an annoyance for anybody who actually uses that feature to check on errors
in their own (or others') JavaScript, as it leads to there being a bunch of
bogus stuff cluttering the console every time.

Comment 48

[Bradley Baetz (:bbaetz)]

The 'other changes' were blake's menu stuff. I still get this on a build from
after that landing, although I don't seem to get the wallet context menu items
any more. Is there a new way to bring up a wallet context menu which I can't
find, or do we just need to remove the import of the now-uneeded overlay?

Comment 49

[Stephen P. Morse]

Wallet is no longer in the context menu -- that is why you can't find it.

The overlay is still needed because it also overlays the status line.

Comment 50

[Bradley Baetz (:bbaetz)]

Ah, ok

This js file is now being brought in by walletTasksOverlay and
walletNavigatorOverlay.

Comment 51

[Stephen P. Morse]

It that's because somebody added the two wallet functions to the form-manager 
flyout?

Comment 52

[rossi]

*sigh* ok, taking bug

Comment 53

[Vadim Berezniker]

*** Bug 139102 has been marked as a duplicate of this bug. ***

Comment 54

[rossi]

Attachment: proposed patch

it would be enough to just remove the javascript include from the navigator
overlay, but i thought that's too insecure. and as tasksOverlay really only
needs the .js file for opening the dialog, i included that one line in the
script part of the tasks overlay xul (which could be in it's own .js file...)
an removed the overlay.js inclusion from it. (so mailnews etc. don't load all
that forms functions they don't need)

Comment 55

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

Comment on attachment 81398 [details] [diff] [review]
proposed patch

r=biesi

Comment 56

[Alec Flett]

Comment on attachment 81398 [details] [diff] [review]
proposed patch

why call it "Dialog"?  JS functions share a global namespace, no matter who
includes them. at the very least, you should keep the Wallet prefix.

Comment 57

[rossi]

Attachment: patch v2

here you go, i renamed the Expire function too...

Comment 58

[Alec Flett]

Comment on attachment 81464 [details] [diff] [review]
patch v2

excellent! So nice to see this cleaned up.

You've used lxr to make sure
1) nobody else is using walletTasksOverlay.xul (or at least if they are, they
don't need walletOverlay.js?)
2) nobody else is calling WalletDialog('signon')? (including some means by
which the 'signon' parameter might be generated or retrieved from somewhere
else in the UI?)

Nothing personal, just want to make sure we don't cause any regressions :)
sr=alecf

Comment 59

[timeless]

http://lxr.mozilla.org/mozilla/search?string=walletTasksOverlay.xul
/extensions/wallet/jar.mn, line 4 -- content/wallet/walletTasksOverlay.xul
(resources/content/walletTasksOverlay.xul)
/extensions/wallet/resources/content/contents.rdf, line 30 --
<RDF:li>chrome://wallet/content/walletTasksOverlay.xul</RDF:li>
/netwerk/test/jarlist.dat, line 171 --
jar:resource:///chrome/comm.jar!/content/wallet/walletTasksOverlay.xul
this last one is just some test thing, the first one is the packaging, and only
the middle one actually does anything.

http://lxr.mozilla.org/mozilla/search?string=WalletDialog
/extensions/wallet/resources/content/walletNavigatorOverlay.xul, line 172 --
oncommand="WalletDialog('wallet');"/>
/extensions/wallet/resources/content/walletNavigatorOverlay.xul, line 176 --
oncommand="WalletDialog('walletsites');"/>
/extensions/wallet/resources/content/walletTasksOverlay.xul, line 67 --
oncommand="WalletDialog('signon');"/>
/extensions/wallet/resources/content/walletOverlay.js, line 357 -- function
WalletDialog(which) {

So the patch removes the only caller for 'signon'

alecf: for bonus points can we kill 

     function WalletDialog(which) {
       switch( which ) {
         case "samples": // <- this case, since there are no consumers?

the patch looks good to me, 

Comment 60

[rossi]

uhm... bugmail is coming in random order... yes i did exactly the same thing
timeless did. (search lxr).

Comment 61

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

Comment on attachment 81464 [details] [diff] [review]
patch v2

r=biesi still valid

Comment 62

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

checked in to trunk - I hope you don't mind rossi

since this message is visible to everybody who opens JS Console (with default
settings), I think this should also be in mozilla 1.0, so leaving open for
branch checkin.

Comment 63

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

eh
reopening, since I didn't intend to close this

Comment 64

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 81464 [details] [diff] [review]
patch v2

approval noted in bug for branch checkin.  Please add keyword fixed1.0.0 once
it's in the branch and marked fixed.

Comment 65

[Matthias Versen [:Matti]]

*** Bug 146024 has been marked as a duplicate of this bug. ***

Comment 66

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

so... firstly, sorry for my forgetting to check this in to the branch... 
however, approval has now expired, and drivers denied approval for 1.0...

hopefully this'll end up in 1.0.1

Comment 67

[Marco Ferretti]

Attachment: just the line I commented

Hi guys. I went through this bug only today ( to see wether or not downloading
b3).
I have got rid of it by just commenting the line that loads walletOverlay.js in
pre-wallet.xul and walletNavigatorOverlay.xul since the js looks to be seen
anyway by only loading from walletTasksOverlay. 
I don't get the upsetting "redeclaration ..." error message and everything
looks like working ( I tried opening all the tasks and running javascript
pages, opening windows etc.) . Is there something I am missing or what I did is
really works?

Comment 68

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

Marco, what you did works but isn't the best way to do it... We already have a
good patch here, and it is already in the trunk. 1.0.1 will probably have the
fix as well (I have already mailed drivers again)

Comment 69

[Judson Valeski]

please checkin to the 1.0.1 branch ASAP. once there please remove the
mozilla1.0.1+ keyword and add the fixed1.0.1 keyword.

Comment 70

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

Checking in resources/content/walletOverlay.js;
/cvsroot/mozilla/extensions/wallet/resources/content/walletOverlay.js,v  <-- 
walletOverlay.js
new revision: 1.20.2.2; previous revision: 1.20.2.1
done
Checking in resources/content/walletTasksOverlay.xul;
/cvsroot/mozilla/extensions/wallet/resources/content/walletTasksOverlay.xul,v 
<--  walletTasksOverlay.xul
new revision: 1.12.2.3; previous revision: 1.12.2.2
done


Checked into Branch. Marking Fixed. Adding fixed1.0.1 keyword.

finally.

Comment 71

[Matthias Versen [:Matti]]

*** Bug 148765 has been marked as a duplicate of this bug. ***

Comment 72

[Matthias Versen [:Matti]]

*** Bug 150220 has been marked as a duplicate of this bug. ***

Comment 73

[Alfonso Martinez]

*** Bug 150461 has been marked as a duplicate of this bug. ***

Comment 74

[Terri Preston]

Verified win XP branch 2002061208 linux Directory: 2002061110 Mac OS X 2002061205

Comment 75

[Gilles Durys]

*** Bug 152319 has been marked as a duplicate of this bug. ***

Comment 76

[Matthias Versen [:Matti]]

*** Bug 156057 has been marked as a duplicate of this bug. ***