Remove CommonUtils.exceptionStr()

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Irving, Assigned: reznord, Mentored)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(5 attachments, 57 obsolete attachments)

17.12 KB, patch
markh
: review+
Details | Diff | Splinter Review
1.57 KB, patch
markh
: review+
Details | Diff | Splinter Review
12.22 KB, patch
markh
: review+
Details | Diff | Splinter Review
31.79 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
43.51 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
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
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.
Assignee: nobody → svoboj71

Updated

5 years ago
Assignee: svoboj71 → nobody

Comment 2

5 years ago
Hi. I am really sorry, but finally I have decided to not working on this bug. It's free again.
Hi. I'm just getting started contributing to Firefox, and I would like to take this bug. Can you assign it to me, please?
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.
Assignee: nobody → lonnelars
Status: NEW → ASSIGNED
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.
Mentor: irving
Whiteboard: [mentor=irving][lang=js] → [lang=js]

Comment 6

5 years ago
Hi I want to work on this bug. Please help me.
Lars, are you making any progress on this bug? If not, there's another contributor interested in working on it.

ug201310004@iitj.ac.in, 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
Flags: needinfo?(lonnelars)

Comment 8

5 years ago
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.
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.
Assignee: lonnelars → ug201310004
Flags: needinfo?(lonnelars)

Updated

