Re: NS_WARN_IF_FALSE has been renamed NS_WARNING_ASSERTION

[ISHIKAWA,chiaki]

On 2016/09/02 16:02, Nicholas Nethercote wrote:
> Greetings,
>
> NS_WARN_IF_FALSE has been renamed NS_WARNING_ASSERTION.
>
> See https://bugzilla.mozilla.org/show_bug.cgi?id=1299727 for details,
> including the justification.
>
> See https://bugzilla.mozilla.org/show_bug.cgi?id=1300007 for a bug
> that this change exposed, thus adding weight to the justification.
>
> See https://bugzilla.mozilla.org/show_bug.cgi?id=1300002 for a
> comm-central patch.
>
> Nick


I think it is a good time to ask about a confusing issue.

In the following URL about coding style,
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

there is a section about NS_WARN_IF.
I quote it below.

--- begin quote ---

Use the NS_WARN_IF macro when errors are unexpected.

The NS_WARN_IF macro can be used to issue a console warning in debug
builds if the condition fails. This should only be used when the failure
is unexpected and cannot be caused by normal web content.

If you are writing code that wants to issue warnings when methods fail,
please either use NS_WARNING directly or use the new NS_WARN_IF macro.

if (NS_WARN_IF(somethingthatshouldbetrue)) {
return NS_ERROR_INVALID_ARG;
}

if (NS_WARN_IF(NS_FAILED(rv))) {
return rv;
}

--- end quote ----

I am not a native speaker of English, but shouldn't the variable name in
the first if-statement example be
|somethingthatshouldNOTbetrue| instead of |somethingthatshouldbetrue|?

If so, Japanese, Chinese and Russian translation ought to be changed as
well.

I may have found a few code fragments that may have been miswritten due
to misunderstanding.

TIA

[Daniel Holbert]

On 09/21/2016 12:48 PM, ISHIKAWA,chiaki wrote:
> In the following URL about coding style,
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

> --- begin quote ---

> if (NS_WARN_IF(somethingthatshouldbetrue)) {
> return NS_ERROR_INVALID_ARG;
> }
>
> if (NS_WARN_IF(NS_FAILED(rv))) {
> return rv;
> }
>
> --- end quote ----
>
> I am not a native speaker of English, but shouldn't the variable name in
> the first if-statement example be
> |somethingthatshouldNOTbetrue| instead of |somethingthatshouldbetrue|?

You're very right! I've fixed it to say "somethingthatshouldbefalse"
(s/true/false/):
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style$compare?locale=en-US&to=1122081&from=1086401

> If so, Japanese, Chinese and Russian translation ought to be changed as
> well.

I am not qualified to fix those. I'm hoping you & others can take care
of that, perhaps! :)

> I may have found a few code fragments that may have been miswritten due
> to misunderstanding.

Yikes! Please file bugs on those. Hopefully they are just cases of of us
accidentally warning in the wrong condition, rather than us actually
behaving incorrectly.

[ISHIKAWA,chiaki]

On 2016/09/22 5:56, Daniel Holbert wrote:
> On 09/21/2016 12:48 PM, ISHIKAWA,chiaki wrote:
>> In the following URL about coding style,
>> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
>> --- begin quote ---
>> if (NS_WARN_IF(somethingthatshouldbetrue)) {
>> return NS_ERROR_INVALID_ARG;
>> }
>>
>> if (NS_WARN_IF(NS_FAILED(rv))) {
>> return rv;
>> }
>>
>> --- end quote ----
>>
>> I am not a native speaker of English, but shouldn't the variable name in
>> the first if-statement example be
>> |somethingthatshouldNOTbetrue| instead of |somethingthatshouldbetrue|?
>
> You're very right! I've fixed it to say "somethingthatshouldbefalse"
> (s/true/false/):
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style$compare?locale=en-US&to=1122081&from=1086401
>

Thank you for fixing the issue so quickly.

>> If so, Japanese, Chinese and Russian translation ought to be changed as
>> well.
>
> I am not qualified to fix those. I'm hoping you & others can take care
> of that, perhaps! :)

I am not sure how to alert the translation contributors. Does anyone
know how?

>> I may have found a few code fragments that may have been miswritten due
>> to misunderstanding.
>
> Yikes! Please file bugs on those. Hopefully they are just cases of of us
> accidentally warning in the wrong condition, rather than us actually
> behaving incorrectly.

