1003204 - Remove CommonUtils.exceptionStr()

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[:Irving Reid (No longer working on Firefox)]

The exception and stack trace formatting code has been moved from CommonUtils to Log.jsm in bug 966674, but that bug left behind a compatibility shim and didn't change other consumers of the API. The work required is:

- In all cases where code is using CommonUtils.exceptionStr() to format an exception before passing it to Log.jsm as a string, just pass the exception to Log.jsm directly and it will now be formatted correctly; e.g.

logger.warn("Message Text: " + exceptionStr(error));

should be changed to

logger.warn("Message Text", error);

Note that trailing punctuation and spaces should be removed from the string.


- All other uses of CommonUtils.exceptionStr() should change to Log.exceptionStr() (with appropriate changes to JS modules loaded in the file)

- uses of CommonUtils.stackTrace() should change to Log.stackTrace() (or be treated as exceptionStr(), if it's appropriate to log the entire exception instead of just the stack trace)

- Move the exceptionStr / stackTrace tests from services/common/utils to toolkit/modules

- Remove the exceptionStr and stackTrace declarations from services/common/utils.js

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Assining to Jan, because bug 1003888 got snagged out from under Jan's fingers! :D

Jan - you can needinfo? irving for tips on this sone - though the bug description should help you get started.

Comment 2

[Jan Svoboda]

Hi. I am really sorry, but finally I have decided to not working on this bug. It's free again.

Comment 3

[Lars Lønne [:larlon]]

Hi. I'm just getting started contributing to Firefox, and I would like to take this bug. Can you assign it to me, please?

Comment 4

[:Irving Reid (No longer working on Firefox)]

Lars, thanks for taking this on. The first step is to get you set up with a development environment and get you to build Firefox from source; if you've done that already, great! If not, I recommend starting with the tutorials and links at http://codefirefox.com/

If you need help getting started, ask in the #introduction channel on irc.mozilla.org (if you don't have an IRC client, there's a web-based one at https://www.irccloud.com/#!/ircs://irc.mozilla.org:6697/%23introduction or http://client01.chat.mibbit.com/?server=irc.mozilla.org/#introduction).

You can get my attention (if I'm on line) by saying 'irving: ping', but many others on that channel will be happy to help if I'm not around.

Comment 5

[Lars Lønne [:larlon]]

Thank you. I have already built Firefox, and I've started looking through more of the videos at codefirefox.com. I'll get started, and I'll let you know when I need some more help.

Comment 6

[Aman Singh]

Hi I want to work on this bug. Please help me.

Comment 7

[:Irving Reid (No longer working on Firefox)]

Lars, are you making any progress on this bug? If not, there's another contributor interested in working on it.

[e-mail], while we're waiting to hear back about that, have you successfully set up a Firefox development environment and built a working Firefox from source? If not, you should get started on that. The best way to do that is start with the instructions at https://developer.mozilla.org/en-US/docs/Simple_Firefox_build, and ask for help on IRC at irc.mozilla.org/#introduction

Comment 8

[Aman Singh]

Yes. I have already build Firefox. I was searching for some easy bug to start with and I found this bug interesting. Please help me.

Comment 9

[:Irving Reid (No longer working on Firefox)]

OK Aman, Lars let me know that he's not working on this bug, so you can go ahead. Feel free to ask questions here in the bug comments, or on IRC in #introduction, for anything you need help with.

Comment 10

[Sudheesh Singanamalla (:ShellHacker)]

Taking this bug :)

I've looked through Log.jsm in toolkit/modules
I can't find CommonUtils.exceptionStr(), Am I heading in the wrong direction ? Where exactly should be looking at, I tried searching through mxr for the same string and ended up hitting a brick wall.

Comment 11

[:Irving Reid (No longer working on Firefox)]

The code search tool at https://dxr.mozilla.org is going to be your best friend for this patch; see hthttp://dxr.mozilla.org/mozilla-central/search?q=exceptionStr&case=true for a search showing all the places it's used (or defined).

exceptionStr was originally defined in services/common/utils.js, and there is a lot of code that imports that definition (usually with the name 'CommonUtils.exceptionStr', but sometimes as 'Utils.exceptionStr'). When I moved the definition of the exceptionStr() function over to Log.jsm a few months ago, I left behind a definition of exceptionStr in the utils.js module (pointing at my new implementation) so that I didn't need to fix all the existing callers in order to implement my new feature.

Looking through the list of matches in dxr, I'd recommend making separate patches for each subdirectory (e.g. services/common, services/datareporting, toolkit/, etc.) because we will want to get them reviewed by separate module owners.

Comment 12

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: Patch_1003204_toolkit.patch

I have attached the toolkit portion of this, I don't know why the file that is moved from services/common/utils.js toolkit/modules/ isn't showing up even after including the file . What should I do about this ?

Comment 13

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: Patch_1003204_services.patch

This is the patch for the services/ folder of the codebase.

Comment 14

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8487323 [details] [diff] [review]
Patch_1003204_toolkit.patch

Review of attachment 8487323 [details] [diff] [review]:
-----------------------------------------------------------------

Lots of comments on this one...

First, on the last bug I reviewed for you we went over the correct format for the patch description line quite specifically, and this one doesn't match that format (or the desired content). As a reminder, it should (a) start with "Bug NNNN - " and then describe the actual change you are making (for example, on this patch, "Update or remove references to CommonUtils.exceptionStr in toolkit/"

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #12)
> Created attachment 8487323 [details] [diff] [review]
> Patch_1003204_toolkit.patch
> 
> I have attached the toolkit portion of this, I don't know why the file that
> is moved from services/common/utils.js toolkit/modules/ isn't showing up
> even after including the file . What should I do about this ?

My description in Bugzilla may not have been clear, but we do *not* want to move the entire 'utils.js' file into toolkit/modules. Only the tests for the 'exceptionStr' and 'stackTrace' functions should move. Based on a quick 'dxr.mozilla.org' search, those tests are in http://dxr.mozilla.org/mozilla-central/source/services/common/tests/unit/test_utils_exceptionStr.js and .../test_utils_stackTrace.js. Those two files should be moved to the toolkit/modules/tests/xpcshell directory.

To move those files you should use the 'hg rename' command, so that the version control system knows they have moved. You will also need to update the 'xpcshell.ini' files in both the source and destination directories; the test harness uses those files to know which files contain test cases. Once you've done that, you can run the tests with the command

./mach xpcshell-test toolkit/modules/tests/xpcshell

and review the output to make sure those test files do run and all the tests pass.

::: toolkit/components/crashes/CrashManager.jsm
@@ +268,5 @@
>                                 "file a bug: " + result);
>              }
>            } catch (ex if ex instanceof OS.File.Error) {
>              this._log.warn("I/O error reading " + entry.path + ": " +
> +                           Log.exceptionStr(ex));

As I described in the bug, for calls to the Log.jsm logging methods don't need to use Log.exceptionStr to format the exception, because Log.jsm now has that built in.

You can tell that this "this._log" is a Log.jsm object because it was assigned earlier in this file from a call to "Log.repository.getLogger(...)"

Log.jsm logger objects like this one define functions to log messages at different levels, so along with 'warn' you might see 'error', 'info', 'debug', 'trace' and others; it's worth reading through http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Log.jsm to understand everything that it can do.

So, the "this._log.warn" call should change to

this._log.warn("I/O error reading " + entry.path, ex);

and similarly for all other logging calls that are currently done by building a string by appending the result of Log.exceptionStr(whatever) to a message.

@@ +277,5 @@
>              //
>              // If we get here, report the error and delete the source file
>              // so we don't see it again.
>              Cu.reportError("Exception when processing crash event file: " +
> +                           Log.exceptionStr(ex));

Cu.reportError, on the other hand, doesn't know how to format exceptions by itself, so this one is correct ;-)

@@ +292,5 @@
>            try {
>              yield OS.File.remove(path);
>            } catch (ex) {
>              this._log.warn("Error removing event file (" + path + "): " +
> +                           Log.exceptionStr(ex));

This one needs to change to ... path + ")", ex);

And one more thing to watch for - in this file, all the references to 'CommonUtils' in the file have been removed, so you should also remove the line

Cu.import("resource://services-common/utils.js", this);

because we don't need to import that file any more. You should also check the other files you change, to see if the import of services-common/utils.js can be removed.

::: toolkit/modules/Sqlite.jsm
@@ +422,5 @@
>          // this._inProgressTransaction when called.
>          if (!this._inProgressTransaction) {
>            this._log.warn("Connection was closed while performing transaction. " +
>                           "Received error should be due to closed connection: " +
> +                         Log.exceptionStr(ex));

All the places in this file where exceptionStr() is used are in calls to Log.jsm logging methods, so they can all be changed to remove the punctuation from the end of the message string and pass the exception object as a second argument to the logging method.

Comment 15

[Sudheesh Singanamalla (:ShellHacker)]

(In reply to :Irving Reid from comment #14)
> Comment on attachment 8487323 [details] [diff] [review]
> Patch_1003204_toolkit.patch
> 
> Review of attachment 8487323 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Lots of comments on this one...
> 
> First, on the last bug I reviewed for you we went over the correct format
> for the patch description line quite specifically, and this one doesn't
> match that format (or the desired content). As a reminder, it should (a)
> start with "Bug NNNN - " and then describe the actual change you are making
> (for example, on this patch, "Update or remove references to
> CommonUtils.exceptionStr in toolkit/"
> 
> (In reply to Sudheesh Singanamalla (:ShellHacker) from comment #12)
> > Created attachment 8487323 [details] [diff] [review]
> > Patch_1003204_toolkit.patch
> > 
> > I have attached the toolkit portion of this, I don't know why the file that
> > is moved from services/common/utils.js toolkit/modules/ isn't showing up
> > even after including the file . What should I do about this ?
> 
> My description in Bugzilla may not have been clear, but we do *not* want to
> move the entire 'utils.js' file into toolkit/modules. Only the tests for the
> 'exceptionStr' and 'stackTrace' functions should move. Based on a quick
> 'dxr.mozilla.org' search, those tests are in
> http://dxr.mozilla.org/mozilla-central/source/services/common/tests/unit/
> test_utils_exceptionStr.js and .../test_utils_stackTrace.js. Those two files
> should be moved to the toolkit/modules/tests/xpcshell directory.
> 
> To move those files you should use the 'hg rename' command, so that the
> version control system knows they have moved. You will also need to update
> the 'xpcshell.ini' files in both the source and destination directories; the
> test harness uses those files to know which files contain test cases. Once
> you've done that, you can run the tests with the command
> 
> ./mach xpcshell-test toolkit/modules/tests/xpcshell
> 
> and review the output to make sure those test files do run and all the tests
> pass.
> 
> ::: toolkit/components/crashes/CrashManager.jsm
> @@ +268,5 @@
> >                                 "file a bug: " + result);
> >              }
> >            } catch (ex if ex instanceof OS.File.Error) {
> >              this._log.warn("I/O error reading " + entry.path + ": " +
> > +                           Log.exceptionStr(ex));
> 
> As I described in the bug, for calls to the Log.jsm logging methods don't
> need to use Log.exceptionStr to format the exception, because Log.jsm now
> has that built in.
> 
> You can tell that this "this._log" is a Log.jsm object because it was
> assigned earlier in this file from a call to "Log.repository.getLogger(...)"
> 
> Log.jsm logger objects like this one define functions to log messages at
> different levels, so along with 'warn' you might see 'error', 'info',
> 'debug', 'trace' and others; it's worth reading through
> http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Log.jsm to
> understand everything that it can do.
> 
> So, the "this._log.warn" call should change to
> 
> this._log.warn("I/O error reading " + entry.path, ex);
> 
> and similarly for all other logging calls that are currently done by
> building a string by appending the result of Log.exceptionStr(whatever) to a
> message.
> 
> @@ +277,5 @@
> >              //
> >              // If we get here, report the error and delete the source file
> >              // so we don't see it again.
> >              Cu.reportError("Exception when processing crash event file: " +
> > +                           Log.exceptionStr(ex));
> 
> Cu.reportError, on the other hand, doesn't know how to format exceptions by
> itself, so this one is correct ;-)
> 
> @@ +292,5 @@
> >            try {
> >              yield OS.File.remove(path);
> >            } catch (ex) {
> >              this._log.warn("Error removing event file (" + path + "): " +
> > +                           Log.exceptionStr(ex));
> 
> This one needs to change to ... path + ")", ex);
> 
> And one more thing to watch for - in this file, all the references to
> 'CommonUtils' in the file have been removed, so you should also remove the
> line
> 
> Cu.import("resource://services-common/utils.js", this);
> 
> because we don't need to import that file any more. You should also check
> the other files you change, to see if the import of services-common/utils.js
> can be removed.
> 
> ::: toolkit/modules/Sqlite.jsm
> @@ +422,5 @@
> >          // this._inProgressTransaction when called.
> >          if (!this._inProgressTransaction) {
> >            this._log.warn("Connection was closed while performing transaction. " +
> >                           "Received error should be due to closed connection: " +
> > +                         Log.exceptionStr(ex));
> 
> All the places in this file where exceptionStr() is used are in calls to
> Log.jsm logging methods, so they can all be changed to remove the
> punctuation from the end of the message string and pass the exception object
> as a second argument to the logging method.

I've made the required changes, How do I merge this to the previous commit before exporting the patch file. I seem to be lost. If I rebase I lose my patch content.

Comment 16

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8487438 [details] [diff] [review]
Patch_1003204_services.patch

Review of attachment 8487438 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/common/bagheeraclient.js
@@ +185,5 @@
>          let h = Services.telemetry.getHistogramById(options.telemetryCompressed);
>          h.add(data.length);
>        } catch (ex) {
>          this._log.warn("Unable to record telemetry for compressed payload size: " +
> +                       Log.exceptionStr(ex));

Same comment as for the previous patch - most of these are calls to log.warn, log.info, log.error and you should change the message from

...: " + Log.exceptionStr(whatever)

to

...", whatever

so that the exception is passed as another argument.

::: services/common/rest.js
@@ +494,5 @@
>          }
>        } catch (ex) {
>          this._log.warn("Exception thrown reading " + count + " bytes from " +
>                         "the channel.");
> +        this._log.warn(Log.exceptionStr(ex));

For this one (and others like it) rather than having two separate log messages, one for the text and one for the exception, add the exception as a second argument to the first log.warn() call and remove the second log.warn().

@@ +515,5 @@
>        this.onProgress();
>      } catch (ex) {
>        this._log.warn("Got exception calling onProgress handler, aborting " +
>                       this.method + " " + channel.URI.spec);
> +      this._log.debug("Exception: " + Log.exceptionStr(ex));

Same here, just pass ex to the first log.warn() and remove the log.debug()

::: services/common/tests/unit/test_utils_exceptionStr.js
@@ +7,5 @@
> +
> +Cu.import("resource://gre/modules/Log.jsm");
> +Cu.import("resource://services-common/utils.js");
> +Cu.import("resource://testing-common/httpd.js");
> +Cu.import("resource://testing-common/services/common/logging.js");

You only need to include Log.jsm here, the other three modules aren't needed.

::: services/common/tests/unit/test_utils_stackTrace.js
@@ +6,5 @@
>  function bar(v) baz(v + 1); // line 3
>  function baz(v) { throw new Error(v + 1); } // line 4
>  
>  _("Make sure lazy constructor calling/assignment works");
>  Cu.import("resource://services-common/utils.js");

You should be able to remove this import

::: services/sync/modules/engines.js
@@ +301,5 @@
>          // ex.cause will carry its stack with it when rethrown.
>          throw ex.cause;
>        } catch (ex) {
>          this._log.warn("Failed to apply incoming record " + record.id);
> +        this._log.warn("Encountered exception: " + Log.exceptionStr(ex));

services/sync is a special case; we should probably make a separate patch for everything under this directory.

services/sync/modules/util.js creates another reference to exceptionStr and stackTrace as Utils.exceptionStr and Utils.stackTrace at http://dxr.mozilla.org/mozilla-central/source/services/sync/modules/util.js#38; those should be removed along with changing all the references from Utils.exceptionStr to Log.exceptionStr (and similarly for .stackTrace).

@@ +563,5 @@
>        } else {
>          this._engines[name] = engine;
>        }
>      } catch (ex) {
> +      this._log.error(Log.exceptionStr(ex));

When the exception is the only information passed to the log method, you don't need any extra formatting or message, just

this._log.error(ex);

@@ +956,1 @@
>                         ", aborting processIncoming.");

