Closed Bug 912935 Opened 11 years ago Closed 11 years ago

mWALModeEnable is used uninitialized (in thunderbird)

Categories

(Core :: DOM: Core & HTML, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: ishikawa, Assigned: ishikawa)

Details

Attachments

(1 file, 2 obsolete files)

I found that a member variable for DOMStorageDBThread, mWALModeEnabled
is used uninitialized during the execution of DEBUG BUILD of thunderbird
under valgrind.

I think proper patch would be something like the attached, but I am not sure.

The use of valgrind slows down the execution of the program quite a bit, thus
widens the windows of vulnerability between the time an object is created and
its members get properly initialized.
So maybe there are some thread-race issues that may use mWALModeEnabled before proper setting.
Although MWALModeEnabled is set in TryJournalMode(), mWALModeEnabled gets referenced by SyncPreload before that, I think under some conditions.

This happened only once in the run of |make mozmill| test suite of thunderbird under 64 bit Debian GNU/Linux.

It may or not may happen in other runs (due to the nature of thread races).

TIA

PS: If I put MWALModeEnabled(false) before mDBREady(false) [above the current position in the attached pach ], I get a warning about initialization reorder. A good reminder that the initialization is done in the declaration order.
Attachment #800064 - Flags: review?(honzab.moz)
Sorry, here is the excerpt of the session log that shows the uninitialized use :
Line 145 is DOMStorageDBThread.cpp is


