1122051 - Lots of this spellchecker NS_ENSURE_TRUE spam in mochitest-2 runs: "WARNING: NS_ENSURE_TRUE(mSpellCheck) failed: file [...]/extensions/spellcheck/src/mozInlineSpellChecker.cpp, line 905

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Going through a recent mochitest-2 run log, I noticed that the below line is spammed a LOT.

06:48:00     INFO -  [Parent 1392] WARNING: NS_ENSURE_TRUE(mSpellCheck) failed: file c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp, line 905

Anything we can do to shut it up?

Example log:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32-debug/1421327972/mozilla-central_xp-ix-debug_test-mochitest-2-bm109-tests1-windows-build407.txt.gz

Comment 2

[Daniel Holbert [:dholbert]]

This line dates back to version 1.1 of this file, added in bug 278312, back in 2005:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp&rev=1.47#806

My impression is that this was a time when NS_ENSURE_TRUE was used more liberally for normal error-handling, & people were less concerned about warning-spam. I suspect this wants to just be a null-check-and-return, instead of NS_ENSURE_TRUE.

Comment 3

[Daniel Holbert [:dholbert]]

(Also, for motivation: as I described in bug 1122137, this warning makes up 40% of the log that RyanVM provided here.)

Comment 4

[Daniel Holbert [:dholbert]]

I'm poking at this in a debugger to be sure we can silence this NS_ENSURE_TRUE -- i.e., that this isn't actually a worrisome condition. I think we can.

Basically, what happens is:
  - A test sets innerHTML on some contenteditable stuff
  - That triggers us to invoke SetEnableRealTimeSpell(true)
  - Through a few steps, that:
     (A) initializes a spell-checker as mPendingSpellCheck.
     (B) Queues up a runnable to swap mPendingSpellCheck over to mSpellCheck.
  - Then, before that runnable is flushed, we tweak innerHTML some more, which triggers spell-checking via calls to mozInlineSpellChecker::SpellCheckRange.
  - SpellCheckRange bails early and spams this warning, because mSpellCheck is still null (though we've got a pending runnable that's waiting to set it to something useful!!)

So I think this is a bit of an edge case, rather than being a frightening error of some sort -- basically, spell-checking gets enabled & exercised repeatedly before it gets a chance to asynchronously finish initializing itself.

This probably indicates that stuff is never being spell-checked, in the repeated modifications that happen before initialization. However, that's not a huge user-facing issue, because this isn't a use-case that could actually be triggered by a user at a keyboard.  So, there may be some refactoring/tweaking that should be done here to address this hypothetical bug, but in the meantime I think it's totally fine to silence this warning.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v1

ehsan, see comment 4 for why I think silencing this warning is OK.

Comment 6

[Daniel Holbert [:dholbert]]

