revert release assertions from bug 1461326 back to diagnostic assertions

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P2
normal
RESOLVED FIXED
11 months ago
3 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 unaffected, firefox61 fixed, firefox62 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 months ago
It looks like we've solved the beta crash issue that caused us to add some release assertions in bug 1461326.  Lets change these assertions back to diagnostics to avoid adding unnecessary overhead to release builds.  Note, this is not a straight revert because there is some new code that will remain, just restricted to the diagnostic condition.

Updated

11 months ago
Priority: -- → P2
(Assignee)

Comment 3

11 months ago
Andrew, this converts some release assertions we added to try to track down a crash on beta back to diagnostics.  I decided to leave the magic number checking in place for diagnostic builds as well.
Attachment #8982639 - Attachment is obsolete: true
Attachment #8982640 - Flags: review?(bugmail)
(Assignee)

Comment 4

11 months ago
I compiled this locally with both diagnostics enabled and forcibly disabled like they are on beta.
Comment on attachment 8982640 [details] [diff] [review]
Convert service worker and clients release assertions to diagnostic assertions. r=asuth

Review of attachment 8982640 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/clients/manager/ClientManager.cpp
@@ +27,5 @@
>  
>  namespace {
>  
>  const uint32_t kBadThreadLocalIndex = -1;
>  const uint32_t kThreadLocalMagic1 = 0x8d57eea6;

nit: do the kThreadLocalMagicN's want to be ifdef'ed as well?

::: dom/clients/manager/ClientThing.h
@@ +16,5 @@
>  // of code for handling activation and shutdown of IPC actors.
>  template <typename ActorType>
>  class ClientThing
>  {
>    static const uint32_t kMagic1 = 0xC9FE2C9C;

nit: Do these want an ifdef guard as well?
Attachment #8982640 - Flags: review?(bugmail) → review+

Comment 7

11 months ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/209cc0d0c1df
Convert service worker and clients release assertions to diagnostic assertions. r=asuth

Comment 8

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/209cc0d0c1df
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(Assignee)

Comment 9

11 months ago
Comment on attachment 8982648 [details] [diff] [review]
Convert service worker and clients release assertions to diagnostic assertions. r=asuth

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1461326 added some release assertions to diagnose a beta-channel crash.
[User impact if declined]: Leaving the release assertions introduces some minor overhead.  I'd like to revert the assertions back to diagnostics to avoid impacting release channel.
[Is this code covered by automated tests?]: This code is exercised by a lot of tests.
[Has the fix been verified in Nightly?]:  Landed in m-c a few days ago.
[Needs manual test from QE? If yes, steps to reproduce]:  None
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: Minimal
[Why is the change risky/not risky?]: Flips some release assertions to diagnostic assertions.  No behavior change.
[String changes made/needed]: None
Attachment #8982648 - Flags: approval-mozilla-beta?
Comment on attachment 8982648 [details] [diff] [review]
Convert service worker and clients release assertions to diagnostic assertions. r=asuth

Switches some release asserts back to diagnostic asserts, approved for 61.0b11.
Attachment #8982648 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.