Henrik Gemal Reporter
	Description • 24 years ago

After turning on strict js warning I'm getting this in the console:
------
WEBSHELL+ = 4
has multiple monitor apis is 1
JavaScript strict warning: line 9: function onget does not always return a value
JavaScript strict warning: line 9: function onset does not always return a value
WEBSHELL+ = 5
------

but it doesn't say any filename. Should the error always contain a filename?

	Phil Schwartau
	Comment 1 • 24 years ago

Browser, not engine. Reassigning to Browser-General component -

	Phil Schwartau
	Comment 2 • 24 years ago

Assigning to dveditz for further triage, based on his work on 
javascript.options.strict in bug 50645. Cc'ing rginda -

	Daniel Veditz [:dveditz]
	Comment 3 • 24 years ago

Updating summary to mention which set of warnings we're talking about.

Actually this might be in the engine. At least the JSErrorReport passed in from 
the engine has no filename in it. Perhaps that's a problem on the DOM side 
compiling a buffer without a filename or something. In any case I need to 
bow out since all I did was turn on the reporting option, I don't know 
where the errors come from originally. CC'ing more folks.

	Henrik Gemal Reporter
	Comment 4 • 24 years ago

it seems that the "onget/onset" are present in many .xml files, if it helps...:)

strict error:
onset="this.setAttribute('searchattribute',val.id)"
no strict error:
onset="return this.searchAttribute=val;"
I think...

	Brendan Eich [:brendan]
	Comment 5 • 24 years ago