For this one, rearrange the message to

...warn("Aborting processIncoming", ex);

::: services/sync/modules/engines/bookmarks.js
@@ +362,1 @@
>                         "\" backing up bookmarks, but continuing with sync.");

...warn("Error while backing up bookmarks, but continuing with sync", ex);

@@ +378,2 @@
>                         "\" building GUID map." +
>                         " Skipping all other incoming items.");

...warn("Error building GUID map. Skipping all other incoming items", ex);

::: services/sync/modules/engines/passwords.js
@@ +206,5 @@
>      try {
>        Services.logins.addLogin(login);
>      } catch(ex) {
>        this._log.debug("Adding record " + record.id +
> +                      " resulted in exception " + Log.exceptionStr(ex));

...debug("Failed to add record " + record.id, ex);

@@ +242,1 @@
>                        ". Not modifying.");

...debug("Failed to modify record " + record.id, ex);

::: services/sync/modules/notifications.js
@@ +116,5 @@
>        callback.apply(this, arguments);
>      } catch (e) {
>        let logger = Log.repository.getLogger("Sync.Notifications");
> +      logger.error("An exception occurred: " + Log.exceptionStr(e));
> +      logger.info(Log.stackTrace(e));

The log message for the exception always includes the stack trace, so the logger.info(Log.stackTrace(e)) line can be removed.

::: services/sync/modules/resource.js
@@ +275,2 @@
>                       " in _onComplete.");
> +      this._log.debug(Log.stackTrace(ex));

.warn("Unexpected exception in _onComplete", ex);

and remove the ..debug(log.stackTrace... line

@@ +307,2 @@
>                        " visiting headers in _onComplete.");
> +      this._log.debug(Log.stackTrace(ex));

Similar change to my previous comment.

::: services/sync/modules/util.js
@@ +35,5 @@
>    // In the ideal world, references to these would be removed.
>    nextTick: CommonUtils.nextTick,
>    namedTimer: CommonUtils.namedTimer,
> +  exceptionStr: Log.exceptionStr,
> +  stackTrace: Log.stackTrace,

I mentioned this a few comments back, but if we change all the references from Utils.exceptionStr to Log.exceptionStr in other files, we can remove these two declarations.

