Closed Bug 1465103 Opened 2 years ago Closed 2 years ago

revert release assertions from bug 1461326 back to diagnostic assertions

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox60 --- unaffected
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Priority: -- → P2
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)
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+
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
https://hg.mozilla.org/mozilla-central/rev/209cc0d0c1df
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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.