Hyatt, where can you get the .xml filename to supply when compiling getters and 
setters in layout/xbl/src/*.cpp?

/be

	Alec Flett
	Comment 6 • 24 years ago

I've got a fix. Attaching for review.

	Daniel Veditz [:dveditz]
	Comment 8 • 24 years ago

I know you didn't ask me specifically, but r=dveditz

Instead of creating the extra nsCAutoString cname in each case I think you 
could have used functionUri.AppendWithConversion(name.GetUnicode) instead, but 
it doesn't really matter.

	Daniel Veditz [:dveditz]
	Comment 10 • 24 years ago

r=dveditz for this one, too. 

	Mike McCabe
	Comment 11 • 24 years ago

Looks like the last declaration of cname

+            nsCAutoString cname; cname.AssignWithConversion(name.GetUnicode());

is unused.

There's no way to get line numbers in this context, is there?  If no, I suppose
the descriptive function name is better than just a filename...  is there anyway
to get a filename in this context?  (Also, is functionName better than
functionUri?)

r=mccabe

	Alec Flett
	Comment 12 • 24 years ago

I removed the excess cname variable - thanks mccabe.

I still need a sr=hyatt on this one

	David Hyatt
	Comment 13 • 24 years ago

Yes, you can get a filename.  Call GetDocumentURI to get the URL.  Or call 
GetBindingURI to get the URL and the binding ID.

	Alec Flett
	Comment 14 • 24 years ago

talked to dave in person last week or so, he sez sr=hyatt
reassigning to me and checking into the tip

	Alec Flett
	Comment 15 • 24 years ago

fix checked in.

	Henrik Gemal Reporter
	Comment 16 • 24 years ago

With build 20001027 I'm still getting:

JavaScript strict warning:
 line 9: function onget does not always return a value

	Henrik Gemal Reporter
	Comment 17 • 24 years ago

Very easy way to reproduce a javascript strict error without filename:
JavaScript strict warning:
 line 0: function onclick does not always return a value

Just go to:
http://bugzilla.mozilla.org/enter_bug.cgi?product=Browser
and press Commit without entering any data.

The problem is the javascript onclick on the Commit button.

	Alec Flett
	Comment 18 • 24 years ago

yes, please try a new build, I haven't seen such a warning in days.

	Alec Flett
	Comment 19 • 24 years ago

by the way, this fix went in ONLY into the tip, not the branch.

	Alec Flett
	Comment 20 • 24 years ago

Ugh, you're right. I don't know what's up with that particular dialog.
I don't think it's XBL though... while admittedly the original problem is more
global, my fix was specifically for xbl. I think the error you're seeing is
specific to the common dialogs.

	Henrik Gemal Reporter
	Comment 21 • 24 years ago

Well anyways.... This bug is not fixed, since I still see js strict errors 
without filenames.
Using build 2000110204 from trunk

	Alec Flett
	Comment 22 • 24 years ago

ugh, looks like the only case left is for stuff like onclick="if (foo) return
bar();" - i.e. right in the XML

	Henrik Gemal Reporter
	Comment 23 • 24 years ago

Any update on this "the only case left"...?

	Alec Flett
	Comment 24 • 24 years ago

no update - it's a big pain in the ass because it's harder to get XML line
numbers than JS line numbers.

	Henrik Gemal Reporter
	Comment 25 • 24 years ago

seems like we're just missing the filename when a "onclick" is fired.

	Henrik Gemal Reporter
	Comment 26 • 24 years ago

this is ofcourse also happening for errors.

Seems like all "on" something are missing.

Going to:
http://mobile.box.sk/
also produces an "empty" error:

Error:
MakeRemoteWindow is not defined

	John Bandhauer
	Comment 27 • 24 years ago

Are you gonna fix all of bug 11240 too?

	Paul Chen
	Comment 28 • 24 years ago

nav triage team:

Not a 1.0 stopper and a debug build only issue, pushing out to mozilla1.1

	Alex Vincent [:WeirdAl]
	Comment 29 • 23 years ago

I have been seeing a few strict JS warnings from the Mozilla suite without a
filename.  Unknown where they're coming from.

Here's one I've gotten in the last 30 minutes, 11 times.  And no, I don't know
how I got it.

Warning: reference to undefined property
this.parentNode.parentNode.parentNode.addToMissedIconCache

This is on a Mozilla nightly build, which I believe is NOT a debug build. 
Please reconsider.

Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.7+) Gecko/20020105

	Henrik Gemal Reporter
	Comment 30 • 23 years ago

In the console I see the following without filenames:

Warning: anonymous function does not always return a value
Source File: 
Line: 159, Column: 8
Source Code:
        },

------------------------------

Error: uncaught exception: [Exception... "Component returned failure code:
0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIOutlinerView.isContainer]"  nsresult:
"0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: <unknown filename>
:: BMOLController_isCommandEnabled :: line 48"  data: no]

------------------------------

Deprecated method document.getSelection() called.  Please use
window.getSelection() instead.

------------------------------

Warning: reference to undefined property this.parentNode.updateCurrentBrowser

etc...

	Henrik Gemal Reporter
	Comment 31 • 23 years ago

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

	Henrik Gemal Reporter
	Comment 32 • 23 years ago

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

	Alex Vincent [:WeirdAl]
	Comment 33 • 23 years ago

Bug 119719 is NOT a duplicate of this bug.  119719 has to do with the JavaScript
throw statement.

	Alec Flett
	Comment 34 • 23 years ago

no time for this - I welcome anyone else to take it.

	David G King
	Comment 35 • 22 years ago

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

	David G King
	Comment 36 • 22 years ago

This is not a debug build only problem (as said in Comment #28). It happens to
me with build 2002072604-TRUNK as downloaded as a Win32 installer. As such, I'm
raising this to a Blocker as it blocks testing (and bug fixing).

Also changing summary to help with finding this bug.

	Alec Flett
	Comment 37 • 22 years ago

this is not a blocker, I'm sorry. It doesn't PREVENT you from testing/bug
fixing, it just makes it harder. I know that seems like a fine line, but
honestly its the difference between a blocker and a normal bug.

	David G King
	Comment 38 • 22 years ago

I received a JavaScript error about Colgroup containing no properties. With no
filename or line number, how on earth am I supposed to debug that.

	Alex Vincent [:WeirdAl]
	Comment 39 • 22 years ago

Can you attach your file as a testcase, please?  :) Preferably, reduce it to as 
small a testcase as you can that will still produce that strict warning.

	David G King
	Comment 40 • 22 years ago

Re Comment #49, you're not serious are you? As the Javascript Console gave me no
Source File, nor a URL ref, nor a line number, I'm afraid I can't attach a
testcase. And I'm told that this doesn't "Blocks development and/or testing
work" to quote http://bugzilla.mozilla.org/bug_status.html#severity

	Alex Vincent [:WeirdAl]
	Comment 41 • 22 years ago

I am quite serious.

Specifically, we need a reproducible set of steps to test this strict warning 
so that, when it's fixed, we can check it.  What page did you visit to find 
that warning?

	David G King
	Comment 42 • 22 years ago

I generally have multiple windows open, each wich several tabs open, so I'm
afraid I don't know which page caused this problem. Mind you, if Javascript
Console worked as it normally does, it would have given me an URL.

To quote Comment #37 this "...doesn't PREVENT you from testing/bug fixing...".

	Robert Ginda
	Comment 43 • 22 years ago

Comment on attachment 15775 [details] [diff] [review]
better fix than my copy-n-paste garbage (thanks dveditz)

Passing null as a filename (or 0 as a line number) to the js engine should be
considered a misuse of the api.  Reviewers shouldn't let this kind of stuff get
into the tree.

Script authors NEED this information to track down the error.  Imagine if your
C++ compiler didn't tell you what file had a missing semicolon?  Sounds like a
blocker to me.

Also, the JS debugger needs a REAL url in the filename field, in order to
locate the source.  There are already places in XBL where hyatt made up his own
bogus URLs, lets not do that again.

	David G King
	Comment 44 • 22 years ago

Thankyou Robert.

The patch (which is almost 2 years old), does have problems.

And, for Comment #41, I did a search with Bugzilla, and found I needn't have
bothered. Take a look at the Bug this one is blocking.

And finally, as per Robert's and my thoughts about debuggers that don't tell you
anything about which file, line or URL is the problem (I like his C++ example),
marking this as a Blocker.

	David G King
	Comment 45 • 22 years ago

Comment on attachment 15775 [details] [diff] [review]
better fix than my copy-n-paste garbage (thanks dveditz)

The current patch is essentially obsolete. The file it is patching has moved
from /cvsroot/mozilla/layout/xbl/src/nsXBLBinding.cpp to
/cvsroot/mozilla/content/xbl/src/nsXBLBinding.cpp and seems to be totally
different from the patch.

	Alec Flett
	Comment 46 • 22 years ago

as you can see from comment 22, this only occurs for event handlers - meaning
that we know the code in question is in some kind of onfoo="..." - and we know
its not XBL, because I fixed that. So narrowing it down should get easier based
on this information. (in fact you could probably come up with a regexp that
would find it for you!)

blocker also means "Blocks development and/or testing work" on _Mozilla_ - what
part of mozilla are you debugging? that will help you narrow down the XBL or XUL
that is causing this. If this is mozilla development, you should have a bugzilla
bug number, which means narrowing it down should be a whole lot easier.

	Alec Flett
	Comment 47 • 22 years ago

yes, that patch was LONG AGO checked in, and the code that it touched has LONG
AGO been migrated to another file. it does us no good to get reviews of this,
since it was checked in almost two years ago.

	David G King
	Comment 48 • 22 years ago

To answer Comment #46, I'm trying to debug Bookmarks as detailed in the bug that
this one blocks (i.e. Bug #152504).

	Daniel Veditz [:dveditz]
	Comment 49 • 22 years ago

It's still not a blocker: you found a bug, obviously. This is preventing you
from DE-bugging. that is, it's hindering DEVELOPMENT, not testing.

	Robert Ginda
	Comment 50 • 22 years ago

alecf: So YOUR the one who introduced these so-called urls?  Shame on you.  And
all this time I had been blaming Hyatt for bug 127567.

	Alec Flett
	Comment 51 • 22 years ago

hee hee :)
anyway yeah the urls are pretty arbitrary.. but I'll go follow up in that bug.

	David G King
	Comment 52 • 22 years ago

Re Comment #49. So, you agree that this hinders Development. And, the definition
of a Blocker bug is one that prevents Development
(http://bugzilla.mozilla.org/bug_status.html#severity).

So, as you say this bug is "...hindering DEVELOPMENT..." you agree with me that
it should be a Blocker.

	Alec Flett
	Comment 53 • 22 years ago

for god's sake this is not a blocker. Please stop this game, this is toy is not
intended for children. I will request that your bugzilla privileges be taken
away if you change it again.

	David G King
	Comment 54 • 22 years ago

You don't need to use expletives or cheap insults to get your way. If anything,
I should request your privs be removed for the foul language and insulting slurs.

I using the definition of blocker that is found as a link off this page that
says "Blocks development and/or testing work". In my opinion this bug blocks
development and testing on Bug #152504. Your insistance that debugging has no
part of Development or Testing is a rather interesting concept.

	Alex Vincent [:WeirdAl]
	Comment 55 • 22 years ago

Guys, let's stay cool here.  This bug does not block people from developing 
Mozilla in a serious manner -- which is why AlecF is marking this bug normal.  
(He's also the bug owner and it is ultimately his decision.  Unless you want to 
take the bug off his hands and attempt fixing it yourself.)

I do not see any expletives or foul language from AlecF.  As I see it, this bug 
is making QA on bug 152504 harder, yes -- but if you poke around 
lxr.mozilla.org, you should be able to find an XUL file which corresponds to 
your file bookmarks page.

You can work around your problem, Mr. King.  It just takes a little exploration 
on lxr of the source code.  I know, because (a) all the XUL in our chrome is 
duplicated on LXR, and (b) I've done it before.  I'd do it now, except I don't 
have a Mozilla build handy atm (public computer).

	Daniel Veditz [:dveditz]
	Comment 56 • 22 years ago

Don't twist my words, this is in no way, shape, or form a blocking bug. The fact
that this happily sat here for months targeted at "Future" shows that the vast
majority of developers and testers are not blocked by this.

I'm sorry if you personally are blocked by this, but blocking a single
individual doesn't magically vault a bug's importance above "critical"
(crash/data-loss). Use your head.

	David G King
	Comment 57 • 22 years ago

OK, so I took the definition of Blocker as a strict definition. Comes from
working in the tax business on the government side of things where no
interpretation is allowed.

Anyway, I'd like to offer to update http://bugzilla.mozilla.org/bug_status.html
to expand of the definitions so a newbie like myself doesn't do something really
stupid like I just have. I someone could point me to a contact person for that
page, I can get started.

Oh, yes this is way off topic, so apologies for the spam.

	David G King
	Comment 58 • 22 years ago

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

	timeless Assignee
	Comment 59 • 21 years ago

here's the xbl event handler unhandled code path:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp&rev=1.76&mark=442-444#441


but here's a draft for the html event handler case. yes it complains about evil
stuff. if people don't like it, they can fix it later.

i'll let the errors / warnings from my jsconsole speak for themselves:

Warning: function onclick does not always return a value
Source File: data:text/html,<a onclick="if (0) 1; else return 'this';">a
Line: 1, Column: 28
Source Code:
if (0) 1; else return 'this';

Warning: reference to undefined property event.target.onclick
Source File: data:text/html,<a onmouseover="alert(event.target.onclick);
event.target.onclick='alert(3);if(0)2; else return true;';">b
Line: 1

Warning: function onclick does not always return a value
Source File: data:text/html,<a onmouseover="alert(event.target.onclick);
event.target.setAttribute('onclick','alert(3);if(0)2; else return true;');">b
Line: 1, Column: 33
Source Code:
alert(3);if(0)2; else return true;

Warning: function onclick does not always return a value
Source File: data:text/html,<a
onmouseover="event.target.setAttribute('onclick','alert(3);if(0)2; else return
true;');">b
Line: 1, Column: 33
Source Code:
alert(3);if(0)2; else return true;

	Alec Flett
	Comment 61 • 21 years ago

I like the idea in theory, but what the heck is with
"-moz-evil:lying-event-listener"? How about we try something non-obtuse like:
""
"(none)"
"null"

I vote for the first or the 2nd one.

Also, I'm not a big fan of "data:text/html,<a href=..." but I do like the <a
href= bit. 

	Johnny Stenback (:jst)
	Comment 62 • 21 years ago

Comment on attachment 148847 [details] [diff] [review]
handle html event listeners and handlers

+      PRUint32 lineNo = 0;
+      nsCAutoString url
(NS_LITERAL_CSTRING("-moz-evil:lying-event-listener"));

Yeah, I'm with alecf here, this won't do. I vote for "(none)" too, thought that
should be localized, so maybe "" is easiest and best here.

@@ -1378,26 +1391,40 @@ nsEventListenerManager::CompileEventHand
[...]
+	 PRUint32 lineNo = 0;
+	 nsCAutoString url
(NS_LITERAL_CSTRING("-moz-evil:lying-event-handler"));
+	 nsCOMPtr<nsIContent> content = do_QueryInterface(aCurrentTarget);
+	 if (content) {
+	   nsIDocument *doc = content->GetDocument();
+	   if (doc) {
+	     nsIURI *uri = doc->GetDocumentURI();
+	     if (uri) {
+	       uri->GetSpec(url);
+	       lineNo = 1;
+	     }
+	   }
+	 }

What if aCurrentTarget is a document, and not an nsIContent? Try to QI to
nsIDocument first, if that doesn't work, QI to nsIContent and do what you do
above...

r+sr=jst

	timeless Assignee
	Comment 63 • 21 years ago

Comment on attachment 148847 [details] [diff] [review]
handle html event listeners and handlers

mozilla/content/events/src/nsEventListenerManager.cpp	1.169
mozilla/content/events/src/nsEventListenerManager.h	1.68

	Boris Zbarsky [:bzbarsky]
	Comment 65 • 20 years ago

Comment on attachment 153459 [details] [diff] [review]
stop lying

>Index: nsEventListenerManager.cpp
>+    if (win) {
>+      nsCOMPtr<nsIDOMDocument> domdoc;
>+      win->GetDocument(getter_AddRefs(domdoc));
>+      doc = do_QueryInterface(domdoc);
>+      global = do_QueryInterface(win);
>     }
>+    if (!doc)
>+      doc = do_QueryInterface(aObject);
>+
>+    if (!global)

If win is true, then aObject will not QI to nsIDocument and you can't have
!global.  So I'd do:

if (win) {
  // stuff
} else {
  doc = do_QueryInterface(aObject);

and then:  

>+      if (doc) {
>+        global = doc->GetScriptGlobalObject();
>+      } else {
>+        global = do_QueryInterface(aObject);
>+      }

With that change, r+sr=bzbarsky

	neil@parkwaycc.co.uk
	Comment 66 • 20 years ago

Comment on attachment 153459 [details] [diff] [review]
stop lying

>-    doc = do_QueryInterface(aObject);
>-
>     nsCOMPtr<nsIScriptGlobalObject> global;
>-
>-    if (doc) {
>-      global = doc->GetScriptGlobalObject();
>-    } else {
>-      global = do_QueryInterface(aObject);
>     }
> 
>     if (global) {
>       context = global->GetContext();
>     }
So, aObject is either an nsIDocument, from which we get its script global
object, or it already is an nsIScriptGlobalObject.

>+    nsCOMPtr<nsIDOMWindow> win(do_QueryInterface(aObject));
>     nsCOMPtr<nsIScriptGlobalObject> global;
>+    if (win) {
>+      nsCOMPtr<nsIDOMDocument> domdoc;
>+      win->GetDocument(getter_AddRefs(domdoc));
>+      doc = do_QueryInterface(domdoc);
>+      global = do_QueryInterface(win);
>     }
Now, if aObject is a script global object and a dom window, then we use it.

>+    if (!doc)
>+      doc = do_QueryInterface(aObject);
Otherwise, if it's a doc (but not a dom window with a doc) then we'll use the
doc's SGO below.

>+
>+    if (!global)
>+      if (doc) {
>+        global = doc->GetScriptGlobalObject();
In this case aObject is a dom window but not a script global object.

>+      } else {
>+        global = do_QueryInterface(aObject);
In this case aObject is a script global object but not a dom window.

>+      }
> 
>     if (global) {
>       context = global->GetContext();
>     }

Rearranging gives me this:
nsCOMPtr<nsIScriptGlobalObject> global(do_QueryInterface(aObject));
if (!global) {
  nsCOMPtr<nsIDOMWindow> win(do_QueryInterface(aObject));
  if (win) {
    nsCOMPtr<nsIDOMDocument> domdoc;
    win->GetDocument(getter_AddRefs(domdoc));
    doc = do_QueryInterface(domdoc);
  }
  if (!doc) {
    doc = do_QueryInterface(aObject);
  }
  if (doc) {
    global = doc->GetScriptGlobalObject();
  }
}

Now I believe a dom window must be an SGO. If this is true then the above
reduces to:
nsCOMPtr<nsIScriptGlobalObject> global(do_QueryInterface(aObject));
if (!global) {
  doc = do_QueryInterface(aObject);
  if (doc) {
    global = doc->GetScriptGlobalObject();
  }
}

Now I don't think a document can be an SGO, in which case I can rearrange
again:
doc = do_QueryInterface(aObject);
nsCOMPtr<nsIScriptGlobalObject> global;
if (doc) {
  global = doc->GetScriptGlobalObject();
} else {
  global = do_QueryInterface(aObject);
}

Looks familiar? ;-)

	Boris Zbarsky [:bzbarsky]
	Comment 67 • 20 years ago

> Now I believe a dom window must be an SGO. If this is true then the above
> reduces to:

This reduction no longer sets doc when aObject is a window.  That's the bug
timeless is trying to fix.

	timeless Assignee
	Comment 68 • 20 years ago

mozilla/content/events/src/nsEventListenerManager.cpp 	1.174

I claim that this is "mostly" fixed.
CAPS doesn't do the right thing, but that should be covered in another bug.
Line numbers are frequently wrong, but the summary is about always including a 
source file filename (which we should now do), not (correct) line numbers.

and most importantly, comment 0 focussed on an event handler which is what this 
last change affected.

If there are other cases, people are welcome to file other bugs against 
appropriate components.

	Mrten
	Comment 69 • 20 years ago

http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventListenerManager.cpp#1205

timeless, you forgot to remove the reference to "-moz-evil:lying-event-handler"

	timeless Assignee
	Comment 70 • 20 years ago

bugzilla@ii.nl: don't reopen my bugs. if you want to file a bug and change the
text to something else, feel free. this bug is fixed.

	Mrten
	Comment 71 • 20 years ago

i'm sorry, only trying to help...

i was, reading the bug, under the impression that the r+sr from jst from comment
#62 specifically said "this text won't do". same drift from alecf in comment #61.

seems like a very minor patch which really belongs here, so i saw no need for a
new bug...

	timeless Assignee
	Comment 72 • 20 years ago

Comment on attachment 153459 [details] [diff] [review]
stop lying

seeking approval for:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla%2Fcontent%2Fevents%2Fsrc%2Fns
EventListenerManager.cpp&mark=1.169,1.174

	Christopher Aillon (sabbatical, not receiving bugmail)
	Comment 73 • 20 years ago

Comment on attachment 148847 [details] [diff] [review]
handle html event listeners and handlers

Too late for 1.7.6

	Christopher Aillon (sabbatical, not receiving bugmail)
	Comment 74 • 20 years ago

Comment on attachment 153459 [details] [diff] [review]
stop lying

Too late for 1.7.6

	timeless Assignee
	Comment 75 • 20 years ago

caillon@gmail.com: then i'm now seeking approval for 1.7.7, how precisely am i
supposed to do that? i asked for approval two months ago.