Remove logging from OS.File

RESOLVED FIXED in Firefox 17

Status

()

Toolkit
OS.File
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: avp)

Tracking

unspecified
mozilla18
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox16 affected, firefox17 fixed, firefox18 fixed)

Details

(Whiteboard: [mentor=Yoric][lang=js][qa-])

Attachments

(2 attachments, 3 obsolete attachments)

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

Comment 1

5 years ago
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
Assignee: nobody → abhishekp.bugzilla
(Assignee)

Comment 3

5 years ago
Created attachment 655287 [details] [diff] [review]
made the suggested changes in files in osfile directory
Attachment #655287 - Flags: feedback?(dteller)
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.
Attachment #655287 - Flags: feedback?(dteller) → feedback+
(Assignee)

Comment 5

5 years ago
Created attachment 655295 [details] [diff] [review]
made the suggested changes in files in osfile directory
Attachment #655287 - Attachment is obsolete: true
Attachment #655295 - Flags: feedback?(dteller)
Duplicate of this bug: 785674
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?
Attachment #655295 - Flags: feedback?(dteller) → feedback+
(Assignee)

Comment 8

5 years ago
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 ?
(Assignee)

Comment 9

5 years ago
Created attachment 656371 [details] [diff] [review]
Conditioned logging in osfiles with exports.OS.Shared.DEBUG
Attachment #655295 - Attachment is obsolete: true
Attachment #656371 - Flags: feedback?(dteller)
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).
Attachment #656371 - Flags: feedback?(dteller) → review+
Actually, I will handle that.
https://tbpl.mozilla.org/?tree=Try&rev=1a5267984f80
Blocks: 786592
(Assignee)

Comment 13

5 years ago
Thanks a lot Yoric ! :)
Keywords: checkin-needed
(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
Flags: in-testsuite-
Keywords: checkin-needed
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
Duplicate of this bug: 786860
We need to get the fix to Aurora too, right?
Created attachment 656789 [details] [diff] [review]
Remove OS.File logging

My bad for fixing the issue and not attaching the fix.
Here we go.
Attachment #656371 - Attachment is obsolete: true
Attachment #656789 - Flags: review+
(In reply to Olli Pettay [:smaug] from comment #17)
> We need to get the fix to Aurora too, right?

Indeed.
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Try&rev=d377932cacd1
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.)
(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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f68d4b2ac619
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Should we fix aurora and beta too?
status-firefox16: --- → affected
status-firefox17: --- → affected
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
Attachment #656789 - Flags: approval-mozilla-beta?
Attachment #656789 - Flags: approval-mozilla-aurora?
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):
Attachment #656789 - Flags: approval-mozilla-release?
Comment on attachment 656789 [details] [diff] [review]
Remove OS.File logging

(removing approval-mozilla-aurora: patch is already in Aurora)
Attachment #656789 - Flags: approval-mozilla-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.
Attachment #656789 - Flags: approval-mozilla-release?
Attachment #656789 - Flags: approval-mozilla-release-
Attachment #656789 - Flags: approval-mozilla-beta?
Attachment #656789 - Flags: approval-mozilla-beta+
So, this needs checkin for beta.
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-beta/rev/e668895c7407
status-firefox17: affected → fixed
status-firefox18: --- → fixed
Keywords: checkin-needed
Duplicate of this bug: 800208
Whiteboard: [mentor=Yoric][lang=js] → [mentor=Yoric][lang=js][qa-]

Updated

5 years ago
Duplicate of this bug: 805065

Comment 38

5 years ago
Still present in 16.0.2 release :(
Yes, I got approval-mozilla-release- :(
Duplicate of this bug: 806421
You need to log in before you can comment on or make changes to this bug.