The particular issue I was struggling with turned out to be NOT the
warning in the wrong condition, but the failure to use the return value
of NS_WARN_IF, and I am not sure if I got it right now that I re-read
the developer page :-(
Bug 1304504 - Check the return value of GetIntPref() properly or an
initialized bogus value may be used
https://bugzilla.mozilla.org/show_bug.cgi?id=1304504

I was confused to no end when I read the original coding guideline page.
Thank you again for the quick fix. I hope future developers won't be
confused any longer :-)

[Samael Wang]

The NS_ASSERTION document [1] says "Don't use NS_ASSERTION", which could be a bit confusing that now some of the similar named macros may be deprecated but some are new and encouraged.

Could we possibly have a section in the coding guideline explaining all these NS_ASSERTION / MOZ_ASSERT / NS_ENSURE_* / NS_WARN_IF and other related macros, and point (or add a link on) these macros' MDN reference pages to the guideline so new contributors can have an overview of when to / not-to use these macros?

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/NS_ASSERTION

[Daniel Holbert]

On 09/21/2016 08:41 PM, Samael Wang wrote:
> The NS_ASSERTION document [1] says "Don't use NS_ASSERTION", which could be a bit confusing that now some of the similar named macros may be deprecated but some are new and encouraged.

I think that document's advice is too severe.

roc made a compelling case for *preferring* non-fatal assertions (i.e.
NS_ASSERTION) in many scenarios, particularly when failure is not
catastrophic. See his blog post here for more details:
http://robert.ocallahan.org/2011/12/case-for-non-fatal-assertions.html

I found (and still find) his argument there quite compelling. I think
it's important that debug builds are able to proceed past (inevitable)
unexpected-but-not-horrible conditions -- while at the same time,
perhaps shouting about those unexpected conditions noisily so that
people can report them if they care to do so (and so that test suites fail).

Personally, I use *both* MOZ_ASSERT and NS_ASSERTION, in different
scenarios, depending on how catastrophic it would be if the asserted
condition doesn't hold, and depending on how sure I am that the
condition does-currently & must-always-in-the-future hold true.

> Could we possibly have a section in the coding guideline explaining all these NS_ASSERTION / MOZ_ASSERT / NS_ENSURE_* / NS_WARN_IF and other related macros, and point (or add a link on) these macros' MDN reference pages to the guideline so new contributors can have an overview of when to / not-to use these macros?

That is a great idea! I would support the creation of such a page. :)

~Daniel

[Gerald Squelart]

On Thursday, September 22, 2016 at 2:58:46 PM UTC+10, Daniel Holbert wrote:
> On 09/21/2016 08:41 PM, Samael Wang wrote:
> > The NS_ASSERTION document [1] says "Don't use NS_ASSERTION", which could be a bit confusing that now some of the similar named macros may be deprecated but some are new and encouraged.
>
> I think that document's advice is too severe.
>
> roc made a compelling case for *preferring* non-fatal assertions (i.e.
> NS_ASSERTION) in many scenarios, particularly when failure is not
> catastrophic. See his blog post here for more details:
> http://robert.ocallahan.org/2011/12/case-for-non-fatal-assertions.html
>
> I found (and still find) his argument there quite compelling. I think
> it's important that debug builds are able to proceed past (inevitable)
> unexpected-but-not-horrible conditions -- while at the same time,
> perhaps shouting about those unexpected conditions noisily so that
> people can report them if they care to do so (and so that test suites fail).
>
> Personally, I use *both* MOZ_ASSERT and NS_ASSERTION, in different
> scenarios, depending on how catastrophic it would be if the asserted
> condition doesn't hold, and depending on how sure I am that the
> condition does-currently & must-always-in-the-future hold true.

> [...]
> ~Daniel

Sitting on the shoulders of giants, an idea, in the unlikely case it's not been thought of yet:
How about an assertion that files a crash report (or something lighter like a telemetry blip) but does not actually crash?

g.

[Paul Adenot]

On Thu, Sep 22, 2016, at 07:33 AM, Gerald Squelart wrote:
> Sitting on the shoulders of giants, an idea, in the unlikely case it's
> not been thought of yet:
> How about an assertion that files a crash report (or something lighter
> like a telemetry blip) but does not actually crash?

I think you can do that with telemetry, we do that, for example, when we
encounter a severe failure when trying to open audio devices [0], trying
to get a sense of how frequent it is in the field.

Cheer,
Paul.

[0]:
http://searchfox.org/mozilla-central/source/dom/media/GraphDriver.cpp#595

[Gerald Squelart]

Nice one Paul.

But I was hoping for something easier and more flexible than telemetry, where I can just write an assertion, and something will land in Socorro when it fails, but without crashing Firefox.

Cheers,
Gerald