1540401 - LSNG: Convert most of MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT

Status: RESOLVED WORKSFORME

(Core :: Storage: localStorage & sessionStorage, task, P3)

Description

[Jan Varga [:janv]]

MOZ_ASSERT is only defined in debug builds. All the automatic tests run in debug builds, but there are obviously corner cases which are not covered by our tests. So the only chance how to better identify these corner cases is to have assertions in optimized Nightly builds which are used daily by many users.
It adds some overhead, but I think we don't have any other options.

This is primarly intended to identify remaining deadlocks and shutdown hangs.

Comment 1

[Jan Varga [:janv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73ac0759c5bf43e95dd25fe38f0ed441fd202e5f

Comment 2

[Jan Varga [:janv]]

Attachment: Bug 1540401 - LSNG: Convert most of MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT; r=asuth

Comment 3

[Jan Varga [:janv]]

Basically, all MOZ_ASSERT should be converted to MOZ_DIAGNOSTIC_ASSERT, except the ones for thread checks.
Also, all #ifdef DEBUG should be converted to #ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED, except the ones for NS_WARNING or LS_WARNING.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97d88752079f
LSNG: Convert most of MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT; r=asuth

Comment 5

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/97d88752079f

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/82ba38c4aa6a
Backed out changeset 97d88752079f for causing a nightly startup crash a=backout

Comment 7

[Jan Varga [:janv]]

Ok this was expected to happen, sorry for the trouble.
We obtained really good data and now we can fix remaining bugs.

Comment 8

[Jan Varga [:janv]]

Attachment: Bug 1540401 - Part 1: Convert MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT to verify that bug 1541972 has been fixed; r=asuth

Comment 9

[Jan Varga [:janv]]

Attachment: Bug 1540401 - Part 2: Convert MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT to verify that bug 1541775 has been fixed; r=asuth

Comment 10

[Jan Varga [:janv]]

Attachment: Bug 1540401 - Part 3: Convert MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT to verify that bug 1541772 has been fixed; r=asuth

Comment 11

[Jan Varga [:janv]]

Attachment: Bug 1540401 - Part 4: Convert MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT to verify that bug 1541771 has been fixed; r=asuth

Comment 12

[Jan Varga [:janv]]

I plan to land only one part/patch a day.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d64e4d9400ec
Part 1: Convert MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT to verify that bug 1541972 has been fixed; r=asuth

Comment 14

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/d64e4d9400ec

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dcc6eec59f4
Backout - Part 1: Convert MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT to verify that bug 1541972 has been fixed; r=asuth

Comment 16

[Jan Varga [:janv]]

So, the incorrect usage is still stored in some usage files. We need to change the "cookie" of the usage file so all usage files get recalculated value.

Comment 17

[Calixte Denizet (:calixte)]

There are 13 crashes with signature 'mozilla::dom::(anonymous namespace)::QuotaClient::InitOrigin' and 4 with 'mozilla::dom::`anonymous namespace'::QuotaClient::InitOrigin'.
For all these crashes the moz_crash_reason is: MOZ_RELEASE_ASSERT(usage >= 0).

Comment 18

[Jan Varga [:janv]]

Yeah, the patch has been backed out.

Comment 19

[Julien Cristau [:jcristau]]

Jan, next time can you please ask sheriffs to back out the crashing patch from central directly? That way we avoid an extra unnecessary crashy build. Thanks :)

Comment 20

[Jan Varga [:janv]]

There were no issues on my local machine and all the tests passed on try. I needed to verify with bigger audience.
Sorry about that.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout of part 1: https://hg.mozilla.org/mozilla-central/rev/3dcc6eec59f4

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d134fc3b9e29
Part 1: Convert MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT to verify that bug 1541972 has been fixed; r=asuth

Comment 23

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/d134fc3b9e29

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/729f59649b3f
Part 2: Convert MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT to verify that bug 1541775 has been fixed; r=asuth

Comment 25

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/729f59649b3f

Comment 26

[Calixte Denizet (:calixte)]

There are 8 crashes with signatures 'mozilla::dom::(anonymous namespace)::Snapshot::RecvCheckpoint' and 'mozilla::dom::`anonymous namespace'::Snapshot::RecvCheckpoint'.
They all hit the assertion: MOZ_RELEASE_ASSERT(mPeakUsage >= mUsage).

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b02147ae4804
Part 3: Convert MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT to verify that bug 1541772 has been fixed; r=asuth

Comment 28

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/b02147ae4804

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dad8b92996d5
Part 4: Convert MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT to verify that bug 1541771 has been fixed; r=asuth

Comment 30

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/dad8b92996d5

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29bf13f9b35a
Backout Part 2: Convert MOZ_DIAGNOSTIC_ASSERT back to MOZ_ASSERT since bug 1541775 hasn't been fixed yet; r=asuth

Comment 32

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/29bf13f9b35a

Comment 33

[Julien Cristau [:jcristau]]

Setting checkin-needed-beta to get the latest backout (from comment 32) grafted to 68 to stop crashing DevEdition builds.

Comment 34

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/298f7d8bb0c0

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/288c3317b2fc
Backout Part 3: Convert MOZ_DIAGNOSTIC_ASSERT back to MOZ_ASSERT since bug 1541772 hasn't been fixed yet; r=asuth

Comment 36

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/288c3317b2fc

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d394d2c86db
Backout Part 4: Convert MOZ_DIAGNOSTIC_ASSERT back to MOZ_ASSERT since bug 1541771 hasn't been fixed yet; r=asuth

Comment 38

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1d394d2c86db

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/585544d6e2cd
Backout Part 1: Convert MOZ_DIAGNOSTIC_ASSERT back to MOZ_ASSERT since bug 1541972 hasn't been fixed yet; r=asuth

Comment 40

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/585544d6e2cd

Comment 41

[Jan Varga [:janv]]

Julien, do we also want to uplift patches from comment 36, comment 38 and comment 40 ?
All those crashes will automatically stop once early beta ends, not sure if it will happen in beta 9.

Comment 42

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8a44e86052bb5439b6f71ee0c616ba2f6251a655
https://hg.mozilla.org/releases/mozilla-beta/rev/8814bb1352784e68fc0944c17104d308166a93b7
https://hg.mozilla.org/releases/mozilla-beta/rev/927b23f07645dd3f1ff4b63d8cf477bec77b9e22

Comment 43

[Jens Stutte [:jstutte]]

Hi :janv, is the leave-open still needed?

Comment 44

[Jan Varga [:janv]]

Yes, the ultimate goal is to have most of the assertions defined as diagnostic asserts, but it's not a high priority right now.

Comment 45

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jstutte, maybe it's time to close this bug?

Comment 46

[Jens Stutte [:jstutte]]

See comment 44.

Comment 47

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.