@@ +76,5 @@
>        try {
>          return func.call(thisArg);
>        }
>        catch(ex) {
> +        thisArg._log.debug("Exception: " + Log.exceptionStr(ex));

The message text "Exception: " doesn't add any information here, so just use

thisArg._log.debug(ex);

Comment 17

[:Irving Reid (No longer working on Firefox)]

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #15)
> I've made the required changes, How do I merge this to the previous commit
> before exporting the patch file. I seem to be lost. If I rebase I lose my
> patch content.

The best place to get help for this is on the Mozilla "introduction" IRC channel irc://irc.mozilla.org/#introduction

How familiar are you with version control systems (and which ones)?

Most Mozilla developers use an hg extension called "Mercurial Queues" or mq for short; using mq is roughly the equivalent of working on a git branch where you always squash your changes in to a single commit. There is some documentation at https://developer.mozilla.org/en/docs/Mercurial_Queues.

If you have the first version of your patch committed to Mercurial as a single check-in, you can get that under mq by doing:

  hg log | less

to see the revision ID of your committed change, then

  hg qimport -r <revision-id> -n "1003204-toolkit"

Now if you run

  hg qseries -V

you'll see

  0 A 1003204-toolkit

which means that the 0-th patch on your stack, which is currently applied in your working directory (the A), is the one named 1003204-toolkit.

This still leaves your not-checked-in changes where they are in your working copy. Next, you can add the new changes you've made to your working copy to the mq patch with the command

  hg qrefresh

This merges the previously-checked-in changes in your patch with the new changes in your working copy, resulting in a new patch that includes all the changes.

You can also bring up an editor to change the patch description with the command:

  hg qrefresh -e


Because mq manages the patch as a single hg change, when you are finally ready to 'hg export' your work, it should all come out as one patch file in the format we want.

Comment 18

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: Patch_1003204_toolkit_v1.patch

Sorry for the delay, I was stuck due to exams and couldn't look into this.

I am very comfortable with Git and Bzr. I've managed to work this out and taking the opportunity to learn and experience Hg with mq.

Obsoletes older patch for toolkit/ New patch should look good ?

Comment 19

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: Patch_1003204_services_common.patch

Removes the older services patch.

Attaches

1. Patch for services/common

Comment 20

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: Patch_1003204_services_datareporting.patch

2. Patch for services/datareporting

Comment 21

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: Patch_1003204_toolkit.patch

Sorry for the previous attachment. Commit message had the wrong bug number. My bad.

3. toolkit/

Comment 22

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: Patch_1003204_services_healthreport.patch

4. services/healthreport

Comment 23

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: Patch_1003204_services_metrics.patch

5. services/metrics

Comment 24

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: Patch_1003204_services_sync.patch

6. services/sync

Comment 25

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8499482 [details] [diff] [review]
Patch_1003204_services_common.patch

Review of attachment 8499482 [details] [diff] [review]:
-----------------------------------------------------------------

Please give each patch file that you upload a unique file name, so it's easier for the reviewers and people who check in your patches; it's best if the file name includes:

- the bug number
- for multiple patches on one bug, the order they should be applied
- a *very* brief indication of the content

The descriptions you gave for the attachments would have been OK as the file name, but unfortunately Bugzilla remembered the actual file name you used (file_1003204.txt) and that's the name I get for every patch I try to save to my computer.

There are still some steps in the initial bug description that haven't been done yet - moving the test cases from services/common/utils to toolkit/modules (more details in comment 14), and removing the declarations from services/common/utils.js (and fixing the one use of CommonUtils.exceptionStr in utils.js). You should add one more patch to this set, numbered so that it gets landed last, to make those final changes.

::: services/common/bagheeraclient.js
@@ +184,5 @@
>        try {
>          let h = Services.telemetry.getHistogramById(options.telemetryCompressed);
>          h.add(data.length);
>        } catch (ex) {
> +        this._log.warn("Unable to record telemetry for compressed payload size: ", ex);

The Log.jsm formatter adds ": " to the end of the log message before it appends the formatted exception, so for every log message you need to remove any punctuation and spaces at the end of the string; for example, the line above should be:

this._log.warn("Unable to record telemetry for compressed payload size", ex);

This same change needs to be made to almost every message string in all the patches.

@@ +249,5 @@
>    _onComplete: function _onComplete(request, deferred, result, error) {
>      result.request = request;
>  
>      if (error) {
> +      this._log.info("Transport failure on request: ", error);

... Like here, remove the ': '. I won't add comments on any of the other places this change is needed, unless it's a special case.

::: services/common/rest.js
@@ +493,5 @@
>            this.response.body += str.value;
>          }
>        } catch (ex) {
>          this._log.warn("Exception thrown reading " + count + " bytes from " +
> +                       "the channel.", ex);

Remove '.' at the end of the string.

Comment 26

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8499484 [details] [diff] [review]
Patch_1003204_services_datareporting.patch

Review of attachment 8499484 [details] [diff] [review]:
-----------------------------------------------------------------

Clean up the ': ' on the ends of strings where necessary and this will be ready for r+.

::: services/datareporting/DataReportingService.js
@@ +126,5 @@
>  
>            this._os.addObserver(this, "sessionstore-windows-restored", true);
>          } catch (ex) {
>            Cu.reportError("Exception when initializing data reporting service: " +
> +                         Log.exceptionStr(ex));

This ': ' we need to keep, because we're building a string for Cu.reportError rather than passing to Log.jsm.

::: services/datareporting/policy.jsm
@@ +732,5 @@
>      deferred.promise.then((function onSuccess() {
>        this._recordDataPolicyNotification(this.now(), this.currentPolicyVersion);
>        this._userNotifyPromise = null;
>      }).bind(this), ((error) => {
> +      this._log.warn("Data policy notification presentation failed: ", error);

This ': ' needs to be removed (as do the others that are being passed to Log.jsm)

Comment 27

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8499485 [details] [diff] [review]
Patch_1003204_toolkit.patch

Review of attachment 8499485 [details] [diff] [review]:
-----------------------------------------------------------------

There's one 'CommonUtils.exceptionStr' call still in Sqlite.jsm, around line 904. This may have been added after you made your patch.

::: toolkit/modules/Sqlite.jsm
@@ +419,5 @@
>          // Assertion: close() will unset
>          // this._inProgressTransaction when called.
>          if (!this._inProgressTransaction) {
>            this._log.warn("Connection was closed while performing transaction. " +
> +                         "Received error should be due to closed connection: ", ex);

Remove ': ' (and on all the other similar messages).

Comment 28

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8499489 [details] [diff] [review]
Patch_1003204_services_healthreport.patch

Review of attachment 8499489 [details] [diff] [review]:
-----------------------------------------------------------------

After the ': ' issue is fixed, this one will be done.

::: services/healthreport/healthreporter.jsm
@@ +155,5 @@
>                 ex.becauseNoSuchFile) {
>          this._log.warn("Saved state file does not exist.");
>          resetObjectState();
>        } catch (ex) {
> +        this._log.error("Exception when reading state from disk: ", ex);

Remove ': ' from all Log.jsm messages.

Comment 29

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8499490 [details] [diff] [review]
Patch_1003204_services_metrics.patch

Review of attachment 8499490 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/metrics/providermanager.jsm
@@ +271,5 @@
>                          "to finish before registering.");
>          try {
>            yield inFlightPromise;
>          } catch (ex) {
> +          this._log.warn("Error when waiting for existing pull-only promise: ", ex);

Remove ': '

Comment 30

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8499515 [details] [diff] [review]
Patch_1003204_services_sync.patch

Review of attachment 8499515 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/modules/addonsreconciler.js
@@ +486,5 @@
>      for each (let listener in this._listeners) {
>        try {
>          listener.changeListener.call(listener, date, change, state);
>        } catch (ex) {
> +        this._log.warn("Exception calling change listener: ", ex);

Remove ': '

::: services/sync/modules/engines/bookmarks.js
@@ +618,5 @@
>                                         PlacesUtils.bookmarks.DEFAULT_INDEX);
>          return true;
>        }
>      } catch(ex) {
> +      this._log.debug("Failed to reparent item. ", ex);

Remove '. '

::: services/sync/modules/engines/passwords.js
@@ +205,5 @@
>                      "formSubmitURL: " + JSON.stringify(login.formSubmitURL));
>      try {
>        Services.logins.addLogin(login);
>      } catch(ex) {
> +      this._log.debug("Failed to add record " + record.id, ex);

Remove ' '

::: services/sync/modules/resource.js
@@ +270,5 @@
>  
>      } catch(ex) {
>        // Got a response, but an exception occurred during processing.
>        // This shouldn't occur.
> +      this._log..warn("Unexpected exception in _onComplete", ex);

two dots between _log..warn should be one - this._log.warn(

@@ +315,5 @@
>      XPCOMUtils.defineLazyGetter(ret, "obj", function() {
>        try {
>          return JSON.parse(ret);
>        } catch (ex) {
> +        this._log.warn("Got exception parsing response body: \"", ex);

Remove ': \"'

@@ +521,5 @@
>      try {
>        siStream = Cc["@mozilla.org/scriptableinputstream;1"].createInstance(Ci.nsIScriptableInputStream);
>        siStream.init(stream);
>      } catch (ex) {
> +      this._log.warn("Exception creating nsIScriptableInputStream.", ex);

Remove '.'

@@ +654,5 @@
>      // We let all redirects proceed.
>      try {
>        callback.onRedirectVerifyCallback(Cr.NS_OK);
>      } catch (ex) {
> +      this._log.error("onRedirectVerifyCallback threw!", ex);

remove '!'

Comment 31

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_datareporting.patch

Obsolete older patch for datareporting, renames patch to 1003204_datareporting.patch for making it easier to review.

Comment 32

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_toolkit.patch

I've found the new CommonUtils.exceptionStr() here [1], I am unable to pull these changes to the file to my repository. The rest of the errors have been corrected.

http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Sqlite.jsm

Comment 33

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_healthreport.patch

Obsolete older patch, fixes all the ': ' problem in the logging in healthreport.

Comment 34

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_metrics.patch

Obsolete older patch for metrics, fixes all the ': ' and removes ": " from log messages which have += ": " + Log....

Comment 35

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_sync.patch

Obsolete older patch for sync, fixes up ': ' for logging which uses Log.jsm

Comment 36

[Sudheesh Singanamalla (:ShellHacker)]

Setting a need info for myself to fix the /common later. Need some more time for that

Comment 37

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_common_part1.patch

Fixes errors and the missing work for utils.js file

Comment 38

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_common_part2.patch

This contains the hg rename for the required test_utils_* files and updates to the xpcshell.ini files,

./mach xpcshell-test toolkit/modules/tests/xpcshell

passes all the tests.

Comment 39

[Sudheesh Singanamalla (:ShellHacker)]

Irving,

Sorry for delay in sending in the last patch, I was kind of stuck with some homework assignments for my university course work.

Could you review these now and say if any changes are required ?

Comment 40

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8504461 [details] [diff] [review]
1003204_datareporting.patch

Review of attachment 8504461 [details] [diff] [review]:
-----------------------------------------------------------------

One tiny change needed. For the final review on this, please request review by :gfritzsche to make sure this fits with other work in services/datareporting

::: services/datareporting/DataReportingService.js
@@ +218,5 @@
>        this._loadHealthReporter();
>      } catch (ex) {
>        this._healthReporter = null;
> +       Cu.reportError("Exception when obtaining health reporter" +
> +                     Log.exceptionStr(ex));

This is a string concatenation for Cu.reportError, so it needs to keep the ': ' on the message string.

Comment 41

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_datareporting.patch

(In reply to :Irving Reid from comment #40)
> Comment on attachment 8504461 [details] [diff] [review]
> 1003204_datareporting.patch
> 
> Review of attachment 8504461 [details] [diff] [review]:
> -----------------------------------------------------------------

Change made.

> One tiny change needed. For the final review on this, please request review
> by :gfritzsche to make sure this fits with other work in
> services/datareporting
> 
> ::: services/datareporting/DataReportingService.js
> @@ +218,5 @@
> >        this._loadHealthReporter();
> >      } catch (ex) {
> >        this._healthReporter = null;
> > +       Cu.reportError("Exception when obtaining health reporter" +
> > +                     Log.exceptionStr(ex));
> 
> This is a string concatenation for Cu.reportError, so it needs to keep the
> ': ' on the message string.

Requesting Georg for review.

Obsoletes older patch for data_Reporting.

Comment 42

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8506593 [details] [diff] [review]
1003204_datareporting.patch

Review of attachment 8506593 [details] [diff] [review]:
-----------------------------------------------------------------

Can we please update the Log.jsm documentation to show what automatic parameter formatting is supported?
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Log.jsm

Otherwise we will probably end up with custom solutions again because not everyone will find out about this.

::: services/datareporting/DataReportingService.js
@@ -7,5 @@
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  Cu.import("resource://gre/modules/Preferences.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> -Cu.import("resource://services-common/utils.js");

Heads-up: if you land after bug 1064333, you can't remove this.

Comment 43

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8504468 [details] [diff] [review]
1003204_healthreport.patch

Review of attachment 8504468 [details] [diff] [review]:
-----------------------------------------------------------------

One tiny fix and this one is done. Please request the final review from :gfritzsche for this patch.

::: services/healthreport/profile.jsm
@@ +155,5 @@
>        }
>  
>        function onStatFailure(e) {
>          // Never mind.
> +        self._log.debug("Stat failure" + Log.exceptionStr(e));

this should be self._log.debug("Stat failure", e);

While the existing code didn't do this, I would love to have this error message include the file name.

Comment 44

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8504470 [details] [diff] [review]
1003204_metrics.patch

Review of attachment 8504470 [details] [diff] [review]:
-----------------------------------------------------------------

Again, just needs one tiny fix. Please request final review from :gps or :rnewman.

::: services/metrics/providermanager.jsm
@@ +526,5 @@
>     */
>    _recordProviderError: function (name, msg, ex) {
>      msg = "Provider error: " + name + ": " + msg;
>      if (ex) {
> +      msg += Log.exceptionStr(ex);

For building message strings like this, you need to keep the ": " +

Comment 45

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8504474 [details] [diff] [review]
1003204_sync.patch

Review of attachment 8504474 [details] [diff] [review]:
-----------------------------------------------------------------

This looks clean. Requesting superreview from people responsible for the services/sync module to make sure they approve.

Comment 46

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8506017 [details] [diff] [review]
1003204_common_part1.patch

Review of attachment 8506017 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me; asking a peer of services/common for final review

Comment 47

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8506019 [details] [diff] [review]
1003204_common_part2.patch

Review of attachment 8506019 [details] [diff] [review]:
-----------------------------------------------------------------

The file moves aren't included in this patch. Did you use 'hg rename' to move the files?

Comment 48

[Richard Newman [:rnewman]]

Comment on attachment 8506017 [details] [diff] [review]
1003204_common_part1.patch

Review of attachment 8506017 [details] [diff] [review]:
-----------------------------------------------------------------

There's some chance that you'll break tests with this change. Make sure you've run all of the tests:

  ./mach test services/common

::: services/common/tests/unit/test_utils_exceptionStr.js
@@ +4,5 @@
>  "use strict";
>  
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +Cu.import("resource://gre/modules/Log.jsm");

Kill the changes in this file; they're not necessary.

Eventually you'll want to move these tests to toolkit/modules/tests/xpcshell.

Comment 49

[Richard Newman [:rnewman]]

Comment on attachment 8504474 [details] [diff] [review]
1003204_sync.patch

Review of attachment 8504474 [details] [diff] [review]:
-----------------------------------------------------------------

Have you run both Sync's xpcshell tests and TPS with these changes applied? One or both of those suites have tests for logging of errors, and you've altered the format of errors in lots of places.

See here for running TPS:

https://developer.mozilla.org/en-US/docs/TPS_Tests

Please re-flag me for review when you've confirmed that both sets of tests pass, and addressed comments below.

::: services/sync/modules/engines/bookmarks.js
@@ +372,5 @@
>        let guidMap;
>        try {
>          guidMap = this._buildGUIDMap();
>        } catch (ex) {
> +        this._log.warn("Error building GUID map. Skipping all other incoming items", ex);

I have a preference for log messages that are complete sentences -- including finishing punctuation. (This comment applies throughout, but especially here.)

::: services/sync/modules/policies.js
@@ +679,5 @@
>          file.remove(false);
>          errorHandler._log.trace("Deleted " + file.leafName + ".");
>        } catch (ex) {
>          errorHandler._log._debug("Encountered error trying to clean up old log file '"
> +                                 + oldLogs[index].leafName, ex);

You've accidentally trimmed the closing single quote that wraps the leafName.

::: services/sync/modules/stages/enginesync.js
@@ -13,5 @@
>  Cu.import("resource://gre/modules/Log.jsm");
>  Cu.import("resource://services-sync/constants.js");
>  Cu.import("resource://services-sync/engines.js");
>  Cu.import("resource://services-sync/policies.js");
> -Cu.import("resource://services-sync/util.js");

This import provides Svc, which is used elsewhere in this file.

::: services/sync/tests/unit/test_addon_utils.js
@@ -6,3 @@
>  Cu.import("resource://gre/modules/Preferences.jsm");
>  Cu.import("resource://services-sync/addonutils.js");
> -Cu.import("resource://services-sync/util.js");

Again, you shouldn't remove this.

Comment 50

[:Irving Reid (No longer working on Firefox)]

(In reply to Richard Newman [:rnewman] from comment #49)
> Comment on attachment 8504474 [details] [diff] [review]
> 1003204_sync.patch

> > +        this._log.warn("Error building GUID map. Skipping all other incoming items", ex);
> 
> I have a preference for log messages that are complete sentences --
> including finishing punctuation. (This comment applies throughout, but
> especially here.)

The Log.jsm formatter adds punctuation; this will be output as

... Skipping all other outcoming items: <formatted exception>

Comment 51

[Richard Newman [:rnewman]]

(In reply to :Irving Reid from comment #50)

> The Log.jsm formatter adds punctuation…

:thumbsup:

Comment 52

[Sudheesh Singanamalla (:ShellHacker)]

(In reply to :Irving Reid from comment #47)
> Comment on attachment 8506019 [details] [diff] [review]
> 1003204_common_part2.patch
> 
> Review of attachment 8506019 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The file moves aren't included in this patch. Did you use 'hg rename' to
> move the files?

Yes I used hg rename to move the files before committing them

This is present in the patch

diff --git a/services/common/tests/unit/test_utils_exceptionStr.js b/toolkit/modules/tests/xpcshell/test_utils_exceptionStr.js
rename from services/common/tests/unit/test_utils_exceptionStr.js
rename to toolkit/modules/tests/xpcshell/test_utils_exceptionStr.js
diff --git a/services/common/tests/unit/test_utils_stackTrace.js b/toolkit/modules/tests/xpcshell/test_utils_stackTrace.js
rename from services/common/tests/unit/test_utils_stackTrace.js
rename to toolkit/modules/tests/xpcshell/test_utils_stackTrace.js
diff --git a/toolkit/modules/tests/xpcshell/xpcshell.ini b/toolkit/modules/tests/xpcshell/xpcshell.ini

Comment 53

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_healthreport.patch

(In reply to :Irving Reid from comment #43)
> Comment on attachment 8504468 [details] [diff] [review]
> 1003204_healthreport.patch
> 
> Review of attachment 8504468 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One tiny fix and this one is done. Please request the final review from
> :gfritzsche for this patch.
> 
> ::: services/healthreport/profile.jsm
> @@ +155,5 @@
> >        }
> >  
> >        function onStatFailure(e) {
> >          // Never mind.
> > +        self._log.debug("Stat failure" + Log.exceptionStr(e));
> 
> this should be self._log.debug("Stat failure", e);
> 
> While the existing code didn't do this, I would love to have this error
> message include the file name.

Obsoletes older patch. Fixes the _log.debug and adds filename in the stat error log.

Comment 54

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_metrics.patch

Fixes the way message strings are built with catenation += ": "

Comment 55

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8508505 [details] [diff] [review]
1003204_healthreport.patch

Review of attachment 8508505 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/healthreport/healthreporter.jsm
@@ +737,5 @@
>      let logMessage = message;
>  
>      if (ex) {
> +      recordMessage += Log.exceptionStr(ex);
> +      logMessage += Log.exceptionStr(ex);

I don't think you should remove the ": " here.

::: services/healthreport/providers.jsm
@@ +498,5 @@
>          }
>  
>          yield m[method](v, value);
>        } catch (ex) {
> +        this._log.warn("Error obtaining system info field" + k, ex);

This breaks the formatting here, especially without any whitespace before k.

Comment 56

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_healthreport.patch

I corrected those small errors, hope this goes well ?

Comment 57

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_sync.patch

Carries over the previous r+ from Irving.

According to comment 50, the punctuation is added automatically by Log.jsm formatter.

I've fixed the other issues present in comment 49.

Sorry for the delay in the patch.

Comment 58

[:Irving Reid (No longer working on Firefox)]

Georg, can you finish up mentoring this bug?

Comment 59

[Georg Fritzsche [:gfritzsche]]

I am really busy this week. I can catch up here next week or someone else should take over.

Comment 60

[Sudheesh Singanamalla (:ShellHacker)]

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #52)
> (In reply to :Irving Reid from comment #47)
> > Comment on attachment 8506019 [details] [diff] [review]
> > 1003204_common_part2.patch
> > 
> > Review of attachment 8506019 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The file moves aren't included in this patch. Did you use 'hg rename' to
> > move the files?
> 
> Yes I used hg rename to move the files before committing them
> 
> This is present in the patch
> 
> diff --git a/services/common/tests/unit/test_utils_exceptionStr.js
> b/toolkit/modules/tests/xpcshell/test_utils_exceptionStr.js
> rename from services/common/tests/unit/test_utils_exceptionStr.js
> rename to toolkit/modules/tests/xpcshell/test_utils_exceptionStr.js
> diff --git a/services/common/tests/unit/test_utils_stackTrace.js
> b/toolkit/modules/tests/xpcshell/test_utils_stackTrace.js
> rename from services/common/tests/unit/test_utils_stackTrace.js
> rename to toolkit/modules/tests/xpcshell/test_utils_stackTrace.js
> diff --git a/toolkit/modules/tests/xpcshell/xpcshell.ini
> b/toolkit/modules/tests/xpcshell/xpcshell.ini

Irving, What about this comment here ? for common_patch_2, it does have the hg renames

Comment 61

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8512710 [details] [diff] [review]
1003204_healthreport.patch

Review of attachment 8512710 [details] [diff] [review]:
-----------------------------------------------------------------

Nearly there, thanks.

::: services/healthreport/profile.jsm
@@ +155,5 @@
>        }
>  
>        function onStatFailure(e) {
>          // Never mind.
> +        self._log.debug("Healthreport/profile.jsm, Stat failure", e);

Why that change? This doesn't match any other logging statement in that file.

Comment 62

[Georg Fritzsche [:gfritzsche]]

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #60)
> (In reply to Sudheesh Singanamalla (:ShellHacker) from comment #52)
> > (In reply to :Irving Reid from comment #47)
> > > Comment on attachment 8506019 [details] [diff] [review]
> > > 1003204_common_part2.patch
> > > 
> > > Review of attachment 8506019 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > The file moves aren't included in this patch. Did you use 'hg rename' to
> > > move the files?
> > 
> > Yes I used hg rename to move the files before committing them
> > 
> > This is present in the patch
> > 
> > diff --git a/services/common/tests/unit/test_utils_exceptionStr.js
> > b/toolkit/modules/tests/xpcshell/test_utils_exceptionStr.js
> > rename from services/common/tests/unit/test_utils_exceptionStr.js
> > rename to toolkit/modules/tests/xpcshell/test_utils_exceptionStr.js
> > diff --git a/services/common/tests/unit/test_utils_stackTrace.js
> > b/toolkit/modules/tests/xpcshell/test_utils_stackTrace.js
> > rename from services/common/tests/unit/test_utils_stackTrace.js
> > rename to toolkit/modules/tests/xpcshell/test_utils_stackTrace.js
> > diff --git a/toolkit/modules/tests/xpcshell/xpcshell.ini
> > b/toolkit/modules/tests/xpcshell/xpcshell.ini
> 
> Irving, What about this comment here ? for common_patch_2, it does have the
> hg renames

Indeed, they are in the patch. They don't show up in the splinter review here, hence the confusion.

Comment 63

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8506017 [details] [diff] [review]
1003204_common_part1.patch

Review of attachment 8506017 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/common/tests/unit/test_utils_exceptionStr.js
@@ +13,5 @@
>  
>  add_test(function test_exceptionStr_non_exceptions() {
>    do_check_eq(CommonUtils.exceptionStr(null), "null");
>    do_check_eq(CommonUtils.exceptionStr(false), "false");
>    do_check_eq(CommonUtils.exceptionStr(undefined), "undefined");

I think you forgot to change these checks to use Log.exceptionStr()?

Comment 64

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8506019 [details] [diff] [review]
1003204_common_part2.patch

Review of attachment 8506019 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, but don't forget to fix the test i commented on above for common_part1

Comment 65

[Sudheesh Singanamalla (:ShellHacker)]

(In reply to :Irving Reid from comment #43)

> > +        self._log.debug("Stat failure" + Log.exceptionStr(e));
> 
> this should be self._log.debug("Stat failure", e);
> 
> While the existing code didn't do this, I would love to have this error
> message include the file name.

(In reply to Georg Fritzsche [:gfritzsche] from comment #61)
> >        function onStatFailure(e) {
> >          // Never mind.
> > +        self._log.debug("Healthreport/profile.jsm, Stat failure", e);
> 
> Why that change? This doesn't match any other logging statement in that file.

When Irving mentioned that he wants the error to include the file name, I thought that it should be put as Healthreport/profile.jsm, <error msg>

Could you mention what change I exactly need to be making ?

Comment 66

[Sudheesh Singanamalla (:ShellHacker)]

Comment on attachment 8506017 [details] [diff] [review]
1003204_common_part1.patch

# HG changeset patch
# User Sudheesh Singanamalla <[e-mail]>
# Date 1412331320 -19800
#      Fri Oct 03 15:45:20 2014 +0530
# Node ID b3f9940053b45089a452ad0ca87313766dbeeb9b
# Parent  9f1935f4d4b6f5faa4578c9edf10702e8ec616a7
Bug 1003204 - Removes CommonUtils.exceptionStr() from services/common

diff --git a/services/common/bagheeraclient.js b/services/common/bagheeraclient.js
--- a/services/common/bagheeraclient.js
+++ b/services/common/bagheeraclient.js
@@ -180,18 +180,17 @@ BagheeraClient.prototype = Object.freeze
     // consist of only low bytes first.
     data = CommonUtils.encodeUTF8(data);
     data = CommonUtils.convertString(data, "uncompressed", "deflate");
     if (options.telemetryCompressed) {
       try {
         let h = Services.telemetry.getHistogramById(options.telemetryCompressed);
         h.add(data.length);
       } catch (ex) {
-        this._log.warn("Unable to record telemetry for compressed payload size: " +
-                       CommonUtils.exceptionStr(ex));
+        this._log.warn("Unable to record telemetry for compressed payload size", ex);
       }
     }
 
     // TODO proper header per bug 807134.
     request.setHeader("Content-Type", "application/json+zlib; charset=utf-8");
 
     this._log.info("Request body length: " + data.length);
 
@@ -246,18 +245,17 @@ BagheeraClient.prototype = Object.freeze
 
     return this.baseURI + "1.0/submit/" + namespace + "/" + id;
   },
 
   _onComplete: function _onComplete(request, deferred, result, error) {
     result.request = request;
 
     if (error) {
-      this._log.info("Transport failure on request: " +
-                     CommonUtils.exceptionStr(error));
+      this._log.info("Transport failure on request", error);
       result.transportSuccess = false;
       deferred.resolve(result);
       return;
     }
 
     result.transportSuccess = true;
 
     let response = request.response;
diff --git a/services/common/hawkclient.js b/services/common/hawkclient.js
--- a/services/common/hawkclient.js
+++ b/services/common/hawkclient.js
@@ -23,17 +23,16 @@
  * a single remote server.  Therefore, callers who want to interact with
  * multiple HAWK services should instantiate one HawkClient per service.
  */
 
 this.EXPORTED_SYMBOLS = ["HawkClient"];
 
 const {interfaces: Ci, utils: Cu} = Components;
 
-Cu.import("resource://services-common/utils.js");
 Cu.import("resource://services-crypto/utils.js");
 Cu.import("resource://services-common/hawkrequest.js");
 Cu.import("resource://services-common/observers.js");
 Cu.import("resource://gre/modules/Promise.jsm");
 Cu.import("resource://gre/modules/Log.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 
@@ -244,18 +243,17 @@ this.HawkClient.prototype = {
     };
 
     function onComplete(error) {
       try {
         // |this| is the RESTRequest object and we need to ensure _onComplete
         // gets the same one.
         _onComplete.call(this, error);
       } catch (ex) {
-        log.error("Unhandled exception processing response:" +
-                  CommonUtils.exceptionStr(ex));
+        log.error("Unhandled exception processing response", ex);
         deferred.reject(ex);
       }
     }
 
     let extra = {
       now: this.now(),
       localtimeOffsetMsec: this.localtimeOffsetMsec,
     };
diff --git a/services/common/hawkrequest.js b/services/common/hawkrequest.js
--- a/services/common/hawkrequest.js
+++ b/services/common/hawkrequest.js
@@ -163,17 +163,17 @@ this.Intl.prototype = {
   },
 
   readPref: function() {
     this._everRead = true;
     try {
       this._accepted = Services.prefs.getComplexValue(
         "intl.accept_languages", Ci.nsIPrefLocalizedString).data;
     } catch (err) {
-      this._log.error("Error reading intl.accept_languages pref: " + CommonUtils.exceptionStr(err));
+      this._log.error("Error reading intl.accept_languages pref", err);
     }
   },
 
   get accept_languages() {
     if (!this._everRead) {
       this.readPref();
     }
     return this._accepted;
diff --git a/services/common/modules-testing/bagheeraserver.js b/services/common/modules-testing/bagheeraserver.js
--- a/services/common/modules-testing/bagheeraserver.js
+++ b/services/common/modules-testing/bagheeraserver.js
@@ -154,18 +154,17 @@ BagheeraServer.prototype = {
                                     path.substr("/1.0/submit/".length));
       } else {
         throw HTTP_404;
       }
     } catch (ex) {
       if (ex instanceof HttpError) {
         this._log.info("HttpError thrown: " + ex.code + " " + ex.description);
       } else {
-        this._log.warn("Exception processing request: " +
-                       CommonUtils.exceptionStr(ex));
+        this._log.warn("Exception processing request", ex);
       }
 
       throw ex;
     }
   },
 
   /**
    * Handles requests to /submit/*.
diff --git a/services/common/modules-testing/storageserver.js b/services/common/modules-testing/storageserver.js
--- a/services/common/modules-testing/storageserver.js
+++ b/services/common/modules-testing/storageserver.js
@@ -558,18 +558,17 @@ StorageServerCollection.prototype = {
           if (record.sortindex) {
             bso.sortindex = parseInt(record.sortindex, 10);
           }
 
         } else {
           failed[record.id] = "no bso configured";
         }
       } catch (ex) {
-        this._log.info("Exception when processing BSO: " +
-                       CommonUtils.exceptionStr(ex));
+        this._log.info("Exception when processing BSO", ex);
         failed[record.id] = "Exception when processing.";
       }
     }
     return {success: success, failed: failed};
   },
 
   delete: function delete_(options) {
     options = options || {};
@@ -918,17 +917,17 @@ StorageServer.prototype = {
       this.port = this.server.identity.primaryPort;
       this.started = true;
       if (cb) {
         cb();
       }
     } catch (ex) {
       _("==========================================");
       _("Got exception starting Storage HTTP server on port " + this.port);
-      _("Error: " + CommonUtils.exceptionStr(ex));
+      _("Error: " + Log.exceptionStr(ex));
       _("Is there a process already listening on port " + this.port + "?");
       _("==========================================");
       do_throw(ex);
     }
   },
 
   /**
    * Start the server synchronously.
@@ -1263,17 +1262,17 @@ StorageServer.prototype = {
     this.requestCount++;
     let timestamp = new_timestamp();
     try {
       this._handleDefault(handler, req, resp, timestamp);
     } catch (e) {
       if (e instanceof HttpError) {
         this.respond(req, resp, e.code, e.description, "", {}, timestamp);
       } else {
-        this._log.warn(CommonUtils.exceptionStr(e));
+        this._log.warn("StorageServer: Handle Default Exception", e);
         throw e;
       }
     }
   },
 
   _handleDefault: function _handleDefault(handler, req, resp, timestamp) {
     let path = req.path;
     if (req.queryString.length) {
@@ -1368,18 +1367,17 @@ StorageServer.prototype = {
     }
 
     // Hand off to the appropriate handler for this path component.
     if (first in this.toplevelHandlers) {
       let handler = this.toplevelHandlers[first];
       try {
         return handler.call(this, handler, req, resp, version, username, rest);
       } catch (ex) {
-        this._log.warn("Got exception during request: " +
-                       CommonUtils.exceptionStr(ex));
+        this._log.warn("Got exception during request", ex);
         throw ex;
       }
     }
     this._log.debug("StorageServer: Unknown top-level " + first);
     throw HTTP_404;
   },
 
   /**
diff --git a/services/common/rest.js b/services/common/rest.js
--- a/services/common/rest.js
+++ b/services/common/rest.js
@@ -334,17 +334,17 @@ RESTRequest.prototype = {
     // as to what the response might be encoded as.
     channel.contentCharset = this.charset;
 
     // Blast off!
     try {
       channel.asyncOpen(this, null);
     } catch (ex) {
       // asyncOpen can throw in a bunch of cases -- e.g., a forbidden port.
-      this._log.warn("Caught an error in asyncOpen: " + CommonUtils.exceptionStr(ex));
+      this._log.warn("Caught an error in asyncOpen" , ex);
       CommonUtils.nextTick(onComplete.bind(this, ex));
     }
     this.status = this.SENT;
     this.delayTimeout();
     return this;
   },
 
   /**
@@ -489,18 +489,17 @@ RESTRequest.prototype = {
       try {
         let str = {};
         let num = this._converterStream.readString(count, str);
         if (num != 0) {
           this.response.body += str.value;
         }
       } catch (ex) {
         this._log.warn("Exception thrown reading " + count + " bytes from " +
-                       "the channel.");
-        this._log.warn(CommonUtils.exceptionStr(ex));
+                       "the channel", ex);
         throw ex;
       }
     } else {
       this.response.charset = null;
 
       if (!this._inputStream) {
         this._inputStream = Cc["@mozilla.org/scriptableinputstream;1"]
                               .createInstance(Ci.nsIScriptableInputStream);
@@ -510,18 +509,17 @@ RESTRequest.prototype = {
 
       this.response.body += this._inputStream.read(count);
     }
 
     try {
       this.onProgress();
     } catch (ex) {
       this._log.warn("Got exception calling onProgress handler, aborting " +
-                     this.method + " " + channel.URI.spec);
-      this._log.debug("Exception: " + CommonUtils.exceptionStr(ex));
+                     this.method + " " + channel.URI.spec, ex);
       this.abort();
 
       if (!this.onComplete) {
         this._log.error("Unexpected error: onComplete not defined in " +
                         "onDataAvailable.");
         this.onProgress = null;
         return;
       }
@@ -578,17 +576,17 @@ RESTRequest.prototype = {
     try {
       if (this.shouldCopyOnRedirect(oldChannel, newChannel, flags)) {
         this._log.trace("Copying headers for safe internal redirect.");
         for (let key in this._headers) {
           newChannel.setRequestHeader(key, this._headers[key], false);
         }
       }
     } catch (ex) {
-      this._log.error("Error copying headers: " + CommonUtils.exceptionStr(ex));
+      this._log.error("Error copying headers", ex);
     }
 
     this.channel = newChannel;
 
     // We let all redirects proceed.
     callback.onRedirectVerifyCallback(Cr.NS_OK);
   }
 };
@@ -614,50 +612,47 @@ RESTResponse.prototype = {
   /**
    * HTTP status code
    */
   get status() {
     let status;
     try {
       status = this.request.channel.responseStatus;
     } catch (ex) {
-      this._log.debug("Caught exception fetching HTTP status code:" +
-                      CommonUtils.exceptionStr(ex));
+      this._log.debug("Caught exception fetching HTTP status code", ex);
       return null;
     }
     Object.defineProperty(this, "status", {value: status});
     return status;
   },
 
   /**
    * HTTP status text
    */
   get statusText() {
     let statusText;
     try {
       statusText = this.request.channel.responseStatusText;
     } catch (ex) {
-      this._log.debug("Caught exception fetching HTTP status text:" +
-                      CommonUtils.exceptionStr(ex));
+      this._log.debug("Caught exception fetching HTTP status text", ex);
       return null;
     }
     Object.defineProperty(this, "statusText", {value: statusText});
     return statusText;
   },
 
   /**
    * Boolean flag that indicates whether the HTTP status code is 2xx or not.
    */
   get success() {
     let success;
     try {
       success = this.request.channel.requestSucceeded;
     } catch (ex) {
-      this._log.debug("Caught exception fetching HTTP success flag:" +
-                      CommonUtils.exceptionStr(ex));
+      this._log.debug("Caught exception fetching HTTP success flag", ex);
       return null;
     }
     Object.defineProperty(this, "success", {value: success});
     return success;
   },
 
   /**
    * Object containing HTTP headers (keyed as lower case)
@@ -666,18 +661,17 @@ RESTResponse.prototype = {
     let headers = {};
     try {
       this._log.trace("Processing response headers.");
       let channel = this.request.channel.QueryInterface(Ci.nsIHttpChannel);
       channel.visitResponseHeaders(function (header, value) {
         headers[header.toLowerCase()] = value;
       });
     } catch (ex) {
-      this._log.debug("Caught exception processing response headers:" +
-                      CommonUtils.exceptionStr(ex));
+      this._log.debug("Caught exception processing response headers", ex);
       return null;
     }
 
     Object.defineProperty(this, "headers", {value: headers});
     return headers;
   },
 
   /**
diff --git a/services/common/storageservice.js b/services/common/storageservice.js
--- a/services/common/storageservice.js
+++ b/services/common/storageservice.js
@@ -767,18 +767,17 @@ StorageServiceRequest.prototype = {
       if (!this._handler.onComplete) {
         this._log.warn("No onComplete installed in handler!");
         return;
       }
 
       try {
         this._handler.onComplete(this._error, this);
       } catch (ex) {
-        this._log.warn("Exception when invoking handler's onComplete: " +
-                       CommonUtils.exceptionStr(ex));
+        this._log.warn("Exception when invoking handler's onComplete", ex);
         throw ex;
       }
     }.bind(this);
 
     try {
       if (error) {
         this._error = new StorageServiceRequestError();
         this._error.network = error;
@@ -1328,18 +1327,17 @@ StorageCollectionBatchedSet.prototype = 
 
     if (!this._onFinishCallback) {
       return;
     }
 
     try {
       this._onFinishCallback(this);
     } catch (ex) {
-      this._log.warn("Exception when calling finished callback: " +
-                     CommonUtils.exceptionStr(ex));
+      this._log.warn("Exception when calling finished callback", ex);
     }
   },
 };
 Object.freeze(StorageCollectionBatchedSet.prototype);
 
 /**
  * Manages a batch of BSO deletion requests.
  *
@@ -1474,18 +1472,17 @@ StorageCollectionBatchedDelete.prototype
 
     if (!this._finishedCallback) {
       return;
     }
 
     try {
       this._finishedCallback(this);
     } catch (ex) {
-      this._log.warn("Exception when invoking finished callback: " +
-                     CommonUtils.exceptionStr(ex));
+      this._log.warn("Exception when invoking finished callback", ex);
     }
   },
 };
 Object.freeze(StorageCollectionBatchedDelete.prototype);
 
 /**
  * Construct a new client for the SyncStorage API, version 2.0.
  *
diff --git a/services/common/tests/unit/head_helpers.js b/services/common/tests/unit/head_helpers.js
--- a/services/common/tests/unit/head_helpers.js
+++ b/services/common/tests/unit/head_helpers.js
@@ -71,17 +71,17 @@ function httpd_setup (handlers, port=-1)
   for (let path in handlers) {
     server.registerPathHandler(path, handlers[path]);
   }
   try {
     server.start(port);
   } catch (ex) {
     _("==========================================");
     _("Got exception starting HTTP server on port " + port);
-    _("Error: " + CommonUtils.exceptionStr(ex));
+    _("Error: " + Log.exceptionStr(ex));
     _("Is there a process already listening on port " + port + "?");
     _("==========================================");
     do_throw(ex);
   }
 
   // Set the base URI for convenience.
   let i = server.identity;
   server.baseURI = i.primaryScheme + "://" + i.primaryHost + ":" + i.primaryPort;
diff --git a/services/common/tests/unit/test_utils_exceptionStr.js b/services/common/tests/unit/test_utils_exceptionStr.js
--- a/services/common/tests/unit/test_utils_exceptionStr.js
+++ b/services/common/tests/unit/test_utils_exceptionStr.js
@@ -1,13 +1,17 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
+const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
+
+Cu.import("resource://gre/modules/Log.jsm");
+
 function run_test() {
   run_next_test();
 }
 
 add_test(function test_exceptionStr_non_exceptions() {
   do_check_eq(CommonUtils.exceptionStr(null), "null");
   do_check_eq(CommonUtils.exceptionStr(false), "false");
   do_check_eq(CommonUtils.exceptionStr(undefined), "undefined");
diff --git a/services/common/tests/unit/test_utils_stackTrace.js b/services/common/tests/unit/test_utils_stackTrace.js
--- a/services/common/tests/unit/test_utils_stackTrace.js
+++ b/services/common/tests/unit/test_utils_stackTrace.js
@@ -2,26 +2,26 @@
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 _("Define some functions in well defined line positions for the test");
 function foo(v) bar(v + 1); // line 2
 function bar(v) baz(v + 1); // line 3
 function baz(v) { throw new Error(v + 1); } // line 4
 
 _("Make sure lazy constructor calling/assignment works");
-Cu.import("resource://services-common/utils.js");
+Cu.import("resource://gre/modules/Log.jsm");
 
 function run_test() {
   _("Make sure functions, arguments, files are pretty printed in the trace");
   let trace = "";
   try {
     foo(0);
   }
   catch(ex) {
-    trace = CommonUtils.stackTrace(ex);
+    trace = Log.stackTrace(ex);
   }
   _("Got trace:", trace);
   do_check_neq(trace, "");
 
   let bazPos = trace.indexOf("baz@test_utils_stackTrace.js:7");
   let barPos = trace.indexOf("bar@test_utils_stackTrace.js:6");
   let fooPos = trace.indexOf("foo@test_utils_stackTrace.js:5");
   _("String positions:", bazPos, barPos, fooPos);
diff --git a/services/common/tokenserverclient.js b/services/common/tokenserverclient.js
--- a/services/common/tokenserverclient.js
+++ b/services/common/tokenserverclient.js
@@ -11,17 +11,16 @@ this.EXPORTED_SYMBOLS = [
   "TokenServerClientServerError",
 ];
 
 const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
 
 Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://gre/modules/Log.jsm");
 Cu.import("resource://services-common/rest.js");
-Cu.import("resource://services-common/utils.js");
 Cu.import("resource://services-common/observers.js");
 
 const Prefs = new Preferences("services.common.tokenserverclient.");
 
 /**
  * Represents a TokenServerClient error that occurred on the client.
  *
  * This is the base type for all errors raised by client operations.
@@ -266,28 +265,26 @@ TokenServerClient.prototype = {
         if (!cb) {
           self._log.warn("Callback already called! Did it throw?");
           return;
         }
 
         try {
           cb(error, result);
         } catch (ex) {
-          self._log.warn("Exception when calling user-supplied callback: " +
-                         CommonUtils.exceptionStr(ex));
+          self._log.warn("Exception when calling user-supplied callback", ex);
         }
 
         cb = null;
       }
 
       try {
         client._processTokenResponse(this.response, callCallback);
       } catch (ex) {
-        this._log.warn("Error processing token server response: " +
-                       CommonUtils.exceptionStr(ex));
+        this._log.warn("Error processing token server response", ex);
 
         let error = new TokenServerClientError(ex);
         error.response = this.response;
         callCallback(error, null);
       }
     });
   },

diff --git a/services/common/utils.js b/services/common/utils.js
--- a/services/common/utils.js
+++ b/services/common/utils.js
@@ -97,17 +97,17 @@ this.CommonUtils = {
    */
   makeURI: function makeURI(URIString) {
     if (!URIString)
       return null;
     try {
       return Services.io.newURI(URIString, null, null);
     } catch (e) {
       let log = Log.repository.getLogger("Common.Utils");
-      log.debug("Could not create URI: " + CommonUtils.exceptionStr(e));
+      log.debug("Could not create URI", e);
       return null;
     }
   },
 
   /**
    * Execute a function on the next event loop tick.
    *
    * @param callback

diff --git a/services/common/tests/unit/test_utils_exceptionStr.js b/services/common/tests/unit/test_utils_exceptionStr.js
--- a/services/common/tests/unit/test_utils_exceptionStr.js
+++ b/services/common/tests/unit/test_utils_exceptionStr.js
@@ -7,15 +7,15 @@ const {classes: Cc, interfaces: Ci, util
 
 Cu.import("resource://gre/modules/Log.jsm");
 
 function run_test() {
   run_next_test();
 }
 
 add_test(function test_exceptionStr_non_exceptions() {
-  do_check_eq(CommonUtils.exceptionStr(null), "null");
-  do_check_eq(CommonUtils.exceptionStr(false), "false");
-  do_check_eq(CommonUtils.exceptionStr(undefined), "undefined");
-  do_check_eq(CommonUtils.exceptionStr(12), "12 No traceback available");
+  do_check_eq(Log.exceptionStr(null), "null");
+  do_check_eq(Log.exceptionStr(false), "false");
+  do_check_eq(Log.exceptionStr(undefined), "undefined");
+  do_check_eq(Log.exceptionStr(12), "12 No traceback available");
 
   run_next_test();
 });

Comment 67

[Georg Fritzsche [:gfritzsche]]

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #66)
> Comment on attachment 8506017 [details] [diff] [review]
> 1003204_common_part1.patch

I think something went wrong here, did you mean to attach an updated patch?

Comment 68

[Sudheesh Singanamalla (:ShellHacker)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #67)
> (In reply to Sudheesh Singanamalla (:ShellHacker) from comment #66)
> > Comment on attachment 8506017 [details] [diff] [review]
> > 1003204_common_part1.patch
> 
> I think something went wrong here, did you mean to attach an updated patch?

Oops yeah.

Comment 69

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_common_part1.patch

Carrying over the review+ given by irving and rnewman.

Added the content that I missed out to the patch

Comment 70

[Georg Fritzsche [:gfritzsche]]

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #65)
> (In reply to :Irving Reid from comment #43)
> 
> > > +        self._log.debug("Stat failure" + Log.exceptionStr(e));
> > 
> > this should be self._log.debug("Stat failure", e);
> > 
> > While the existing code didn't do this, I would love to have this error
> > message include the file name.
> 
> (In reply to Georg Fritzsche [:gfritzsche] from comment #61)
> > >        function onStatFailure(e) {
> > >          // Never mind.
> > > +        self._log.debug("Healthreport/profile.jsm, Stat failure", e);
> > 
> > Why that change? This doesn't match any other logging statement in that file.
> 
> When Irving mentioned that he wants the error to include the file name, I
> thought that it should be put as Healthreport/profile.jsm, <error msg>
> 
> Could you mention what change I exactly need to be making ?

Ah, thanks for the context.
I think we don't actually need this here - the logger instance there is inherited from the provider, which sets it up to include the name here:
http://hg.mozilla.org/mozilla-central/annotate/5999e92e89ff/services/metrics/dataprovider.jsm#l505

We set up the name here:
http://hg.mozilla.org/mozilla-central/annotate/5999e92e89ff/services/healthreport/profile.jsm#l231
... so i think we have enough context in the logger output.

Comment 71

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8516181 [details] [diff] [review]
1003204_common_part1.patch

Review of attachment 8516181 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/common/rest.js
@@ +338,5 @@
>      try {
>        channel.asyncOpen(this, null);
>      } catch (ex) {
>        // asyncOpen can throw in a bunch of cases -- e.g., a forbidden port.
> +      this._log.warn("Caught an error in asyncOpen" , ex);

Nit: unneeded whitespace before the comma.

Comment 72

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_common_part1.patch

Addresses the Nit. Fixes the space.

Comment 73

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_healthreport.patch

Removes the file name put in the logger message. Obsoletes older patch. Hopefully should be fine now.

Comment 74

[Sudheesh Singanamalla (:ShellHacker)]

Comment on attachment 8506019 [details] [diff] [review]
1003204_common_part2.patch

This should be landed after landing part1

Comment 75

[:Irving Reid (No longer working on Firefox)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #70)
> (In reply to Sudheesh Singanamalla (:ShellHacker) from comment #65)
> > (In reply to :Irving Reid from comment #43)
> > 
> > > > +        self._log.debug("Stat failure" + Log.exceptionStr(e));
> > > 
> > > this should be self._log.debug("Stat failure", e);
> > > 
> > > While the existing code didn't do this, I would love to have this error
> > > message include the file name.
> > 
> > (In reply to Georg Fritzsche [:gfritzsche] from comment #61)
> > > >        function onStatFailure(e) {
> > > >          // Never mind.
> > > > +        self._log.debug("Healthreport/profile.jsm, Stat failure", e);
> > > 
> > > Why that change? This doesn't match any other logging statement in that file.
> > 
> > When Irving mentioned that he wants the error to include the file name, I
> > thought that it should be put as Healthreport/profile.jsm, <error msg>
> > 
> > Could you mention what change I exactly need to be making ?
> 
> Ah, thanks for the context.
> I think we don't actually need this here - the logger instance there is
> inherited from the provider, which sets it up to include the name here:
> http://hg.mozilla.org/mozilla-central/annotate/5999e92e89ff/services/metrics/
> dataprovider.jsm#l505

I didn't mean the name of the file containing the log message, I meant the name of the file the 'stat' call is executed on. There are few things I find more frustrating than trying to debug a "file access failed" error message that doesn't tell me which file the program was trying to access.

Comment 76

[Georg Fritzsche [:gfritzsche]]

(In reply to :Irving Reid from comment #75)
> I didn't mean the name of the file containing the log message, I meant the
> name of the file the 'stat' call is executed on. There are few things I find
> more frustrating than trying to debug a "file access failed" error message
> that doesn't tell me which file the program was trying to access.

Ah, thanks for clearing this up, that makes sense. Lets fix this.

Comment 77

[Georg Fritzsche [:gfritzsche]]

(Just moving files, so taking the liberty for direct r+ on that patch)

Comment 78

[Sudheesh Singanamalla (:ShellHacker)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #76)
> (In reply to :Irving Reid from comment #75)
> > I didn't mean the name of the file containing the log message, I meant the
> > name of the file the 'stat' call is executed on. There are few things I find
> > more frustrating than trying to debug a "file access failed" error message
> > that doesn't tell me which file the program was trying to access.
> 
> Ah, thanks for clearing this up, that makes sense. Lets fix this.

OS.File.stat(entry.path) is returned, should the message include the concatenation with OS.File.name ? OS.File returns the file creation time.

Comment 79

[Georg Fritzsche [:gfritzsche]]

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #78)
> OS.File.stat(entry.path) is returned, should the message include the
> concatenation with OS.File.name ? OS.File returns the file creation time.

Just adding |OS.Path.basename(entry.path)| to the message would be fine with me.

Comment 80

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_healthreport.patch

Updated to show file name which fails. Thanks for this opportunity.

Comment 81

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8516212 [details] [diff] [review]
1003204_healthreport.patch

Review of attachment 8516212 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/healthreport/profile.jsm
@@ +155,5 @@
>        }
>  
>        function onStatFailure(e) {
>          // Never mind.
> +        self._log.debug("Stat failure" + OS.Path.basename(entry.path), e);

For a file "somefile.txt", this would result in something like "Stat failuresomefile.txt: ...".

I think we want something like "Stat failure for " here.

Comment 82

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: 1003204_healthreport.patch

Nice catch. Fixed it now.

Comment 83

[Georg Fritzsche [:gfritzsche]]

Alright, while we wait on the sr=rnewman, lets push this to try and see if this breaks any tests.
Sudheesh, are you able to push to try?
If not, can you let me know what order the patches go in so i can push them?

Comment 84

[Sudheesh Singanamalla (:ShellHacker)]

I think the right order for these patches is

1003204_common_part1.patch
1003204_datareporting.patch
1003204_healthreport.patch
1003204_metrics.patch
1003204_sync.patch
1003204_toolkit.patch
1003204_common_part2.patch

Comment 85

[Georg Fritzsche [:gfritzsche]]

That patch order doesn't apply cleanly on a current mozilla-central.
Please rebase and update the patches.

Comment 86

[Richard Newman [:rnewman]]

Comment on attachment 8512715 [details] [diff] [review]
1003204_sync.patch

Review of attachment 8512715 [details] [diff] [review]:
-----------------------------------------------------------------

Rubberstamp.

Comment 87

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #85)
> That patch order doesn't apply cleanly on a current mozilla-central.
> Please rebase and update the patches.

Comment 88

[Sudheesh Singanamalla (:ShellHacker)]

I am behind a proxy and unable to pull the changes because of the university rules. Is there a workaround ?

Sorry for the delay

Comment 89

[Georg Fritzsche [:gfritzsche]]

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #88)
> I am behind a proxy and unable to pull the changes because of the university
> rules. Is there a workaround ?

You could use a bundle from here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial/Bundles
But note that they can be a few days out of date, which may be problematic with the broad changes here.

Alternatively, if github access works you could try that: https://github.com/mozilla/gecko-dev

I wonder if you could argue with the university over the rules when it's blocking access to learning and development resources :)

Comment 90

[Sudheesh Singanamalla (:ShellHacker)]

Georg,

GitHub works fine. I'll look through the gecko-dev and see if I can do anything through that. Can I use git for the patches then ?

> I wonder if you could argue with the university over the rules when it's
> blocking access to learning and development resources :)

Already did. They said they'd allow it in a few days. We set up a local mirror of the source code bundles so that they can be distributed faster within the campus. ;)

I'll let you know once I am done with getting it set up.

Comment 91

[Georg Fritzsche [:gfritzsche]]

(In reply to Sudheesh Singanamalla (:ShellHacker) [Exams Nov 18 - Dec 5] from comment #90)
> GitHub works fine. I'll look through the gecko-dev and see if I can do
> anything through that. Can I use git for the patches then ?

Sure, you should use whatever works for you. The patches should come out the same.

Comment 92

[Georg Fritzsche [:gfritzsche]]

Note that i'm away on vacation after this week, so we can try to find someone else to look at it or finish this when i get back.

Comment 93

[Georg Fritzsche [:gfritzsche]]

Did you have any success here?
I imagine with the long break the patches might require some updating?

Comment 94

[Sudheesh Singanamalla (:ShellHacker)]

I am still working on it, I took a long break till December 5th because of my end semester exams and project submissions. Things look better now, I was back on it since yesterday.

Comment 95

[Georg Fritzsche [:gfritzsche]]

This looks inactive for a while now, clearing assignee.

Comment 96

[chaithanya]

Hi
  I would like  to work on this bug. Could you please assign it to me ?

Comment 97

[chaithanya]

Hi
  I would like  to work on this bug. Could you please assign it to me ?

Comment 98

[Georg Fritzsche [:gfritzsche]]

Hi Chaithanya, this bug is pretty big in scope and the existing patches here are probably rather outdated.
I recommend that we talk about a different bug to pick.

Comment 99

[chaithanya]

(In reply to Georg Fritzsche [:gfritzsche] from comment #98)
> Hi Chaithanya, this bug is pretty big in scope and the existing patches here
> are probably rather outdated.
> I recommend that we talk about a different bug to pick.

Cool,Okay.

Comment 100

[Anup Kumar [:reznord]]

Attachment: made the changes required

Hi Georg, 

I have made the changes according to what is requested in the comment 0 (the bug description). But I'm not sure whether the patch is complete or not, just giving it for a review. Please tell me if I'm going in the right direction or not.

Should I make changes to the services/sync/ files also in the same patch or should I make it as a separate patch, because in comment 16, Irvind Reid suggested to make it a separate patch.

Comment 101

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8691798 [details] [diff] [review]
made the changes required

Review of attachment 8691798 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking great, thanks alot!

There are a couple of problems I see, but they are easy to fix! What we are looking for, basically, is:in relation to Log.jsm meth

* Whenever exceptionStr() is called in relation to Log.jsm methods, it should always be passed as the 2nd arg. Most places you've done exactly that, but in some places it has been passed as the first arg - in that case we just pass it as the 2nd arg of the line above, or add a simple first arg.

* There are a couple of places where you pass it as the 2nd arg to Cu.reportError() or other non-Log.jsm methods - this isn't going to - you'll need to import Log.jsm for the method instead of utils.js

* I think you forgot to remote the test file itself.

(Note that I didn't get to the very end of the patch - so you'll need to find similar issues after my last comment)


But this is looking great. The next step is to upload a new patch, and if you are keen, google how you can request "try access" to push a test run - or I can do that for you. Let me know if you need more help or information.

::: services/common/rest.js
@@ +516,5 @@
>          }
>        } catch (ex) {
>          this._log.warn("Exception thrown reading " + count + " bytes from " +
>                         "the channel.");
> +        this._log.warn(ex);

The ex should be passed on the line above and this line removed.

@@ +537,5 @@
>        this.onProgress();
>      } catch (ex) {
>        this._log.warn("Got exception calling onProgress handler, aborting " +
>                       this.method + " " + channel.URI.spec);
> +      this._log.debug("Exception", ex);

ditto here

::: services/common/tests/unit/xpcshell.ini
@@ -18,5 @@
>  [test_utils_deepCopy.js]
>  [test_utils_encodeBase32.js]
>  [test_utils_encodeBase64URL.js]
>  [test_utils_ensureMillisecondsTimestamp.js]
> -[test_utils_exceptionStr.js]

I don't see this file being removed in the patch

::: services/datareporting/DataReportingService.js
@@ +118,5 @@
>            this.policy = new DataReportingPolicy(policyPrefs, this._prefs, this);
>  
>            this._os.addObserver(this, "sessionstore-windows-restored", true);
>          } catch (ex) {
> +          Cu.reportError("Exception when initializing data reporting service", ex);

Cu.reportError doesn't take the extra arg - only Log.jsm methods do. You need to import Log.jsm here and use its exceptionStr() method instead. Ditto other places in this patch.

::: services/healthreport/healthreporter.jsm
@@ +724,5 @@
>      let recordMessage = message;
>      let logMessage = message;
>  
>      if (ex) {
> +      recordMessage += , ex;

ditto here

::: services/metrics/providermanager.jsm
@@ +539,5 @@
>     */
>    _recordProviderError: function (name, msg, ex) {
>      msg = "Provider error: " + name + ": " + msg;
>      if (ex) {
> +      msg += , ex;

and here

::: services/sync/modules/engines.js
@@ +307,5 @@
>          // ex.cause will carry its stack with it when rethrown.
>          throw ex.cause;
>        } catch (ex if !Async.isShutdownException(ex)) {
>          this._log.warn("Failed to apply incoming record " + record.id);
> +        this._log.warn("Encountered exception: " + Log.exceptionStr(ex));

same here: just pass as 2nd param of the above line and kill this one

@@ +572,5 @@
>        } else {
>          this._engines[name] = engine;
>        }
>      } catch (ex) {
> +      this._log.error(ex);

here we should write: this._log.error("Failed to register engine", ex); or similar.

@@ +811,5 @@
>    get previousFailed() {
>      return this._previousFailed;
>    },
>    set previousFailed(val) {
> +    let cb = (error) => this._log.error(Log.exceptionStr(error));

same as above - this._log.error("set previousFailed error", error);

@@ +994,5 @@
>          failed = failed.concat(this._store.applyIncomingBatch(applyBatch));
>        } catch (ex if !Async.isShutdownException(ex)) {
>          // Catch any error that escapes from applyIncomingBatch. At present
>          // those will all be abort events.
> +        this._log.warn("Got exception " + Log.exceptionStr(ex) +

.warn("Got exception, aborting ...", ex)

@@ +1063,5 @@
>              case SyncEngine.kRecoveryStrategy.retry:
>                self._log.debug("Ignoring second retry suggestion.");
>                // Fall through to error case.
>              case SyncEngine.kRecoveryStrategy.error:
> +              self._log.warn("Error decrypting record: " + Log.exceptionStr(ex));

just pas ex as the 2nd param like you've done elsewhere - here and the rest of this file

::: services/sync/modules/resource.js
@@ +283,1 @@
>                       " in _onComplete.");

same here: just pass as 2nd param of the above line and kill this one

@@ +318,5 @@
>                         " doesn't match the header's content-length of: " +
>                         contentLength + ".");
>        }
>      } catch (ex) {
> +      this._log.debug("Caught exception", ex +" visiting headers in _onComplete.");

adjust the string and arrange to pas ex as the 2nd param

@@ +559,5 @@
>        this._onProgress();
>      } catch (ex if !Async.isShutdownException(ex)) {
>        this._log.warn("Got exception calling onProgress handler during fetch of "
>                       + req.URI.spec);
> +      this._log.debug(ex);

same here: just pass as 2nd param of the above line and kill this one

Comment 102

[Anup Kumar [:reznord]]

(In reply to Mark Hammond [:markh] from comment #101)
> Comment on attachment 8691798 [details] [diff] [review]
> made the changes required
> 
> Review of attachment 8691798 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking great, thanks alot!
> 
> There are a couple of problems I see, but they are easy to fix! What we are
> looking for, basically, is:in relation to Log.jsm meth
> 
> * Whenever exceptionStr() is called in relation to Log.jsm methods, it
> should always be passed as the 2nd arg. Most places you've done exactly
> that, but in some places it has been passed as the first arg - in that case
> we just pass it as the 2nd arg of the line above, or add a simple first arg.
> 
> * There are a couple of places where you pass it as the 2nd arg to
> Cu.reportError() or other non-Log.jsm methods - this isn't going to - you'll
> need to import Log.jsm for the method instead of utils.js
> 
> * I think you forgot to remote the test file itself.
> 
> (Note that I didn't get to the very end of the patch - so you'll need to
> find similar issues after my last comment)
> 
> 
> But this is looking great. The next step is to upload a new patch, and if
> you are keen, google how you can request "try access" to push a test run -
> or I can do that for you. Let me know if you need more help or information.
> 
> ::: services/common/rest.js
> @@ +516,5 @@
> >          }
> >        } catch (ex) {
> >          this._log.warn("Exception thrown reading " + count + " bytes from " +
> >                         "the channel.");
> > +        this._log.warn(ex);
> 
> The ex should be passed on the line above and this line removed.
> 
> @@ +537,5 @@
> >        this.onProgress();
> >      } catch (ex) {
> >        this._log.warn("Got exception calling onProgress handler, aborting " +
> >                       this.method + " " + channel.URI.spec);
> > +      this._log.debug("Exception", ex);
> 
> ditto here
> 
> ::: services/common/tests/unit/xpcshell.ini
> @@ -18,5 @@
> >  [test_utils_deepCopy.js]
> >  [test_utils_encodeBase32.js]
> >  [test_utils_encodeBase64URL.js]
> >  [test_utils_ensureMillisecondsTimestamp.js]
> > -[test_utils_exceptionStr.js]
> 
> I don't see this file being removed in the patch
> 
> ::: services/datareporting/DataReportingService.js
> @@ +118,5 @@
> >            this.policy = new DataReportingPolicy(policyPrefs, this._prefs, this);
> >  
> >            this._os.addObserver(this, "sessionstore-windows-restored", true);
> >          } catch (ex) {
> > +          Cu.reportError("Exception when initializing data reporting service", ex);
> 
> Cu.reportError doesn't take the extra arg - only Log.jsm methods do. You
> need to import Log.jsm here and use its exceptionStr() method instead. Ditto
> other places in this patch.
> 
> ::: services/healthreport/healthreporter.jsm
> @@ +724,5 @@
> >      let recordMessage = message;
> >      let logMessage = message;
> >  
> >      if (ex) {
> > +      recordMessage += , ex;
> 
> ditto here
> 
> ::: services/metrics/providermanager.jsm
> @@ +539,5 @@
> >     */
> >    _recordProviderError: function (name, msg, ex) {
> >      msg = "Provider error: " + name + ": " + msg;
> >      if (ex) {
> > +      msg += , ex;
> 
> and here
> 
> ::: services/sync/modules/engines.js
> @@ +307,5 @@
> >          // ex.cause will carry its stack with it when rethrown.
> >          throw ex.cause;
> >        } catch (ex if !Async.isShutdownException(ex)) {
> >          this._log.warn("Failed to apply incoming record " + record.id);
> > +        this._log.warn("Encountered exception: " + Log.exceptionStr(ex));
> 
> same here: just pass as 2nd param of the above line and kill this one
> 
> @@ +572,5 @@
> >        } else {
> >          this._engines[name] = engine;
> >        }
> >      } catch (ex) {
> > +      this._log.error(ex);
> 
> here we should write: this._log.error("Failed to register engine", ex); or
> similar.
> 
> @@ +811,5 @@
> >    get previousFailed() {
> >      return this._previousFailed;
> >    },
> >    set previousFailed(val) {
> > +    let cb = (error) => this._log.error(Log.exceptionStr(error));
> 
> same as above - this._log.error("set previousFailed error", error);
> 
> @@ +994,5 @@
> >          failed = failed.concat(this._store.applyIncomingBatch(applyBatch));
> >        } catch (ex if !Async.isShutdownException(ex)) {
> >          // Catch any error that escapes from applyIncomingBatch. At present
> >          // those will all be abort events.
> > +        this._log.warn("Got exception " + Log.exceptionStr(ex) +
> 
> .warn("Got exception, aborting ...", ex)
> 
> @@ +1063,5 @@
> >              case SyncEngine.kRecoveryStrategy.retry:
> >                self._log.debug("Ignoring second retry suggestion.");
> >                // Fall through to error case.
> >              case SyncEngine.kRecoveryStrategy.error:
> > +              self._log.warn("Error decrypting record: " + Log.exceptionStr(ex));
> 
> just pas ex as the 2nd param like you've done elsewhere - here and the rest
> of this file
> 
> ::: services/sync/modules/resource.js
> @@ +283,1 @@
> >                       " in _onComplete.");
> 
> same here: just pass as 2nd param of the above line and kill this one
> 
> @@ +318,5 @@
> >                         " doesn't match the header's content-length of: " +
> >                         contentLength + ".");
> >        }
> >      } catch (ex) {
> > +      this._log.debug("Caught exception", ex +" visiting headers in _onComplete.");
> 
> adjust the string and arrange to pas ex as the 2nd param
> 
> @@ +559,5 @@
> >        this._onProgress();
> >      } catch (ex if !Async.isShutdownException(ex)) {
> >        this._log.warn("Got exception calling onProgress handler during fetch of "
> >                       + req.URI.spec);
> > +      this._log.debug(ex);
> 
> same here: just pass as 2nd param of the above line and kill this one

Thanks for the feedback markh, I will update the patch by tonight and will let you know. 

I have the try server access, will put a pull request after I get the updated patch reviewed. 

How can I contact you in IRC, have a few doubts to clarify.

Comment 103

[Georg Fritzsche [:gfritzsche]]

(In reply to Anup Kumar from comment #100)
> Should I make changes to the services/sync/ files also in the same patch or
> should I make it as a separate patch, because in comment 16, Irvind Reid
> suggested to make it a separate patch.

I think it's easier to review and manage this as a separate patch.
However, if it's easier for you to keep things in one patch, that's fine from my side.

Comment 104

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/common

Removed the CommonUtils.exceptionStr() calls in services/common and made it in a separate patch.

Comment 105

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in rest of services/

Removed the CommonUtils.exceptionStr() calls in rest of services/ and created a separate patch

Comment 106

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/sync

Removed the CommonUtils.exceptionStr() calls in services/sync and made it a separate patch. 

Gave review request to gfritzsche and markh on this patch.

Comment 107

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in toolkit/

Removed the CommonUtils.exceptionStr() calls in toolkit/ and made it as a spearate patch.

Comment 108

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/common

Fixed the Log.exceptionStir() errors, as markh mentioned in IRC.

Comment 109

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in rest of services/

Fixed the Cu.reportError() calls as markh requested in IRC.

Comment 110

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/sync

Fixed the problems in passwords.js as requested by markh

Comment 111

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in toolkit/

Fixed the Cu.reportError in CrashManager.jsm as requested by markh.

Comment 112

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8693019 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/common

Review of attachment 8693019 [details] [diff] [review]:
-----------------------------------------------------------------

I just did a quick drive-by of this and many of the files use things from CommonUtils other than exceptionStr(), so the imports must remain. Can you please also check the other files in all your patches - every file that you removed the import from, search that file for "CommonUtils" - if any occurrences remain you will need to leave the import in place.

::: services/common/bagheeraclient.js
@@ -25,5 @@
>  Cu.import("resource://gre/modules/Promise.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/Log.jsm");
>  Cu.import("resource://services-common/rest.js");
> -Cu.import("resource://services-common/utils.js");

this file still references CommonUtils for other methods, so the import must remain.

::: services/common/hawkrequest.js
@@ -15,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Log.jsm");
>  Cu.import("resource://services-common/rest.js");
> -Cu.import("resource://services-common/utils.js");

This file uses other symbols from CommonUtils so the import must remain.

::: services/common/modules-testing/bagheeraserver.js
@@ -8,5 @@
>  
>  this.EXPORTED_SYMBOLS = ["BagheeraServer"];
>  
>  Cu.import("resource://gre/modules/Log.jsm");
> -Cu.import("resource://services-common/utils.js");

This file uses other symbols from CommonUtils so the import must remain.

::: services/common/modules-testing/storageserver.js
@@ -20,5 @@
>  
>  Cu.import("resource://testing-common/httpd.js");
>  Cu.import("resource://services-common/async.js");
>  Cu.import("resource://gre/modules/Log.jsm");
> -Cu.import("resource://services-common/utils.js");

This file uses other symbols from CommonUtils so the import must remain.

::: services/common/rest.js
@@ -17,5 @@
>  Cu.import("resource://gre/modules/Preferences.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Log.jsm");
> -Cu.import("resource://services-common/utils.js");

This file uses other symbols from CommonUtils so the import must remain.

::: services/common/tests/unit/head_helpers.js
@@ -2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  Cu.import("resource://gre/modules/Log.jsm");
> -Cu.import("resource://services-common/utils.js");

This file uses other symbols from CommonUtils so the import must remain.

Comment 113

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/common

Fixed the problems in services/common as requested in comment 112.

Comment 114

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in rest of services/

Fixed the problems in rest of services/ as requested in comment 112

Comment 115

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/sync

Fixed the problems in services/sync as requested in comment 112

Comment 116

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in toolkit/

Fixed the problems in toolkit/ as requested in comment 112

Comment 117

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8693409 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/common

Review of attachment 8693409 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, although it doesn't actually remove CommonUtils.exceptionStr

::: services/common/hawkrequest.js
@@ +10,5 @@
>    "HAWKAuthenticatedRESTRequest",
>    "deriveHawkCredentials"
>  ];
>  
> +Cu.import("resource://gre/modules/Credentials.jsm");

I think for the sake of sanity we should keep the modules in the same order they were.

::: services/common/modules-testing/storageserver.js
@@ +18,5 @@
>    "storageServerForUsers",
>  ];
>  
> +Cu.import("resource://services-common/async.js");
> +Cu.import("resource://services-common/utils.js");

ditto here

::: services/common/utils.js
@@ +97,2 @@
>     */
>    makeURI: function makeURI(URIString) {

exceptionStr() should be removed from this file.

Comment 118

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8693410 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in rest of services/

Review of attachment 8693410 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/datareporting/DataReportingService.js
@@ -10,2 @@
>  Cu.import("resource://gre/modules/Preferences.jsm");
> -Cu.import("resource://gre/modules/XPCOMUtils.jsm");

let's keep the imports in the same order.

@@ +119,5 @@
>            this.policy = new DataReportingPolicy(policyPrefs, this._prefs, this);
>  
>            this._os.addObserver(this, "sessionstore-windows-restored", true);
>          } catch (ex) {
> +          Cu.reportError("Exception when initializing data reporting service: " 

trailing whitespace on this line, and remove the trailing ": " on the literal string - the log module adds them.

@@ +212,5 @@
>      try {
>        this._loadHealthReporter();
>      } catch (ex) {
>        this._healthReporter = null;
> +      Cu.reportError("Exception when obtaining health reporter: "

remove trailing ": "

::: services/datareporting/policy.jsm
@@ +23,5 @@
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/Promise.jsm");
>  Cu.import("resource://gre/modules/Log.jsm");
> +Cu.import("resource://gre/modules/UpdateUtils.jsm");

keep module imports in the same order.

::: services/healthreport/healthreporter.jsm
@@ +25,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/Task.jsm");
>  Cu.import("resource://gre/modules/TelemetryStopwatch.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://services-common/utils.js");

keep the import order the same.

::: services/healthreport/providers.jsm
@@ +35,5 @@
>  Cu.import("resource://gre/modules/Metrics.jsm");
>  
>  #endif
>  
> +Cu.import("resource://gre/modules/osfile.jsm");

keep import order the same

::: services/metrics/providermanager.jsm
@@ +11,5 @@
>  
>  Cu.import("resource://gre/modules/services/metrics/dataprovider.jsm");
>  #endif
>  
> +Cu.import("resource://gre/modules/Log.jsm");

keep imports in the same order

@@ +539,5 @@
>     * Record an error that occurred operating on a provider.
>     */
>    _recordProviderError: function (name, msg, ex) {
>      msg = "Provider error: " + name + ": " + msg;
>      if (ex) {

I think we can kill this block and change the .warn call to |this._log.warn(msg, ex);| - it will change the message a little when ex is null, but I think that's ok.

::: services/metrics/storage.jsm
@@ +19,5 @@
>  
>  #endif
>  
> +Cu.import("resource://gre/modules/AsyncShutdown.jsm");
> +Cu.import("resource://gre/modules/Log.jsm");

let's keep the import order the same.

Comment 119

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8693412 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/sync

Review of attachment 8693412 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/modules/engines.js
@@ +12,5 @@
>  
>  var {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
>  
>  Cu.import("resource://services-common/async.js");
> +Cu.import("resource://services-common/observers.js");

keep the import order the same

::: services/sync/modules/resource.js
@@ +11,5 @@
>  var Ci = Components.interfaces;
>  var Cr = Components.results;
>  var Cu = Components.utils;
>  
> +Cu.import("resource://gre/modules/Log.jsm");

keep the imports in the same order.

::: services/sync/modules/util.js
@@ +11,5 @@
>  Cu.import("resource://services-common/stringbundle.js");
>  Cu.import("resource://services-common/utils.js");
>  Cu.import("resource://services-common/async.js", this);
>  Cu.import("resource://services-crypto/utils.js");
> +Cu.import("resource://services-common/utils.js");

This import is now duplicated. exceptionStr() should also be removed from this file.

::: services/sync/tests/unit/test_addon_utils.js
@@ +7,3 @@
>  Cu.import("resource://gre/modules/Preferences.jsm");
>  Cu.import("resource://services-sync/addonutils.js");
>  Cu.import("resource://services-sync/util.js");

it looks like this import can be removed (there are no references to |Utils.| remaining)

::: services/sync/tests/unit/test_bookmark_engine.js
@@ +1,5 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +Cu.import("resource://gre/modules/BookmarkJSONUtils.jsm");
> +Cu.import("resource://gre/modules/Log.jsm");

keep the import order the same.

::: services/sync/tests/unit/test_fxa_node_reassignment.js
@@ +8,5 @@
>  // Fxa cluster manager grabs a new token.
>  
>  Cu.import("resource://gre/modules/Log.jsm");
>  Cu.import("resource://services-common/rest.js");
> +Cu.import("resource://services-sync/browserid_identity.js");

keep import order the same.

Comment 120

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8693420 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in toolkit/

Review of attachment 8693420 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/crashes/CrashManager.jsm
@@ +277,5 @@
>              // converted to return codes.
>              //
>              // If we get here, report the error and delete the source file
>              // so we don't see it again.
> +            Cu.reportError("Exception when processing crash event file: " + 

trailing whitespace on this line.

Comment 121

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/common

Modified the patch according to the comment 117.

Comment 122

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in rest of services/

Modified the patch according to comment 118

Comment 123

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/sync

Modified the patch according to the comment 119

Comment 124

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in toolkit/

Modifies the patch according to comment 120

Comment 125

[Georg Fritzsche [:gfritzsche]]

High-level note:
We are missing a patch here that actually removes CommonUtils.exceptionStr():
https://dxr.mozilla.org/mozilla-central/rev/a44dde7aaa7d5b80b13dc946ccbea0801f772ef7/services/common/utils.js#73

... and its test:
https://dxr.mozilla.org/mozilla-central/source/services/common/tests/unit/test_utils_exceptionStr.js
https://dxr.mozilla.org/mozilla-central/rev/a44dde7aaa7d5b80b13dc946ccbea0801f772ef7/services/common/tests/unit/xpcshell.ini#22

Comment 126

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8693519 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/sync

Review of attachment 8693519 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good and i see you caught all the occurences in services/sync.
Given a few minor typos, i think you should run the sync tests after updating the patch, this should catch those.

After addressing these changes this should be ready for final review.

::: services/sync/modules/addonutils.js
@@ +163,5 @@
>          install.addListener(listener);
>          install.install();
>        }
>        catch (ex) {
> +        this._log.error("Error installing add-on", ex);

Heh, good find - this had a typo before your change and would never have logged a proper exception string.

::: services/sync/modules/engines.js
@@ +572,5 @@
>        } else {
>          this._engines[name] = engine;
>        }
>      } catch (ex) {
> +      this._log.error("Failed to create engine", ex);

This seems redundant with the error() a few lines down.

@@ +580,5 @@
>        name = name.prototype || "";
>        name = name.name || "";
>  
>        let out = "Could not initialize engine '" + name + "': " + mesg;
>        this._log.error(out);

Let's remove the error statement above and change this to:
this._log.error(out, ex);

::: services/sync/modules/engines/passwords.js
@@ +264,5 @@
>  
>      try {
>        Services.logins.modifyLogin(loginItem, newinfo);
>      } catch(ex) {
> +      this._log.debug(Modifying record ${record.id} resulted in exception; not modifying`, ex);

This is missing the starting "`" before "Modifying".

::: services/sync/modules/resource.js
@@ +279,5 @@
>  
>      } catch(ex) {
>        // Got a response, but an exception occurred during processing.
>        // This shouldn't occur.
> +      this._log.warn("Caught unexpected exception in _onComplete", ex");

Stray " after ex.

::: services/sync/modules/service.js
@@ +1694,5 @@
>      let url = this.userBaseURL + info_type;
>      return this.getStorageRequest(url).get(function onComplete(error) {
>        // Note: 'this' is the request.
>        if (error) {
> +        this._log.debug("Failed to retrieve '" + info_type, error);

With your change, the last "'" is missing - you want:
... info_type + "'", error);

::: services/sync/tests/unit/test_addon_utils.js
@@ +7,3 @@
>  Cu.import("resource://gre/modules/Preferences.jsm");
>  Cu.import("resource://services-sync/addonutils.js");
>  Cu.import("resource://services-sync/util.js");

You can remove this import now.

::: services/sync/tests/unit/test_fxa_node_reassignment.js
@@ +95,5 @@
>      return Services.prefs.getBoolPref("services.sync.lastSyncReassigned");
>    } catch (ex if (ex.result == Cr.NS_ERROR_UNEXPECTED)) {
>      return false;
>    } catch (ex) {
> +    do_throw("Got exception retrieving lastSyncReassigned", ex);

This should be:
... lastSyncReassigned: " + Log.exceptionStr(ex));

Comment 127

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8693521 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in toolkit/

Review of attachment 8693521 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thanks.

Comment 128

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8693518 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in rest of services/

Review of attachment 8693518 [details] [diff] [review]:
-----------------------------------------------------------------

This is close, with the issues below fixed this can be flagged for review.

::: services/datareporting/DataReportingService.js
@@ +13,2 @@
>  Cu.import("resource://gre/modules/osfile.jsm");
> +Cu.import("resource://gre/modules/Log.jsm");

Log.jsm was intentionally imported lazily below.
Let's make this import lazy to keep that behavior:
XPCOMUtils.defineLazyModuleGetter(this, "Log",
                                  "resource://gre/modules/Log.jsm");

@@ +119,5 @@
>  
>            this._os.addObserver(this, "sessionstore-windows-restored", true);
>          } catch (ex) {
> +          Cu.reportError("Exception when initializing data reporting service",
> +                         Log.exceptionStr(ex));

Keep the "+" and ":" here.

@@ +212,5 @@
>        this._loadHealthReporter();
>      } catch (ex) {
>        this._healthReporter = null;
> +      Cu.reportError("Exception when obtaining health reporter",
> +                     Log.exceptionStr(ex));

Ditto, keep the "+" and ":" here.

Comment 129

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8693517 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/common

Review of attachment 8693517 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, i see this patch removes the CommonUtils.exceptionStr() test, but not its implementation.
As per comment 125, i would prefer to have the removal of test_utils_exceptionStr.js, the xpcshell.ini change in a separate patch which also removes the implementation.

Comment 130

[Georg Fritzsche [:gfritzsche]]

Adding Alessio as a mentor here to cover during my PTO.

Comment 131

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/common

Removed all the corresponding entries of exceptionStr() in services/common/

Comment 132

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/common

Removed all the corresponding entries of exceptionStr() in services/common/

Comment 133

[Anup Kumar [:reznord]]

Attachment: Removed test_utils_exceptionStr.js and its entry in xpcshell.ini

Created a separate patch for the removal of test_utils_exceptionStr.js and the xpcshell.ini as requested in comment 129

Comment 134

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/

Made the changes as requested in comment 128

Comment 135

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/sync

Made the changes as requested in comment 126

Comment 136

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8699058 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/common

Review of attachment 8699058 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome!

::: services/common/utils.js
@@ +68,5 @@
>      }
>      return true;
>    },
>  
>    // Import these from Log.jsm for backward compatibility

"these" should now read "this" :) If you are keen, feel free to open a new bug like this, but to remove CommonUtils.stackTrace() as a followup.

Comment 137

[Anup Kumar [:reznord]]

(In reply to Mark Hammond [:markh] from comment #136)
> Comment on attachment 8699058 [details] [diff] [review]
> Removed the CommonUtils.exceptionStr() calls in services/common
> 
> Review of attachment 8699058 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome!
> 
> ::: services/common/utils.js
> @@ +68,5 @@
> >      }
> >      return true;
> >    },
> >  
> >    // Import these from Log.jsm for backward compatibility
> 
> "these" should now read "this" :) If you are keen, feel free to open a new
> bug like this, but to remove CommonUtils.stackTrace() as a followup.

Okay, will open a new bug and will make the changes for that bug after this gets merged into the tree.

Comment 138

[Anup Kumar [:reznord]]

needinfo'ing Georg on this bug for a try server request

Comment 139

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8699060 [details] [diff] [review]
Removed test_utils_exceptionStr.js and its entry in xpcshell.ini

Mark, i'm running short on time today.
Can you review these or redirect them to someone suitable?
Thanks!

Comment 140

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in toolkit/

Removing the old r+'ed patch and updated with the new one because there are a few code changes in the tree.

Comment 141

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8699061 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/

Review of attachment 8699061 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good - just needs 1 trivial fix.

::: services/metrics/providermanager.jsm
@@ +276,5 @@
>                          "to finish before registering.");
>          try {
>            yield inFlightPromise;
>          } catch (ex) {
> +          this._log.warn("Error when waiting for existing pull-only promise", exs);

typo here - should be |ex|

Comment 142

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8699064 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/sync

Review of attachment 8699064 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but those 2 lines must be removed (and the other minor fix to the other 2 lines might as well be done too!)

::: services/sync/modules/engines.js
@@ +307,5 @@
>          // ex.cause will carry its stack with it when rethrown.
>          throw ex.cause;
>        } catch (ex if !Async.isShutdownException(ex)) {
>          this._log.warn("Failed to apply incoming record " + record.id);
> +        this._log.warn("Encountered exception", ex);

I think we should just remove this line and pass |ex| as the 2nd param on the line above. I'm not giong to r- for this as your code will still work fine - but it's still an improvement you should please make.

@@ +1084,5 @@
>          failed.push(item.id);
>          aborting = ex.cause;
>        } catch (ex if !Async.isShutdownException(ex)) {
>          self._log.warn("Failed to reconcile incoming record " + item.id);
> +        self._log.warn("Encountered exception", ex);

ditto here - let's kill this line and pass ex as the 2nd param above.

::: services/sync/modules/resource.js
@@ +284,1 @@
>        this._log.debug(CommonUtils.stackTrace(ex));

this line needs to be deleted.

@@ +323,1 @@
>        this._log.debug(CommonUtils.stackTrace(ex));

and this

Comment 143

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8699951 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in toolkit/

Review of attachment 8699951 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome - nice work :) I'm guessing you will get your ssh keys sorted in the next few days, so after you make the requested changes, please push a full try run and upload the changed patches, then we can get this pushed!

Comment 144

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/sync

Made the changes as requested in comment 142

Comment 145

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/

Fixed the typo mistake.

Comment 146

[Anup Kumar [:reznord]]

(In reply to Mark Hammond [:markh] from comment #143)
> Comment on attachment 8699951 [details] [diff] [review]
> Removed the CommonUtils.exceptionStr() calls in toolkit/
> 
> Review of attachment 8699951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome - nice work :) I'm guessing you will get your ssh keys sorted in the
> next few days, so after you make the requested changes, please push a full
> try run and upload the changed patches, then we can get this pushed!

Thanks :-) I will pull a try server request once I get the ssh keys sorted. Can you tell me what is the syntax that I should give for the try request.

Comment 147

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8700269 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/sync

Review of attachment 8700269 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome \o/

Comment 148

[Mark Hammond [:markh] [:mhammond]]

(In reply to Anup Kumar [:Anupkumar] from comment #146)
> Thanks :-) I will pull a try server request once I get the ssh keys sorted.
> Can you tell me what is the syntax that I should give for the try request.

I think "-b do -p linux64,linux64_gecko -u all -t none" will be ok.

Comment 149

[Anup Kumar [:reznord]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f5e9d9bf34f

Comment 150

[Anup Kumar [:reznord]]

(In reply to Anup Kumar [:Anupkumar] from comment #149)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f5e9d9bf34f

oops, looks like our patch is not correct, too many tests got busted. :(

Comment 151

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8700270 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/

Review of attachment 8700270 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/metrics/providermanager.jsm
@@ -541,5 @@
>  
>    /**
>     * Record an error that occurred operating on a provider.
>     */
> -  _recordProviderError: function (name, msg, ex) {

oops - I missed this :( We've removed the function definition.

Comment 152

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/

Updated the patch

Comment 153

[Anup Kumar [:reznord]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b718dda59ec

Comment 154

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/sync

Added the import which resulted in the error during the tryserver run.

Comment 155

[Anup Kumar [:reznord]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d545b8c57e93

Comment 156

[Mark Hammond [:markh] [:mhammond]]

services/sync/tests/unit/test_utils_catch.js is failing for reasons that aren't obvious to me :(

Comment 157

[Anup Kumar [:reznord]]

Yeah, even that test failed for me also, when I tried locally. Will try to debug the errors from the test results. Will be a bit busy till 2nd Jan, mostly will do it after then.

Comment 158

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/sync

Fixed the error with the test locally.

Comment 159

[Anup Kumar [:reznord]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54878d613d61

Comment 160

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/

Fixed the problem in the tryserver.

Comment 161

[Anup Kumar [:reznord]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ccfc19c40fe

Comment 162

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8704137 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/

Review of attachment 8704137 [details] [diff] [review]:
-----------------------------------------------------------------

This is nearly done, only one smaller change needed.

::: services/metrics/providermanager.jsm
@@ +546,2 @@
>      }
> +    this._log.warn(`Provider error: ${name}: ${msg}`, ex);

With the above "msg = ..." assignment, this should just stay as |this._log.warn(msg)|.

Comment 163

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8704137 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/

Review of attachment 8704137 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/metrics/providermanager.jsm
@@ +548,1 @@
>  

Nice catch Georg, thanks, I missed that! In addition, as written we are going to get 2 exception strings. Anupkumar asked me very early about this code over IRC, and I recall suggesting he add that line (547), but remove the 4 lines above it, which I think is the correct change required here (ie, that 543-546 are removed, 547 is unchanged)

Comment 164

[Anup Kumar [:reznord]]

Attachment: remo

Fixed the test errors from the previous failed tests in the tryserver

Comment 165

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/

Comment 166

[Anup Kumar [:reznord]]

sorry for the problems, the above two patches are wrong, will update the new patches.

Comment 167

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/

Re-attaching the patch here with fixing the errors from the previous test failures in the last try run.

Comment 168

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/sync

Fixed the errors for the failed tests from the previous try server results.

Comment 169

[Anup Kumar [:reznord]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bfdbf07b956

Comment 170

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8704640 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/

Review of attachment 8704640 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/metrics/providermanager.jsm
@@ +539,5 @@
>    /**
>     * Record an error that occurred operating on a provider.
>     */
>    _recordProviderError: function (name, msg, ex) {
> +    this._log.warn(`Provider error: ${name}: ${msg}`, ex);

The failing test seems to be relying on the fact that the exceptionStr() result is passed to onProviderError(). So sorry for the bad advice - I think you should change this block back to how Georg suggested (ie, leave this block unchanged from the original code, but swapping CommonUtils.exceptionStr() with Log.exceptionStr()) - this will ensure the message passed to the callback includes the exception string and the test should pass.

Sorry about that.

Comment 171

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/

Fixed the error in the test and made the changes as Georg suggested.

Comment 172

[Anup Kumar [:reznord]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5814c9453b02

Comment 173

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8704920 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/

Review of attachment 8704920 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks :)

Comment 174

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8704641 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/sync

Review of attachment 8704641 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me with the one issue below fixed, thanks!

::: services/sync/modules/service.js
@@ +488,5 @@
>          }
>  
>          this.engineManager.register(ns[engineName]);
>        } catch (ex) {
> +        this._log.warn("Could not register engine" + name, ex);

Don't drop the space after engine, i.e.:
this._log.warn("Could not register engine " + name, ex);

::: services/sync/tests/unit/test_service_sync_locked.js
@@ +13,5 @@
>      let i = old.info;
> +    // For the purposes of this test we don't need to do full formatting
> +    // of the 2nd param, as the ones we care about are always strings.
> +    old.debug = function(m, p) { debug.push(p ? m + ": " + p : m); d.call(old, m, p); }
> +    old.info  = function(m, p) { info.push(p ? m + ": " + p : m);  i.call(old, m, p); }

This looks odd to me without knowing more, but apparently markh knows that we need this test fix here.
So, fine with me.

Comment 175

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/sync

Added the extra space as requested in the above comment.

Comment 176

[Alessio Placitelli [:Dexter]]

Anupkumar, your services_sync.patch doesn't apply cleanly to the latest m-c:

applying services_common.patch
applying test_removed.patch
applying toolkit.patch
patching file toolkit/modules/SessionRecorder.jsm
Hunk #1 succeeded at 12 with fuzz 2 (offset -4 lines).
applying services.patch
applying services_sync.patch
patching file services/sync/modules/addonutils.js
Hunk #1 FAILED at 158
1 out of 1 hunks FAILED -- saving rejects to file services/sync/modules/addonutils.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh services_sync.patch

Comment 177

[Anup Kumar [:reznord]]

Attachment: Removed the CommonUtils.exceptionStr() calls in services/sync

patch is applicable to the latest m-c.

Comment 178

[Alessio Placitelli [:Dexter]]

Comment on attachment 8705154 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/sync

This patch was already reviewed by :gfritzsche and was only rebased. Nothing changed, there's no need to request a new review.

Comment 179

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/46b5a8591cf54dfac433a3153058237a413dfe33
Bug 1003204: Removed CommonUtils.exceptionStr() in services/common/ r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/e8acf773c3d6ae9e5ad8d587d2ce7c65f9e1e645
Bug 1003204: Removed test_utils_exceptionStr.js and its entry in xpcshell.ini r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/ec660a46136a6df47ebbcf9274ac35738c550331
Bug 1003204: Removed CommonUtils.exceptionStr() in toolkit/ r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/9433893449d376f76d22faed6cdffe20ee3ab4ac
Bug 1003204: Removed CommonUtils.exceptionStr() in services/ r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/d818de93b48f531eb13789bed8ca9917771e1c9a
Bug 1003204: Removed CommonUtils.exceptionStr() in services/sync r=markh r=gfritzsche

Comment 180

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/46b5a8591cf5
https://hg.mozilla.org/mozilla-central/rev/e8acf773c3d6
https://hg.mozilla.org/mozilla-central/rev/ec660a46136a
https://hg.mozilla.org/mozilla-central/rev/9433893449d3
https://hg.mozilla.org/mozilla-central/rev/d818de93b48f