1149403 - Better logging and error handling for ReadingList module

Status: RESOLVED WORKSFORME

(Firefox Graveyard :: Reading List, defect)

Description

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

Attachment: 0007-Better-logging-and-reading-list-specific-errors-for-.patch

This patch does the following:

* Removes the setup of appenders for the "root" logger - the scheduler already does this, so doing it here causes many messages to appear twice.

* Adds a few extra logs for when items are added, updated and removed to try and help diagnose the other problems we are seeing.

* Adds 2 errors ReadingListErrors.ItemDeleted() and ReadingListErrors.ItemAlreadyExists() - the idea is that the Sync engine can catch these errors rather than needing to introspect the error objects it sees.

* Adds a ._deleted flag to a readinglist item, only so an attempt to delete an already deleted one throws ReadingListErrors.ItemDeleted() rather than the generic "Item must belong to a list" error.

* Tweaks the readinglist test to expect these new errors. The test also has a change to ensure a deleted item can't be re-fetched later - which is can't, but I figure that test is worthwhile so I left it in.

Blocking bug 1149105 as I think this will make that much saner.

Drew, I'm not particularly happy with how I create the error objects - maybe it should go directly on the ReadingList itself - eg, maybe ReadingList.Errors.ItemDeleted etc?  But putting it up for feedback anyway :)

Comment 1

[Marco Mucci [:MarcoM]]

Hi Mark, can you provide a point value.

Comment 2

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

(In reply to Mark Hammond [:markh] from comment #0)

> Drew, I'm not particularly happy with how I create the error objects - maybe
> it should go directly on the ReadingList itself - eg, maybe
> ReadingList.Errors.ItemDeleted etc?

On reflection, I think it should definitely do that :)  We probably want something like:

ReadingList.Errors = {
  Error: {the base class all others are isinstance of.}
  Exists: ...
  Deleted: ...
}

?

Drew, feel free to take this bug/patch and flip it back to me for review if you think the idea is worthwhile...

Comment 3

[Drew Willcoxon :adw]

Hey Mark, just an update -- I took your patch and started adding ReadingList.Errors and reading up on how to "subclass" Error so that stacks and names show up properly.  But I'm not sure what we're getting by making subclasses.  Is it so that you can use instanceof to check specific error types?  I'm not sure whether we actually need to do that outside of tests, even for Sync.jsm.  Or is it more of a design/philosophical thing?  I might be OK with that.

I'm definitely OK with improving the error messages at least, even if we continue to only use Error objects and not subclasses.  And the other changes in this patch look good to me.

Comment 4

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

(In reply to Drew Willcoxon :adw from comment #3)
> Hey Mark, just an update -- I took your patch and started adding
> ReadingList.Errors and reading up on how to "subclass" Error so that stacks
> and names show up properly.  But I'm not sure what we're getting by making
> subclasses.  Is it so that you can use instanceof to check specific error
> types?  I'm not sure whether we actually need to do that outside of tests,
> even for Sync.jsm.

I don't think we need to check a hierarchy, but do think there is value in being able to know an error was definitively due to an item already being deleted or already existing, mainly for the Sync engine for these races.  Eg, I think there's value that when the sync engine wants to add an item it can expect "already exists" and make a log note, whereas every other error would log.error(), which via bug 1148980, would cause an "error" log-file to be written and probably resulting bug reports.

We should also consider having the new error objects having a property with the original error - I'm sure I've seen this pattern used and good logs being written in this case (ie, the stack trace of both the new and original errors)

Comment 5

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

(In reply to Drew Willcoxon :adw from comment #3)
> Hey Mark, just an update -- I took your patch and started adding
> ReadingList.Errors and reading up on how to "subclass" Error so that stacks
> and names show up properly.

An alternative used by FxAccounts is just an Error object with a pre-defined message global - eg,:

> new Error(SOME_ERROR_CONSTANT);

with some additional metadata tacked on.  This seems less ideal than instanceof errors, but I could certainly live with it if it helps :)
> this._error(ERROR_INVALID_PARAMETER, "Missing or invalid 'scope' option");
  But I'm not sure what we're getting by making
> subclasses.  Is it so that you can use instanceof to check specific error
> types?  I'm not sure whether we actually need to do that outside of tests,
> even for Sync.jsm.  Or is it more of a design/philosophical thing?  I might
> be OK with that.
> 
> I'm definitely OK with improving the error messages at least, even if we
> continue to only use Error objects and not subclasses.  And the other
> changes in this patch look good to me.

Comment 6

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

Being done in bug 1149105