1465103 - revert release assertions from bug 1461326 back to diagnostic assertions

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Convert service worker and clients release assertions to diagnostic assertions. r=asuth

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Convert service worker and clients release assertions to diagnostic assertions. r=asuth

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Convert service worker and clients release assertions to diagnostic assertions. r=asuth

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.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I compiled this locally with both diagnostics enabled and forcibly disabled like they are on beta.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

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?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Convert service worker and clients release assertions to diagnostic assertions. r=asuth

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/209cc0d0c1df
Convert service worker and clients release assertions to diagnostic assertions. r=asuth

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/209cc0d0c1df

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c13cb09f0c25