1410186 - make Maybe<> use release or diagnostic asserts for isSome() checks

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Ben Kelly [:bkelly, not reviewing]]

Calls like Maybe<>::ref() current MOZ_ASSERT(isSome()) to make sure you're not referencing a non-existent object.  We should probably convert these to release or diagnostic asserts.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: turn Maybe assertions into release assertions

We out-of-line the relevant functions because release assertions can
generate quite a bit of code, and we'd rather let the compiler determine
if these functions should be inlined now.

I am open to making these MOZ_DIAGNOSTIC_ASSERTs if we think that would be less
risky.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8926909 [details] [diff] [review]
turn Maybe assertions into release assertions

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

Thank you!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15b89e515c94
turn Maybe assertions into release assertions; r=bkelly

Comment 4

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/15b89e515c94

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/adfd4d3245467c54507316b9be82b86424585031
Backout 15b89e515c94 (bug 1410186) for causing too many crashes on Nightly.

Comment 6

[Nicholas Nethercote [inactive]]

I backed this out because it was working too well -- bug 1416344 was causing extremely high crash rates in Nightly.

bkelly, I believe froydnj will be on leave for some time. Would you like to take over this and ensure it gets relanded once bug 1416344 is fixed?

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

I don't have time to do much here.  I personally think we should fix bug 1410186 and not back this out. Referencing uninitialized memory is not safe and hiding the crashes is not doing our users any favors.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

I re-read that other bug and see the crash rates are higher than I would have thought last time I read it.  We should definitely get that fixed before re-landing this.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

This missed the cut-off for Fx58 before the final merge to Beta, so I grafted the backout there as well.
https://hg.mozilla.org/releases/mozilla-beta/rev/b188777d8f972c4eb09aeb9471155751c29f62d6

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/adfd4d324546

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: fix linking errors with the Maybe<> rewrite

It turns out that several bits of a all-platforms try build fail with the
previous patch, because OptimizationInfo::CompilerWarmupThreshold doesn't
actually get assigned storage, and we need it to have storage for
Maybe::valueOr (which takes CompilerWarmupThreshold by reference).  This patch
moves CompilerWarmupThreshold out of the class definition, which seems to solve
the issue.

I'll fold this into the previous patch before commit.

Comment 12

[Chris Peterson [:cpeterson]]

Nathan, can we re-land these Maybe release assertions now that the trimmingPolicy bug 1416344 has been fixed?

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Chris Peterson [:cpeterson] from comment #12)
> Nathan, can we re-land these Maybe release assertions now that the
> trimmingPolicy bug 1416344 has been fixed?

Sure, we could do that.  Leaving ni? open to remind myself to put the patches back into landable shape.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c82274a23aea
turn Maybe assertions into diagnostic assertions; r=bkelly

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to Chris Peterson [:cpeterson] from comment #12)
> Nathan, can we re-land these Maybe release assertions now that the
> trimmingPolicy bug 1416344 has been fixed?

Relanded, but as MOZ_DIAGNOSTIC_ASSERT, so as to try to limit the performance impact on release.

Comment 16

[Eliza Balazs [:ebalazs_]]

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

Comment 17

[Andreea Pavel [:apavel]]

Backed out on request from igoldan. From IRC
igoldan> froydnj: bug 1410186 caused an increase in installer size

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

Backout link: https://hg.mozilla.org/mozilla-central/rev/77c06979d9e88979ec96263eccdbd750cb9221a4

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aab1afd86d77
turn Maybe assertions into diagnostic assertions; r=bkelly

Comment 20

[Nathan Froyd [:froydnj]]

(In reply to Andreea Pavel [:apavel] from comment #17)
> Backed out on request from igoldan. From IRC
> igoldan> froydnj: bug 1410186 caused an increase in installer size

Yup, relanded since that bug is wontfix.

Comment 21

[Stefan Hindli [:stefan_hindli]]

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

Comment 22

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #20)
> (In reply to Andreea Pavel [:apavel] from comment #17)
> > Backed out on request from igoldan. From IRC
> > igoldan> froydnj: bug 1410186 caused an increase in installer size
> 
> Yup, relanded since that bug is wontfix.

That bug is this bug, I'm assuming you meant bug 1461934. This regressed libxul by 256k [1], but as noted it's nightly/beta only so unlikely to be fixed.



[1] https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=mozilla-inbound,1299711,1,2&zoom=1526348067705.438,1526629766980.3625,126492537.31343284,133059701.49253732&selected=mozilla-inbound,1299711,337469,469582724,2