Closed Bug 112908 Opened 23 years ago Closed 22 years ago

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

Categories

(Toolkit :: Form Manager, defect)

Product:
Toolkit
Component:
Form Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: samir_bugzilla, Unassigned)

References

Details

(Keywords: memory-leak, perf)

Attachments

(2 files, 2 obsolete files)

patch v2
2.48 KB, patch
Biesinger
: review+
alecf
: superreview+
Details | Diff | Splinter Review
just the line I commented
288 bytes, text/plain
Details 
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
Yes, I see it too.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7
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.
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.
I can rename them, but that's not the problem.  I know because I already tried 
that.
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.  
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?
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").
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!
now that we've found the source of the problem, lets get the overlays clenaed up
correctly
Alec, how are you suggesting we clean it up?
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.
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.
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.
Attached patch stop using one of them (obsolete) — — Splinter Review 
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.
Target Milestone: mozilla0.9.7 → mozilla0.9.9
trying the patch, it removed the wallet context menuitems..
Just out of curiousity, couldn't you just do:

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

or

if (typeof hide == undefined) {
    const hide = -1
    }
*** Bug 119383 has been marked as a duplicate of this bug. ***
*** Bug 119469 has been marked as a duplicate of this bug. ***
comment #16 : that would remove the symptom, but not remove the problem which is
that this file is loaded twice...
actually comment 16 doens't work, because const is evaluated at compile time not at runtime logic.
*** Bug 120851 has been marked as a duplicate of this bug. ***
*** Bug 121676 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla0.9.9 → mozilla1.0.1
Can we get some action here? This JS warning has been showing up for ages.
OS: Windows 2000 → All
Hardware: PC → All
Simon, see comment #12.
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?
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).
*** Bug 123551 has been marked as a duplicate of this bug. ***
changing summary for easier searching
Summary: Error in walletOverlay.js on startup → Error in walletOverlay.js on startup [redeclaration of const hide]
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
Keywords: nsbeta1
Target Milestone: mozilla1.0.1 → ---
nsbeta1- per Nav triage team, since this should be going away (comment#12)
Keywords: nsbeta1 → nsbeta1-
Marking won't fix per comment #30
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → WONTFIX
*** Bug 125512 has been marked as a duplicate of this bug. ***
*** Bug 126777 has been marked as a duplicate of this bug. ***
*** Bug 126853 has been marked as a duplicate of this bug. ***
Reopening to avoid duplicate reports being filed.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Depends on: 54300
Assigning this to nobody@mozilla.org to indicate that it's not going to be 
worked on but at the same time keeping it open as per Franke's request.
Assignee: morse → nobody
Status: REOPENED → NEW
couldn't, as a short time fix, the const be changed back to var?
This would get rid of the JS error.
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.
Keywords: perf
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.
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.
*sigh* ok, taking bug
Assignee: nobody → cbiesinger
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.
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.
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!
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.
yes, if by "correct" you mean to reevaluate the file three times. Syntactically
correct yes, but the correct solution, no.
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.
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?
Keywords: mozilla1.0
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.
Ah, ok

This js file is now being brought in by walletTasksOverlay and
walletNavigatorOverlay.
It that's because somebody added the two wallet functions to the form-manager 
flyout?
*sigh* ok, taking bug
Assignee: cbiesinger → rossi
*** Bug 139102 has been marked as a duplicate of this bug. ***
Attached patch proposed patch (obsolete) — — Splinter Review 
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)
Attachment #60128 - Attachment is obsolete: true
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.
Attachment #81398 - Flags: needs-work+
Attached patch patch v2 — — Splinter Review 
here you go, i renamed the Expire function too...
Attachment #81398 - Attachment is obsolete: true
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
Attachment #81464 - Flags: superreview+
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, 
uhm... bugmail is coming in random order... yes i did exactly the same thing
timeless did. (search lxr).
Comment on attachment 81464 [details] [diff] [review]
patch v2

r=biesi still valid
Attachment #81464 - Flags: review+
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.
Status: NEW → RESOLVED
Closed: 23 years ago → 22 years ago
Resolution: --- → FIXED
eh
reopening, since I didn't intend to close this
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #81464 - Flags: approval+
Keywords: mlk
*** Bug 146024 has been marked as a duplicate of this bug. ***
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
Target Milestone: --- → mozilla1.0.1
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?
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)
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.
Keywords: mozilla1.0.1+
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.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Keywords: fixed1.0.1
Resolution: --- → FIXED
*** Bug 148765 has been marked as a duplicate of this bug. ***
*** Bug 150220 has been marked as a duplicate of this bug. ***
*** Bug 150461 has been marked as a duplicate of this bug. ***
Verified win XP branch 2002061208 linux Directory: 2002061110 Mac OS X 2002061205
Keywords: fixed1.0.1 → verified1.0.1
*** Bug 152319 has been marked as a duplicate of this bug. ***
Keywords: mozilla1.0.1+
*** Bug 156057 has been marked as a duplicate of this bug. ***
Assignee: rossi → nobody
Product: Core → Toolkit
QA Contact: tpreston → form.manager
Target Milestone: mozilla1.0.1 → ---
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: