809880 - uninitialized data created by nsMsgFilterList::ParseCondition

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[ISHIKAWA, Chiaki]

uninitialized data created by nsMsgFilterList::ParseCondition

Found by valgrind run. See bug 803816 about mozmill run of TB under valgrind.

Source version:

comm-central thunderbird.
$ hg identify
1016cef82fd8+ tip
ishikawa@debian-vm:~/TB-NEW/TB-3HG/new-src$ cd mozilla
ishikawa@debian-vm:~/TB-NEW/TB-3HG/new-src/mozilla$ hg identify
a517f7ea5bef+ tip

For full logs and annotated excerpts:

[1] Full log for valgrind run
   "still more log of valgrind run for classifying the problems "
   https://bug803816.bugzilla.mozilla.org/attachment.cgi?id=679654

[2] Annotated excerpt from the log
   "Manually inserted case ddd labeling to excerpted portion of the previous log."
   https://bug803816.bugzilla.mozilla.org/attachment.cgi?id=679655

I found from the valgrind run that the creation of data using the
following function leaves some variables in the object(?) or allocated
data structure (?) uninitialized which are later referenced and
valgrind prints warnings about it.

	 nsMsgFilterList

case 22: nsMsgFilterList issue	

The part labeled as "CASE 22" in the annotated file [2] above shows
the problem as follows 

--- quote ---
CASE 22: not sure  RunScript -> Interpret 

==13533== Conditional jump or move depends on uninitialised value(s)
==13533==    at 0x66CBA36: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:1573)
==13533==    by 0x66D18BC: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:324)
==13533==    by 0x66D297F: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:379)
==13533==    by 0x66D3047: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==13533==    by 0x66D3109: js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.cpp:485)
==13533==    by 0xFFFFFF86: ???
==13533==  Uninitialised value was created by a heap allocation
==13533==    at 0x40271C4: malloc (vg_replace_malloc.c:270)
==13533==    by 0x4041E10: moz_xmalloc (mozalloc.cpp:54)
==13533==    by 0x5BBB972: nsMsgFilterList::ParseCondition(nsIMsgFilter*, char const*) (mozalloc.h:200)
--

 ... the following part is fetched from the full log in [1] ...

==13533==    by 0x600D69F: NS_InvokeByIndex_P (in /TB-NEW/TB-3HG/objdir-tb3/mozilla/toolkit/library/libxul.so)
==13533==    by 0x5766379: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:3132)
==13533==    by 0x576DC97: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1488)
==13533==    by 0x66D2890: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:364)
==13533==    by 0x66C4424: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2369)
==13533==    by 0x66D18BC: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:324)
==13533==    by 0x66D297F: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:379)
==13533==    by 0x66D3047: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==13533==    by 0x66D3109: js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.cpp:485)
==13533==    by 0x66EA238: js::GetPropertyHelper(JSContext*, JS::Handle<JSObject*>, JS::Handle<int>, unsigned int, JS::MutableHandle<JS::Value>) (jsscopeinlines.h:295)
==13533==    by 0x66D13AC: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterpinlines.h:287)
==13533==    by 0x66D18BC: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:324)
==13533==    by 0x66D297F: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:379)
==13533==    by 0x66D3047: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==13533==    by 0x66D3109: js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.cpp:485)
==13533==    by 0x66E96FB: js::baseops::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<int>, JS::MutableHandle<JS::Value>) (jsscopeinlines.h:295)
==13533==    by 0x6713848: js::DirectProxyHandler::get(JSContext*, JSObject*, JSObject*, int, JS::Value*) (jsobjinlines.h:171)
==13533==    by 0xFFFFFF81: ???
==13533==

--- end quote ---


Observation.

A class object is created using the nsComponentManager mechanism, and
eventually the following function is called

	   nsMsgFilterList::ParseCondition

But it seems that it leaves some variable(s) or data assigned to it
uninitialized. 

