There are still a few occurrences of LOG in OS.File. We should make these occurrences depend on constant DEBUG set to false.

Hi Yoric, I would like to work on this bug. Could you please guide me on getting started with it.... Thanks.

Sure, Abhishek. For this bug, you must modify the .jsm files at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/ 1. in osfile_shared_allthreads.jsm, add a value exports.OS.Shared.DEBUG set to false; 2. in each of the .jsm files of the directory, make any use of LOG (or exports.OS.Shared.LOG) conditioned by exports.OS.Shared.DEBUG, as follows: if (exports.OS.Shared.DEBUG) { LOG(...) } 3. ensure that it does not break any of the OS.File tests, by running, from your object directory make -C toolkit/components/osfile/ && make -C browser/ && make -C toolkit/components/osfile/tests/ && TEST_PATH=toolkit/components/osfile/tests/mochi make mochitest-chrome

Comment on attachment 655287 [details] [diff] [review] made the suggested changes in files in osfile directory Review of attachment 655287 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, provided you make the following changes. ::: toolkit/components/osfile/osfile_unix_allthreads.jsm @@ +38,5 @@ > } > exports.OS.Shared.Unix = {}; > + if (exports.OS.Shared.DEBUG) { > + let LOG = OS.Shared.LOG.bind(OS.Shared, "Unix", "allthreads"); > + } No, this breaks the definition of |let LOG|. You should really try and understand how |let| works :) Anyway, just remove this change, it should not be necessary. ::: toolkit/components/osfile/osfile_unix_back.jsm @@ +26,5 @@ > } > exports.OS.Unix.File = {}; > + if (exports.OS.Shared.DEBUG) { > + let LOG = exports.OS.Shared.LOG.bind(OS.Shared, "Unix", "back"); > + } As above, please remove that change. ::: toolkit/components/osfile/osfile_unix_front.jsm @@ +29,5 @@ > let Const = exports.OS.Constants.libc; > let UnixFile = exports.OS.Unix.File; > + if (exports.OS.Shared.DEBUG) { > + let LOG = OS.Shared.LOG.bind(OS.Shared, "Unix front-end"); > + } Remove that one. ::: toolkit/components/osfile/osfile_win_allthreads.jsm @@ +38,5 @@ > } > exports.OS.Shared.Win = {}; > + if (exports.OS.Shared.DEBUG) { > + let LOG = OS.Shared.LOG.bind(OS.Shared, "Win", "allthreads"); > + } Please remove this change. ::: toolkit/components/osfile/osfile_win_back.jsm @@ +44,5 @@ > } > exports.OS.Win.File = {}; > + if (exports.OS.Shared.DEBUG) { > + let LOG = OS.Shared.LOG.bind(OS.Shared, "Win", "back"); > + } Please remove this change. ::: toolkit/components/osfile/osfile_win_front.jsm @@ +31,5 @@ > let Const = exports.OS.Constants.Win; > let WinFile = exports.OS.Win.File; > + if (exports.OS.Shared.DEBUG) { > + let LOG = OS.Shared.LOG.bind(OS.Shared, "Win front-end"); > + } Please remove this change.

Comment on attachment 655295 [details] [diff] [review] made the suggested changes in files in osfile directory Review of attachment 655295 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks good. However, there are occurrences of |LOG| still missing, e.g. in |declareFFI|, |projector|, perhaps others. Could you please make sure that you have all the occurrences of |LOG|? ::: toolkit/components/osfile/osfile_win_allthreads.jsm @@ +133,4 @@ > }); > > exports.OS.Shared.Win.Error = OSError; > +})(this); I can't see what you changed in this file. If there is no change in this file, could you remove it from the patch? ::: toolkit/components/osfile/osfile_win_back.jsm @@ +46,3 @@ > let LOG = OS.Shared.LOG.bind(OS.Shared, "Win", "back"); > let libc = exports.OS.Shared.Win.libc; > Here, too, if you have not changed anything in this file, could you remove it from the patch?