142   // Bypass sync load when an update is pending in the queue to write, we would
143   // get incosistent data in the cache.  Also don't allow sync main-thread preload
144   // when DB open and init is still pending on the background thread.
145   if (mWALModeEnabled && mDBReady) {
146     bool pendingTasks;

mDBReady is already initialized to false upon creation.
mWALModeEnabled was not. It is only set within the call to TryJournalMode().


==31341== Conditional jump or move depends on uninitialised value(s)
==31341==    at 0x7A751C2: mozilla::dom::DOMStorageDBThread::SyncPreload(mozilla::dom::DOMStorageCacheBridge*, bool) (DOMStorageDBThread.cpp:145)
==31341==    by 0x7A697C4: mozilla::dom::DOMStorageCache::WaitForPreload(mozilla::Telemetry::ID) (DOMStorageCache.cpp:345)
==31341==    by 0x7A69F9D: mozilla::dom::DOMStorageCache::GetItem(mozilla::dom::DOMStorage const*, nsAString_internal const&, nsAString_internal&) (DOMStorageCache.cpp:452)
==31341==    by 0x7A68155: mozilla::dom::DOMStorage::GetItem(nsAString_internal const&, nsAString_internal&) (DOMStorage.cpp:90)
==31341==    by 0x7FA4B81: nsIDOMStorage_GetItem(JSContext*, unsigned int, JS::Value*) (dom_quickstubs.cpp:693)
==31341==    by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219)
==31341==    by 0x9EC4F90: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:489)
==31341==    by 0x9ECBCA4: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2484)
==31341==    by 0x9ECE813: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:446)
==31341==    by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508)
==31341==    by 0xA07A6E1: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsfun.cpp:1026)
==31341==    by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219)
==31341==    by 0x9EC4F90: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:489)
==31341==    by 0x9ECBCA4: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2484)
==31341==    by 0x9ECE813: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:446)
==31341==    by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508)
==31341==    by 0x9ECF3BF: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:539)
==31341==    by 0xA00586F: JS_CallFunctionValue(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::Value*) (jsapi.cpp:5411)
==31341==    by 0x7F7EBF2: nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (XPCWrappedJSClass.cpp:1445)
==31341==    by 0x7F76678: nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (XPCWrappedJS.cpp:587)
==31341==    by 0x9503AE9: PrepareAndDispatch (xptcstubs_x86_64_linux.cpp:122)
==31341==    by 0x9502EF6: SharedStub (in /TEST-MAIL-DIR/objdir-tb3/mozilla/toolkit/library/libxul.so)
==31341==    by 0x763CE29: nsEventListenerManager::HandleEventSubType(nsListenerStruct*, mozilla::dom::CallbackObjectHolder<mozilla::dom::EventListener, nsIDOMEventListener> const&, nsIDOMEvent*, mozilla::dom::EventTarget*, nsCxPusher*) (nsEventListenerManager.cpp:977)
==31341==    by 0x763D14A: nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*, nsCxPusher*) (nsEventListenerManager.cpp:1048)
==31341==    by 0x763532D: nsEventTargetChainItem::HandleEventTargetChain(nsTArray<nsEventTargetChainItem>&, nsEventChainPostVisitor&, nsDispatchingCallback*, ELMCreationDetector&, nsCxPusher*) (nsEventListenerManager.h:330)
==31341==    by 0x7638265: nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*) (nsEventDispatcher.cpp:605)
==31341==    by 0x7638E0F: nsEventDispatcher::DispatchDOMEvent(nsISupports*, nsEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) (nsEventDispatcher.cpp:669)
==31341==    by 0x75260CF: nsINode::DispatchEvent(nsIDOMEvent*, bool*) (nsINode.cpp:1140)
==31341==    by 0x746A427: nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool, bool*) (nsContentUtils.cpp:3348)
==31341==    by 0x746A511: nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool*) (nsContentUtils.cpp:3319)
==31341==    by 0x74C1B41: nsDocument::DispatchContentLoadedEvents() (nsDocument.cpp:4639)
==31341==    by 0x74DE6F3: nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run() (nsThreadUtils.h:363)
==31341==    by 0x94D6CDC: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:622)
==31341==    by 0x94674B5: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:238)
==31341==    by 0x80E5F51: nsXULWindow::ShowModal() (nsXULWindow.cpp:365)
==31341==    by 0x80D3335: nsContentTreeOwner::ShowAsModal() (nsContentTreeOwner.cpp:533)
==31341==    by 0x8084176: nsWindowWatcher::OpenWindowInternal(nsIDOMWindow*, char const*, char const*, char const*, bool, bool, bool, nsIArray*, nsIDOMWindow**) (nsWindowWatcher.cpp:998)
==31341==    by 0x8084D7F: nsWindowWatcher::OpenWindow2(nsIDOMWindow*, char const*, char const*, char const*, bool, bool, bool, nsISupports*, nsIDOMWindow**) (nsWindowWatcher.cpp:416)
==31341==    by 0x797202D: nsGlobalWindow::OpenInternal(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, bool, bool, bool, bool, bool, nsIArray*, nsISupports*, nsIPrincipal*, JSContext*, nsIDOMWindow**) (nsGlobalWindow.cpp:10086)
==31341==    by 0x7971666: nsGlobalWindow::OpenInternal(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, bool, bool, bool, bool, bool, nsIArray*, nsISupports*, nsIPrincipal*, JSContext*, nsIDOMWindow**) (nsGlobalWindow.cpp:9969)
==31341==    by 0x7972F39: nsGlobalWindow::OpenDialog(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsIDOMWindow**) (nsGlobalWindow.cpp:6530)
==31341==    by 0x9502DC6: NS_InvokeByIndex (xptcinvoke_x86_64_unix.cpp:164)
==31341==    by 0x7F8D930: CallMethodHelper::Call() (XPCWrappedNative.cpp:2809)
==31341==    by 0x7F89C58: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2115)
==31341==    by 0x7F9928E: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1316)
==31341==    by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219)
==31341==    by 0x9EC4F90: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:489)
==31341==    by 0x9ECBCA4: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2484)
==31341==    by 0x9ECE813: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:446)
==31341==    by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508)
==31341==  Uninitialised value was created by a heap allocation
==31341==    at 0x402A914: malloc (vg_replace_malloc.c:291)
==31341==    by 0x404633B: moz_xmalloc (mozalloc.cpp:54)
==31341==    by 0x7A6ABF3: mozilla::dom::DOMStorageCache::StartDatabase() (mozalloc.h:201)
==31341==    by 0x7A6AD2B: mozilla::dom::DOMStorageCache::Preload() (DOMStorageCache.cpp:230)
==31341==    by 0x7A6AF18: mozilla::dom::DOMStorageCache::Init(mozilla::dom::DOMStorageManager*, bool, nsIPrincipal*, nsACString_internal const&) (DOMStorageCache.cpp:137)
==31341==    by 0x7A7BB41: mozilla::dom::DOMStorageManager::PutCache(nsACString_internal const&, nsIPrincipal*) (DOMStorageManager.cpp:291)
==31341==    by 0x7A7C5AD: mozilla::dom::DOMStorageManager::GetStorageInternal(bool, nsIPrincipal*, nsAString_internal const&, bool, nsIDOMStorage**) (DOMStorageManager.cpp:352)
==31341==    by 0x7A7C734: mozilla::dom::DOMStorageManager::CreateStorage(nsIPrincipal*, nsAString_internal const&, bool, nsIDOMStorage**) (DOMStorageManager.cpp:379)
==31341==    by 0x7A793F8: mozilla::dom::DOMStorageManager::GetLocalStorageForPrincipal(nsIPrincipal*, nsAString_internal const&, bool, nsIDOMStorage**) (DOMStorageManager.cpp:465)
==31341==    by 0x9502DC6: NS_InvokeByIndex (xptcinvoke_x86_64_unix.cpp:164)
==31341==    by 0x7F8D930: CallMethodHelper::Call() (XPCWrappedNative.cpp:2809)
==31341==    by 0x7F89C58: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2115)
==31341==    by 0x7F9928E: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1316)
==31341==    by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219)
==31341==    by 0x9EC4F90: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:489)
==31341==    by 0x9ECBCA4: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2484)
==31341==    by 0x9ECE813: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:446)
==31341==    by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508)
==31341==    by 0xA07A1D1: js_fun_call(JSContext*, unsigned int, JS::Value*) (jsfun.cpp:873)
==31341==    by 0xA07A60D: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsfun.cpp:912)
==31341==    by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219)
==31341==    by 0x9EC4F90: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:489)
==31341==    by 0x9ECF3BF: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:539)
==31341==    by 0xA12201A: js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jsproxy.cpp:445)
==31341==    by 0xA18256E: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jswrapper.cpp:438)
==31341==    by 0xA122B2C: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jsproxy.cpp:2626)
==31341==    by 0xA122C64: proxy_Call(JSContext*, unsigned int, JS::Value*) (jsproxy.cpp:3203)
==31341==    by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219)
==31341==    by 0x9EC50B3: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:482)
==31341==    by 0x9ECBCA4: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2484)
==31341==    by 0x9ECE813: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:446)
==31341==    by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508)
==31341==    by 0x9ECF3BF: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:539)
==31341==    by 0xA12201A: js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jsproxy.cpp:445)
==31341==    by 0xA18256E: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jswrapper.cpp:438)
==31341==    by 0xA122B2C: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jsproxy.cpp:2626)
==31341==    by 0xA122C64: proxy_Call(JSContext*, unsigned int, JS::Value*) (jsproxy.cpp:3203)
==31341==    by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219)
==31341==    by 0x9EC50B3: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:482)
==31341==    by 0x9ECF3BF: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:539)
==31341==    by 0x9ECF5C0: js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:610)
==31341==    by 0xA0E2585: bool NativeGetInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::HandleType, unsigned int, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) (Shape-inl.h:256)
==31341==    by 0xA0E33BF: bool GetPropertyHelperInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, unsigned int, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) (jsobj.cpp:4242)
==31341==    by 0xA0E3853: js::GetPropertyHelper(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, unsigned int, JS::MutableHandle<JS::Value>) (jsobj.cpp:4251)
==31341==    by 0x9ED6A39: GetPropertyOperation(JSContext*, js::StackFrame*, JS::Handle<JSScript*>, unsigned char*, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) (Interpreter.cpp:292)
==31341==    by 0x9EC77C8: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2323)
==31341==    by 0x9ECE813: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:446)
==31341==    by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508)
==31341==    by 0xA07A6E1: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsfun.cpp:1026)
==31341==    by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219)
Comment on attachment 800064 [details] [diff] [review]
initialize mWALModeEnabled to false upon creation of  DOMStorageDBThread::DOMStorageDBThread()