One of them is eventually referenced during the execution of
javascript interpreter and valgrind issues warning about a usage of
uninitialized value.

This should be fixed.

Comment 1

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, chiaki from comment #0)
> Observation.
> 
> A class object is created using the nsComponentManager mechanism, and
> eventually the following function is called
I was copy-pasting the same paragraph from similar bug entries. I don't think
we are using nsComponentManager mechanism here, but the bug itself still stands.

> 
> 	   nsMsgFilterList::ParseCondition
> 

TIA

Comment 2

[ISHIKAWA, Chiaki]

(I am afraid that I hit a wrong button and an empty message may be posted...)

In mailnews/base/search/src/nsMsgFilterList.cpp

NS_IMETHODIMP nsMsgFilterList::ParseCondition(nsIMsgFilter *aFilter, const char *aCondition)

    nsMsgSearchTerm *newTerm = new nsMsgSearchTerm;

There are two "new" operations for nsMsgSearchTerm inn the function.
One of them (or both?) must have a variable (at least) undefined and
it is referenced later.

I wish I could specify --read-var-info=yes to valgrind.
Then it would have been easier to figure out which field of the object structure
is undefined as the man page of valgrind says.

However, if I specify --read-var-info=yes on my PC, valgrind aborts as follows:

==17606==
==17606==     Valgrind's memory management: out of memory:
==17606==	 newSuperblock's request for 334843904 bytes failed.
==17606==	 2211033088 bytes have already been allocated.
==17606==     Valgrind cannot continue.	 Sorry.
==17606==
==17606==     There are several possible reasons for this.
==17606==     - You have some kind of memory limit in place.  Look at the
==17606==	output of 'ulimit -a'.	Is there a limit on the size of
==17606==	virtual memory or address space?
==17606==     - You have run out of swap space.
==17606==     - Valgrind has a bug.  If you think this is the case or you are
==17606==     not sure, please let us know and we'll try to fix it.
==17606==     Please note that programs can take substantially more memory than
==17606==     normal when running under Valgrind tools, eg. up to twice or
==17606==     more, depending on the tool.  On a 64-bit machine, Valgrind
==17606==     should be able to make use of up 32GB memory.  On a 32-bit
==17606==     machine, Valgrind should be able to use all the memory available
==17606==     to a single process, up to 4GB if that's how you have your
==17606==     kernel configured.  Most 32-bit Linux setups allow a maximum of
==17606==     3GB per process.
==17606==
==17606==     Whatever the reason, Valgrind cannot continue.  Sorry.

ulimit -a shows there is no artificial limit.
I have a swap about 1 GB, and added 2GB special file using swapon.
I don't know if there is valgrind problem or not.

Let us hope that someone with a more powerful PC than mine investigate the issue.

Comment 3

[:aceman]