(If the hypothetical bug -- i.e. immediate scripted modifications not being guaranteed to be spell-checked -- affected real-world behavior for users, and was not too spammy, *then* I could see a case for keeping this warning around until the hypothetical bug was fixed.

But since real users are extremely unlikely to hit that situation, and given that this makes up 40% of some of our mochitest logs, I don't think it makes sense to keep this warning.)

Comment 7

[(no longer active)]

Comment on attachment 8549828 [details] [diff] [review]
fix v1

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

I'd r+ a patch that adds the warning as per below.

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp
@@ +902,5 @@
>  nsresult
>  mozInlineSpellChecker::SpellCheckRange(nsIDOMRange* aRange)
>  {
> +  if (!mSpellCheck) {
> +    return NS_ERROR_NOT_INITIALIZED;

We do want to warn here if mPendingSpellCheck is null, since that would indicate broken spell checking.

Comment 8

[Daniel Holbert [:dholbert]]

Sounds good. I'd thought about adding that, but I wasn't 100% sure that was a warn-worthy condition, since I don't really know this code well. (But it did seem warn-worthy when I was tracing through in gdb, and the fact that you suggested it makes me more confident in adding it.)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Here's the updated version, with a warning added.

(I made the warning complain that spell-checking seems to be disabled, since according to the GetEnableRealTimeSpell method, that's what it means when mSpellCheck and mPendingSpellCheck are both null.)

Comment 10

[(no longer active)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Sounds good. I'd thought about adding that, but I wasn't 100% sure that was
> a warn-worthy condition, since I don't really know this code well. (But it
> did seem warn-worthy when I was tracing through in gdb, and the fact that
> you suggested it makes me more confident in adding it.)

Yeah it's pretty warn-worthy, since if we emit that warning, your spell check should be completely broken.  :-)

Comment 11

[(no longer active)]

Comment on attachment 8549878 [details] [diff] [review]
fix v2

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

Nit: please use !! to turn the pointer into a boolean.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #11)
> Nit: please use !! to turn the pointer into a boolean.

I'm OK doing this, though it somewhat contradicts this coding style guideline, which says:
> When testing a pointer, use (!myPtr) or (myPtr); don't use myPtr != nullptr or myPtr == nullptr.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices

Given that, do you still prefer that I add the "!!"?  (And if so, do you think we should update the coding style to allow and/or recommend the "!!" form?)

Comment 13

[Daniel Holbert [:dholbert]]

(The "!!" also technically wouldn't match local style -- the only instance of "!!" in that file is inside of a warning-message.)

Comment 14

[Daniel Holbert [:dholbert]]

I asked for thoughts on "!!" in #developers just now.  The only feedback I got (from seth, who I trust on C++ arcana) was that "!!" is an anti-pattern in C++.

IIUC, the only case where "!!myPtr" makes some sense would be when you're explicitly comparing against a boolean variable. e.g.
  if (myBool == myPtr) {  // This check will probably fail!

In a case like this, I'd need "!!" to explicitly downconvert myPtr, because otherwise we'd upconvert myBool to a pointer-type (probably 0x0 or 0x1) and compare that for equality against the pointer. So we'd need:
  if (myBool == !!myPtr) {
(though seth says, and I tend to agree, that an explicit bool cast would be clearer than "!!" even in this case)

I do also recall "!!" being semi-necessary back in the PRBool days, since PRBool was technically backed by an integer, so "PRBool isEnabled = myPtr;" would give you a PRBool that was some arbitrary nonzero value, which was bad because it wouldn't compare as == to true. That's not a concern anymore, though, because we use bool instead of PRBool, and "bool isEnabled = myPtr;" will Just Work.

So -- excluding cases where we're comparing a pointer to a boolean value with "==", I don't think !! adds any value (and instead reduces readability a bit).

Comment 15

[Daniel Holbert [:dholbert]]

> which was bad because it wouldn't compare as == to true
er, I meant PR_TRUE

Comment 16

[Seth Fowler [:seth] [:s2h]]

<chiming in>

Yeah, so in my experience "!!" is a C-ism from the pre-C99 days when there was no real bool type.

In most situations it isn't necessary in C++, and if you are in a situation where you cannot rely on implicit conversion to bool, as can arise sometimes with templates for example, IMO "bool(ptrValue)" is much clearer and cleaner.

That said, Ehsan, I think of you as being a C++ expert; maybe you have a reason to do it in this specific case that I'm not aware of? (I have not looked at the definition of the macro that's used here.)

Comment 17

[Daniel Holbert [:dholbert]]

FWIW, the macro expands to:
> 88 #define NS_WARN_IF_FALSE(_expr,_msg)                          \
> 89   do {                                                        \
> 90     if (!(_expr)) {                                           \
> 91       NS_DebugBreak(NS_DEBUG_WARNING, _msg, #_expr, __FILE__, __LINE__); \
[...]

https://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#88

So NS_WARN_IF_FALSE(ptr, "...") expands to "if (!(ptr)) {", which is what you'd hope.

Comment 18

[Seth Fowler [:seth] [:s2h]]

Yeah, that should be OK.

Comment 19

[(no longer active)]

The reason why I suggested !! was that the result was being used in a macro, so in the future one could change the macro to cause the pointer to be compared against bool, or be passed to something which can be constructed explicitly from a bool, and so on.  IOW, just future proofing.

That being said, landing this either with !!, static_cast<bool>, or as is would be fine by me, no big deal.  :-)

Comment 20

[Daniel Holbert [:dholbert]]

Gotcha -- that makes some sense, thanks.

I think it's unlikely that we'd break the macro in that way in the future, though, since we already pass non-bool values into NS_WARN_IF_FALSE & NS_ASSERTION all over the place.  If we break that use-case, we're in serious trouble. :)

So I'll opt to land this as-is, without static_cast or !!, for readability's sake.

Thanks for the review!

Comment 21

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d6beadb2e715

Comment 22

[Carsten Book [:Tomcat]]

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