Closed Bug 1122051 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Spelling checker, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: RyanVM, Assigned: dholbert)

References

Details

Attachments

(1 file, 1 obsolete file)

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
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.
(Also, for motivation: as I described in bug 1122137, this warning makes up 40% of the log that RyanVM provided here.)
Depends on: 278312
Summary: Lots of spellchecker NS_ENSURE_TRUE spam in mochitest-2 runs → 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
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.
Attached patch fix v1 (obsolete) — Splinter Review
ehsan, see comment 4 for why I think silencing this warning is OK.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8549828 - Flags: review?(ehsan)
(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 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.
Attachment #8549828 - Flags: review?(ehsan) → review-
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.)
Attached patch fix v2Splinter Review
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.)
Attachment #8549828 - Attachment is obsolete: true
Attachment #8549878 - Flags: review?(ehsan)
(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 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.
Attachment #8549878 - Flags: review?(ehsan) → review+
(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?)
Flags: needinfo?(ehsan)
(The "!!" also technically wouldn't match local style -- the only instance of "!!" in that file is inside of a warning-message.)
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).
> which was bad because it wouldn't compare as == to true
er, I meant PR_TRUE
<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.)
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.
Yeah, that should be OK.
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.  :-)
Flags: needinfo?(ehsan)
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!
https://hg.mozilla.org/mozilla-central/rev/d6beadb2e715
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.