You could try setting m_booleanOp to some value (like the other version of nsMsgSearchTerm constructor does at http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgSearchTerm.cpp#361) and see if it helps. You just need to find out which member it is, not what the correct value should be (in the first step).

Comment 4

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #3)
> You could try setting m_booleanOp to some value (like the other version of
> nsMsgSearchTerm constructor does at
> http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/
> nsMsgSearchTerm.cpp#361) and see if it helps. You just need to find out
> which member it is, not what the correct value should be (in the first step).

Thank you for your comment.

I will see what I can do.
Due to the slowness of running TB under valgrind, I can report the result
in a couple of days. (But it may be that the bug may have been already
solved in one way or the other... Come to think of it, when I checked the output of valgrind run I was not seeing this bug a couple of weeks ago. Strange. It could be that the particular test that triggered this bug may have been prematurely aborted due to timeout [this has happened sometimes :-( ]) Stay tuned.

TIA

Comment 5

[ISHIKAWA, Chiaki]

Attachment: parsecondition-valgrind-uninitialized-warning.patch

After reporting this bug last year and encouraged by :aceman to check what happens if we
set other members of the structure last month, I finally could
check the operation (under 64 bit Debian GNU/Linux.)
The strange usage of uninitialized memory usage does not happen
always.

In the normal condition, there is only a very short period/delay
between the creation of an object and proper setting of some fields
(later in different function) : this later setting is not quite correct in
multi-threaded environment if proper locking, etc. is not used.

Use of valgrind slows down the execution of program, and thus widens 
the windows of vulnerability, so to speak. So the chance of incomplete data access by other asynchornous threads (without proper locking) increases.

Anyway, the attached patch at least solves the issue for me, but
I think the people in the know should check whether there is a deeper cause or not.

It turns out, at least on my PC, setting mDirectory was necessary until
I didn't see the recurrence of the usage of uninitialized usage.
Something is fishy here, but again I leave this to people in the know :-)

: aceman, can you check the patch before asking for a review (I am not sure who to put in for this file).

TIA

Comment 6

[:aceman]

Comment on attachment 800057 [details] [diff] [review]
parsecondition-valgrind-uninitialized-warning.patch

Initializing object members inside its constructor seems sane.
A good reviewer for this would be rkent or Neil.

Comment 7

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #6)
> Comment on attachment 800057 [details] [diff] [review]
> parsecondition-valgrind-uninitialized-warning.patch
> 
> Initializing object members inside its constructor seems sane.
> A good reviewer for this would be rkent or Neil.

Right. I thought I would put [e-mail] in the review field, but
it got changed to [e-mail] (?!).

Comment 8

[:aceman]

Yes, [e-mail] is correct (this is his email, the other is his "username").

Comment 9

[[e-mail]]

Comment on attachment 800057 [details] [diff] [review]
parsecondition-valgrind-uninitialized-warning.patch

>+    // valgrind warning during GC/java data check!?
[Don't really need the comment, anyone interested can look up the check in and link it back to the bug.]

>+    m_headerAddressParser = nullptr;
>+    mDirectory = nullptr;
These are nsCOMPtr which already default initialise.

>+    m_arbitraryHeader = nullptr;
>+    m_hdrProperty = nullptr;
>+    m_customId = nullptr;
And these are nsCString which also initialise.

>+    m_booleanOp = nsMsgSearchBooleanOp::BooleanAND;
So this is the only line that makes sense. r=me on this line only.

Comment 10

[:aceman]

Strange, Chiaki specifically says it was needed to initialize mDirectory to make valgrind happy.

Comment 11

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #10)
> Strange, Chiaki specifically says it was needed to initialize mDirectory to
> make valgrind happy.

Right, that was what happened.
I will see if leaving the initialization only to m_booleanOp
will solve the observed issue.
(But the issue discovered by GC is highly timing-dependent and 
So it may not be easy to re-produce the problem.
That is why I could only re-discover this issue almost 8, 9 months from the original report :-(
Problem is that just running a session without seeing the problem may not indicate the absence of
the problem. Oh well.


TIA

Comment 12

[ISHIKAWA, Chiaki]

It would be a couple of days before I can confirm that the bug is no longer there with the shortened patch (wih only m_booleanOp initialization). Running |make mozmill| test on my slow PC takes more than 24 hours I am afraid.

By the way, here is the excerpt from the session log that identified the problem.


==14197== Conditional jump or move depends on uninitialised value(s)
==14197==    at 0x75CBF3D: JS::MutableHandle<JS::Value>::set(JS::Value) (Value.h:1233)
==14197==    by 0xA1B8B02: js::jit::DoSetPropFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICSetProp_Fallback*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) (BaselineIC.cpp:6347)
==14197==    by 0x189C8E0E: ???
==14197==    by 0x1AFD606D: ???
==14197==    by 0x3A439F8F: ???
==14197==    by 0x189C095D: ???
==14197==    by 0xA1CEFFB: EnterBaseline(JSContext*, js::jit::EnterJitData&) (BaselineJIT.cpp:112)
==14197==    by 0xA1CFA0B: js::jit::EnterBaselineMethod(JSContext*, js::RunState&) (BaselineJIT.cpp:143)
==14197==    by 0x9ECCFBE: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2522)
==14197==    by 0x9ECE813: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:446)
==14197==    by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508)
==14197==    by 0x9ECF3BF: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:539)
==14197==    by 0x9ECF5C0: js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:610)
==14197==    by 0xA0F6BF9: js::Shape::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, bool, JS::MutableHandle<JS::Value>) (Shape-inl.h:275)
==14197==    by 0xA0EA6D0: js::baseops::SetPropertyHelper(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, unsigned int, JS::MutableHandle<JS::Value>, bool) (jsobj.cpp:4668)
==14197==    by 0xA1256CF: js::DirectProxyHandler::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>) (jsobj.h:859)
==14197==    by 0xA1822BD: js::CrossCompartmentWrapper::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>) (jswrapper.cpp:314)
==14197==    by 0xA127325: js::Proxy::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>) (jsproxy.cpp:2553)
==14197==    by 0xA127486: proxy_SetGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, bool) (jsproxy.cpp:2871)
==14197==    by 0xA0DDF0D: JSObject::nonNativeSetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, bool) (jsobj.cpp:1700)
==14197==    by 0x9ED0299: JSObject::setGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, bool) (jsobj.h:858)
==14197==    by 0x9ED6E33: SetPropertyOperation(JSContext*, JS::Handle<JSScript*>, unsigned char*, JS::Handle<JS::Value>, JS::Handle<JS::Value>) (Interpreter.cpp:365)
==14197==    by 0x9EC776C: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2364)
==14197==    by 0x9ECE813: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:446)
==14197==    by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508)
==14197==    by 0x9ECF3BF: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:539)
==14197==    by 0xA1AC3FF: js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (BaselineIC.cpp:7066)
==14197==    by 0x189C8C5E: ???
==14197==    by 0x2EEFAD2F: ???
==14197==    by 0x189C095D: ???
==14197==    by 0xA1CEFFB: EnterBaseline(JSContext*, js::jit::EnterJitData&) (BaselineJIT.cpp:112)
==14197==    by 0xA1CFA0B: js::jit::EnterBaselineMethod(JSContext*, js::RunState&) (BaselineJIT.cpp:143)
==14197==    by 0x9ECE89E: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:435)
==14197==    by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508)
==14197==    by 0x9ECF3BF: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:539)
==14197==    by 0xA12201A: js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jsproxy.cpp:445)
==14197==    by 0xA18256E: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jswrapper.cpp:438)
==14197==    by 0xA122B2C: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jsproxy.cpp:2626)
==14197==    by 0xA122C64: proxy_Call(JSContext*, unsigned int, JS::Value*) (jsproxy.cpp:3203)
==14197==    by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219)
==14197==    by 0x9EC50B3: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:482)
==14197==    by 0x9ECF3BF: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:539)
==14197==    by 0xA1AC3FF: js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (BaselineIC.cpp:7066)
==14197==    by 0x189C8C5E: ???
==14197==    by 0x2D00A7E7: ???
==14197==    by 0x199F75BD: ???
==14197==    by 0x2DA4253F: ???
==14197==    by 0x199F75BD: ???
==14197==    by 0x2D2D5637: ???
==14197==    by 0x189C095D: ???
==14197==  Uninitialised value was created by a heap allocation
==14197==    at 0x402A914: malloc (vg_replace_malloc.c:291)
==14197==    by 0x404633B: moz_xmalloc (mozalloc.cpp:54)
==14197==    by 0x85F6CD8: nsMsgFilterList::ParseCondition(nsIMsgFilter*, char const*) (mozalloc.h:201)
==14197==    by 0x9502DC6: NS_InvokeByIndex (xptcinvoke_x86_64_unix.cpp:164)