I think the more proper way is to reverse the condition

if (mWALModeEnabled && mDBReady) {

to 

if (mDBReady && mWALModeEnabled) {
Attachment #800064 - Flags: review?(honzab.moz) → review-
Here is an updated patch.
I think it doesn't hurt to initialize the value upon creation. 
So I keep the initialization, and changed the order of variable accesses in the
if expression.

TIA
Attachment #802094 - Flags: review?(honzab.moz)
Attachment #800064 - Attachment is obsolete: true
Assignee: nobody → ishikawa
Attachment #802094 - Attachment description: initialize mWALModeEnabled to false upon creation of DOMStorageDBThread::DOMStorageDBThread() and also reorder of access in an if-expression. → initialize mWALModeEnabled to false upon creation of DOMStorageDBThread::DOMStorageDBThread() and also reorder of access in an if-expression. (Take Two)
Comment on attachment 802094 [details] [diff] [review]
initialize mWALModeEnabled to false upon creation of DOMStorageDBThread::DOMStorageDBThread() and also reorder of access in an if-expression. (Take Two)

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

Thanks.

::: dom/src/storage/DOMStorageDBThread.cpp
@@ +57,5 @@
>  DOMStorageDBThread::DOMStorageDBThread()
>  : mThread(nullptr)
>  , mMonitor("DOMStorageThreadMonitor")
>  , mStopIOThread(false)
> +, mWALModeEnabled(false)        // valgrind caught uninitialized usage.

I think you don't need the comment.
Attachment #802094 - Flags: review?(honzab.moz) → review+
Attachment #802164 - Attachment description: initialize mWALModeEnabled to false upon creation of DOMStorageDBThread::DOMStorageDBThread() and also reorder of access in an if-expression. (Take Three) [removed unnecessary comment] → initialize mWALModeEnabled to false upon creation of DOMStorageDBThread::DOMStorageDBThread() and also reorder of access in an if-expression. (Take Three) [removed unnecessary comment] (already review+)
Attachment #802094 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/67dfa713bd14
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: