Because using throw Components.Exception("", Cr.ERROR); instead so you get stack info is better.

Code should be using throw Components.Exception("", Cr.ERROR); instead,
since new Error() just converts the int value of the Cr.ERROR into a string,
whereas Exception constructs an error object with the result property set to
the Cr.ERROR value, so other code can identify it.

I won't be around to look at these until Tuesday. I will note that I'm a bit concerned about the --fix ones, we might need to have people from the appropraite areas to review to make sure we're not unintentially breaking things. Not quite sure, will take a better look next week.

That's fine, no rush. And yeah to other feedback. I also thought it might be a good idea to get some feedback from someone more familiar with the intricacies of xpcom and our error codes. I did have a conversation with bz about this before, which is how I realised uses of new Error(Cr.ERROR) (and thus ironically some of the changes in bug 1250254) are bogus.

Ian, sorry for the delays in getting to these.

I've changed the reviewer for the main changes to Dave Townsend - I'm not quite comfortable reviewing those. The ESLint changes themselves look fine.

We suggest that this shouldn't be landed until after mozilla-central has been branched (should be next Monday), and has become 69. Also, it would be good to do a mailing list post (dev.platform & firefox-dev) to let everyone know this has landed and it'd be good for them to update exception messages if appropriate.

Thank you for all your work on these.

(In reply to Mark Banner (:standard8) from comment #7)

We suggest that this shouldn't be landed until after mozilla-central has been branched (should be next Monday), and has become 69. Also, it would be good to do a mailing list post (dev.platform & firefox-dev) to let everyone know this has landed and it'd be good for them to update exception messages if appropriate.

Just to note, mozilla-central is now 69. If you need help let me know.

Ian, what's the status of these patches? Are we good to go now?

Ah, sorry, I meant to post a status update this weekend but forgot, thanks for the reminder!

Unfortunately this is currently blocked due to an issue with JavaScript-implemented xpcom components (I think that's the right terminology...). When such components throw Cr.NS_ERROR the result is preserved, but when they throw Components.Exception("", Cr.NS_ERROR) the result is sanitised along with the rest of the exception info, and replaced with NS_ERROR_UNEXPECTED, which was identified by a test failure that I'd missed before because I'd only done browser-chrome mochitest runs. I discussed this a bit with bz in #content, see my original question to bholley, the related bug 1295322, and the ensuing discussion. I think either the components where the result matters need to be identified and excluded, or the rule needs to be changed to somehow auto-exclude throws inside component implementations (perhaps via checking the source for contractID declarations?). Sadly I'm slightly lacking in free time at the moment so haven't been able to move it further.

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:Kwan, could you have a look please?
For more information, please visit auto_nag documentation.

Clearing out stale needinfos.

Mark do you know if this is still important? Is there a blocking bug we can set?

I think it might be good to do it at some stage. I can't see another bug that would be the blocker for this, but I don't really want to file one at the moment, as I think it might be worth a re-investigation to confirm the current status when we start looking at it again.

Hmm. Carrying error-handling bugs forever sucks. I feel like we've got one really bad bug (see comment 10) blocking another really bad bug (this bug) from being fixed here. Let's try and find a way out of the logjam. I spoke with standard8 about it; some of our ideas:

• try again and see if the problem in comment 10 is still there, if so, file a blocking bug (against XPConnect?)

  I'll happily volunteer to whine beg and cajole for it to get fixed, if we go this route

• land the lint, but don't fix the existing 100+ cases, and explicitly disable the lint for those files

• or, like that but only whitelist cases related to that one test failure, assuming we can figure that out

• maybe also land the rule as not auto-fixable, but give people some short text & a fuller description in docs?

Ian, are you willing & able to try again here?

Hey, sorry for the long absence and delay, and thanks for the ping to check this out again.

So after some more chatting with folks in #dom:mozilla.org (thanks nika and kmag) it seems like we're fine to go ahead and remove raw Cr.ERRORs everywhere (except for TestInterfaceJS, which is explicitly testing raw ones in combination with test_exceptionSanitization.html), since nothing should be relying on the preservation (and the sanitisation only affects unprivileged code anyway?). So I've rebased to tip and done that (I've also re-ordered the patches, to get rid of the annoying dance of adding all those rule exceptions only to remove them again in the next patch).
(Did a try run last week that looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69b33cd9834b623d8e2ff4edc573e8d78a69239e)
I also happened to notice rule tests seemed to have received a s/require("eslint/lib/testers/rule-tester");/require("eslint").RuleTester;/ with the ESLint 6.1 upgrade so did the same.

Mark, is it worth these having a more thorough re-review?

And just for my own records, since I researched it, if the rule did need to be modified to ignore objects that implement QueryInterface, it could be done to catch most instances automatically by walking to the parent object and checking all its properties roughly as follows:

let ancestor = node;
while (ancestor.parent) {
  ancestor = ancestor.parent;
  if (ancestor.type == "ObjectExpression") {
    for (const property of ancestor.properties) {
      if (property.key.name == "QueryInterface") {
        return;
      }
    }
    break;
  }
}

It would miss more complicated/split-up declarations, but would bring the number down to a more manageable level for manual verification and annotation.

(In reply to Ian Moody [:Kwan] (UTC+1) from comment #15)

Mark, is it worth these having a more thorough re-review?

I'd at least like to have a quick look over the diffs again and check the updates. Unfortunately I'm probably not going to get to that until Monday.

I'll leave the NI open to remind me.

Sorry for the delay in looking back at these. I've taken a quick look through the interdiffs and they seem fine.

We're in soft freeze at the moment as the next release branches off to beta on Monday. Therefore I suggest we land these once the soft freeze is over (sometime Monday) - that will also give us a good amount of time on nightly where any regressions can be found/resolved.

Could you make sure the patches still apply on Monday, and then we can land it?

https://hg.mozilla.org/mozilla-central/rev/6936e9640d4e
https://hg.mozilla.org/mozilla-central/rev/dbb0b0a21eef
https://hg.mozilla.org/mozilla-central/rev/d416f4101b4e
https://hg.mozilla.org/mozilla-central/rev/f0db8d736910