It is interesting to note that 
the problem is noticed in JS interpreter/compiler (?).
With XPCOM RPC magic, the data in C++ world can be communicated with
JS runtime, but occasionally Java side tries to access such data in an unexpected manner.
The following excerpt is not GC-related (it seems that way).
But in the past, I saw GC-related routines producing uninitialzed warnings and
now I have a suspicion that GC thread may be running asynchronously and
may access data in an inconsistent state sometimes causing 
valgrind to report strange errors. 

In the excerpted case, Value.h:1233 is a check for IsObject()

1228 inline bool
1229 IsPoisonedValue(const Value &v)
1230 {
1231     if (v.isString())
1232         return IsPoisonedPtr(v.toString());
1233     if (v.isObject())
1234         return IsPoisonedPtr(&v.toObject());
1235     return false;
1236 }

Hmm, |IsPoinsedValue| sounds interesting. Uninitialized value is taken to mean
poisoned value in JS?

I am mentioning asynchronous thread (possibly GC) because if
the access to the newly created object and its members including m_boolenOp
is orderly [and I think the programmers who wrote the code *MEANT* it that way. This
uninitialized warning does NOT happen ALWAYS. I think there is a thread-race issue.], 
by the time a routine, even on the JS side, accesses this m_booleanOP through XPCOM,
in a serialized manner,  m_booleanOP ought to have been set to a proper value 
(and it *is* most of the time seeing that
the "uninitialized" warning is not printed that often. 
I don't see this warning in many |make mozmill| sessions running thunderbird under valgrind.
 
So I think we have a thread race issue on top of
the seemingly forgotten initialization of m_booleanOp.

Also, I mentioned GC in this context, because I have seen some GC-related routines in
the stack in a similar uninitialized value warning before, and during the investigation
of this particular bug, I thought mDirectory was necessary since, I felt
as if GC stomped on the newly created data structure in a manner similar to Boehm-Demers-Weiser conservative garbage collector would without proper synchronization.
Seeing the warning printed again after I initialized all the members except for mDirectory (which
was in a hard to notice place in the source),  I got quite confused and initialized all the members.

It may be hard to believe, but I have a suspicion that there is a subtle thread-race issues
with the particular GC implemented for mozilla software, and rest of the code.
(Chromium had such GC-related issue until it was noticed by ThreadSanitizer and fixed in 2009.)
https://code.google.com/p/chromium/issues/detail?id=18488

If I am not mistaken, I saw an uninitialized warning when only m_booleanOp and few others were
initialized. Only when I put in the explicit initialization of
mDirectory the warning did not appear (but again, 
it *may* again if the root cause is thread race of low-level routines. it is highly timing dependent.)
Due to this timing-dependency and the need to shut up the warning with all the possible
members EXPLICITLY initialized led me to suspect a timing issue with GC thread.
If there is such an issue, the warning may still happen, at future time.

For now, if with the initialization of only m_booleanOp, the symptom would not appear for a few days, I will re-submit the patch and ask for checkin.

Later, if the problem manifests again, I would blame asynchronous GC.
In my setup, the window of vulnerability for thread-race issues 
is extended timing-wise due to the slow operation under valgrind. 
Normal operating conditions are unlikely to get hit by this, but the
probability is *not zero*.
 
TIA

Comment 13

[ISHIKAWA, Chiaki]

Attachment: Cleaned up patch (already got review+)

OK, the cleaned patch that contains only one additional initialization for
m_booleanOp is here. 

PS: I read SpiderMonkey document (which may be outdated, but
GC can run at the moment a thread among many threads invokes API.
What this means is that if the initialization of a member/variable of an object
is not quite atomic when it is created, GC thread can intervene and may take a look at uninitailized data and may do funny things: one of them might be
a crash caused by accessing strange addresses.
Now that I know many of crashes I got in the last several months were 
caused by invalid memory accesses during GC, I suspect that such failure to
initialize a member variable (that may be checked by GC) could be a cause of some crashes. Just a theory, and I have no idea how many such instances of
failure to initialize object members explicitly at creation time exist.

TIA

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/bf32439d28d4