1328686 - add some diagnostic asserts in dom/cache

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

I'd like to use diagnostic assertions in more places.  Cache API seems like a good place to start.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Add diagnostic assertions to dom/cache. r=asuth

In an effort to catch more bugs sooner, this converts all of the low-cost assertions in dom/cache to MOZ_DIAGNOSTIC_ASSERT().  Things that were O(n), required TLS lookup, etc I left as DEBUG-only assertions.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=117040ba614438a4d6a6fa63e2a9d2436d78f229

Comment 2

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

Comment on attachment 8824476 [details] [diff] [review]
Add diagnostic assertions to dom/cache. r=asuth

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

In a corruption of the immortal words of Barry and LeVon, that's a lot of diagnostic asserts.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46cb4ee49786
Add diagnostic assertions to dom/cache. r=asuth

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/46cb4ee49786

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8824476 [details] [diff] [review]
Add diagnostic assertions to dom/cache. r=asuth

Approval Request Comment
[Feature/Bug causing the regression]: Generic service worker stability
[User impact if declined]: Purpose of this bug is to help us narrow down crashes in service worker code.  Does not address a specific problem, though.
[Is this code covered by automated tests?]: The purpose of the diagnostic assertions are to perform additional checks in non-DEBUG builds of dev-edition and nightly.  Any revealed problems will show up in crash-stats.
[Has the fix been verified in Nightly?]:  The assertions have been on nightly for a couple days.
[Needs manual test from QE? If yes, steps to reproduce]:   No
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The diagnostic assertions have no impact on release or beta.
[String changes made/needed]: None

Comment 6

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8824476 [details] [diff] [review]
Add diagnostic assertions to dom/cache. r=asuth

convert some dom/cache asserts to diagnostic asserts, aurora52+

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6bbe26dd67152c1768d9c8e17e5cddae4f02fa7c

Comment 8

[Mike Hommey [:glandium]]

This change is going to break debug builds on beta when it reaches there:

/home/worker/workspace/build/src/dom/cache/DBSchema.cpp:2561:90: error: 'lastVersion' was not declared in this scope
/home/worker/workspace/build/src/dom/cache/DBSchema.cpp:2561:102: error: template argument 1 is invalid
/home/worker/workspace/build/src/dom/cache/Manager.cpp:1533:100: error: 'oldRef' was not declared in this scope
/home/worker/workspace/build/src/dom/cache/Manager.cpp:1533:107: error: template argument 1 is invalid
/home/worker/workspace/build/src/dom/cache/Manager.cpp:1584:99: error: 'oldRef' was not declared in this scope
/home/worker/workspace/build/src/dom/cache/Manager.cpp:1584:106: error: template argument 1 is invalid
/home/worker/workspace/build/src/dom/cache/StreamControl.cpp:57:7: error: 'closedCount' was not declared in this scope
/home/worker/workspace/build/src/dom/cache/StreamControl.cpp:61:71: error: 'closedCount' was not declared in this scope
/home/worker/workspace/build/src/dom/cache/StreamControl.cpp:61:87: error: template argument 1 is invalid

Comment 9

[Mike Hommey [:glandium]]

It also breaks non-debug builds:

/home/worker/workspace/build/src/dom/cache/StreamControl.cpp:57:7: error: use of undeclared identifier 'closedCount'