608818 - nsFilePicker::Show uninitialised use of mAllowURLs

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Doug Turner (:dougt)]

I saw this when testing creating a PDF in fennec:

Saving a PDF to disk, looks like we don't call AppendFilter.


==22492== Conditional jump or move depends on uninitialised value(s)
==22492==    at 0x6B0E51C: nsFilePicker::Show(short*) (nsFilePicker.cpp:453)
==22492==    by 0x6CA7E28: NS_InvokeByIndex_P (xptcinvoke_x86_64_unix.cpp:208)
==22492==    by 0x690628E: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (xpcwrappednative.cpp:3054)
==22492==    by 0x690BD60: XPC_WN_CallMethod(JSContext*, unsigned int, unsigned long*) (xpcwrappednativejsops.cpp:1631)
==22492==    by 0x6F8F50D: js::Interpret(JSContext*, JSStackFrame*, unsigned int, JSInterpMode) (jscntxtinlines.h:684)
==22492==    by 0x6DC9008: js::Invoke(JSContext*, js::CallArgs const&, unsigned int) (jsinterp.cpp:665)
==22492==    by 0x6DC9967: js::ExternalInvoke(JSContext*, js::Value const&, js::Value const&, unsigned int, js::Value*, js::Value*) (jsinterp.cpp:881)
==22492==    by 0x6D5A6B0: JS_CallFunctionValue (jsinterp.h:954)
==22492==    by 0x6676D5F: nsJSContext::CallEventHandler(nsISupports*, void*, void*, nsIArray*, nsIVariant**) (nsJSEnvironment.cpp:2171)
==22492==    by 0x66B4CE9: nsJSEventListener::HandleEvent(nsIDOMEvent*) (nsJSEventListener.cpp:228)
==22492==    by 0x657DE5C: nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsPIDOMEventTarget*, unsigned int, nsCxPusher*) (nsEventListenerManager.cpp:1112)
==22492==    by 0x657E1F8: nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, nsPIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) (nsEventListenerManager.cpp:1208)
==22492==

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Regression from: 

http://hg.mozilla.org/mozilla-central/rev/61b96b507f7c

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 487416 [details] [diff] [review]
patch v.1

glanced over this at dougt's request -- looks fine to me.

AFAICT, we only would want to allow URLs in our FilePicker if it were specifically requested via AppendFilter().  So if AppendFilter() never gets called (as is the case here, per comment 0), then we *don't* want to allow URLs.  

So PR_FALSE makes sense as a default value for mAllowURLs.

r=dholbert, but you might technically need karl or roc (widget GTK owner/peer) to rubber-stamp this.

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 487416 [details] [diff] [review]
patch v.1

low risk - just sets default value to a valid state.

Comment 4

[Doug Turner (:dougt)]

https://bugzilla.mozilla.org/show_bug.cgi?id=608818

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: trivial followup to fix constructor init order

(In comment 4, dougt meant to quote the checkin:)
http://hg.mozilla.org/mozilla-central/rev/b8024b980248

Sadly, this patch was responsible for a silly build warning:
> gtk2/nsFilePicker.h: In constructor ‘nsFilePicker::nsFilePicker()’:
> gtk2/nsFilePicker.h:86: warning: ‘nsFilePicker::mAllowURLs’ will be initialized after
> gtk2/nsFilePicker.h:85: warning:   ‘PRInt16 nsFilePicker::mSelectedType’
> gtk2/nsFilePicker.cpp:195: warning:   when initialized here

(sorry for not having caught that in review)

Attaching trivial followup to fix this warning.

Comment 6

[Daniel Holbert [:dholbert]]

Landed followup: http://hg.mozilla.org/mozilla-central/rev/9ec42a471706