785200 - Remove logging from OS.File

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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

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] - still alive but not very active]

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

Attachment: made the suggested changes in files in osfile directory

Comment 4

[David Teller [:Yoric] - still alive but not very active]

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

Attachment: made the suggested changes in files in osfile directory

Comment 7

[David Teller [:Yoric] - still alive but not very active]

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

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

Attachment: Conditioned logging in osfiles with exports.OS.Shared.DEBUG

Comment 10

[David Teller [:Yoric] - still alive but not very active]

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] - still alive but not very active]

Actually, I will handle that.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

https://tbpl.mozilla.org/?tree=Try&rev=1a5267984f80

Comment 13

[Abhishek Potnis [:avp]]

Thanks a lot Yoric ! :)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(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]]

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 17

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Attachment: Remove OS.File logging

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

Comment 19

[David Teller [:Yoric] - still alive but not very active]

(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]]

https://tbpl.mozilla.org/?tree=Try&rev=d377932cacd1

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: 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] - still alive but not very active]

(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]]

(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]]

https://hg.mozilla.org/mozilla-central/rev/f68d4b2ac619

Comment 25

[Mats Palmgren (inactive)]

Should we fix aurora and beta too?

Comment 26

[Landry Breuil (:gaston)]

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

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] - still alive but not very active]

(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] - still alive but not very active]

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] - still alive but not very active]

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] - still alive but not very active]

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] - still alive but not very active]

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

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] - still alive but not very active]

So, this needs checkin for beta.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e668895c7407

Comment 38

[ilf]

Still present in 16.0.2 release :(

Comment 39

[David Teller [:Yoric] - still alive but not very active]

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