Re: JavaScript (strict) Warning from FF mochitest-plain test.
Dear Steve, Thank you for the feedback. Also I would like to thank other people whose e-mails also give an glimpse of the depth of the problem. On 2014/10/03 4:49, Steve Fink wrote: On 10/02/2014 11:59 AM, ISHIKAWA,chiaki wrote: Hi, I was looking at a large number of JavaScript (strict) warnings, and errors from the recording of |make mozmill| test run of locall-built DEBUG BUILD of TB last several days after a refresh of C-C source tree. The number of such has increased very much both - due to the seemingly stricter checks done by JS infrastructure, and - new carelessly written code, I think. Did you compare the count from a while ago to the count today? I was kind of assuming that we've always had lots of these. I just did. It is true that warnings and errors come and go. Only the hard ones (or ignored ones) remain constantly. That said, compared with the local mochitest-plain done on June 18, the different types of the JS warnings in the log jumped from 45 to 67, and the total count of such warnings jumped from 82 to 594. In the case of TB, it was worse: from |make mozmill| test suite runs. June 18: 11 types, 137 count.=== well manageable. Sept 28: 89 types, 5699 counts. === Ha! (Gasp. OK, a single type of getter only warning accounted for about 3100 lines. But still there are other 2200 lines of warnings.) Now, the particular # mentioned above was culled by the lines picked up by the following command ($1 is the log file recorded by script command from which the test was invoked.) --- begin quote echo echo echo JavaScript strict warning echo jquery.js and jquery-ui.js are ignored. echo echo # # 24.4.0/comm-esr had jquery-1.5.1.min.js # grep ^JavaScript $1 | sort | uniq -c | sort -r -n | egrep -v (jquery.js|jquery-ui.js|jquery-1.5.1.min.js) --- end quote --- This means that, for the moment, I am IGNORING the C++ side bugs/warnings to limit the scope of discussion. And there are so many of them, I have no idea what to make of them actually. And, of course, outright ERROR are also excluded mostly. They seem to have jumped, too, but I have not bothered to check now. To me, the whole situation looks to be a serious regression in terms of quality assurance and testing.] BTW, C-C source tree had more such warnings than June 18 figures, say, around the beginning of 2012. But thanks to the efforts of aceman and many others and my effort to alert the situation in Bug 826732 - JavaScript strict warning seen during make mozmill run of TB (DEBUG BUILD) the warnings diminished to the June 18th figures, and only very hard ones remain there. But suddenly the number has exploded out of hands after this summer, so to speak. Regarding the warnings/errors reported in DEBUG build test runs, after hearing the opinions voiced, I have no idea how to tackle this situation from the perspective of a TB user who occasionally experiences real-world bugs and reports them, and in some cases tries to create a patch and is overwhelmed by the bad noise/signal ratio in the log when the patch is applied and tested locally. I completely share the sentiment that I couldn't believe that such serious-sounding errors could have existed before my patch, but eventually discovered that they did. mentioned by Steve. At least, a concerted efforts over 24 months or so had diminished the numbers of conspicuous warnings to manageable (easy to monitor and pay attention occasionally) in TB JS area until June this year. - I am for a strict check and I am of the opinion that we should remove such warnings/errors. They often cover real bugs underneath. Once I see the log, it is difficult to trust mozilla software completely :-) - How do we assign precious man-power to fixing these issues, which seems to be delegated to a second-class citizen in the mozilla dev community, is a problem, I think. - Raising the awareness of very poor situation is a must, I think. -- That it hurts the quality, and a quick detection of real-world bugs is now difficult because of noise ought to be understood by many who are contributing new code. -- The bad signal-to-noise ratio does not help someone creating a new patch since it may mask the newly introduced issues. - Automatic detection and assignment of bugzilla mentioned by Mike de Boer may be a worthwhile idea to pursue. I think a discussion about this is in order. (After all, we get a regular intermittent (?)/orange bugzilla today for permanent(?) orange failures. They get reported to respective bugzilla entries, I think.) Setting assignment aspect aside for a moment, at least, automatic detection of such errors and warnings and see whether it has already been filed, and if not, create the list of not-yet-filed such warnings/errors in a web page or something [in a manner so that a filing of the bug and possible assignment from the
Re: JavaScript (strict) Warning from FF mochitest-plain test.
I'm in the process of doing that for uncaught rejections, fwiw. On 03/10/14 03:34, Boris Zbarsky wrote: On 10/2/14, 4:53 PM, Jeff Gilbert wrote: If you want to guarantee it, don't warn, assert. Note that mochitest-plain tests will go orange on unexpected uncaught exceptions. Not least because we added that pretty early on in their evolution. We're nowhere close to doing that with mochitest-chrome or browser-chrome. :( But it sure would be nice; I've had several instances of tests being effectively self-disabling because of an exception they were throwing early on. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform -- David Rajchenbach-Teller, PhD Performance Team, Mozilla signature.asc Description: OpenPGP digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: JavaScript (strict) Warning from FF mochitest-plain test.
We should most likely assert when we have strict violations in chrome JS. We should *definitely* assert when we have JS errors in chrome JS. I don't see how there should be any excuse for either of these cases. We certainly can't assert on non-browser JS errors, but we should keep our own scripts clean. - Original Message - From: chiaki ISHIKAWA ishik...@yk.rim.or.jp To: dev-platform@lists.mozilla.org Sent: Thursday, October 2, 2014 11:59:04 AM Subject: JavaScript (strict) Warning from FF mochitest-plain test. Hi, I was looking at a large number of JavaScript (strict) warnings, and errors from the recording of |make mozmill| test run of locall-built DEBUG BUILD of TB last several days after a refresh of C-C source tree. The number of such has increased very much both - due to the seemingly stricter checks done by JS infrastructure, and - new carelessly written code, I think. But the sheer number of them overwhelmed me and I began wondering is FF faring better? I don't think so. Here are the list of such errors summarized from the log of |./mach mochitest-plain| of locally-built DEBUG BUILD of FF (from M-C portion of C-C source tree). The # at the beginning of each line is the frequency of the occurrences. [I was looking at getter only message which is printed for more than 3K times during |make mozmill| test for TB for Services when checking the health of logs from FF tests came to my mind.) I think we need a sort of sheriff to look at these warnings and file bugzilla entries accordingly from time to time: I say this because it seems not many people look at debug build run of test suite often (well, at least on TB side. I don't know if TB debug run is created regularly.) Excerpt from summary of the log : ./mach mochitest-plain JavaScript strict warning jquery.js and jquery-ui.js are ignored. 206 JavaScript error: file:///home/ishikawa/ff-objdir-tb3/dist/bin/components/nsHandlerService.js, line 891: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get] 99 JavaScript strict warning: , line 0: TypeError: setting getter-only property PrivateBrowsingUtils 66 JavaScript strict warning: , line 0: TypeError: setting getter-only property Services 33 JavaScript strict warning: , line 0: TypeError: setting getter-only property TelemetryStopwatch 33 JavaScript strict warning: , line 0: TypeError: setting getter-only property Task 33 JavaScript strict warning: , line 0: TypeError: setting getter-only property PlacesUtils 33 JavaScript strict warning: , line 0: TypeError: setting getter-only property NewTabUtils 11 JavaScript strict warning: chrome://specialpowers/content/specialpowersAPI.js, line 214: ReferenceError: reference to undefined property x.SpecialPowers_wrappedObject 4 JavaScript error: resource://gre/modules/XPCOMUtils.jsm, line 110: SyntaxError: missing ; before statement 4 JavaScript error: http://example.com/tests/content/base/test/bug696301-script-1.js, line 3: ReferenceError: c is not defined 4 JavaScript error: , line 0: uncaught exception: 2147500033 3 JavaScript strict warning: resource://specialpowers/MockFilePicker.jsm, line 92: SyntaxError: octal literals and octal escape sequences are deprecated 3 JavaScript strict warning: chrome://specialpowers/content/specialpowersAPI.js, line 662: ReferenceError: reference to undefined property wrapPrivileged(...).classes 3 JavaScript error: , line 0: uncaught exception: Permission denied to add http://mochi.test:/%sas a protocol handler 2 JavaScript strict warning: chrome://specialpowers/content/specialpowersAPI.js, line 665: ReferenceError: reference to undefined property wrapPrivileged(...).results 2 JavaScript error: resource://gre/modules/vtt.jsm, line 1261: TypeError: self.window.VTTRegion is not a constructor 2 JavaScript error: , line 0: uncaught exception: Permission denied to add http://remotehost:/%s as a content or protocol handler 2 JavaScript error: , line 0: uncaught exception: Permission denied to add ftp://mochi.test:/%s as a content or protocol handler 2 JavaScript error: , line 0: uncaught exception: Permission denied to add foo://mochi.test:/%s as a content or protocol handler 2 JavaScript error: , line 0: uncaught exception: 2152923148 1 JavaScript warning: resource://gre/modules/Preferences.jsm, line 381: mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create 1 JavaScript strict warning: resource://gre/modules/Webapps.jsm, line 539: ReferenceError: reference to undefined property Components.classes['@mozilla.org/app-migrator;1'] 1 JavaScript strict warning: resource://gre/modules/Webapps.jsm, line 389: ReferenceError:
Re: JavaScript (strict) Warning from FF mochitest-plain test.
On 10/02/2014 11:59 AM, ISHIKAWA,chiaki wrote: Hi, I was looking at a large number of JavaScript (strict) warnings, and errors from the recording of |make mozmill| test run of locall-built DEBUG BUILD of TB last several days after a refresh of C-C source tree. The number of such has increased very much both - due to the seemingly stricter checks done by JS infrastructure, and - new carelessly written code, I think. Did you compare the count from a while ago to the count today? I was kind of assuming that we've always had lots of these. But the sheer number of them overwhelmed me and I began wondering is FF faring better? I don't think so. No. It's awful. I wasted a couple of hours the other day on similar errors in xpcshell-test. I couldn't believe that such serious-sounding errors could have existed before my patch, but eventually discovered that they did. I think we need a sort of sheriff to look at these warnings and file bugzilla entries accordingly from time to time: I say this because it seems not many people look at debug build run of test suite often (well, at least on TB side. I don't know if TB debug run is created regularly.) Yeah, it seems nobody looks at it. Except when some random developer happens to break a test, and tries to figure out what warnings/errors are real and what are expected noise. It's pretty awful. Then again, we can't get people to fix intermittent test failures, and instead disable tests frequently. So finding such a sheriff is going to be... challenging. The understanding of many of these warnings tends to be specific to very localized parts of the codebase, so a single sheriff would need to pester a lot of people. Or not -- actually, a lot of these are probably easily solvable. If someone were willing to triage, they could probably at least cut down the number by quite a bit if they were willing to ignore the harder ones. These references to undefined whatever seem to have appeared suddenly thanks to the stricter checking of JS infrastructure. I wonder if that is a side effect of the 'let' TDZ (temporal dead zone) changes. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: JavaScript (strict) Warning from FF mochitest-plain test.
See bug 807862 - Use strict mode for all builtin JS Note that some of those warnings are from SpiderMonkey's non-standard extra warnings mode, not ES5 strict mode. To confuse matters, extra warnings were called strict warnings before ES5 and are controlled by the javascript.options.strict pref. SpiderMonkey already forces ES5 strict mode and extra warnings mode for all self-hosted JS: bug 784295. chris On 10/2/14 12:22 PM, Jeff Gilbert wrote: We should most likely assert when we have strict violations in chrome JS. We should *definitely* assert when we have JS errors in chrome JS. I don't see how there should be any excuse for either of these cases. We certainly can't assert on non-browser JS errors, but we should keep our own scripts clean. - Original Message - From: chiaki ISHIKAWA ishik...@yk.rim.or.jp To: dev-platform@lists.mozilla.org Sent: Thursday, October 2, 2014 11:59:04 AM Subject: JavaScript (strict) Warning from FF mochitest-plain test. Hi, I was looking at a large number of JavaScript (strict) warnings, and errors from the recording of |make mozmill| test run of locall-built DEBUG BUILD of TB last several days after a refresh of C-C source tree. The number of such has increased very much both - due to the seemingly stricter checks done by JS infrastructure, and - new carelessly written code, I think. But the sheer number of them overwhelmed me and I began wondering is FF faring better? I don't think so. Here are the list of such errors summarized from the log of |./mach mochitest-plain| of locally-built DEBUG BUILD of FF (from M-C portion of C-C source tree). The # at the beginning of each line is the frequency of the occurrences. [I was looking at getter only message which is printed for more than 3K times during |make mozmill| test for TB for Services when checking the health of logs from FF tests came to my mind.) I think we need a sort of sheriff to look at these warnings and file bugzilla entries accordingly from time to time: I say this because it seems not many people look at debug build run of test suite often (well, at least on TB side. I don't know if TB debug run is created regularly.) Excerpt from summary of the log : ./mach mochitest-plain JavaScript strict warning jquery.js and jquery-ui.js are ignored. 206 JavaScript error: file:///home/ishikawa/ff-objdir-tb3/dist/bin/components/nsHandlerService.js, line 891: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get] 99 JavaScript strict warning: , line 0: TypeError: setting getter-only property PrivateBrowsingUtils 66 JavaScript strict warning: , line 0: TypeError: setting getter-only property Services 33 JavaScript strict warning: , line 0: TypeError: setting getter-only property TelemetryStopwatch 33 JavaScript strict warning: , line 0: TypeError: setting getter-only property Task 33 JavaScript strict warning: , line 0: TypeError: setting getter-only property PlacesUtils 33 JavaScript strict warning: , line 0: TypeError: setting getter-only property NewTabUtils 11 JavaScript strict warning: chrome://specialpowers/content/specialpowersAPI.js, line 214: ReferenceError: reference to undefined property x.SpecialPowers_wrappedObject 4 JavaScript error: resource://gre/modules/XPCOMUtils.jsm, line 110: SyntaxError: missing ; before statement 4 JavaScript error: http://example.com/tests/content/base/test/bug696301-script-1.js, line 3: ReferenceError: c is not defined 4 JavaScript error: , line 0: uncaught exception: 2147500033 3 JavaScript strict warning: resource://specialpowers/MockFilePicker.jsm, line 92: SyntaxError: octal literals and octal escape sequences are deprecated 3 JavaScript strict warning: chrome://specialpowers/content/specialpowersAPI.js, line 662: ReferenceError: reference to undefined property wrapPrivileged(...).classes 3 JavaScript error: , line 0: uncaught exception: Permission denied to add http://mochi.test:/%sas a protocol handler 2 JavaScript strict warning: chrome://specialpowers/content/specialpowersAPI.js, line 665: ReferenceError: reference to undefined property wrapPrivileged(...).results 2 JavaScript error: resource://gre/modules/vtt.jsm, line 1261: TypeError: self.window.VTTRegion is not a constructor 2 JavaScript error: , line 0: uncaught exception: Permission denied to add http://remotehost:/%s as a content or protocol handler 2 JavaScript error: , line 0: uncaught exception: Permission denied to add ftp://mochi.test:/%s as a content or protocol handler 2 JavaScript error: , line 0: uncaught exception: Permission denied to add foo://mochi.test:/%s as a content or protocol handler 2 JavaScript error: , line 0: uncaught exception: 2152923148 1 JavaScript warning:
Re: JavaScript (strict) Warning from FF mochitest-plain test.
On 02/10/14 12:52, Chris Peterson wrote: SpiderMonkey already forces ES5 strict mode and extra warnings mode for all self-hosted JS: bug 784295. The problem is that this only generates an exception, not a test failure, so if the error is in code that isn't depended upon (also a problem), then it just disappears. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: JavaScript (strict) Warning from FF mochitest-plain test.
Thanks for bringing this up! So ever since extra super awesome strict warning were turned on we indeed have to endure longer tail logs during test runs and when running debug builds. Just like I get the information telling me which video card my computer has for about the 1000th time. All in all, for someone looking for it, enough source for frustration! Now that we got past that point, let’s try to solve this! So how about starting with a tool that scrapes the warnings from the tbpl logs? I’m thinking it might be a cool Treeherder plugin. We got logs filenames and line numbers, a vcs, so we can even run a blame command and auto-create bugs and and auto-assign them to the one who introduced the error (or at least knows more about it)… Most of the errors I usually see are about uninitialised variables and undefined properties. JS errors are very bad. If they’re expected for some reason, it’d at least be nice to put a try…catch block around it to suppress the log clutter. These are all _very_ easy to fix. I think a tool would help greatly. Mike. On 02 Oct 2014, at 21:52, Chris Peterson cpeter...@mozilla.com wrote: See bug 807862 - Use strict mode for all builtin JS Note that some of those warnings are from SpiderMonkey's non-standard extra warnings mode, not ES5 strict mode. To confuse matters, extra warnings were called strict warnings before ES5 and are controlled by the javascript.options.strict pref. SpiderMonkey already forces ES5 strict mode and extra warnings mode for all self-hosted JS: bug 784295. chris On 10/2/14 12:22 PM, Jeff Gilbert wrote: We should most likely assert when we have strict violations in chrome JS. We should *definitely* assert when we have JS errors in chrome JS. I don't see how there should be any excuse for either of these cases. We certainly can't assert on non-browser JS errors, but we should keep our own scripts clean. - Original Message - From: chiaki ISHIKAWA ishik...@yk.rim.or.jp To: dev-platform@lists.mozilla.org Sent: Thursday, October 2, 2014 11:59:04 AM Subject: JavaScript (strict) Warning from FF mochitest-plain test. Hi, I was looking at a large number of JavaScript (strict) warnings, and errors from the recording of |make mozmill| test run of locall-built DEBUG BUILD of TB last several days after a refresh of C-C source tree. The number of such has increased very much both - due to the seemingly stricter checks done by JS infrastructure, and - new carelessly written code, I think. But the sheer number of them overwhelmed me and I began wondering is FF faring better? I don't think so. Here are the list of such errors summarized from the log of |./mach mochitest-plain| of locally-built DEBUG BUILD of FF (from M-C portion of C-C source tree). The # at the beginning of each line is the frequency of the occurrences. [I was looking at getter only message which is printed for more than 3K times during |make mozmill| test for TB for Services when checking the health of logs from FF tests came to my mind.) I think we need a sort of sheriff to look at these warnings and file bugzilla entries accordingly from time to time: I say this because it seems not many people look at debug build run of test suite often (well, at least on TB side. I don't know if TB debug run is created regularly.) Excerpt from summary of the log : ./mach mochitest-plain JavaScript strict warning jquery.js and jquery-ui.js are ignored. 206 JavaScript error: file:///home/ishikawa/ff-objdir-tb3/dist/bin/components/nsHandlerService.js, line 891: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get] 99 JavaScript strict warning: , line 0: TypeError: setting getter-only property PrivateBrowsingUtils 66 JavaScript strict warning: , line 0: TypeError: setting getter-only property Services 33 JavaScript strict warning: , line 0: TypeError: setting getter-only property TelemetryStopwatch 33 JavaScript strict warning: , line 0: TypeError: setting getter-only property Task 33 JavaScript strict warning: , line 0: TypeError: setting getter-only property PlacesUtils 33 JavaScript strict warning: , line 0: TypeError: setting getter-only property NewTabUtils 11 JavaScript strict warning: chrome://specialpowers/content/specialpowersAPI.js, line 214: ReferenceError: reference to undefined property x.SpecialPowers_wrappedObject 4 JavaScript error: resource://gre/modules/XPCOMUtils.jsm, line 110: SyntaxError: missing ; before statement 4 JavaScript error: http://example.com/tests/content/base/test/bug696301-script-1.js, line 3: ReferenceError: c is not defined 4 JavaScript error: , line 0: uncaught exception: 2147500033 3 JavaScript strict warning:
Re: JavaScript (strict) Warning from FF mochitest-plain test.
Indeed, which is why I recommended assert, not warn. Warnings are basically useless from a CI standpoint. If you want to guarantee it, don't warn, assert. -Jeff - Original Message - From: Martin Thomson m...@mozilla.com To: dev-platform@lists.mozilla.org Sent: Thursday, October 2, 2014 1:05:44 PM Subject: Re: JavaScript (strict) Warning from FF mochitest-plain test. On 02/10/14 12:52, Chris Peterson wrote: SpiderMonkey already forces ES5 strict mode and extra warnings mode for all self-hosted JS: bug 784295. The problem is that this only generates an exception, not a test failure, so if the error is in code that isn't depended upon (also a problem), then it just disappears. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: JavaScript (strict) Warning from FF mochitest-plain test.
On 2014-10-02, 4:40 PM, Mike de Boer wrote: Thanks for bringing this up! So ever since extra super awesome strict warning were turned on we indeed have to endure longer tail logs during test runs and when running debug builds. Just like I get the information telling me which video card my computer has for about the 1000th time. All in all, for someone looking for it, enough source for frustration! Now that we got past that point, let’s try to solve this! So how about starting with a tool that scrapes the warnings from the tbpl logs? I’m thinking it might be a cool Treeherder plugin. We got logs filenames and line numbers, a vcs, so we can even run a blame command and auto-create bugs and and auto-assign them to the one who introduced the error (or at least knows more about it)… Most of the errors I usually see are about uninitialised variables and undefined properties. JS errors are very bad. If they’re expected for some reason, it’d at least be nice to put a try…catch block around it to suppress the log clutter. These are all _very_ easy to fix. I think a tool would help greatly. Are we going to spend the time to fix these, however? For context, for as long as I remember, if you kept the browser console open and used the browser, we'd occasionally report an unhandled chrome JS errors (for example when attempting to access properties off of null variables). And those are not even strict errors. I've occasionally filed bugs for them, but my impression was that unless I can construct a case for those warnings to map to a user visible bug, they would not be fixed. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: JavaScript (strict) Warning from FF mochitest-plain test.
Are we going to spend the time to fix these, however? I’d be up for that, certainly! For context, for as long as I remember, if you kept the browser console open and used the browser, we'd occasionally report an unhandled chrome JS errors (for example when attempting to access properties off of null variables). And those are not even strict errors. I've occasionally filed bugs for them, but my impression was that unless I can construct a case for those warnings to map to a user visible bug, they would not be fixed. Hence my - perhaps controversial - idea to auto-create and auto-assign bugs. So for new warnings that appear in the integration branch and m-c logs we could even make those bugs block the bugs that introduced them. Mike. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: JavaScript (strict) Warning from FF mochitest-plain test.
On 10/2/14, 4:53 PM, Jeff Gilbert wrote: If you want to guarantee it, don't warn, assert. Note that mochitest-plain tests will go orange on unexpected uncaught exceptions. Not least because we added that pretty early on in their evolution. We're nowhere close to doing that with mochitest-chrome or browser-chrome. :( But it sure would be nice; I've had several instances of tests being effectively self-disabling because of an exception they were throwing early on. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
