Preferences.jsm should throw error objects instead of strings

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

Preference.jsm throwing strings means that the test and logging infrastructure can't get stacks out, which can be a PITA.

Jeff, sorry to pick on you, but hg log tells me you made the most recent significant changes to this module, so I'm flagging you for review. Let me know if that's a problem and I'll find another reviewer.
Not a *problem* per se -- nothing about the changes looks wrong to me.

But I'm not a reviewer in this component, from a process point of view.  (And I'm too lazy to check whether you are, and would be implicitly delegating review authority to me.  :-) )

And arguably more importantly, I don't regularly work with JSMs.  I'm sure this change would result in behavior where users couldn't do |instanceof Error| tests on thrown exceptions, because the errors would be from this JSM's global.  I don't *think* there's an alternative that doesn't have that effect, yet provides the stack info comment 0 wants.  But I'm not certain.

I'd prefer if someone confident in an answer to that last concern stamped this, but if someone knowledgeable says there's no other way, I can offer whatever authority an r+ from me should convey.
Comment on attachment 8814298 [details]
Bug 1320249 - Preferences.jsm now throws error objects instead of strings.

Thanks Jeff.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> And arguably more importantly, I don't regularly work with JSMs.  I'm sure
> this change would result in behavior where users couldn't do |instanceof
> Error| tests on thrown exceptions, because the errors would be from this
> JSM's global.  I don't *think* there's an alternative that doesn't have that
> effect, yet provides the stack info comment 0 wants.  But I'm not certain.

I'm fairly confident that almost no-one attempts to catch these errors as they are truly unexpected and tend to imply a logic error while developing a new patch. For example, if you try and save a millisecond timestamp you will get one of those errors (which means you forgot to, eg, convert it to seconds or a string before writing it as a pref), and one of the other errors means you probably just passed the wrong param in (eg, trying to write an object to prefs). When making these mistakes, a stack is useful, but we never expect to see these in "real" logs.

But your points are certainly worth raising. I don't care too much about this patch and would be happy to drop it, but I made the patch while diagnosing one such mistake and figured I'd get it into a bug.

Gijs, what do you think about this patch? Should we use it or lose it? :)
Attachment #8814298 - Flags: review?(jwalden+bmo) → review?(gijskruitbosch+bugs)
Comment on attachment 8814298 [details]
Bug 1320249 - Preferences.jsm now throws error objects instead of strings.

https://reviewboard.mozilla.org/r/95546/#review95910

r=me, though for bonus points please switch these error messages to template strings.
Attachment #8814298 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Mark Hammond [:markh] from comment #3)
> Comment on attachment 8814298 [details]
> Bug 1320249 - Preferences.jsm now throws error objects instead of strings.
> 
> Thanks Jeff.
> 
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> > And arguably more importantly, I don't regularly work with JSMs.  I'm sure
> > this change would result in behavior where users couldn't do |instanceof
> > Error| tests on thrown exceptions, because the errors would be from this
> > JSM's global.  I don't *think* there's an alternative that doesn't have that
> > effect, yet provides the stack info comment 0 wants.  But I'm not certain.
> 
> I'm fairly confident that almost no-one attempts to catch these errors as
> they are truly unexpected and tend to imply a logic error while developing a
> new patch. For example, if you try and save a millisecond timestamp you will
> get one of those errors (which means you forgot to, eg, convert it to
> seconds or a string before writing it as a pref), and one of the other
> errors means you probably just passed the wrong param in (eg, trying to
> write an object to prefs). When making these mistakes, a stack is useful,
> but we never expect to see these in "real" logs.

I agree with this. I don't think any of the Prefs consumers actually do Error checks.

More generally, I think testing with instanceof is usually not the best way to deal with broken error provision by something (it implies there are other types of things it can throw) and certainly not until we fix instanceof (which might never happen because specs and web breakage, I guess).

I don't think this list:

 https://dxr.mozilla.org/mozilla-central/search?q=%27instanceof%20Error&=mozilla-central

is checking anything returned by Preferences.jsm, but I expect that at least some of these checks are already broken for the "this is not the global you're in" reason. :-(
Assignee: nobody → markh
Status: NEW → ASSIGNED
(In reply to :Gijs Kruitbosch from comment #4)
> r=me, though for bonus points please switch these error messages to template
> strings.

I did consider doing that originally, but I couldn't decide how to sanely do that, so I took the cowards option and left it alone. I really wish template strings had a good multi-line string option - without that I'm left with something like:

  `here's the first part of a string with a ${value} and ` +
  `here's the second part`

which I find suboptimal as the second string is a template string without any values. Using regular quotes for just that second line seems ugly. I've also seem some code do, eg, 

  `here's the first part of a string with a ${
   value} and here's the second part`

which avoids string concatenation, but (a) seems a little too tricky and (b) doesn't always work as there's often not a value in the right place for this trick to work.

I'm putting up a new patch that does as the first example above (ie, concatenation with template strings that don't actually include a value) and given I strike this same basic problem regularly, I'd be interested to know if this is what you had in mind, or prefer some other style?
Attachment #8814298 - Flags: review+ → review?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a86c91c947de
Preferences.jsm now throws error objects instead of strings. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/a86c91c947de
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Attachment #8814298 - Flags: review?(gijskruitbosch+bugs) → review+
(Somehow the mozreview-to-bugzilla integration didn't propagate the flag when I reviewed in mozreview, so I've done that manually.)
You need to log in before you can comment on or make changes to this bug.