The War on Warnings Redux
TL;DR - We have reduced the amount of runtime warnings during testing by 90%. As of today we are down to 66K warnings during linux64 debug testing; this is down from 600K back in June. There are bugs on file for most of the remaining top offenders blocking tracking bug 765224 [1]. It's time to declare victory, or at least transition to a skirmish, or perhaps an extended conflict. It's been a fun ride, I've more than surpassed my goal of a sub 100K count and it's time for me to write some real code again. Reducing the amount of warnings has made it *much* easier to weed out real bugs and many of the remaining warnings most likely *are* bugs. This is where you can help out: take a look at the tracking bug (and the following list) and see if there is a warning in an area you can help out with. Even just triaging by adding a needinfo? for folks who could help out would be super helpful. For those interested I have put up my (somewhat sketchy) set of scripts for parsing logs and counting warnings on github [2]. Contributions and feedback are welcome. Going forward I can think of a few interesting projects for the automation inclined: - Setup some sort of daily warning analysis, bisect and send out nag emails - Add support for automatically filing bugs to the log spam scripts - Turn the tree red if warnings go over some threshold - Get to a warning free startup state, add tests that enforce no new warnings on startup Again, please take a moment to go over the list, file more bugs and clean out unhelpful warnings so that we can focus on the real issues Big thanks to smaug, bz, dholbert and everyone else who helped get patches up, reviewed and in general dealt with my incessant nagging. -e The final countdown: TOP 40 == 3997 [N] WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file dom/events/EventListenerManager.cpp, line 370 2523 [N] WARNING: '!mContentCache.CacheEditorRect(this, aIMENotification)', file widget/PuppetWidget.cpp, line 833 2523 [N] WARNING: '!editorRectEvent.mSucceeded', file widget/ContentCache.cpp, line 256 1156 [N] WARNING: NS_ENSURE_TRUE(selCon) failed: file editor/libeditor/nsEditor.cpp, line 631 1112 [N] WARNING: No inner window available!: file dom/base/nsGlobalWindow.cpp, line 10013 1080 [N] WARNING: Failed to unlock the wakelock.: '!rv.Failed()', file dom/html/HTMLMediaElement.cpp, line 2354 1049 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file extensions/spellcheck/src/mozInlineSpellChecker.cpp, line 1995 948 [N] WARNING: Failed to retarget HTML data delivery to the parser thread.: file parser/html/nsHtml5StreamParser.cpp, line 951 918 [N] WARNING: NS_ENSURE_TRUE(selection-RangeCount()) failed: file editor/libeditor/nsHTMLEditRules.cpp, line 320 892 [N] WARNING: Silently denied access to property |undefined|: Access to privileged JS object not permitted (@resource://gre/modules/commonjs/toolkit/loader.js - resource://gre/modules/devtools/server/actors/script.js:1149): file js/xpconnect/wrappers/XrayWrapper.cpp, line 208 824 [N] WARNING: attempt to modify an immutable nsStandardURL: file netwerk/base/nsStandardURL.cpp, line 1264 823 [N] WARNING: NS_ENSURE_TRUE(selcon) failed: file editor/libeditor/nsEditor.cpp, line 658 823 [N] WARNING: NS_ENSURE_SUCCESS(res, nullptr) failed with result 0xC1F30001: file editor/libeditor/nsEditor.cpp, line 667 687 [N] WARNING: Trying to spellcheck, but checking seems to be disabled: 'mPendingSpellCheck', file extensions/spellcheck/src/mozInlineSpellChecker.cpp, line 907 684 [N] WARNING: NS_ENSURE_TRUE(aSelection-RangeCount()) failed: file editor/libeditor/nsEditor.cpp, line 3725 684 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file editor/libeditor/nsEditor.cpp, line 3704 683 [N] WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80004005: file editor/libeditor/nsHTMLEditRules.cpp, line 8083 675 [N] WARNING: Re-registering a CID?: file xpcom/components/nsComponentManager.cpp, line 551 655 [N] WARNING: NS_ENSURE_SUCCESS(rv, BadImage(newImage)) failed with result 0x80004005: file image/ImageFactory.cpp, line 248 653 [N] WARNING: NS_ENSURE_TRUE(globalObject globalObject-GetGlobalJSObject()) failed: file dom/base/nsDocument.cpp, line 8323 628 [N] WARNING: Suboptimal indexes for the SQL statement 0x7f30821a34a0 (http://mzl.la/1FuID0j).: file storage/mozStoragePrivateHelpers.cpp, line 109 604 [N] WARNING: NS_ENSURE_SUCCESS(status, status) failed with result 0x805303F4: file dom/security/nsCORSListenerProxy.cpp, line 542 600 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805E000F: file dom/xbl/nsXBLService.cpp, line 732 597 [N] WARNING: '!aCharRect.width', file
Re: The War on Warnings
On Wednesday, June 17, 2015 at 6:49:00 AM UTC-7, kgu...@mozilla.com wrote: 4606 [N] WARNING: No widget found in TabParent::UpdateDimensions: file dom/ipc/TabParent.cpp, line 974 Do you know if this one occurs on b2g or also on other platforms? I added this warning recently in bug 1125325 after smaug said [1]. It seems to be happening a lot, so we should investigate. If you have a bug on file for it please cc me. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1125325#c134 These are all within mochitest-e10s-browser-chrome tests, which AFAICT are not run on B2G, OSX, Windows debug builds. Filed bug 1175631 for the verbose warning. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 2015/06/17 8:29, Eric Rahm wrote: An update on progress. I've landed bugs which should clean up ~180,000 warnings. I have bugs pending for another ~26,000. The latest top 40: *Note: I improved my normalization a bit so it might look a bit different Are there documents explaining why these warnings are generated? I mean, say, I see the first warning during the run of C-C TB's |make mozmill| test and I wonder, what C-C TB is doing wrong to see this Subdocument container has no frame? Maybe each month, we should pick up the top-10 warnings (without the description given below) in the frequency, and ask the author of the patch that inserted the warning to explain the following: this will help the testers to asses the situation, and more importantly, help people maintain the legacy code such C-C TB to figure out what can be possibly wrong in the existing code and how we can fix them.: - severity of the message: INFO, DEBUG, WARNING, etc. - explain why there is a warning (background), - is there going to be a bad thing happening soon after the situation occurs? what bad symptoms should we expect? - what can a caller do to prevent the bad thing happening (I mean the caller of the invocation chain that eventually leads to the particular warning.) Without such information, warnings are not so useful in the log. I see about 25 of the following warnings in C-C TB |make mozmill| run. And I have been wondering OK, if Subdocument container has no frame, is this bad?, is there going to be something bad?, is this message printed because C-C TB code invokes document API incorrectly?, if so, what is the correct procedure, and such. As a few mentioned that there are simply so many warnings in the log and we are overwhelmed, but anything that can help us to cope with the tidal wave of warnings ought to be implemented and tried. And among the warnings, there *ARE* warnings that lead to real bugs. e.g.: 1627 [N] WARNING: Called close() before start()!: 'mStarted', file dom/workers/Message Obviously, the program is sloppy and calling some functions in incorrect order. We should fix this. 1149 [N] WARNING: RemoveObserver() called for unregistered observer: file hal/Hal.cpp, line 205 Again, the program is sloppy and is not keeping track of its resources and thus calling non-existing, phantom observer to be removed. This ought to be fixed. Another real bug is this. Although it did not seem to be in the top 40, I see a few types of messages that make me thing that the shutting down of thread are not properly synchronized at the end of the program termination. Obviously some threads try to dump data while the subsystem/module that handles the writing of the data is already closed/shutdown and the writing fails, etc. This ought to be fixed. These high volume warnings can be very useful if only the description about them can be found easily. I am not claiming the description for EVERY warning, but we can start with top-most 10 warnings (without such description). 18012 [N] WARNING: Subdocument container has no frame: file layout/base/nsDocumentViewer.cpp, line 2520 14816 [N] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x: file docshell/base/nsDocShell.cpp, line 4605 9809 [N] WARNING: No docshells for remote frames!: file dom/base/nsFrameLoader.cpp, line 459 8929 [N] WARNING: Someone passed native anonymous content directly into frame construction. Stop doing that!: file layout/base/nsCSSFrameConstructor.cpp, line 6559 8062 [N] WARNING: Suboptimal indexes for the SQL statement 0x (http://mzl.la/1FuID0j).: file storage/mozStoragePrivateHelpers.cpp, line 109 7760 [N] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file embedding/browser/nsWebBrowser.cpp, line 363 7454 [N] WARNING: anonymous nodes should not be in child lists (bug 439258): file layout/base/RestyleManager.cpp, line 1439 6294 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x: file netwerk/base/nsFileStreams.cpp, line 492 6294 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x: file netwerk/base/nsFileStreams.cpp, line 205 6131 [N] WARNING: No outer window available!: file dom/base/nsGlobalWindow.cpp, line 3920 5207 [N] WARNING: NS_ENSURE_TRUE(domWindow) failed: file embedding/browser/nsDocShellTreeOwner.cpp, line 83 5193 [N] WARNING: NS_ENSURE_TRUE(aInBrowser) failed: file embedding/browser/nsDocShellTreeOwner.cpp, line 79 5073 [N] WARNING: zero axis length: file dom/svg/nsSVGLength2.cpp, line 124 4606 [N] WARNING: No widget found in TabParent::UpdateDimensions: file dom/ipc/TabParent.cpp, line 974 4574 [N] WARNING: Shouldn't call SchedulePaint in a detached pres context: file layout/generic/nsFrame.cpp, line 5181 3891 [N] WARNING: Please do not use mouseenter/leave events in chrome. They
Re: The War on Warnings
On Wed, Jun 17, 2015 at 9:29 AM, Eric Rahm er...@mozilla.com wrote: An update on progress. I've landed bugs which should clean up ~180,000 warnings. I have bugs pending for another ~26,000. Lovely! Thank you. The log size changes are impressive. Top 40 sorted by top level directory: I couldn't help but notice that js/ does not appear there, simply because SpiderMonkey doesn't *have* runtime warnings... Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
4606 [N] WARNING: No widget found in TabParent::UpdateDimensions: file dom/ipc/TabParent.cpp, line 974 Do you know if this one occurs on b2g or also on other platforms? I added this warning recently in bug 1125325 after smaug said [1]. It seems to be happening a lot, so we should investigate. If you have a bug on file for it please cc me. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1125325#c134 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
Do we have a all encompassing meta bug to collect all of the bugs that fix the warnings? — - Milan On Jun 16, 2015, at 19:29 , Eric Rahm er...@mozilla.com wrote: An update on progress. I've landed bugs which should clean up ~180,000 warnings. I have bugs pending for another ~26,000. The latest top 40: *Note: I improved my normalization a bit so it might look a bit different 18012 [N] WARNING: Subdocument container has no frame: file layout/base/nsDocumentViewer.cpp, line 2520 14816 [N] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x: file docshell/base/nsDocShell.cpp, line 4605 9809 [N] WARNING: No docshells for remote frames!: file dom/base/nsFrameLoader.cpp, line 459 8929 [N] WARNING: Someone passed native anonymous content directly into frame construction. Stop doing that!: file layout/base/nsCSSFrameConstructor.cpp, line 6559 8062 [N] WARNING: Suboptimal indexes for the SQL statement 0x (http://mzl.la/1FuID0j).: file storage/mozStoragePrivateHelpers.cpp, line 109 7760 [N] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file embedding/browser/nsWebBrowser.cpp, line 363 7454 [N] WARNING: anonymous nodes should not be in child lists (bug 439258): file layout/base/RestyleManager.cpp, line 1439 6294 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x: file netwerk/base/nsFileStreams.cpp, line 492 6294 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x: file netwerk/base/nsFileStreams.cpp, line 205 6131 [N] WARNING: No outer window available!: file dom/base/nsGlobalWindow.cpp, line 3920 5207 [N] WARNING: NS_ENSURE_TRUE(domWindow) failed: file embedding/browser/nsDocShellTreeOwner.cpp, line 83 5193 [N] WARNING: NS_ENSURE_TRUE(aInBrowser) failed: file embedding/browser/nsDocShellTreeOwner.cpp, line 79 5073 [N] WARNING: zero axis length: file dom/svg/nsSVGLength2.cpp, line 124 4606 [N] WARNING: No widget found in TabParent::UpdateDimensions: file dom/ipc/TabParent.cpp, line 974 4574 [N] WARNING: Shouldn't call SchedulePaint in a detached pres context: file layout/generic/nsFrame.cpp, line 5181 3891 [N] WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file dom/events/EventListenerManager.cpp, line 367 3746 [N] WARNING: Graph thread slowdown?: 'std::abs(framePosition - CurrentDriver()-StateComputedTime()) MillisecondsToMediaTime(5)', file dom/media/MediaStreamGraph.cpp, line 1193 3182 [N] WARNING: Enabling vsync compositor: file gfx/layers/ipc/CompositorParent.cpp, line 674 3042 [N] WARNING: NS_ENSURE_TRUE(mCallback) failed: file dom/base/nsFrameMessageManager.cpp, line 805 2859 [N] WARNING: '!mContentCache.CacheEditorRect(this, aIMENotification)', file widget/PuppetWidget.cpp, line 819 2859 [N] WARNING: '!editorRectEvent.mSucceeded', file widget/ContentCache.cpp, line 499 2690 [N] WARNING: nsWindow::GetNativeData not implemented for this type: file widget/PuppetWidget.cpp, line 1058 2527 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x: file dom/base/nsContentUtils.cpp, line 3712 2526 [N] WARNING: NS_ENSURE_TRUE(domDoc target) failed: file dom/base/nsContentUtils.cpp, line 3657 2478 [N] WARNING: Subdocument container has non-subdocument frame: file layout/base/nsDocumentViewer.cpp, line 2517 2064 [N] WARNING: NS_ENSURE_TRUE(newRoot) failed: file dom/base/nsRange.cpp, line 1132 2038 [N] WARNING: NS_ENSURE_TRUE(newRoot) failed: file dom/base/nsRange.cpp, line 1231 1912 [N] WARNING: NS_ENSURE_TRUE(rootContent) failed: file editor/composer/nsEditorSpellCheck.cpp, line 715 1627 [N] WARNING: Called close() before start()!: 'mStarted', file dom/workers/MessagePort.cpp, line 215 1612 [N] WARNING: NS_ENSURE_TRUE(sf) failed: file docshell/base/nsDocShell.cpp, line 6485 1488 [N] WARNING: NS_ENSURE_TRUE(isFileURI) failed: file dom/base/ThirdPartyUtil.cpp, line 368 1417 [N] WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x: file netwerk/protocol/http/HttpBaseChannel.cpp, line 2077 1417 [N] WARNING: NS_ENSURE_SUCCESS(result, result) failed with result 0x: file docshell/base/nsDocShell.cpp, line 14076 1393 [N] WARNING: Break suggested inside cluster!: file gfx/thebes/gfxTextRun.cpp, line 220 1288 [N] WARNING: NS_ENSURE_TRUE(mDisabledJSAndPlugins) failed: file editor/composer/nsEditingSession.cpp, line 209 1175 [N] WARNING: NS_ENSURE_TRUE(txToRemove) failed: file docshell/shistory/nsSHistory.cpp, line 1318 1151 [N] WARNING: NS_ENSURE_TRUE(selCon) failed: file editor/libeditor/nsEditor.cpp, line 631 1149 [N] WARNING: RemoveObserver() called for unregistered observer: file hal/Hal.cpp, line 205 1142 [N] WARNING: Failed to unlock
Re: The War on Warnings
On Wednesday, June 17, 2015 at 7:40:14 AM UTC-7, Milan Sreckovic wrote: Do we have a all encompassing meta bug to collect all of the bugs that fix the warnings? -- - Milan Bug 765224 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
An update on progress. I've landed bugs which should clean up ~180,000 warnings. I have bugs pending for another ~26,000. The latest top 40: *Note: I improved my normalization a bit so it might look a bit different 18012 [N] WARNING: Subdocument container has no frame: file layout/base/nsDocumentViewer.cpp, line 2520 14816 [N] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x: file docshell/base/nsDocShell.cpp, line 4605 9809 [N] WARNING: No docshells for remote frames!: file dom/base/nsFrameLoader.cpp, line 459 8929 [N] WARNING: Someone passed native anonymous content directly into frame construction. Stop doing that!: file layout/base/nsCSSFrameConstructor.cpp, line 6559 8062 [N] WARNING: Suboptimal indexes for the SQL statement 0x (http://mzl.la/1FuID0j).: file storage/mozStoragePrivateHelpers.cpp, line 109 7760 [N] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file embedding/browser/nsWebBrowser.cpp, line 363 7454 [N] WARNING: anonymous nodes should not be in child lists (bug 439258): file layout/base/RestyleManager.cpp, line 1439 6294 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x: file netwerk/base/nsFileStreams.cpp, line 492 6294 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x: file netwerk/base/nsFileStreams.cpp, line 205 6131 [N] WARNING: No outer window available!: file dom/base/nsGlobalWindow.cpp, line 3920 5207 [N] WARNING: NS_ENSURE_TRUE(domWindow) failed: file embedding/browser/nsDocShellTreeOwner.cpp, line 83 5193 [N] WARNING: NS_ENSURE_TRUE(aInBrowser) failed: file embedding/browser/nsDocShellTreeOwner.cpp, line 79 5073 [N] WARNING: zero axis length: file dom/svg/nsSVGLength2.cpp, line 124 4606 [N] WARNING: No widget found in TabParent::UpdateDimensions: file dom/ipc/TabParent.cpp, line 974 4574 [N] WARNING: Shouldn't call SchedulePaint in a detached pres context: file layout/generic/nsFrame.cpp, line 5181 3891 [N] WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file dom/events/EventListenerManager.cpp, line 367 3746 [N] WARNING: Graph thread slowdown?: 'std::abs(framePosition - CurrentDriver()-StateComputedTime()) MillisecondsToMediaTime(5)', file dom/media/MediaStreamGraph.cpp, line 1193 3182 [N] WARNING: Enabling vsync compositor: file gfx/layers/ipc/CompositorParent.cpp, line 674 3042 [N] WARNING: NS_ENSURE_TRUE(mCallback) failed: file dom/base/nsFrameMessageManager.cpp, line 805 2859 [N] WARNING: '!mContentCache.CacheEditorRect(this, aIMENotification)', file widget/PuppetWidget.cpp, line 819 2859 [N] WARNING: '!editorRectEvent.mSucceeded', file widget/ContentCache.cpp, line 499 2690 [N] WARNING: nsWindow::GetNativeData not implemented for this type: file widget/PuppetWidget.cpp, line 1058 2527 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x: file dom/base/nsContentUtils.cpp, line 3712 2526 [N] WARNING: NS_ENSURE_TRUE(domDoc target) failed: file dom/base/nsContentUtils.cpp, line 3657 2478 [N] WARNING: Subdocument container has non-subdocument frame: file layout/base/nsDocumentViewer.cpp, line 2517 2064 [N] WARNING: NS_ENSURE_TRUE(newRoot) failed: file dom/base/nsRange.cpp, line 1132 2038 [N] WARNING: NS_ENSURE_TRUE(newRoot) failed: file dom/base/nsRange.cpp, line 1231 1912 [N] WARNING: NS_ENSURE_TRUE(rootContent) failed: file editor/composer/nsEditorSpellCheck.cpp, line 715 1627 [N] WARNING: Called close() before start()!: 'mStarted', file dom/workers/MessagePort.cpp, line 215 1612 [N] WARNING: NS_ENSURE_TRUE(sf) failed: file docshell/base/nsDocShell.cpp, line 6485 1488 [N] WARNING: NS_ENSURE_TRUE(isFileURI) failed: file dom/base/ThirdPartyUtil.cpp, line 368 1417 [N] WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x: file netwerk/protocol/http/HttpBaseChannel.cpp, line 2077 1417 [N] WARNING: NS_ENSURE_SUCCESS(result, result) failed with result 0x: file docshell/base/nsDocShell.cpp, line 14076 1393 [N] WARNING: Break suggested inside cluster!: file gfx/thebes/gfxTextRun.cpp, line 220 1288 [N] WARNING: NS_ENSURE_TRUE(mDisabledJSAndPlugins) failed: file editor/composer/nsEditingSession.cpp, line 209 1175 [N] WARNING: NS_ENSURE_TRUE(txToRemove) failed: file docshell/shistory/nsSHistory.cpp, line 1318 1151 [N] WARNING: NS_ENSURE_TRUE(selCon) failed: file editor/libeditor/nsEditor.cpp, line 631 1149 [N] WARNING: RemoveObserver() called for unregistered observer: file hal/Hal.cpp, line 205 1142 [N] WARNING: Failed to unlock the wakelock.: '!rv.Failed()', file dom/html/HTMLMediaElement.cpp, line 2341 1113 [N] WARNING: NS_ENSURE_TRUE(aURI) failed: file netwerk/dns/nsEffectiveTLDService.cpp, line 158 I have patches pending for #1 (bug 1175289) and #5 (bug 1175249) which
Re: The War on Warnings
On 10/06/15 16:10, Ehsan Akhgari wrote: Concrete examples would help. I can't find any test in browser/components/sessionstore right now with an assertion annotation so it's hard to say what the exact issue that you saw was, but I think you are complaining about the lack of documentation and that nobody cared. Neither of those issues will be solved with what you are suggesting. Well, that was at least 2 years ago. [...] Anyway, I'm just saying that if/when we redesign our warnings (and non-fatal assertions are one category of warnings, even if they are not formally called warnings), we need to make sure that warnings are as actionable as possible. One way to do this is to progressively convert each the warning into either a real assertion or into something that, by design, needs to be whitelisted individually (and documented). Best regards, David -- David Rajchenbach-Teller, PhD Performance Team, Mozilla ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
However, I do believe in the following scenario: - oh, there is an assertion warning, it's not my fault, let's allow one assertion; - wait, there are a few, let's allow several; - oh, they are intermittent, let's make it an interval; - [at some point, some of the warnings are fixed, but nobody realized]; - [at some point, something causes other warnings to be triggered, but nobody realized]; - we realize the regression years later. I am almost sure that I have witnessed this kind of scenario while working on Session Restore. It used to cause assertion warnings at some place in the DOM, which were not documented in any bug that I could find, and nobody cared. Also, second issue of the current approach: I have seen very few (if any) bugs yet that documents exactly the assertion warnings that they trigger, explaining why they are normal and should be ignored. That's a codetrap for whoever comes next. Requiring a test to actively opt-in for whitelisting a specific assertion warning means that there is a good place to r- the test until this is documented. Frankly, I'm really sick of warnings, and anything we can do to make them more actionable would be one step towards restoring sanity in our tests and in some parts of our codebase. Best regards, David On 10/06/15 06:39, Ehsan Akhgari wrote: On 2015-06-09 8:33 AM, Robert O'Callahan wrote: Does anyone know of a case where we had a regression that traded one assertion for another? I don't. I don't either. Which is why I believe that David's point, while being perfectly valid in theory, doesn't matter a lot in practice. Also, you need to consider the fact that one assertion appearing in place of another one in the same commits going into one build compared to the previous not breaking *anything else* or have any observable effect on all of our test suites is extremely unlikely. -- David Rajchenbach-Teller, PhD Performance Team, Mozilla ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 2015-06-10 3:38 AM, David Rajchenbach-Teller wrote: However, I do believe in the following scenario: - oh, there is an assertion warning, it's not my fault, let's allow one assertion; - wait, there are a few, let's allow several; - oh, they are intermittent, let's make it an interval; - [at some point, some of the warnings are fixed, but nobody realized]; Yes, assuming the number of assertions doesn't fall outside of the range, of course. - [at some point, something causes other warnings to be triggered, but nobody realized]; - we realize the regression years later. Yes, like I said, I agree this problem exists _in theory_. I however think that it's unlikely. (Note that we currently have only 82 SimpleTest.expectAssertions annotations in our tests, so we're already talking about an edge case of something that is extremely uncommon itself. :-) I am almost sure that I have witnessed this kind of scenario while working on Session Restore. It used to cause assertion warnings at some place in the DOM, which were not documented in any bug that I could find, and nobody cared. Concrete examples would help. I can't find any test in browser/components/sessionstore right now with an assertion annotation so it's hard to say what the exact issue that you saw was, but I think you are complaining about the lack of documentation and that nobody cared. Neither of those issues will be solved with what you are suggesting. Also, second issue of the current approach: I have seen very few (if any) bugs yet that documents exactly the assertion warnings that they trigger, explaining why they are normal and should be ignored. That's a codetrap for whoever comes next. Requiring a test to actively opt-in for whitelisting a specific assertion warning means that there is a good place to r- the test until this is documented. Not necessarily. Reviewers can choose to require exactly that everywhere SimpleTest.expectAssertions is used (I for one try to do that, but these are so rare that I can't remember the last time I reviewed such a test very well...) My point is, changing |SimpleTest.expectAssertions(2)| to |SimpleTest.expectAssertions([first assertion message, second assertion message])| doesn't really give us much besides the ability to pinpoint the expected assertions to the exact assertions that the test triggers. And I'm not saying that is not valuable, it's just a solution to an extremely rare problem. Issues such as lack of documentation of why the test is triggering an assertion are orthogonal to the syntax we use to declare the whitelist. Frankly, I'm really sick of warnings, and anything we can do to make them more actionable would be one step towards restoring sanity in our tests and in some parts of our codebase. Warnings are different. There is basically a spectrum of things you can do, from setting the MOZ_IGNORE_WARNINGS (and MOZ_QUIET) environment variables if you're just annoyed with them and don't want to see them, to filing bugs for the ones that happen way too often in tests (or during common operations such as opening a tab, typing something in a form, etc) and CCing the relevant people and asking whether the warning can be silenced, etc. I very much welcome the latter and have tried to act on such bugs where I can. Right now we have so many warnings that a project to annotate every test with every single warning that it generates is *really* impractical. And once we fix the problem of having too many frequently occurring one, the value of such a project is heavily diminished. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
I'm 99% sure we've had mis-stars on intermittent assertion oranges where the assertion changed and nobody bothered checking the logs to notice. Various media tests are coming to mind for when that's happened. On 6/10/2015 10:10 AM, Ehsan Akhgari wrote: On 2015-06-10 3:38 AM, David Rajchenbach-Teller wrote: However, I do believe in the following scenario: - oh, there is an assertion warning, it's not my fault, let's allow one assertion; - wait, there are a few, let's allow several; - oh, they are intermittent, let's make it an interval; - [at some point, some of the warnings are fixed, but nobody realized]; Yes, assuming the number of assertions doesn't fall outside of the range, of course. - [at some point, something causes other warnings to be triggered, but nobody realized]; - we realize the regression years later. Yes, like I said, I agree this problem exists _in theory_. I however think that it's unlikely. (Note that we currently have only 82 SimpleTest.expectAssertions annotations in our tests, so we're already talking about an edge case of something that is extremely uncommon itself. :-) I am almost sure that I have witnessed this kind of scenario while working on Session Restore. It used to cause assertion warnings at some place in the DOM, which were not documented in any bug that I could find, and nobody cared. Concrete examples would help. I can't find any test in browser/components/sessionstore right now with an assertion annotation so it's hard to say what the exact issue that you saw was, but I think you are complaining about the lack of documentation and that nobody cared. Neither of those issues will be solved with what you are suggesting. Also, second issue of the current approach: I have seen very few (if any) bugs yet that documents exactly the assertion warnings that they trigger, explaining why they are normal and should be ignored. That's a codetrap for whoever comes next. Requiring a test to actively opt-in for whitelisting a specific assertion warning means that there is a good place to r- the test until this is documented. Not necessarily. Reviewers can choose to require exactly that everywhere SimpleTest.expectAssertions is used (I for one try to do that, but these are so rare that I can't remember the last time I reviewed such a test very well...) My point is, changing |SimpleTest.expectAssertions(2)| to |SimpleTest.expectAssertions([first assertion message, second assertion message])| doesn't really give us much besides the ability to pinpoint the expected assertions to the exact assertions that the test triggers. And I'm not saying that is not valuable, it's just a solution to an extremely rare problem. Issues such as lack of documentation of why the test is triggering an assertion are orthogonal to the syntax we use to declare the whitelist. Frankly, I'm really sick of warnings, and anything we can do to make them more actionable would be one step towards restoring sanity in our tests and in some parts of our codebase. Warnings are different. There is basically a spectrum of things you can do, from setting the MOZ_IGNORE_WARNINGS (and MOZ_QUIET) environment variables if you're just annoyed with them and don't want to see them, to filing bugs for the ones that happen way too often in tests (or during common operations such as opening a tab, typing something in a form, etc) and CCing the relevant people and asking whether the warning can be silenced, etc. I very much welcome the latter and have tried to act on such bugs where I can. Right now we have so many warnings that a project to annotate every test with every single warning that it generates is *really* impractical. And once we fix the problem of having too many frequently occurring one, the value of such a project is heavily diminished. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
In my books, that's pretty footgunish. On 09/06/15 04:04, Boris Zbarsky wrote: You can specify an exact number or a range. But you're right that you can't specify the assertion text involved... -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform -- David Rajchenbach-Teller, PhD Performance Team, Mozilla ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
Does anyone know of a case where we had a regression that traded one assertion for another? I don't. Rob -- oIo otoeololo oyooouo otohoaoto oaonoyooonoeo owohooo oioso oaonogoroyo owoiotoho oao oboroootohoeoro oooro osoiosotoeoro owoiololo oboeo osouobojoeocoto otooo ojouodogomoeonoto.o oAogoaoiono,o oaonoyooonoeo owohooo osoaoyoso otooo oao oboroootohoeoro oooro osoiosotoeoro,o o‘oRoaocoao,o’o oioso oaonosowoeoroaoboloeo otooo otohoeo ocooouoroto.o oAonodo oaonoyooonoeo owohooo osoaoyoso,o o‘oYooouo ofolo!o’o owoiololo oboeo oiono odoaonogoeoro ooofo otohoeo ofoioroeo ooofo ohoeololo. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 6/9/2015 8:33 AM, Robert O'Callahan wrote: Does anyone know of a case where we had a regression that traded one assertion for another? I don't. Rob How would we have found out? :) ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On Wed, Jun 10, 2015 at 12:34 AM, Ryan VanderMeulen rya...@gmail.com wrote: On 6/9/2015 8:33 AM, Robert O'Callahan wrote: Does anyone know of a case where we had a regression that traded one assertion for another? I don't. How would we have found out? :) Detecting the regression some other way. Rob -- oIo otoeololo oyooouo otohoaoto oaonoyooonoeo owohooo oioso oaonogoroyo owoiotoho oao oboroootohoeoro oooro osoiosotoeoro owoiololo oboeo osouobojoeocoto otooo ojouodogomoeonoto.o oAogoaoiono,o oaonoyooonoeo owohooo osoaoyoso otooo oao oboroootohoeoro oooro osoiosotoeoro,o o‘oRoaocoao,o’o oioso oaonosowoeoroaoboloeo otooo otohoeo ocooouoroto.o oAonodo oaonoyooonoeo owohooo osoaoyoso,o o‘oYooouo ofolo!o’o owoiololo oboeo oiono odoaonogoeoro ooofo otohoeo ofoioroeo ooofo ohoeololo. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 2015-06-09 8:33 AM, Robert O'Callahan wrote: Does anyone know of a case where we had a regression that traded one assertion for another? I don't. I don't either. Which is why I believe that David's point, while being perfectly valid in theory, doesn't matter a lot in practice. Also, you need to consider the fact that one assertion appearing in place of another one in the same commits going into one build compared to the previous not breaking *anything else* or have any observable effect on all of our test suites is extremely unlikely. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
Last time I looked at whitelisting non-fatal assertions, there was only the option to accept up-to a number of assertions. Have I missed something? Because a blank check to sweep assertions under the carpet is really an awful mechanism. Now, I fully agree that warnings that are not whitelisted should most likely cause real assertion failures. Cheers, David On 08/06/15 19:09, Ehsan Akhgari wrote: On 2015-06-05 6:08 AM, David Rajchenbach-Teller wrote: This API covers all the needs I have encountered for warnings so far in JS code. I don't think it's terribly different in C++ code. For C++ non-fatal assertions, we already have a mechanism similar to this (but it doesn't rely on the exact message printed in the warning, of course.) I prefer to not try to do what you are doing for C++ warnings, because if there are ones that we should really not trigger in a test, those should be promoted to be assertions, and then we would be able to rely on the existing mechanisms to catch them during test runs. Cheers, Ehsan -- David Rajchenbach-Teller, PhD Performance Team, Mozilla ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
Hi,
On 2015/06/06 2:39, Eric Rahm wrote:
On Friday, June 5, 2015 at 8:23:53 AM UTC-7, ISHIKAWA,chiaki wrote:
After coping with voluminous messages in C-C TB |make mozmill| test
suite log [much smaller volume than full FF logs],
I think we should have NS_INFORMATION() macro that
prints out an information for someone who is developing code and
just wants to know how the code is doing.
I suppose we could add yet another macro, but really this sounds like a use
case for:
MOZ_LOG(myModule, LogLevel::Info, Dev message!);
which could then be enabled with something like:
|NSPR_MODULES=myModule:3 ./mach run|
Thank you for the tips.
(I was referring to *NS*_INFORMATION() since
when I tried to use MOZ_ABORT() or something in some test codes,
I could not compile and link them and learned some MOZ_* macros and NS_*
macros don't mix very well during linking(?).)
Anyway, I am a little confused since MOZ_LOG() is very new.
What is the first argument to the following code?
|MOZ_LOG(myModule, LogLevel::Info, Dev message!);|
Looking at how PR_LOG() was called with PRLogModuleInfo,
typedef struct PRLogModuleInfo {
const char *name;
PRLogModuleLevel level;
struct PRLogModuleInfo *next;
} PRLogModuleInfo;
Is |myModule| it declared as
PRLogModuleInfo myModule;
is passed to as the first argument of MOZ_LOG().?
But it sounds as if we should stay away from PRLog* data structure.
Or is it simply anything (and stringified internally) that
will be specified in environmental variable later?
It might be useful to eventually add a |MOZ_LOG_IF| to make things easier.
Yes, for readability, we need some creative macros.
1576 [10262] WARNING: '!mMainThread', file
/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/threads/nsThreadManager.cpp, line
We're working on this in bug 1171716, it's related to cycle collection on
shutdown.
Anyway, I agree that we should simply try to attack the top entries
first to figure out whether they are indication of genuine issues/bugs, etc.
Yes! Please file blocking bugs for the top c-c entries against bug 765224.
I will try to do so.
Eventually we could cut down the number to manageable size.
(At near the beginning of 2013, the list became only about a dozen items
long, but started to grow again. In a sense it reflects the desire of
code authors to catch strange conditions, but I agree we must keep on
looking at the logs and do something about the top offenders in the log
list.)
It might be nice to create a very simple start/stop test that just makes sure
there are no warnings so that we can avoid regressing that in the future.
Sounds a sensible idea.
TIA
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 2015-06-05 6:08 AM, David Rajchenbach-Teller wrote: This API covers all the needs I have encountered for warnings so far in JS code. I don't think it's terribly different in C++ code. For C++ non-fatal assertions, we already have a mechanism similar to this (but it doesn't rely on the exact message printed in the warning, of course.) I prefer to not try to do what you are doing for C++ warnings, because if there are ones that we should really not trigger in a test, those should be promoted to be assertions, and then we would be able to rely on the existing mechanisms to catch them during test runs. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 6/8/15 4:28 PM, David Rajchenbach-Teller wrote: Last time I looked at whitelisting non-fatal assertions, there was only the option to accept up-to a number of assertions. You can specify an exact number or a range. But you're right that you can't specify the assertion text involved... -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
The question is what is best: ignored warnings or ignored intermittent oranges? I assume that the latter have the best chance of being eventually fixed, but I'm not sure. On 05/06/15 18:17, Ryan VanderMeulen wrote: Uncaught Promise rejections were made fatal and for the most part go ignored when they fail intermittently. I'm not a huge fan of adding more ways for tests to fail if people aren't willing to actually do something about it when they do. -Ryan -- David Rajchenbach-Teller, PhD Performance Team, Mozilla ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
My patch introduces a new Warnings mechanism for JS. If this works
nicely, I'd like to extend this to C++.
Basically, whenever you encounter a situation that should never happen
in production but that is not quite fatal enough to warrant a crash, we
call (from the top of my head)
Warnings.warn(the module that triggered the warning,
`Hey, I still have ${n} splines to reticulate, I should have 0.`);
By default, this causes tests to fail. However, tests that are designed
to cause this kind of warning can whitelist the warning with
Assert.expect(the module that triggered the warning,
/Hey, I still have [0-9]* splines to reticulate, I should have 0./)
(note the regexp)
and tests that are known to cause this kind of warning can graylist it
until the warning has been fixed
Assert.FIXME(the module that triggered the warning
/Hey, I still have [0-9]* splines to reticulate, I should have 0./)
(also a regexp)
This API covers all the needs I have encountered for warnings so far in
JS code. I don't think it's terribly different in C++ code.
Cheers,
David
On 05/06/15 00:14, Ehsan Akhgari wrote:
Families of warnings? I don't really understand what you mean. Also,
looking at the patch in the bug, your changes seem to affect Assert.jsm
stuff, which is not related to NS_WARNING.
--
David Rajchenbach-Teller, PhD
Performance Team, Mozilla
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 2015/06/04 21:38, Robert O'Callahan wrote: Usually I use NS_WARNING to mean something weird and unexpected is happening, e.g. a bug in Web page code, but not necessarily a browser bug. Sometimes I get useful hints from NS_WARNING spew leading up to a serious failure, but for those usages could probably be switched to PR_LOG without losing much. Rob After coping with voluminous messages in C-C TB |make mozmill| test suite log [much smaller volume than full FF logs], I think we should have NS_INFORMATION() macro that prints out an information for someone who is developing code and just wants to know how the code is doing. So many WARNING lines such as the ones below can be turned into INFORMATION messages and we can probably ignore these lines from NG/GO decisions of tests. Hmm. The particular log I picked from two months ago is not quite representative. I specifically excluded NS_ENSURE_TRUE summary from below, but for this particular log, the messages near the top of list (decreasing frequency order) seem too serious to be INFORMATION kind. WARNING: Allow one letter C/c as language code.: The above line below is from my own attempt to make C-C TB accpet C.UTF-8 as locale: I think cygwin's default locale is this. But mozilla code refuses to accept this claiming that the first part of the locale needs to be either two or three letters. And I used dreaded NS_WARNING() instead of something better: if NS_INFORMATION() is there, I will use it. --- begin quote --- Second without NS_ENSURE macros: Count/sum is modulo [without the first PID] 1576 [10262] WARNING: '!mMainThread', file /REF-COMM-CENTRAL/comm-central/mozilla/xpcom/threads/nsThreadManager.cpp, line 395 868 [10262] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /REF-COMM-CENTRAL/comm-central/mozilla/parser/html/nsHtml5StreamParser.cpp, line 951 693 [10262] WARNING: Subdocument container has no frame: file /REF-COMM-CENTRAL/comm-central/mozilla/layout/base/nsDocumentViewer.cpp, line 2511 100 [21074] WARNING: We should have hit the document element...: file /REF-COMM-CENTRAL/comm-central/mozilla/layout/xul/BoxObject.cpp, line 183 58 [10262] WARNING: unable to post continuation event: file /REF-COMM-CENTRAL/comm-central/mozilla/xpcom/io/nsStreamUtils.cpp, line 453 58 [10262] WARNING: An event was posted to a thread that will never run it (rejected): file /REF-COMM-CENTRAL/comm-central/mozilla/xpcom/threads/nsThread.cpp, line 505 45 [10553] WARNING: No inner window available!: file /REF-COMM-CENTRAL/comm-central/mozilla/dom/base/nsGlobalWindow.cpp, line 9658 36 [10262] WARNING: Re-registering a CID?: file /REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/nsComponentManager.cpp, line 531 36 [10262] WARNING: Hardware Vsync support not yet implemented. Falling back to software timers 36 [10262] WARNING: Failed to open external DTD: publicId -//W3C//DTD SVG 1.1//EN systemId http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd; base file:///REF-COMM-CENTRAL/comm-central/mail/themes/linux/mail/tabs/selected-start.svg URL resource://gre/res/dtd/svg11.dtd: file /REF-COMM-CENTRAL/comm-central/mozilla/parser/htmlparser/nsExpatDriver.cpp, line 703 36 [10262] WARNING: Failed to open external DTD: publicId -//W3C//DTD SVG 1.1//EN systemId http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd; base file:///REF-COMM-CENTRAL/comm-central/mail/themes/linux/mail/tabs/selected-end.svg URL resource://gre/res/dtd/svg11.dtd: file /REF-COMM-CENTRAL/comm-central/mozilla/parser/htmlparser/nsExpatDriver.cpp, line 703 36 [10262] WARNING: Allow one letter C/c as language code.: file /REF-COMM-CENTRAL/comm-central/mozilla/intl/locale/unix/nsPosixLocale.cpp, line 128 35 [10262] WARNING: unable to Flush() dirty datasource during XPCOM shutdown: file /REF-COMM-CENTRAL/comm-central/mozilla/rdf/base/nsRDFXMLDataSource.cpp, line 754 35 WARNING: YOU ARE LEAKING THE WORLD (at least one JSRuntime and everything alive inside it, that is) AT JS_ShutDown TIME. FIX THIS! 32 [21074] WARNING: Must complete empty transaction when compositing!: file /REF-COMM-CENTRAL/comm-central/mozilla/layout/base/nsPresShell.cpp, line 6257 30 [21074] WARNING: Someone passed native anonymous content directly into frame construction. Stop doing that!: file /REF-COMM-CENTRAL/comm-central/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 6550 28 [21074] WARNING: nsMsgProtocol::SetContentCharset() not implemented: file /REF-COMM-CENTRAL/comm-central/mailnews/base/util/nsMsgProtocol.cpp, line 580 20 [21074] WARNING: anonymous nodes should not be in child lists (bug 439258): file /REF-COMM-CENTRAL/comm-central/mozilla/layout/base/RestyleManager.cpp, line 1441 18 [8115] WARNING: There is no observer for invalidformsubmit. One should be implemented!: file /REF-COMM-CENTRAL/comm-central/mozilla/dom/html/HTMLFormElement.cpp, line
Re: The War on Warnings
On Thu, Jun 4, 2015 at 1:48 PM, Luke Wagner lwag...@mozilla.com wrote: In addition to judging noisiness by volume over a whole test run, can we also include any warning that happens on normal browser startup, new tab, and other vanilla browser operations? This has always annoyed me. Indeed. Here's an attempt at establishing common ground that hopefully almost everyone can agree with... - There should be no warnings when starting and immediately closing desktop Firefox on a new profile. Or maybe a very small number (e.g. less than 5) on some configurations... certainly not dozens. - There should be no warnings that trigger 1000s of times while running standard test suites. Because this is just silly: 65959 [N] WARNING: Overflowed nscoord_MAX in conversion to nscoord width: file ../../dist/include/nsRect.h, line 83 Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 6/4/15 6:14 PM, Ehsan Akhgari wrote: They are both equally slow, but fatal assertions happen only once, by definition. ;-) Except that for some of our tests we restart after a crash (e.g. for web platform tests). So in that test harness, fatal assertions that are hit are much slower than non-fatal ones, fwiw. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 06/04/2015 01:18 PM, smaug wrote: More likely we need to change a small number of noisy NS_ENSURE_* macro users to use something else, and keep most of the NS_ENSURE_* usage as it is. I agree -- I posted about switching to something opt-in, like MOZ_LOG, for some of the spammier layout NS_WARNINGS, too: https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.tech.layout/YXauN50HDhI ~Daniel ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On Thursday, June 4, 2015 at 11:14:59 AM UTC-7, Martin Thomson wrote: On Jun 4, 2015 10:27 AM, Daniel Holbert dholb...@mozilla.com wrote: Also: in layout, there are various warnings related to coordinate wraparound/overflow, where we're basically throwing up our hands and warning that broken layout is likely to occur because the page is millions of pixels tall. These can be useful hints, when debugging page brokenness. I know that it's more work, but isn't that what the console is best suited for? We have some spammy warnings there too, of course (try looking for rc4 warnings if you have gmail open). This does seem more appropriate for warnings targeted at web developers. Most end users probably will not be using debug builds of Firefox (particularly considering we market a dev edition, you know, for web devs). ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 06/05/2015 12:06 AM, Daniel Holbert wrote: On 06/04/2015 01:18 PM, smaug wrote: More likely we need to change a small number of noisy NS_ENSURE_* macro users to use something else, and keep most of the NS_ENSURE_* usage as it is. I agree -- I posted about switching to something opt-in, like MOZ_LOG, for some of the spammier layout NS_WARNINGS, too: https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.tech.layout/YXauN50HDhI ~Daniel There is also DEBUG_foo and then using it --with-debug-label=foo There are couple of #ifdef DEBUG_smaug checks, but it isn't really nice to pollute the code with developer specific ifdefs. However for your case DEBUG_layout might work. -Olli ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 2015-06-04 9:40 AM, David Rajchenbach-Teller wrote: On 04/06/15 14:30, Ehsan Akhgari wrote: There are very good reasons for warnings to not cause tests to fail. We have a lot of tests that are testing failure conditions that are expected to warn, because they are failure conditions. Well, that's why the patch linked above offers a whitelist API, so that tests can individually decide which families of warnings are either expected, or ok-for-the-time-being. Families of warnings? I don't really understand what you mean. Also, looking at the patch in the bug, your changes seem to affect Assert.jsm stuff, which is not related to NS_WARNING. There are also warnings that were actually meant to be assertions, and we should try to convert them to assertions, and that way they will fail tests. That being said, assertion report stack traces in debug builds, which is excruciatingly slow, so we have had to switch them to be warnings in at least some cases (see bug 756045 for example.) Are you talking about our weird non-fatal assertions or actual fatal assertions? They are both equally slow, but fatal assertions happen only once, by definition. ;-) ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
In addition to judging noisiness by volume over a whole test run, can we also include any warning that happens on normal browser startup, new tab, and other vanilla browser operations? This has always annoyed me. On Thu, Jun 4, 2015 at 3:33 PM, Bobby Holley bobbyhol...@gmail.com wrote: On Thu, Jun 4, 2015 at 1:18 PM, smaug sm...@welho.com wrote: More likely we need to change a small number of noisy NS_ENSURE_* macro users to use something else, and keep most of the NS_ENSURE_* usage as it is. +1. ___ 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: The War on Warnings
On Thursday, June 4, 2015 at 1:48:30 PM UTC-7, Luke Wagner wrote: In addition to judging noisiness by volume over a whole test run, can we also include any warning that happens on normal browser startup, new tab, and other vanilla browser operations? This has always annoyed me. Yes, this bothers me as well. Here's a rough look at a run I just did (this is simply opening the browser to a blank page and then closing it): 27 [N] WARNING: '!mMainThread', file /home/erahm/dev/mozilla-central/xpcom/threads/nsThreadManager.cpp, line 299 10 [N] WARNING: 'NS_FAILED(DebuggerOnGCRunnable::Enqueue(aRuntime, aDesc))', file /home/erahm/dev/mozilla-central/xpcom/base/CycleCollectedJSRuntime.cpp, line 743 5 [N] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/erahm/dev/mozilla-central/xpcom/base/nsTraceRefcnt.cpp, line 148 4 [N] WARNING: RemoveObserver() called for unregistered observer: file /home/erahm/dev/mozilla-central/hal/Hal.cpp, line 205 3 [N] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file /home/erahm/dev/mozilla-central/embedding/browser/nsWebBrowser.cpp, line 363 3 [N] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x80040111: file /home/erahm/dev/mozilla-central/docshell/base/nsDocShell.cpp, line 4592 2 [N] WARNING: Subdocument container has no frame: file /home/erahm/dev/mozilla-central/layout/base/nsDocumentViewer.cpp, line 2506 2 [N] WARNING: Re-registering a CID?: file /home/erahm/dev/mozilla-central/xpcom/components/nsComponentManager.cpp, line 551 2 [N] WARNING: 'NS_FAILED(rv)', file /home/erahm/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp, line 2529 2 [N] WARNING: NS_ENSURE_TRUE(domWindow) failed: file /home/erahm/dev/mozilla-central/embedding/browser/nsDocShellTreeOwner.cpp, line 83 2 [N] WARNING: NS_ENSURE_TRUE(aInBrowser) failed: file /home/erahm/dev/mozilla-central/embedding/browser/nsDocShellTreeOwner.cpp, line 79 2 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /home/erahm/dev/mozilla-central/dom/base/nsFrameLoader.cpp, line 267 2 [N] WARNING: Enabling vsync refresh driver: file /home/erahm/dev/mozilla-central/layout/base/nsRefreshDriver.cpp, line 859 2 [N] WARNING: '!BasePrincipal::IsCodebasePrincipal(aPrincipal)', file /home/erahm/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp, line 2591 1 [N] WARNING: nsWindow::GetNativeData not implemented for this type: file /home/erahm/dev/mozilla-central/widget/PuppetWidget.cpp, line 1162 1 [N] WARNING: NS_ENSURE_TRUE(domDoc target) failed: file /home/erahm/dev/mozilla-central/dom/base/nsContentUtils.cpp, line 3636 1 [N] WARNING: NS_ENSURE_TRUE(currentInner) failed: file /home/erahm/dev/mozilla-central/dom/base/nsGlobalWindow.cpp, line 9004 1 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /home/erahm/dev/mozilla-central/dom/base/nsContentUtils.cpp, line 3691 1 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /home/erahm/dev/mozilla-central/toolkit/xre/nsXREDirProvider.cpp, line 1374 1 [N] WARNING: No docshells for remote frames!: file /home/erahm/dev/mozilla-central/dom/base/nsFrameLoader.cpp, line 491 1 [N] WARNING: Hardware Vsync support not yet implemented. Falling back to software timers: file /home/erahm/dev/mozilla-central/gfx/thebes/gfxPlatform.cpp, line 2410 1 [N] WARNING: Enabling vsync compositor: file /home/erahm/dev/mozilla-central/gfx/layers/ipc/CompositorParent.cpp, line 679 1 [N] WARNING: '!editorRectEvent.mSucceeded', file /home/erahm/dev/mozilla-central/widget/PuppetWidget.cpp, line 800 1 [N] WARNING: Could not get disk status from nsIDiskSpaceWatcher: file /home/erahm/dev/mozilla-central/uriloader/prefetch/nsOfflineCacheUpdateService.cpp, line 317 1 [N] WARNING: Could not get disk information from DiskSpaceWatcher: file /home/erahm/dev/mozilla-central/dom/storage/DOMStorageIPC.cpp, line 320 1 [N] WARNING: '!compMgr', file /home/erahm/dev/mozilla-central/xpcom/glue/nsComponentManagerUtils.cpp, line 63 There's bug 1171716 for the !mMainThread warning. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On Jun 4, 2015 10:27 AM, Daniel Holbert dholb...@mozilla.com wrote: Also: in layout, there are various warnings related to coordinate wraparound/overflow, where we're basically throwing up our hands and warning that broken layout is likely to occur because the page is millions of pixels tall. These can be useful hints, when debugging page brokenness. I know that it's more work, but isn't that what the console is best suited for? We have some spammy warnings there too, of course (try looking for rc4 warnings if you have gmail open). ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 06/04/2015 05:30 AM, Ehsan Akhgari wrote: There are very good reasons for warnings to not cause tests to fail. We have a lot of tests that are testing failure conditions that are expected to warn, because they are failure conditions. Also: in layout, there are various warnings related to coordinate wraparound/overflow, where we're basically throwing up our hands and warning that broken layout is likely to occur because the page is millions of pixels tall. These can be useful hints, when debugging page brokenness. We have a lot of tests that exercise how we behave around this coordinate-overflow threshold [mostly that we don't crash], and it's expected that such tests would trigger these warnings. One thing that might help for noise from these: Jesse filed a bug[1] a while back on adding a flag to detect huge sizes suppress some assertions when we do; we could conceivably use this flag to suppress warnings [perhaps just displaying a single detected huge sizes warning], as well. ~Daniel [1] https://bugzilla.mozilla.org/show_bug.cgi?id=765861 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On Thu, Jun 4, 2015 at 1:18 PM, smaug sm...@welho.com wrote: More likely we need to change a small number of noisy NS_ENSURE_* macro users to use something else, and keep most of the NS_ENSURE_* usage as it is. +1. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On Thu, Jun 4, 2015 at 2:49 AM, Jonas Sicking jo...@sicking.cc wrote: FWIW, I suspect it'll be hard to put a dent in the number of warnings that we emit unless we either change all instances of NS_ENSURE_SUCCESS(rv, rv) to use some other macro which doesn't warn, or unless we change NS_ENSURE_SUCCESS(rv, rv) to not warn. As you can see the from the list, the distribution of these warnings is very uneven. It is not necessary to solve the entire warning issue to make huge reductions in the total volume of warnings we see. For instance, in bug 1170642 I noticed that there were about 16MB of warnings in e10s M2, produced by a single line of code (which Eric fixed by removing the warning). Andrew It feels like right now we have three incompatible desires: * Test lots of failure cases. * Make errors warn in debug builds on all/most frames as the failure is propagated up the callstack. * Don't warn a lot when testing debug builds. It seems like we can only pick two out of these three. / Jonas On Wed, Jun 3, 2015 at 6:14 PM, Eric Rahm er...@mozilla.com wrote: We emit a *lot* of runtime warnings when running debug tests. I inadvertently triggered a max log size failure during a landing this week which encouraged me to take a look at what all is being logged, and what I found was a ton of warnings (sometimes accompanied by stack traces). Most of these should probably be removed (of course if they're real issues they should be fixed, but judging by the frequency most are probably non-issues). I'm currently cleaning up some of these, but if you happen to see something in the following list and are feeling proactive I would appreciate the help. There's even a meta bug for tracking these: https://bugzilla.mozilla.org/show_bug.cgi?id=765224 I generated this list by grabbing the logs for a recent m-c linux64 debug run, normalizing out PIDs and timestamps and then doing some sort/uniq-fu to get counts of unique lines. This is roughly the top 40 offenders: 65959 [N] WARNING: Overflowed nscoord_MAX in conversion to nscoord width: file ../../dist/include/nsRect.h, line 83 63460 [N] WARNING: NS_ENSURE_TRUE(piTarget) failed: file gdom/events/EventDispatcher.cpp, line 469 20039 [N] WARNING: 'NS_FAILED(rv)', file gdom/workers/ServiceWorkerManager.cpp, line 2529 20039 [N] WARNING: '!BasePrincipal::IsCodebasePrincipal(aPrincipal)', file gdom/workers/ServiceWorkerManager.cpp, line 2591 17784 [N] WARNING: Subdocument container has no frame: file glayout/base/nsDocumentViewer.cpp, line 2506 16322 JavaScript warning: file:///builds/slave/test/build/tests/jsreftest/tests/js1_8/extensions/regress-476427.js, line 1: JavaScript 1.6's for-each-in loops are deprecated; consider using ES6 for-of instead 14159 [N] WARNING: NS_ENSURE_TRUE(mMutable) failed: file gnetwerk/base/nsSimpleURI.cpp, line 264 14087 [N] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x80040111: file gdocshell/base/nsDocShell.cpp, line 4592 11315 [N] WARNING: '!mMainThread', file gxpcom/threads/nsThreadManager.cpp, line 299 10574 [N] WARNING: No docshells for remote frames!: file gdom/base/nsFrameLoader.cpp, line 491 9201 [N] WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'psd-mIEnd != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsLineLayout.cpp, line 884 9155 [N] WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'psd-mIEnd != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsLineLayout.cpp, line 3058 9130 [N] WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'aISize != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsLineLayout.cpp, line 160 8844 [N] WARNING: Someone passed native anonymous content directly into frame construction. Stop doing that!: file glayout/base/nsCSSFrameConstructor.cpp, line 6559 7599 [N] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file gembedding/browser/nsWebBrowser.cpp, line 363 7454 [N] WARNING: anonymous nodes should not be in child lists (bug 439258): file glayout/base/RestyleManager.cpp, line 1440 6544 [N] WARNING: Graph thread slowdown?: 'std::abs(framePosition - CurrentDriver()-StateComputedTime()) MillisecondsToMediaTime(5)', file gdom/media/MediaStreamGraph.cpp, line 1195 6126 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file gnetwerk/base/nsFileStreams.cpp, line 492 6126 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file gnetwerk/base/nsFileStreams.cpp, line 205 5637 [N] WARNING: No outer window available!: file gdom/base/nsGlobalWindow.cpp,
Re: The War on Warnings
Nicholas Nethercote writes: Do warnings (as opposed to NS_ASSERTION) do anything in tests? I don't think they do. If that's right, a warning is only useful if a human looks at it and acts on it, and that's clearly not happening for a lot of these. Warnings in tests don't do anything but log that a problem has occurred. But, when the warnings are not noisy, this is often useful for tracking the cause of intermittent failures. Similarly, warnings about real problems are helpful when running debug builds, but we are being spammed with so many warnings that it is hard to imagine they can all be real problems. If they are, then we are doing a poor job of keeping up with them. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 06/04/2015 01:07 PM, David Rajchenbach-Teller wrote: Part of my world domination plans are to turn warnings into something that causes test to actually fail (see bug 1080457 co). Would you like to join forces? Turning warnings into failures sounds odd (but bug 1080457 seems to be actually something different). Warnings aren't anything which should cause tests to fail. Warnings, especially those generated by NS_ENSURE_* macros are things which are super useful for debugging - they often point rather exactly where to start looking for an issue. And since there is NS_ENSURE_*, there isn't necessarily any problem or bug, just an unusual state, which is then handled by the NS_ENSURE_* macro. -Olli Cheers, David On 04/06/15 03:14, Eric Rahm wrote: We emit a *lot* of runtime warnings when running debug tests. I inadvertently triggered a max log size failure during a landing this week which encouraged me to take a look at what all is being logged, and what I found was a ton of warnings (sometimes accompanied by stack traces). Most of these should probably be removed (of course if they're real issues they should be fixed, but judging by the frequency most are probably non-issues). I'm currently cleaning up some of these, but if you happen to see something in the following list and are feeling proactive I would appreciate the help. There's even a meta bug for tracking these: https://bugzilla.mozilla.org/show_bug.cgi?id=765224 I generated this list by grabbing the logs for a recent m-c linux64 debug run, normalizing out PIDs and timestamps and then doing some sort/uniq-fu to get counts of unique lines. This is roughly the top 40 offenders: ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On Thu, Jun 4, 2015 at 5:38 AM, Robert O'Callahan rob...@ocallahan.org wrote: Usually I use NS_WARNING to mean something weird and unexpected is happening, e.g. a bug in Web page code, but not necessarily a browser bug. Sometimes I get useful hints from NS_WARNING spew leading up to a serious failure. Yup. I think this is a quite common, and quite useful, usage of NS_WARNING. But testing runs a lot of weird and unexpected things. That's a good thing because those tend to be things that often regress. But it also leads to a lot of warnings. but for those usages could probably be switched to PR_LOG without losing much. I think this would mean changing most NS_ENSURE_SUCCESS(rv, rv), and likely many other NS_ENSURE_* to macros that call PR_LOG instead. So like I said, we'll have to either change a huge number of NS_ENSURE_* macros to use something else, or change what NS_ENSURE_* does. / Jonas Rob -- oIo otoeololo oyooouo otohoaoto oaonoyooonoeo owohooo oioso oaonogoroyo owoiotoho oao oboroootohoeoro oooro osoiosotoeoro owoiololo oboeo osouobojoeocoto otooo ojouodogomoeonoto.o oAogoaoiono,o oaonoyooonoeo owohooo osoaoyoso otooo oao oboroootohoeoro oooro osoiosotoeoro,o o'oRoaocoao,o'o oioso oaonosowoeoroaoboloeo otooo otohoeo ocooouoroto.o oAonodo oaonoyooonoeo owohooo osoaoyoso,o o'oYooouo ofolo!o'o owoiololo oboeo oiono odoaonogoeoro ooofo otohoeo ofoioroeo ooofo ohoeololo. ___ 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: The War on Warnings
On Thu, Jun 4, 2015 at 2:49 AM, Jonas Sicking jo...@sicking.cc wrote: It feels like right now we have three incompatible desires: * Test lots of failure cases. * Make errors warn in debug builds on all/most frames as the failure is propagated up the callstack. * Don't warn a lot when testing debug builds. It seems like we can only pick two out of these three. Do warnings (as opposed to NS_ASSERTION) do anything in tests? I don't think they do. If that's right, a warning is only useful if a human looks at it and acts on it, and that's clearly not happening for a lot of these. Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
FWIW, I suspect it'll be hard to put a dent in the number of warnings that we emit unless we either change all instances of NS_ENSURE_SUCCESS(rv, rv) to use some other macro which doesn't warn, or unless we change NS_ENSURE_SUCCESS(rv, rv) to not warn. It feels like right now we have three incompatible desires: * Test lots of failure cases. * Make errors warn in debug builds on all/most frames as the failure is propagated up the callstack. * Don't warn a lot when testing debug builds. It seems like we can only pick two out of these three. / Jonas On Wed, Jun 3, 2015 at 6:14 PM, Eric Rahm er...@mozilla.com wrote: We emit a *lot* of runtime warnings when running debug tests. I inadvertently triggered a max log size failure during a landing this week which encouraged me to take a look at what all is being logged, and what I found was a ton of warnings (sometimes accompanied by stack traces). Most of these should probably be removed (of course if they're real issues they should be fixed, but judging by the frequency most are probably non-issues). I'm currently cleaning up some of these, but if you happen to see something in the following list and are feeling proactive I would appreciate the help. There's even a meta bug for tracking these: https://bugzilla.mozilla.org/show_bug.cgi?id=765224 I generated this list by grabbing the logs for a recent m-c linux64 debug run, normalizing out PIDs and timestamps and then doing some sort/uniq-fu to get counts of unique lines. This is roughly the top 40 offenders: 65959 [N] WARNING: Overflowed nscoord_MAX in conversion to nscoord width: file ../../dist/include/nsRect.h, line 83 63460 [N] WARNING: NS_ENSURE_TRUE(piTarget) failed: file gdom/events/EventDispatcher.cpp, line 469 20039 [N] WARNING: 'NS_FAILED(rv)', file gdom/workers/ServiceWorkerManager.cpp, line 2529 20039 [N] WARNING: '!BasePrincipal::IsCodebasePrincipal(aPrincipal)', file gdom/workers/ServiceWorkerManager.cpp, line 2591 17784 [N] WARNING: Subdocument container has no frame: file glayout/base/nsDocumentViewer.cpp, line 2506 16322 JavaScript warning: file:///builds/slave/test/build/tests/jsreftest/tests/js1_8/extensions/regress-476427.js, line 1: JavaScript 1.6's for-each-in loops are deprecated; consider using ES6 for-of instead 14159 [N] WARNING: NS_ENSURE_TRUE(mMutable) failed: file gnetwerk/base/nsSimpleURI.cpp, line 264 14087 [N] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x80040111: file gdocshell/base/nsDocShell.cpp, line 4592 11315 [N] WARNING: '!mMainThread', file gxpcom/threads/nsThreadManager.cpp, line 299 10574 [N] WARNING: No docshells for remote frames!: file gdom/base/nsFrameLoader.cpp, line 491 9201 [N] WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'psd-mIEnd != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsLineLayout.cpp, line 884 9155 [N] WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'psd-mIEnd != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsLineLayout.cpp, line 3058 9130 [N] WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'aISize != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsLineLayout.cpp, line 160 8844 [N] WARNING: Someone passed native anonymous content directly into frame construction. Stop doing that!: file glayout/base/nsCSSFrameConstructor.cpp, line 6559 7599 [N] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file gembedding/browser/nsWebBrowser.cpp, line 363 7454 [N] WARNING: anonymous nodes should not be in child lists (bug 439258): file glayout/base/RestyleManager.cpp, line 1440 6544 [N] WARNING: Graph thread slowdown?: 'std::abs(framePosition - CurrentDriver()-StateComputedTime()) MillisecondsToMediaTime(5)', file gdom/media/MediaStreamGraph.cpp, line 1195 6126 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file gnetwerk/base/nsFileStreams.cpp, line 492 6126 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file gnetwerk/base/nsFileStreams.cpp, line 205 5637 [N] WARNING: No outer window available!: file gdom/base/nsGlobalWindow.cpp, line 3915 5109 [N] WARNING: NS_ENSURE_TRUE(domWindow) failed: file gembedding/browser/nsDocShellTreeOwner.cpp, line 83 5085 [N] WARNING: NS_ENSURE_TRUE(aInBrowser) failed: file gembedding/browser/nsDocShellTreeOwner.cpp, line 79 4856 [N] WARNING: zero axis length: file gdom/svg/nsSVGLength2.cpp, line 124 4708 [N] WARNING: Shouldn't call SchedulePaint in a detached pres context: file
Re: The War on Warnings
On 2015-06-04 6:07 AM, David Rajchenbach-Teller wrote: Part of my world domination plans are to turn warnings into something that causes test to actually fail (see bug 1080457 co). Would you like to join forces? There are very good reasons for warnings to not cause tests to fail. We have a lot of tests that are testing failure conditions that are expected to warn, because they are failure conditions. In my experience, in some cases where we have a particularly spammy warning, the underlying reason is that there is an error condition that we know how to handle properly, but we still warn about it. Or alternatively, there are warnings which indicate that the author of the code did not think at the time that can happen, but a lot of them can be ignored. There are also warnings that were actually meant to be assertions, and we should try to convert them to assertions, and that way they will fail tests. That being said, assertion report stack traces in debug builds, which is excruciatingly slow, so we have had to switch them to be warnings in at least some cases (see bug 756045 for example.) ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 2015-06-04 5:49 AM, Jonas Sicking wrote: FWIW, I suspect it'll be hard to put a dent in the number of warnings that we emit unless we either change all instances of NS_ENSURE_SUCCESS(rv, rv) to use some other macro which doesn't warn, or unless we change NS_ENSURE_SUCCESS(rv, rv) to not warn. Do we really have to do that? It seems like all we need to do is to investigate the highly occurring warnings and silence them somehow. NS_ENSURE_SUCCESS warning is super useful when debugging things such as intermittent test failures, or big complicated web pages that are triggering an issue somehow -- they can often give you a starting point on where you need to look. It feels like right now we have three incompatible desires: * Test lots of failure cases. * Make errors warn in debug builds on all/most frames as the failure is propagated up the callstack. * Don't warn a lot when testing debug builds. It seems like we can only pick two out of these three. It seems to me that if we change #3 to mean fixing highly frequently occurring warnings, we can have all three of the above together. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 06/04/2015 09:52 PM, Jonas Sicking wrote: On Thu, Jun 4, 2015 at 5:38 AM, Robert O'Callahan rob...@ocallahan.org wrote: Usually I use NS_WARNING to mean something weird and unexpected is happening, e.g. a bug in Web page code, but not necessarily a browser bug. Sometimes I get useful hints from NS_WARNING spew leading up to a serious failure. Yup. I think this is a quite common, and quite useful, usage of NS_WARNING. But testing runs a lot of weird and unexpected things. That's a good thing because those tend to be things that often regress. But it also leads to a lot of warnings. but for those usages could probably be switched to PR_LOG without losing much. I think this would mean changing most NS_ENSURE_SUCCESS(rv, rv), and likely many other NS_ENSURE_* to macros that call PR_LOG instead. So like I said, we'll have to either change a huge number of NS_ENSURE_* macros to use something else, or change what NS_ENSURE_* does. More likely we need to change a small number of noisy NS_ENSURE_* macro users to use something else, and keep most of the NS_ENSURE_* usage as it is. -Olli / Jonas Rob -- oIo otoeololo oyooouo otohoaoto oaonoyooonoeo owohooo oioso oaonogoroyo owoiotoho oao oboroootohoeoro oooro osoiosotoeoro owoiololo oboeo osouobojoeocoto otooo ojouodogomoeonoto.o oAogoaoiono,o oaonoyooonoeo owohooo osoaoyoso otooo oao oboroootohoeoro oooro osoiosotoeoro,o o'oRoaocoao,o'o oioso oaonosowoeoroaoboloeo otooo otohoeo ocooouoroto.o oAonodo oaonoyooonoeo owohooo osoaoyoso,o o'oYooouo ofolo!o'o owoiololo oboeo oiono odoaonogoeoro ooofo otohoeo ofoioroeo ooofo ohoeololo. ___ 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: The War on Warnings
Usually I use NS_WARNING to mean something weird and unexpected is happening, e.g. a bug in Web page code, but not necessarily a browser bug. Sometimes I get useful hints from NS_WARNING spew leading up to a serious failure, but for those usages could probably be switched to PR_LOG without losing much. Rob -- oIo otoeololo oyooouo otohoaoto oaonoyooonoeo owohooo oioso oaonogoroyo owoiotoho oao oboroootohoeoro oooro osoiosotoeoro owoiololo oboeo osouobojoeocoto otooo ojouodogomoeonoto.o oAogoaoiono,o oaonoyooonoeo owohooo osoaoyoso otooo oao oboroootohoeoro oooro osoiosotoeoro,o o‘oRoaocoao,o’o oioso oaonosowoeoroaoboloeo otooo otohoeo ocooouoroto.o oAonodo oaonoyooonoeo owohooo osoaoyoso,o o‘oYooouo ofolo!o’o owoiololo oboeo oiono odoaonogoeoro ooofo otohoeo ofoioroeo ooofo ohoeololo. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On 04/06/15 14:30, Ehsan Akhgari wrote: There are very good reasons for warnings to not cause tests to fail. We have a lot of tests that are testing failure conditions that are expected to warn, because they are failure conditions. Well, that's why the patch linked above offers a whitelist API, so that tests can individually decide which families of warnings are either expected, or ok-for-the-time-being. [...] There are also warnings that were actually meant to be assertions, and we should try to convert them to assertions, and that way they will fail tests. That being said, assertion report stack traces in debug builds, which is excruciatingly slow, so we have had to switch them to be warnings in at least some cases (see bug 756045 for example.) Are you talking about our weird non-fatal assertions or actual fatal assertions? -- David Rajchenbach-Teller, PhD Performance Team, Mozilla ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: The War on Warnings
On Wed, Jun 3, 2015 at 6:14 PM, Eric Rahm er...@mozilla.com wrote: I generated this list by grabbing the logs for a recent m-c linux64 debug run, normalizing out PIDs and timestamps and then doing some sort/uniq-fu to get counts of unique lines. If your tool could be turned into a web page that runs against the occasional build, we could use this on a more permanent basis. BTW, I appreciate the effort. I guess that most of these are as a result of actual problems, even if they are minor. And making tests faster, and output easier to find is a real win. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
The War on Warnings
We emit a *lot* of runtime warnings when running debug tests. I inadvertently triggered a max log size failure during a landing this week which encouraged me to take a look at what all is being logged, and what I found was a ton of warnings (sometimes accompanied by stack traces). Most of these should probably be removed (of course if they're real issues they should be fixed, but judging by the frequency most are probably non-issues). I'm currently cleaning up some of these, but if you happen to see something in the following list and are feeling proactive I would appreciate the help. There's even a meta bug for tracking these: https://bugzilla.mozilla.org/show_bug.cgi?id=765224 I generated this list by grabbing the logs for a recent m-c linux64 debug run, normalizing out PIDs and timestamps and then doing some sort/uniq-fu to get counts of unique lines. This is roughly the top 40 offenders: 65959 [N] WARNING: Overflowed nscoord_MAX in conversion to nscoord width: file ../../dist/include/nsRect.h, line 83 63460 [N] WARNING: NS_ENSURE_TRUE(piTarget) failed: file gdom/events/EventDispatcher.cpp, line 469 20039 [N] WARNING: 'NS_FAILED(rv)', file gdom/workers/ServiceWorkerManager.cpp, line 2529 20039 [N] WARNING: '!BasePrincipal::IsCodebasePrincipal(aPrincipal)', file gdom/workers/ServiceWorkerManager.cpp, line 2591 17784 [N] WARNING: Subdocument container has no frame: file glayout/base/nsDocumentViewer.cpp, line 2506 16322 JavaScript warning: file:///builds/slave/test/build/tests/jsreftest/tests/js1_8/extensions/regress-476427.js, line 1: JavaScript 1.6's for-each-in loops are deprecated; consider using ES6 for-of instead 14159 [N] WARNING: NS_ENSURE_TRUE(mMutable) failed: file gnetwerk/base/nsSimpleURI.cpp, line 264 14087 [N] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x80040111: file gdocshell/base/nsDocShell.cpp, line 4592 11315 [N] WARNING: '!mMainThread', file gxpcom/threads/nsThreadManager.cpp, line 299 10574 [N] WARNING: No docshells for remote frames!: file gdom/base/nsFrameLoader.cpp, line 491 9201 [N] WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'psd-mIEnd != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsLineLayout.cpp, line 884 9155 [N] WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'psd-mIEnd != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsLineLayout.cpp, line 3058 9130 [N] WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'aISize != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsLineLayout.cpp, line 160 8844 [N] WARNING: Someone passed native anonymous content directly into frame construction. Stop doing that!: file glayout/base/nsCSSFrameConstructor.cpp, line 6559 7599 [N] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file gembedding/browser/nsWebBrowser.cpp, line 363 7454 [N] WARNING: anonymous nodes should not be in child lists (bug 439258): file glayout/base/RestyleManager.cpp, line 1440 6544 [N] WARNING: Graph thread slowdown?: 'std::abs(framePosition - CurrentDriver()-StateComputedTime()) MillisecondsToMediaTime(5)', file gdom/media/MediaStreamGraph.cpp, line 1195 6126 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file gnetwerk/base/nsFileStreams.cpp, line 492 6126 [N] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file gnetwerk/base/nsFileStreams.cpp, line 205 5637 [N] WARNING: No outer window available!: file gdom/base/nsGlobalWindow.cpp, line 3915 5109 [N] WARNING: NS_ENSURE_TRUE(domWindow) failed: file gembedding/browser/nsDocShellTreeOwner.cpp, line 83 5085 [N] WARNING: NS_ENSURE_TRUE(aInBrowser) failed: file gembedding/browser/nsDocShellTreeOwner.cpp, line 79 4856 [N] WARNING: zero axis length: file gdom/svg/nsSVGLength2.cpp, line 124 4708 [N] WARNING: Shouldn't call SchedulePaint in a detached pres context: file glayout/generic/nsFrame.cpp, line 5181 4051 [N] WARNING: have unconstrained inline-size; this should only result from very large sizes, not attempts at intrinsic inline-size calculation: '(mFrameType == NS_CSS_FRAME_TYPE_INLINE !frame-IsFrameOfType(nsIFrame::eReplaced)) || type == nsGkAtoms::textFrame || ComputedISize() != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsHTMLReflowState.cpp, line 448 4050 [N] WARNING: have unconstrained inline-size; this should only result from very large sizes, not attempts at intrinsic inline-size calculation: 'AvailableISize() != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsHTMLReflowState.cpp, line 360 3897 [N] WARNING: have unconstrained
Re: The War on Warnings
Martin Thomson writes: I guess that most of these are as a result of actual problems, even if they are minor. The ones that are actual problems would be the ones that are harder to resolve. In my experience, however, when I've seen many of one kind of warning, investigation has revealed that the problem is that a warning is presented when there is nothing wrong. Often this is because someone has carelessly used NS_ENSURE_something. Perhaps that is because the macros names don't say what they do. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
