Closed Bug 1357799 Opened 3 years ago Closed 3 years ago

Enable the diagnostic assert during the beta staged roll out period

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1368079
Tracking Status
firefox53 --- wontfix
firefox54 + wontfix
firefox55 --- affected

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

Details

Attachments

(1 file)

As part of the dawn project, as we are removing aurora, we need to still keep the diagnostic assert enabled for some period of time.

I believe that we can enable it during during the beta staged rollout period (which will be the current aurora population anyway).

In parallel, we will probably sync EARLY_BETA_OR_EARLIER with the beta staged roll out period.
Summary: Enable the diagnostic assert during the early beta population → Enable the diagnostic assert during the beta staged roll out period
Comment on attachment 8859637 [details]
Bug 1357799 - Enable the diagnostic assert during the beta staged roll out period

https://reviewboard.mozilla.org/r/131656/#review134492
Attachment #8859637 - Flags: review?(bobbyholley) → review+
Assignee: nobody → sledru
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/19ea645f562e
Enable the diagnostic assert during the beta staged roll out period r=bholley
https://hg.mozilla.org/mozilla-central/rev/19ea645f562e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Should we uplift this MOZ_DIAGNOSTIC_ASSERT change to Beta 54?

I don't see any reason to wait. MOZ_DIAGNOSTIC_ASSERTs are enabled in Nightly and nothing during Nightly 55 is going to make MOZ_DIAGNOSTIC_ASSERTs any less scary for the wider beta population before Beta 55.
Flags: needinfo?(sledru)
The biggest unknown with shipping this is how much crashier will Beta "appear" with this change. We could try uplifting this to 54 and find out. If this makes Beta 2x as crashy, perhaps this is not the right solution. Do we need to update our stability dashboard (arewestableyet) to disregard crashes owing to these diagnostic asserts similar to the infobar crash submissions?
I like the idea of evaluating this with 54.
Flags: needinfo?(sledru)
Comment on attachment 8859637 [details]
Bug 1357799 - Enable the diagnostic assert during the beta staged roll out period

Approval Request Comment
[Feature/Bug causing the regression]: Dawn project (end of aurora)
[User impact if declined]: Less information about unexpected cases 
[Is this code covered by automated tests?]: I don't think so :/
[Has the fix been verified in Nightly?]: With this change, it will enable this feature until the middle of the beta cycle.
[Needs manual test from QE? If yes, steps to reproduce]: Nope
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: I don't think so
[Why is the change risky/not risky?]: because it will enable a feature which has been on nightly & aurora for a while
[String changes made/needed]: none
Attachment #8859637 - Flags: approval-mozilla-beta?
(In reply to Ritu Kothari (:ritu) from comment #6)
> The biggest unknown with shipping this is how much crashier will Beta
> "appear" with this change.
From the time I've spent looking at Nightly and Aurora crashes, I'd informally say that diagnostic crashes are a very small part of our overall crash volume. The bigger issue might be the performance impact, but I don't think that is big enough to worry about most of the time.
(In reply to Andrew McCreight [:mccr8] from comment #9)
> (In reply to Ritu Kothari (:ritu) from comment #6)
> > The biggest unknown with shipping this is how much crashier will Beta
> > "appear" with this change.
> From the time I've spent looking at Nightly and Aurora crashes, I'd
> informally say that diagnostic crashes are a very small part of our overall
> crash volume. The bigger issue might be the performance impact, but I don't
> think that is big enough to worry about most of the time.

Great! Thanks. That is very reassuring.
Comment on attachment 8859637 [details]
Bug 1357799 - Enable the diagnostic assert during the beta staged roll out period

Get more information during beta stage rollout period. Beta54+. Should be in 54 beta 2.
Attachment #8859637 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
backed this changes out from beta for continued bustage to unbreak the tree
Flags: needinfo?(sledru)
Hi Sylvestre, Ben, if we go down the path of doing two builds off of Beta: regular beta + DevEd builds, we could turn on diagnostic asserts on just DevEd builds. Is this fix still needed on 55+? Or do we want to allow diagnotics asserts in DevEd builds only for early beta population?
Flags: needinfo?(bhearsum)
(In reply to Ritu Kothari (:ritu) from comment #16)
> Hi Sylvestre, Ben, if we go down the path of doing two builds off of Beta:
> regular beta + DevEd builds, we could turn on diagnostic asserts on just
> DevEd builds.

Yes

> Is this fix still needed on 55+? Or do we want to allow
> diagnotics asserts in DevEd builds only for early beta population?

Not my call, but it seems like we'd probably want them on permanently for DevEdition, since that's what we've done when it was on Aurora.
Flags: needinfo?(bhearsum)
Sorry about that tomcat, I will have a look on how to reproduce this locally
Flags: needinfo?(sledru)
Should we back this out on the trunk, too, since it's a bustage train rushing toward beta-55, and unlikely to be what we actually want anyway?
yes, thanks for the ping.

Tomcat, could you take care of the backout? Danke
Flags: needinfo?(cbook)
(In reply to Sylvestre Ledru [:sylvestre] from comment #20)
> yes, thanks for the ping.
> 
> Tomcat, could you take care of the backout? Danke

np :)

done in  https://hg.mozilla.org/integration/mozilla-inbound/rev/c5a9ce06152f
Flags: needinfo?(cbook)
Merged backout: https://hg.mozilla.org/mozilla-central/rev/c5a9ce06152f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla55 → ---
Sylvestre, my understanding is that we need diagnostic assertions enabled for early DevEdition builds from each cycle at least. Are you still working on this?
Flags: needinfo?(sledru)
Ben, I was thinking that this flag could be enabled for the whole devedition cycle.
But if we want it only for a part of the devedition cycle, why not.

I can work on this tomorrow or Thursday.
Flags: needinfo?(sledru)
(In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> Ben, I was thinking that this flag could be enabled for the whole devedition
> cycle.
> But if we want it only for a part of the devedition cycle, why not.
> 
> I can work on this tomorrow or Thursday.

I don't have any preferences as to whole vs. part. We need to use a new flag that we can control from a mozconfig if we want to do it for the entire cycle, though. Totally up to you.
This won't go in 54 at this point.
Comment on attachment 8859637 [details]
Bug 1357799 - Enable the diagnostic assert during the beta staged roll out period

resetting approval flag as this was backed out.
Attachment #8859637 - Flags: approval-mozilla-beta+
FWIW I'm opposed to this change if it means we can no longer use diagnostic asserts where we used to.  I've had at least one person infer that outcome as a result of enforcing diagnostics in our larger beta population.
The work is happening here now: bug 1368079
(In reply to Ben Hearsum (:bhearsum) from comment #25)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> > Ben, I was thinking that this flag could be enabled for the whole devedition
> > cycle.
> > But if we want it only for a part of the devedition cycle, why not.
> > 
> > I can work on this tomorrow or Thursday.
> 
> I don't have any preferences as to whole vs. part. We need to use a new flag
> that we can control from a mozconfig if we want to do it for the entire
> cycle, though. Totally up to you.

I would really like to recommend that we enable it for the whole cycle.  When people pick a channel they are opting in to a certain risk profile.  Either that risk includes diagnostic assertions or it doesn't.  Users are not going to understand the differences if we turn them on/off over time.

Also, if the current plan is just the dev-edition binaries then I hope the concerns from comment 28 are addressed.  Sorry for the noise.
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1368079
You need to log in before you can comment on or make changes to this bug.