5 years ago
Assignee: ug201310004 → nobody
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.
Assignee: nobody → sudheesh1995
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.
Posted patch Patch_1003204_toolkit.patch (obsolete) — Splinter Review
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 ?
Posted patch Patch_1003204_services.patch (obsolete) — Splinter Review
This is the patch for the services/ folder of the codebase.
Attachment #8487438 - Flags: review?(irving)
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.
(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.
Flags: needinfo?(irving)
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);
Attachment #8487438 - Flags: review?(irving) → review-
(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.
Flags: needinfo?(irving)
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 ?
Attachment #8487323 - Attachment is obsolete: true
Attachment #8499452 - Flags: review?(irving)
Removes the older services patch.

Attaches

1. Patch for services/common
Attachment #8487438 - Attachment is obsolete: true
Attachment #8499482 - Flags: review?(irving)
2. Patch for services/datareporting
Attachment #8499484 - Flags: review?(irving)
Posted patch Patch_1003204_toolkit.patch (obsolete) — Splinter Review
Sorry for the previous attachment. Commit message had the wrong bug number. My bad.

3. toolkit/
Attachment #8499452 - Attachment is obsolete: true
Attachment #8499452 - Flags: review?(irving)
Attachment #8499485 - Flags: review?(irving)
4. services/healthreport
Attachment #8499489 - Flags: review?(irving)
5. services/metrics
Attachment #8499490 - Flags: review?(irving)
6. services/sync
Attachment #8499515 - Flags: review?(irving)
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.
Attachment #8499482 - Flags: review?(irving) → review-
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)
Attachment #8499484 - Flags: review?(irving) → review-
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).
Attachment #8499485 - Flags: review?(irving) → review-
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.
Attachment #8499489 - Flags: review?(irving) → review-
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 ': '
Attachment #8499490 - Flags: review?(irving) → review-
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 '!'
Attachment #8499515 - Flags: review?(irving) → review-
Posted patch 1003204_datareporting.patch (obsolete) — Splinter Review
Obsolete older patch for datareporting, renames patch to 1003204_datareporting.patch for making it easier to review.
Attachment #8499484 - Attachment is obsolete: true
Attachment #8504461 - Flags: review?(irving)
Posted patch 1003204_toolkit.patch (obsolete) — Splinter Review
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
Attachment #8499485 - Attachment is obsolete: true
Attachment #8504466 - Flags: review?(irving)
Posted patch 1003204_healthreport.patch (obsolete) — Splinter Review
Obsolete older patch, fixes all the ': ' problem in the logging in healthreport.
Attachment #8499489 - Attachment is obsolete: true
Attachment #8504468 - Flags: review?(irving)
Posted patch 1003204_metrics.patch (obsolete) — Splinter Review
Obsolete older patch for metrics, fixes all the ': ' and removes ": " from log messages which have += ": " + Log....
Attachment #8499490 - Attachment is obsolete: true
Attachment #8504470 - Flags: review?(irving)
Posted patch 1003204_sync.patch (obsolete) — Splinter Review
Obsolete older patch for sync, fixes up ': ' for logging which uses Log.jsm
Attachment #8499515 - Attachment is obsolete: true
Attachment #8504474 - Flags: review?(irving)
Setting a need info for myself to fix the /common later. Need some more time for that
Flags: needinfo?(sudheesh1995)
Posted patch 1003204_common_part1.patch (obsolete) — Splinter Review
Fixes errors and the missing work for utils.js file
Attachment #8499482 - Attachment is obsolete: true
Flags: needinfo?(sudheesh1995)
Attachment #8506017 - Flags: review?(irving)
Posted patch 1003204_common_part2.patch (obsolete) — Splinter Review
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.
Attachment #8506019 - Flags: review?(irving)
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 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.
Attachment #8504461 - Flags: review?(irving) → review-
Attachment #8504466 - Flags: review?(irving) → review+
Posted patch 1003204_datareporting.patch (obsolete) — Splinter Review
(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.
Attachment #8504461 - Attachment is obsolete: true
Attachment #8506593 - Flags: review?(georg.fritzsche)
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.
Attachment #8506593 - Flags: review?(georg.fritzsche) → review+
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.
Attachment #8504468 - Flags: review?(irving) → feedback+
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 ": " +
Attachment #8504470 - Flags: review?(irving) → feedback+
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.
Attachment #8504474 - Flags: superreview?(rnewman)
Attachment #8504474 - Flags: review?(irving)
Attachment #8504474 - Flags: review+
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
Attachment #8506017 - Flags: review?(rnewman)
Attachment #8506017 - Flags: review?(irving)
Attachment #8506017 - Flags: review+
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?
Attachment #8506019 - Flags: review?(irving) → review-
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.
Attachment #8506017 - Flags: review?(rnewman) → review+
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.
Attachment #8504474 - Flags: superreview?(rnewman) → superreview-
(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>
(In reply to :Irving Reid from comment #50)

> The Log.jsm formatter adds punctuation…

:thumbsup:
(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
Posted patch 1003204_healthreport.patch (obsolete) — Splinter Review
(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.
Attachment #8504468 - Attachment is obsolete: true
Attachment #8508505 - Flags: review?(georg.fritzsche)
Posted patch 1003204_metrics.patch (obsolete) — Splinter Review
Fixes the way message strings are built with catenation += ": "
Attachment #8504470 - Attachment is obsolete: true
Attachment #8509297 - Flags: review?(gps)
Attachment #8509297 - Flags: review?(gps) → review+
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.
Attachment #8508505 - Flags: review?(georg.fritzsche) → feedback+
Posted patch 1003204_healthreport.patch (obsolete) — Splinter Review
I corrected those small errors, hope this goes well ?
Attachment #8508505 - Attachment is obsolete: true
Attachment #8512710 - Flags: review?(georg.fritzsche)
Posted patch 1003204_sync.patch (obsolete) — Splinter Review
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.
Attachment #8504474 - Attachment is obsolete: true
Attachment #8512715 - Flags: superreview?(rnewman)
Attachment #8512715 - Flags: review+
Georg, can you finish up mentoring this bug?
Mentor: irving
Flags: needinfo?(georg.fritzsche)
I am really busy this week. I can catch up here next week or someone else should take over.
Mentor: georg.fritzsche
Flags: needinfo?(georg.fritzsche)
(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
Flags: needinfo?(irving)
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.
Attachment #8512710 - Flags: review?(georg.fritzsche) → feedback+
(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.
Flags: needinfo?(irving)
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 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
Attachment #8506019 - Flags: review-
(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 ?
Flags: needinfo?(georg.fritzsche)
Comment hidden (spam)
(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?
Flags: needinfo?(georg.fritzsche)
Attachment #8506017 - Flags: review?(georg.fritzsche)
(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.
Posted patch 1003204_common_part1.patch (obsolete) — Splinter Review
Carrying over the review+ given by irving and rnewman.

Added the content that I missed out to the patch
Attachment #8506017 - Attachment is obsolete: true
Attachment #8516181 - Flags: review+
Attachment #8516181 - Flags: review?(georg.fritzsche)
(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 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.
Attachment #8516181 - Flags: review?(georg.fritzsche) → review+
Posted patch 1003204_common_part1.patch (obsolete) — Splinter Review
Addresses the Nit. Fixes the space.
Attachment #8516181 - Attachment is obsolete: true
Attachment #8516188 - Flags: review?(georg.fritzsche)
Posted patch 1003204_healthreport.patch (obsolete) — Splinter Review
Removes the file name put in the logger message. Obsoletes older patch. Hopefully should be fine now.
Attachment #8512710 - Attachment is obsolete: true
Attachment #8516191 - Flags: review?(georg.fritzsche)
Comment on attachment 8506019 [details] [diff] [review]
1003204_common_part2.patch

This should be landed after landing part1
Attachment #8506019 - Flags: review?(georg.fritzsche)
Attachment #8516188 - Flags: review?(georg.fritzsche) → review+
Attachment #8516191 - Flags: review?(georg.fritzsche) → review+
(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.
(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.
Attachment #8506019 - Flags: review?(georg.fritzsche) → review+
(Just moving files, so taking the liberty for direct r+ on that patch)
(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.
Flags: needinfo?(georg.fritzsche)
(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.
Flags: needinfo?(georg.fritzsche)
Posted patch 1003204_healthreport.patch (obsolete) — Splinter Review
Updated to show file name which fails. Thanks for this opportunity.
Attachment #8516191 - Attachment is obsolete: true
Attachment #8516212 - Flags: review?(georg.fritzsche)
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.
Attachment #8516212 - Flags: review?(georg.fritzsche) → feedback+
Posted patch 1003204_healthreport.patch (obsolete) — Splinter Review
Nice catch. Fixed it now.
Attachment #8516212 - Attachment is obsolete: true
Attachment #8516673 - Flags: review?(georg.fritzsche)
Attachment #8516673 - Flags: review?(georg.fritzsche) → review+
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?
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
That patch order doesn't apply cleanly on a current mozilla-central.
Please rebase and update the patches.
Comment on attachment 8512715 [details] [diff] [review]
1003204_sync.patch

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

Rubberstamp.
Attachment #8512715 - Flags: superreview?(rnewman) → superreview+
(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.
Flags: needinfo?(sudheesh1995)
I am behind a proxy and unable to pull the changes because of the university rules. Is there a workaround ?

Sorry for the delay
Flags: needinfo?(sudheesh1995)
(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 :)
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.
(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.
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.
Did you have any success here?
I imagine with the long break the patches might require some updating?
Flags: needinfo?(sudheesh1995)
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.
Flags: needinfo?(sudheesh1995)
This looks inactive for a while now, clearing assignee.
Assignee: sudheesh1995 → nobody
Status: ASSIGNED → NEW

Comment 96

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

Comment 97

4 years ago
Hi
  I would like  to work on this bug. Could you please assign it to me ?
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

4 years ago
(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.
Assignee

Updated

4 years ago
Assignee: nobody → allamsetty.anup
Assignee

Updated

4 years ago
Status: NEW → ASSIGNED
Assignee

Comment 100

4 years ago
Posted patch made the changes required (obsolete) — Splinter Review
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.
Attachment #8504466 - Attachment is obsolete: true
Attachment #8506019 - Attachment is obsolete: true
Attachment #8506593 - Attachment is obsolete: true
Attachment #8509297 - Attachment is obsolete: true
Attachment #8512715 - Attachment is obsolete: true
Attachment #8516188 - Attachment is obsolete: true
Attachment #8516673 - Attachment is obsolete: true
Attachment #8691798 - Flags: review?(gfritzsche)
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
Attachment #8691798 - Flags: review?(gfritzsche) → feedback+
Assignee

Comment 102

4 years ago
(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.
(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.
Assignee

Comment 104

4 years ago
Removed the CommonUtils.exceptionStr() calls in services/common and made it in a separate patch.
Attachment #8691798 - Attachment is obsolete: true
Attachment #8692759 - Flags: review?(gfritzsche)
Assignee

Comment 105

4 years ago
Removed the CommonUtils.exceptionStr() calls in rest of services/ and created a separate patch
Attachment #8692760 - Flags: review?(gfritzsche)
Assignee

Comment 106

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

Gave review request to gfritzsche and markh on this patch.
Attachment #8692762 - Flags: review?(markh)
Attachment #8692762 - Flags: review?(gfritzsche)
Assignee

Comment 107

4 years ago
Removed the CommonUtils.exceptionStr() calls in toolkit/ and made it as a spearate patch.
Attachment #8692763 - Flags: review?(gfritzsche)
Assignee

Comment 108

4 years ago
Fixed the Log.exceptionStir() errors, as markh mentioned in IRC.
Attachment #8692759 - Attachment is obsolete: true
Attachment #8692760 - Attachment is obsolete: true
Attachment #8692762 - Attachment is obsolete: true
Attachment #8692763 - Attachment is obsolete: true
Attachment #8692759 - Flags: review?(gfritzsche)
Attachment #8692760 - Flags: review?(gfritzsche)
Attachment #8692762 - Flags: review?(markh)
Attachment #8692762 - Flags: review?(gfritzsche)
Attachment #8692763 - Flags: review?(gfritzsche)
Attachment #8693019 - Flags: review?(gfritzsche)
Assignee

Comment 109

4 years ago
Fixed the Cu.reportError() calls as markh requested in IRC.
Attachment #8693020 - Flags: review?(gfritzsche)
Assignee

Comment 110

4 years ago
Fixed the problems in passwords.js as requested by markh
Attachment #8693023 - Flags: review?(markh)
Attachment #8693023 - Flags: review?(gfritzsche)
Assignee

Comment 111

4 years ago
Fixed the Cu.reportError in CrashManager.jsm as requested by markh.
Attachment #8693027 - Flags: review?(gfritzsche)
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.
Assignee

Comment 113

4 years ago
Fixed the problems in services/common as requested in comment 112.
Attachment #8693019 - Attachment is obsolete: true
Attachment #8693020 - Attachment is obsolete: true
Attachment #8693023 - Attachment is obsolete: true
Attachment #8693027 - Attachment is obsolete: true
Attachment #8693019 - Flags: review?(gfritzsche)
Attachment #8693020 - Flags: review?(gfritzsche)
Attachment #8693023 - Flags: review?(markh)
Attachment #8693023 - Flags: review?(gfritzsche)
Attachment #8693027 - Flags: review?(gfritzsche)
Attachment #8693409 - Flags: feedback?(markh)
Assignee

Comment 114

4 years ago
Fixed the problems in rest of services/ as requested in comment 112
Attachment #8693410 - Flags: feedback?(markh)
Assignee

Comment 115

4 years ago
Fixed the problems in services/sync as requested in comment 112
Attachment #8693412 - Flags: feedback?(markh)
Assignee

Comment 116

4 years ago
Fixed the problems in toolkit/ as requested in comment 112
Attachment #8693420 - Flags: feedback?(markh)
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.
Attachment #8693409 - Flags: feedback?(markh) → feedback+
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.
Attachment #8693410 - Flags: feedback?(markh) → feedback+
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.
Attachment #8693412 - Flags: feedback?(markh) → feedback+
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.
Attachment #8693420 - Flags: feedback?(markh) → feedback+
Assignee

Comment 121

4 years ago
Modified the patch according to the comment 117.
Attachment #8693409 - Attachment is obsolete: true
Attachment #8693410 - Attachment is obsolete: true
Attachment #8693412 - Attachment is obsolete: true
Attachment #8693420 - Attachment is obsolete: true
Attachment #8693517 - Flags: feedback?(gfritzsche)
Assignee

Comment 122

4 years ago
Modified the patch according to comment 118
Attachment #8693518 - Flags: feedback?(gfritzsche)
Assignee

Comment 123

4 years ago
Modified the patch according to the comment 119
Attachment #8693519 - Flags: feedback?(gfritzsche)
Assignee

Comment 124

4 years ago
Modifies the patch according to comment 120
Attachment #8693521 - Flags: feedback?(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));
Attachment #8693519 - Flags: feedback?(gfritzsche) → feedback+
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.
Attachment #8693521 - Flags: feedback?(gfritzsche) → review+
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.
Attachment #8693518 - Flags: feedback?(gfritzsche) → feedback+
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.
Attachment #8693517 - Flags: feedback?(gfritzsche) → feedback+
Adding Alessio as a mentor here to cover during my PTO.
Mentor: alessio.placitelli
Assignee

Comment 131

4 years ago
Removed all the corresponding entries of exceptionStr() in services/common/
Attachment #8693517 - Attachment is obsolete: true
Attachment #8693518 - Attachment is obsolete: true
Attachment #8693519 - Attachment is obsolete: true
Attachment #8699050 - Flags: review?(gfritzsche)
Attachment #8699050 - Flags: review?(alessio.placitelli)
Attachment #8699050 - Flags: review?(alessio.placitelli)
Assignee

Comment 132

4 years ago
Removed all the corresponding entries of exceptionStr() in services/common/
Attachment #8699050 - Attachment is obsolete: true
Attachment #8699050 - Flags: review?(gfritzsche)
Attachment #8699058 - Flags: review?(gfritzsche)
Assignee

Comment 133

4 years ago
Created a separate patch for the removal of test_utils_exceptionStr.js and the xpcshell.ini as requested in comment 129
Attachment #8699060 - Flags: review?(gfritzsche)
Assignee

Comment 134

4 years ago
Made the changes as requested in comment 128
Attachment #8699061 - Flags: review?(gfritzsche)
Assignee

Comment 135

4 years ago
Made the changes as requested in comment 126
Attachment #8699064 - Flags: review?(markh)
Attachment #8699064 - Flags: review?(gfritzsche)
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.
Attachment #8699058 - Flags: review?(gfritzsche) → review+
Assignee

Comment 137

4 years ago
(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.
Assignee

Comment 138

4 years ago
needinfo'ing Georg on this bug for a try server request
Flags: needinfo?(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!
Flags: needinfo?(gfritzsche)
Attachment #8699060 - Flags: review?(gfritzsche) → review?(markh)
Attachment #8699061 - Flags: review?(gfritzsche) → review?(markh)
Attachment #8699064 - Flags: review?(gfritzsche)
Assignee

Comment 140

4 years ago
Removing the old r+'ed patch and updated with the new one because there are a few code changes in the tree.
Attachment #8693521 - Attachment is obsolete: true
Attachment #8699951 - Flags: review?(gfritzsche)
Attachment #8699951 - Flags: review?(gfritzsche) → review?(markh)
Attachment #8699060 - Flags: review?(markh) → review+
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|
Attachment #8699061 - Flags: review?(markh) → review-
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
Attachment #8699064 - Flags: review?(markh) → review-
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!
Attachment #8699951 - Flags: review?(markh) → review+
Assignee

Comment 144

4 years ago
Made the changes as requested in comment 142
Attachment #8699064 - Attachment is obsolete: true
Attachment #8700269 - Flags: review?(markh)
Assignee

Comment 145

4 years ago
Fixed the typo mistake.
Attachment #8699061 - Attachment is obsolete: true
Attachment #8700270 - Flags: review?(markh)
Assignee

Comment 146

4 years ago
(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.
Attachment #8700270 - Flags: review?(markh) → review+
Comment on attachment 8700269 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/sync

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

Awesome \o/
Attachment #8700269 - Flags: review?(markh) → review+
(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.
Assignee

Comment 150

4 years ago
(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 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.
Attachment #8700270 - Flags: review+
Assignee

Comment 152

4 years ago
Updated the patch
Attachment #8700270 - Attachment is obsolete: true
Attachment #8700457 - Flags: review?(markh)
Assignee

Comment 154

4 years ago
Added the import which resulted in the error during the tryserver run.
Attachment #8700269 - Attachment is obsolete: true
Attachment #8701296 - Flags: review?(markh)
services/sync/tests/unit/test_utils_catch.js is failing for reasons that aren't obvious to me :(
Flags: needinfo?(allamsetty.anup)
Assignee

Comment 157

3 years ago
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.
Flags: needinfo?(allamsetty.anup)
Assignee

Comment 158

3 years ago
Fixed the error with the test locally.
Attachment #8701296 - Attachment is obsolete: true
Attachment #8701296 - Flags: review?(markh)
Attachment #8703573 - Flags: review?(markh)
Attachment #8703573 - Flags: review?(gfritzsche)
Attachment #8703573 - Flags: review?(alessio.placitelli)
Attachment #8703573 - Flags: review?(alessio.placitelli)
Assignee

Comment 160

3 years ago
Fixed the problem in the tryserver.
Attachment #8700457 - Attachment is obsolete: true
Attachment #8700457 - Flags: review?(markh)
Attachment #8704137 - Flags: review?(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)|.
Attachment #8704137 - Flags: review?(gfritzsche) → feedback+
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)
Assignee

Comment 164

3 years ago
Posted patch remo (obsolete) — Splinter Review
Fixed the test errors from the previous failed tests in the tryserver
Attachment #8704137 - Attachment is obsolete: true
Attachment #8704636 - Flags: review?(gfritzsche)
Assignee

Comment 165

3 years ago
Attachment #8704637 - Flags: review?(gfritzsche)
Assignee

Comment 166

3 years ago
sorry for the problems, the above two patches are wrong, will update the new patches.
Assignee

Comment 167

3 years ago
Re-attaching the patch here with fixing the errors from the previous test failures in the last try run.
Attachment #8704636 - Attachment is obsolete: true
Attachment #8704637 - Attachment is obsolete: true
Attachment #8704636 - Flags: review?(gfritzsche)
Attachment #8704637 - Flags: review?(gfritzsche)
Attachment #8704640 - Flags: review?(gfritzsche)
Assignee

Comment 168

3 years ago
Fixed the errors for the failed tests from the previous try server results.
Attachment #8703573 - Attachment is obsolete: true
Attachment #8703573 - Flags: review?(markh)
Attachment #8703573 - Flags: review?(gfritzsche)
Attachment #8704641 - Flags: review?(gfritzsche)
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.
Assignee

Comment 171

3 years ago
Fixed the error in the test and made the changes as Georg suggested.
Attachment #8704640 - Attachment is obsolete: true
Attachment #8704640 - Flags: review?(gfritzsche)
Attachment #8704920 - Flags: review?(gfritzsche)
Comment on attachment 8704920 [details] [diff] [review]
Removed the CommonUtils.exceptionStr() calls in services/

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

Thanks :)
Attachment #8704920 - Flags: review?(gfritzsche) → review+
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.
Attachment #8704641 - Flags: review?(gfritzsche) → review+
Assignee

Comment 175

3 years ago
Added the extra space as requested in the above comment.
Attachment #8704641 - Attachment is obsolete: true
Attachment #8705129 - Flags: review?(gfritzsche)
Attachment #8705129 - Flags: review?(gfritzsche) → review+
Assignee

Updated

3 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed
Assignee

Comment 177

3 years ago
patch is applicable to the latest m-c.
Attachment #8705129 - Attachment is obsolete: true
Attachment #8705154 - Flags: review?(gfritzsche)
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.
Attachment #8705154 - Flags: review?(gfritzsche) → review+
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
You need to log in before you can comment on or make changes to this bug.