52116 - javascript strict warning/error doesn't always contain Source File filename

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P3)

Description

[Henrik Gemal]

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?

Comment 1

[Phil Schwartau]

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

Comment 2

[Phil Schwartau]

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

Comment 3

[Daniel Veditz [:dveditz]]

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.

Comment 4

[Henrik Gemal]

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...

Comment 5

[Brendan Eich [:brendan]]

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

/be

Comment 6

[Alec Flett]

I've got a fix. Attaching for review.

Comment 7

[Alec Flett]

Attachment: tell JS what the URL and function/attribute name of compiled functions are

Comment 8

[Daniel Veditz [:dveditz]]

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.

Comment 9

[Alec Flett]

Attachment: better fix than my copy-n-paste garbage (thanks dveditz)

Comment 10

[Daniel Veditz [:dveditz]]

r=dveditz for this one, too. 

Comment 11

[Mike McCabe]

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

Comment 12

[Alec Flett]

I removed the excess cname variable - thanks mccabe.

I still need a sr=hyatt on this one

Comment 13

[David Hyatt]

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

Comment 14

[Alec Flett]

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

Comment 15

[Alec Flett]

fix checked in.

Comment 16

[Henrik Gemal]

With build 20001027 I'm still getting:

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

Comment 17

[Henrik Gemal]

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.

Comment 18

[Alec Flett]

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

Comment 19

[Alec Flett]

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

Comment 20

[Alec Flett]

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.

Comment 21

[Henrik Gemal]

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

Comment 22

[Alec Flett]

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

Comment 23

[Henrik Gemal]

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

Comment 24

[Alec Flett]

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

Comment 25

[Henrik Gemal]

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

Comment 26

[Henrik Gemal]

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

Comment 27

[John Bandhauer]

Are you gonna fix all of bug 11240 too?

Comment 28

[Paul Chen]

nav triage team:

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

Comment 29

[Alex Vincent [:WeirdAl]]

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

Comment 30

[Henrik Gemal]

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...

Comment 31

[Henrik Gemal]

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

Comment 32

[Henrik Gemal]

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

Comment 33

[Alex Vincent [:WeirdAl]]

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

Comment 34

[Alec Flett]

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

Comment 35

[David G King]

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

Comment 36

[David G King]

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.

Comment 37

[Alec Flett]

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.

Comment 38

[David G King]

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.

Comment 39

[Alex Vincent [:WeirdAl]]

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.

Comment 40

[David G King]

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

Comment 41

[Alex Vincent [:WeirdAl]]

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?

Comment 42

[David G King]

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...".

Comment 43

[Robert Ginda]

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.

Comment 44

[David G King]

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.

Comment 45

[David G King]

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.

Comment 46

[Alec Flett]

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.

Comment 47

[Alec Flett]

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.

Comment 48

[David G King]

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

Comment 49

[Daniel Veditz [:dveditz]]

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.

Comment 50

[Robert Ginda]

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.

Comment 51

[Alec Flett]

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

Comment 52

[David G King]

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.

Comment 53

[Alec Flett]

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.

Comment 54

[David G King]

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.

Comment 55

[Alex Vincent [:WeirdAl]]

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).

Comment 56

[Daniel Veditz [:dveditz]]

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.

Comment 57

[David G King]

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.

Comment 58

[David G King]

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

Comment 59

[timeless]

Attachment: handle html events

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;

Comment 60

[timeless]

Attachment: handle html event listeners and handlers

Comment 61

[Alec Flett]

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. 

Comment 62

[Johnny Stenback (:jst)]

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

Comment 63

[timeless]

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

Comment 64

[timeless]

Attachment: stop lying

Comment 65

[Boris Zbarsky [:bzbarsky]]

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

Comment 66

[[e-mail]]

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? ;-)

Comment 67

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 68

[timeless]

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.

Comment 69

[Mrten]

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"

Comment 70

[timeless]

[e-mail]: 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.

Comment 71

[Mrten]

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...

Comment 72

[timeless]

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

Comment 73

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Too late for 1.7.6

Comment 74

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Too late for 1.7.6

Comment 75

[timeless]

[e-mail]: 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.