Sorry Yoric, I should I have removed the unedited files from the patch, it really did not occur to me at that time........ Now I am experiencing a new problem, I had run the hg pull -u command to update my mozilla central directory recently (after the previous patch), today I checked the previous patch and all my previous edits have been reverted to the original file and also some files I had changed in my previous patch seem to have been modified for eg: I had added a if condition in /toolkit/components/osfile/osfile_unix_front.jsm for a LOG statement and now I dont see the LOG statement at all.....has that file been updated....how do I check that out ? also is there a way I can manage to update files without the changes made by me being overwritten ?

Comment on attachment 656371 [details] [diff] [review] Conditioned logging in osfiles with exports.OS.Shared.DEBUG Review of attachment 656371 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Could you make the minor change suggested below and we will proceed to landing the patch? ::: toolkit/components/osfile/osfile_shared_allthreads.jsm @@ +45,5 @@ > }; > } > + if (exports.OS.Shared.DEBUG) { > + exports.OS.Shared.LOG = LOG; > + } I would remove this |if (...)| as this will make things a bit harder if we ever decide to make |exports.OS.Shared.DEBUG| something more complicated than a constant (e.g. a preference).

Actually, I will handle that.

Thanks a lot Yoric ! :)

(In reply to David Rajchenbach Teller [:Yoric] from comment #12) > https://tbpl.mozilla.org/?tree=Try&rev=1a5267984f80 Green on Try. Thanks for the patch, Abhishek! https://hg.mozilla.org/integration/mozilla-inbound/rev/84103a267a9e

Of course, this Try run only tested opt builds, and debug mochitests all broke. Backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/6137cbeffb2 https://tbpl.mozilla.org/php/getParsedLog.php?id=14818876&tree=Mozilla-Inbound 12917 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_back.xul | an unexpected uncaught JS exception reported through window.onerror - OS.Shared.LOG is undefined at resource://gre/modules/osfile/osfile_unix_allthreads.jsm:41 12921 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_comms.xul | an unexpected uncaught JS exception reported through window.onerror - TypeError: OS.Shared.LOG is undefined at resource:///modules/osfile/osfile_unix_allthreads.jsm:41 12922 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_comms.xul | Test timed out. 12931 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_front.xul | TypeError: OS.Shared.LOG is undefined

We need to get the fix to Aurora too, right?

My bad for fixing the issue and not attaching the fix. Here we go.

(In reply to Olli Pettay [:smaug] from comment #17) > We need to get the fix to Aurora too, right? Indeed.

I also get stuff like the attached file semi-regularly in an opt build. Should it be there? (I can spin it off to a new bug if necessary.)

(In reply to Nicholas Nethercote [:njn] from comment #21) > Created attachment 657073 [details] > more console spam > > I also get stuff like the attached file semi-regularly in an opt build. > Should it be there? (I can spin it off to a new bug if necessary.) This seems unrelated to OS.File. It should probably go to another bug.

(In reply to Ryan VanderMeulen from comment #20) > https://tbpl.mozilla.org/?tree=Try&rev=d377932cacd1 Green on Try (the m-oth failure was from another patch). https://hg.mozilla.org/integration/mozilla-inbound/rev/f68d4b2ac619

Should we fix aurora and beta too?

Yes please; 16.0b2 is extra verbose here... i'll try to backport the commit as-is to aurora/beta if noone beats me to it.

16.0 candidate build 1 still shows this issue... do we want to ship 16.0 to the end users with extra verbose debugging enabled by default ?

(In reply to Mats Palmgren [:mats] from comment #25) > Should we fix aurora and beta too? Yes, by all means!

Sorry about that, mats' bugmail got lost in ~2100 bugmails. So yes, by all means, we want to backport it.

Comment on attachment 656789 [details] [diff] [review] Remove OS.File logging [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 707679 User impact if declined: console spam at startup Testing completed (on m-c, etc.): has been on m-c for 1 month Risk to taking this patch (and alternatives if risky): none that I can think of String or UUID changes made by this patch: none

Comment on attachment 656789 [details] [diff] [review] Remove OS.File logging [Approval Request Comment] Regression caused by (bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky):

Comment on attachment 656789 [details] [diff] [review] Remove OS.File logging (removing approval-mozilla-aurora: patch is already in Aurora)

Comment on attachment 656789 [details] [diff] [review] Remove OS.File logging Almost a no-op, early enough in the cycle to approve for Beta.

So, this needs checkin for beta.

Still present in 16.0.2 release :(

Yes, I got approval-mozilla-release- :(