Last Comment Bug 785200 - Remove logging from OS.File
: Remove logging from OS.File
Status: RESOLVED FIXED
[mentor=Yoric][lang=js][qa-]
:
Product: Toolkit
Classification: Components
Component: OS.File (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla18
Assigned To: Abhishek Potnis [:avp]
:
:
Mentors:
: 785674 786860 800208 805065 806421 (view as bug list)
Depends on:
Blocks: 786592
  Show dependency treegraph
 
Reported: 2012-08-23 13:00 PDT by David Teller [:Yoric] (please use "needinfo")
Modified: 2012-10-30 08:21 PDT (History)
12 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
fixed
fixed


Attachments
made the suggested changes in files in osfile directory (5.14 KB, patch)
2012-08-25 03:05 PDT, Abhishek Potnis [:avp]
dteller: feedback+
Details | Diff | Splinter Review
made the suggested changes in files in osfile directory (3.26 KB, patch)
2012-08-25 03:43 PDT, Abhishek Potnis [:avp]
dteller: feedback+
Details | Diff | Splinter Review
Conditioned logging in osfiles with exports.OS.Shared.DEBUG (4.34 KB, patch)
2012-08-29 01:47 PDT, Abhishek Potnis [:avp]
dteller: review+
Details | Diff | Splinter Review
Remove OS.File logging (6.68 KB, patch)
2012-08-30 02:46 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑release-
Details | Diff | Splinter Review
more console spam (12.12 KB, text/plain)
2012-08-30 16:27 PDT, Nicholas Nethercote [:njn]
no flags Details

Description David Teller [:Yoric] (please use "needinfo") 2012-08-23 13:00:02 PDT
There are still a few occurrences of LOG in OS.File. We should make these occurrences depend on constant DEBUG set to false.
Comment 1 Abhishek Potnis [:avp] 2012-08-24 09:05:47 PDT
Hi Yoric,

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


Thanks.
Comment 2 David Teller [:Yoric] (please use "needinfo") 2012-08-24 23:35:57 PDT
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 3 Abhishek Potnis [:avp] 2012-08-25 03:05:32 PDT
Created attachment 655287 [details] [diff] [review]
made the suggested changes in files in osfile directory
Comment 4 David Teller [:Yoric] (please use "needinfo") 2012-08-25 03:16:19 PDT
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 5 Abhishek Potnis [:avp] 2012-08-25 03:43:23 PDT
Created attachment 655295 [details] [diff] [review]
made the suggested changes in files in osfile directory
Comment 6 David Teller [:Yoric] (please use "needinfo") 2012-08-25 23:32:53 PDT
*** Bug 785674 has been marked as a duplicate of this bug. ***
Comment 7 David Teller [:Yoric] (please use "needinfo") 2012-08-26 00:34:55 PDT
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?
Comment 8 Abhishek Potnis [:avp] 2012-08-27 02:29:55 PDT
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 9 Abhishek Potnis [:avp] 2012-08-29 01:47:24 PDT
Created attachment 656371 [details] [diff] [review]
Conditioned logging in osfiles with exports.OS.Shared.DEBUG
Comment 10 David Teller [:Yoric] (please use "needinfo") 2012-08-29 02:09:01 PDT
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).
Comment 11 David Teller [:Yoric] (please use "needinfo") 2012-08-29 02:22:21 PDT
Actually, I will handle that.
Comment 12 David Teller [:Yoric] (please use "needinfo") 2012-08-29 02:24:43 PDT
https://tbpl.mozilla.org/?tree=Try&rev=1a5267984f80
Comment 13 Abhishek Potnis [:avp] 2012-08-29 03:20:26 PDT
Thanks a lot Yoric ! :)
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-08-29 14:04:01 PDT
(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
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-08-29 15:38:59 PDT
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
Comment 16 David Teller [:Yoric] (please use "needinfo") 2012-08-30 00:11:42 PDT
*** Bug 786860 has been marked as a duplicate of this bug. ***
Comment 17 Olli Pettay [:smaug] (reviewing overload) 2012-08-30 00:19:21 PDT
We need to get the fix to Aurora too, right?
Comment 18 David Teller [:Yoric] (please use "needinfo") 2012-08-30 02:46:15 PDT
Created attachment 656789 [details] [diff] [review]
Remove OS.File logging

My bad for fixing the issue and not attaching the fix.
Here we go.
Comment 19 David Teller [:Yoric] (please use "needinfo") 2012-08-30 02:47:42 PDT
(In reply to Olli Pettay [:smaug] from comment #17)
> We need to get the fix to Aurora too, right?

Indeed.
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-08-30 14:46:47 PDT
https://tbpl.mozilla.org/?tree=Try&rev=d377932cacd1
Comment 21 Nicholas Nethercote [:njn] 2012-08-30 16:27:15 PDT
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.)
Comment 22 David Teller [:Yoric] (please use "needinfo") 2012-08-30 21:01:33 PDT
(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.
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-08-31 03:39:50 PDT
(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
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-08-31 18:48:04 PDT
https://hg.mozilla.org/mozilla-central/rev/f68d4b2ac619
Comment 25 Mats Palmgren (:mats) 2012-09-04 17:10:40 PDT
Should we fix aurora and beta too?
Comment 26 Landry Breuil (:gaston) 2012-09-22 03:54:58 PDT
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.
Comment 27 Landry Breuil (:gaston) 2012-10-08 23:29:53 PDT
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 ?
Comment 28 David Teller [:Yoric] (please use "needinfo") 2012-10-09 00:32:24 PDT
(In reply to Mats Palmgren [:mats] from comment #25)
> Should we fix aurora and beta too?

Yes, by all means!
Comment 29 David Teller [:Yoric] (please use "needinfo") 2012-10-09 00:33:51 PDT
Sorry about that, mats' bugmail got lost in ~2100 bugmails. So yes, by all means, we want to backport it.
Comment 30 David Teller [:Yoric] (please use "needinfo") 2012-10-09 01:28:08 PDT
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 31 David Teller [:Yoric] (please use "needinfo") 2012-10-09 02:31:05 PDT
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 32 David Teller [:Yoric] (please use "needinfo") 2012-10-09 05:45:11 PDT
Comment on attachment 656789 [details] [diff] [review]
Remove OS.File logging

(removing approval-mozilla-aurora: patch is already in Aurora)
Comment 33 Alex Keybl [:akeybl] 2012-10-10 15:45:45 PDT
Comment on attachment 656789 [details] [diff] [review]
Remove OS.File logging

Almost a no-op, early enough in the cycle to approve for Beta.
Comment 34 David Teller [:Yoric] (please use "needinfo") 2012-10-14 01:14:50 PDT
So, this needs checkin for beta.
Comment 35 Ryan VanderMeulen [:RyanVM] 2012-10-14 13:04:18 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/e668895c7407
Comment 36 Virgil Dicu [:virgil] [QA] 2012-10-16 08:11:08 PDT
*** Bug 800208 has been marked as a duplicate of this bug. ***
Comment 37 Loic 2012-10-24 15:53:53 PDT
*** Bug 805065 has been marked as a duplicate of this bug. ***
Comment 38 ilf 2012-10-27 02:56:35 PDT
Still present in 16.0.2 release :(
Comment 39 David Teller [:Yoric] (please use "needinfo") 2012-10-27 03:35:29 PDT
Yes, I got approval-mozilla-release- :(
Comment 40 Virgil Dicu [:virgil] [QA] 2012-10-30 08:21:52 PDT
*** Bug 806421 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.