While a fix is available for bug 1486984, backing this out has less chances of regressions.

While I think it's fine to land a backout without a review, I think it's pretty strange to land it and claim (in the commit message) it has review by person X when that person has not, in fact, reviewed it... was there a mistake here? Or a conversation that happened elsewhere that I missed / forgot about?

(In reply to :Gijs (he/him) from comment #3) > While I think it's fine to land a backout without a review, I think it's > pretty strange to land it and claim (in the commit message) it has review by > person X when that person has not, in fact, reviewed it... was there a > mistake here? Or a conversation that happened elsewhere that I missed / > forgot about? Hi. this was landed on mozilla-beta and i only added the approval, i did not change anything else. If that was wrong, let me know. thank you.

(In reply to Andreea Pavel [:apavel] from comment #4) > (In reply to :Gijs (he/him) from comment #3) > > While I think it's fine to land a backout without a review, I think it's > > pretty strange to land it and claim (in the commit message) it has review by > > person X when that person has not, in fact, reviewed it... was there a > > mistake here? Or a conversation that happened elsewhere that I missed / > > forgot about? > > Hi. this was landed on mozilla-beta and i only added the approval, i did not > change anything else. If that was wrong, let me know. thank you. I've just approved the phabricator revision and I'm happy for it to be on beta (ie it doesn't need backing out), I was just surprised to see something land that implied I reviewed something that I hadn't. So, irrespective of other bits of process that I'll mention below, the commit message definitely shouldn't have had r=<someone> without the <someone> actually reviewing the patch. Given you used "a=backout", in that case "r=backout" would have been more appropriate -- that or wait until (or otherwise ensure) <someone> actually reviewed the patch. I don't know how sheriffs determine what is ready to land on beta (I thought it was generally semi-automated via bugzilla queries?) and/or how this revision ended up in that list, as it didn't have a granted beta approval request nor had it been reviewed. So I guess I'm confused how this happend, and what verifications we do prior to landing things on beta. That is, I assume this had "r=Gijs" in the commit message because it was in the commit message that Paolo used when submitting to phabricator, and it didn't get edited out. But I expect Paolo submitted to phabricator with this commit message on the understanding that it wouldn't get landed on beta until I had actually reviewed it. Other people might have used "r?Gijs", in which case the commit hook might have stopped you landing - but then would you have edited to r=Gijs without checking if I'd reviewed the patch? :-) It so happens that I don't think backouts (that don't require conflict resolution, ie any results of just running `hg backout`) should really need review, or (often) the same level of approval scrutiny that other patches do, and so I personally don't have a problem with this patch landing even without my review (which I've retrospectively given anyway). However, I thought that was just my personal opinion, and I thought our process still required review + approval from relman. The first (review) didn't happen here, and it's unclear to me if/when/where the second happened. It's also very possible I misunderstand what our process is for this type of landing. But it feels like maybe some of this could do with some tightening up so it doesn't happen with other non-reviewed patches. Maybe Ryan can clarify some of this.

(In reply to :Gijs (he/him) from comment #5) > But I expect Paolo submitted to phabricator with this commit > message on the understanding that it wouldn't get landed on beta until I had > actually reviewed it. Correct, that was my expectation. I also think using r=backout,a=<release manager> on Beta wouldn't have been a problem in this particular case, if that allowed getting more time on Beta for testing. In the end, two relevant people would have looked at the patch, that is the Release Manager and myself. > It so happens that I don't think backouts (that don't require conflict > resolution, ie any results of just running `hg backout`) should really need > review, or (often) the same level of approval scrutiny that other patches do For new landings on mozilla-central, my opinion and the status quo is that the review requirements should be the same regardless of whether the patch is technically a backout or whether it applies cleanly or not. Think for example of backing out a pref flip. In all these cases, we still have the flexibility of using r=me when appropriate.

I think the backout request came from IRC, redirecting to Pascal who owns the 63 release.

Yes I mentioned the bug to sheriffs and asked for the back out to have it in time for our last beta today.