Closed Bug 702487 Opened 11 years ago Closed 10 years ago

crash [@ PK11PasswordPromptRunnable::RunOnTargetThread]


(Core :: Security, defect, P2)




Tracking Status
firefox14 - affected
firefox15 + fixed
firefox16 --- verified
blocking-fennec1.0 --- -


(Reporter: nhirata, Assigned: wesj)


(Keywords: crash, qawanted, reproducible, Whiteboard: [native-crash:P4][QA^])

Crash Data


(2 files)

This bug was filed from the Socorro interface and is 
report bp-ea810b4c-1943-4a29-b46a-ca6622111107 .
Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	PK11PasswordPromptRunnable::RunOnTargetThread 	nsCharTraits.h:378
1 	PK11PasswordPrompt 	security/manager/ssl/src/nsNSSCallbacks.cpp:827
2 	PK11_DoPassword 	pk11auth.c:535
3 	PK11_Authenticate 	pk11auth.c:334
4 	nsSecretDecoderRing::Decrypt 	security/manager/ssl/src/nsSDR.cpp:147
5 	nsSecretDecoderRing::DecryptString 	security/manager/ssl/src/nsSDR.cpp:212
6 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:194
7 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2907
8 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1553
9 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
10 	js::Interpret 	js/src/jsinterp.cpp:3948
11 	js::RunScript 	js/src/jsinterp.cpp:584
12 	js::Invoke 	js/src/jsinterp.cpp:647
13 	JS_CallFunctionValue 	js/src/jsapi.cpp:5144
14 	nsXPCWrappedJSClass::CallMethod 	js/xpconnect/src/XPCWrappedJSClass.cpp:1533
15 	nsXPCWrappedJS::CallMethod 	js/xpconnect/src/XPCWrappedJS.cpp:553
16 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:131
18 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:194
19 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2907
20 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1553
21 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
22 	js::Interpret 	js/src/jsinterp.cpp:3948
23 	js::RunScript 	js/src/jsinterp.cpp:584
24 	js::Invoke 	js/src/jsinterp.cpp:647
25 	JS_CallFunctionValue 	js/src/jsapi.cpp:5144
26 	nsXPCWrappedJSClass::CallMethod 	js/xpconnect/src/XPCWrappedJSClass.cpp:1533
27 	nsXPCWrappedJS::CallMethod 	js/xpconnect/src/XPCWrappedJS.cpp:553
28 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:131
30 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:194
31 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2907
32 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1553
33 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
34 	js::Interpret 	js/src/jsinterp.cpp:3948
35 	js::RunScript 	js/src/jsinterp.cpp:584
36 	js::Invoke 	js/src/jsinterp.cpp:647
37 	JS_CallFunctionValue 	js/src/jsapi.cpp:5144
38 	nsXPCWrappedJSClass::CallMethod 	js/xpconnect/src/XPCWrappedJSClass.cpp:1533
39 	nsXPCWrappedJS::CallMethod 	js/xpconnect/src/XPCWrappedJS.cpp:553
40 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:131
42 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:194
43 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2907
44 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1553
45 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
46 	js::Interpret 	js/src/jsinterp.cpp:3948
47 	js::RunScript 	js/src/jsinterp.cpp:584
48 	js::InvokeGetterOrSetter 	js/src/jsinterp.cpp:647
49 	js_GetPropertyHelper 	js/src/jsscopeinlines.h:279
50 	js::Interpret 	js/src/jsinterp.cpp:3478
51 	js::RunScript 	js/src/jsinterp.cpp:584
52 	js::InvokeGetterOrSetter 	js/src/jsinterp.cpp:647
53 	js_GetPropertyHelper 	js/src/jsscopeinlines.h:279
54 	js::Interpret 	js/src/jsinterp.cpp:3478
55 	js::RunScript 	js/src/jsinterp.cpp:584
56 	js::InvokeGetterOrSetter 	js/src/jsinterp.cpp:647
57 	js_GetPropertyHelper 	js/src/jsscopeinlines.h:279
58 	js::Interpret 	js/src/jsinterp.cpp:3478
59 	js::RunScript 	js/src/jsinterp.cpp:584
60 	js::InvokeGetterOrSetter 	js/src/jsinterp.cpp:647
61 	js_GetPropertyHelper 	js/src/jsscopeinlines.h:279
62 	js::Interpret 	js/src/jsinterp.cpp:3478
63 	js::RunScript 	js/src/jsinterp.cpp:584
64 	js::InvokeKernel 	js/src/jsinterp.cpp:647
65 	js_fun_call 	js/src/jsinterp.h:148
66 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
67 	js::Interpret 	js/src/jsinterp.cpp:3948
68 	js::RunScript 	js/src/jsinterp.cpp:584
69 	js::InvokeKernel 	js/src/jsinterp.cpp:647
70 	js_fun_call 	js/src/jsinterp.h:148
71 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
72 	js::Interpret 	js/src/jsinterp.cpp:3948
73 	js::RunScript 	js/src/jsinterp.cpp:584
74 	js::InvokeKernel 	js/src/jsinterp.cpp:647
75 	js_fun_call 	js/src/jsinterp.h:148
76 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
77 	js::Interpret 	js/src/jsinterp.cpp:3948
78 	js::RunScript 	js/src/jsinterp.cpp:584
79 	js::InvokeKernel 	js/src/jsinterp.cpp:647
80 	js_fun_call 	js/src/jsinterp.h:148
81 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
82 	js::Interpret 	js/src/jsinterp.cpp:3948
83 	js::RunScript 	js/src/jsinterp.cpp:584
84 	js::InvokeKernel 	js/src/jsinterp.cpp:647
85 	js_fun_call 	js/src/jsinterp.h:148
86 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
87 	js::Interpret 	js/src/jsinterp.cpp:3948
88 	js::RunScript 	js/src/jsinterp.cpp:584
89 	js::InvokeKernel 	js/src/jsinterp.cpp:647
90 	js::CallOrConstructBoundFunction 	js/src/jsinterp.h:148
91 	js::Invoke 	js/src/jscntxtinlines.h:297
92 	JS_CallFunctionValue 	js/src/jsapi.cpp:5144
93 	nsXPCWrappedJSClass::CallMethod 	js/xpconnect/src/XPCWrappedJSClass.cpp:1533
94 	nsXPCWrappedJS::CallMethod 	js/xpconnect/src/XPCWrappedJS.cpp:553
95 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:131
97 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:631
Whiteboard: [native-crash] → [native-crash], str-wanted
Whiteboard: [native-crash], str-wanted → [native-crash:P4], str-wanted
Attached file log
I was able to reproduce this crash on the latest Nightly build.

Steps to reproduce:
1. Open Fennec App
2. Go to Settings and set a Master Password
3. Restart app
4. Go to > Log in
5. Fill in email and password fields and tap on Login button
6. When Master Password popup is triggered, just tap OK button

Expected result:
OK button should be grayed out. However, even if OK button is still active, Master Password popup will be triggered again until the correct password is typed. No crash should occur after step 6.

Actual result:
After step 6, Fennec crashes. Please see the attached log.

Crash report:

Mozilla/5.0 (Android;Linux armv7l;rv:12.0a1)Gecko/20120108
Firefox/12.0a1 Fennec/12.0a1
Devices: Samsung Galaxy S
OS: Android 2.2
Whiteboard: [native-crash:P4], str-wanted → [native-crash:P4]
Version: 10 Branch → 12 Branch
Need regression range; still reproducible.
According the crash report the crashing thread is not the main thread but thread number 8.  Main thread (0) is just sleeping - I assume on arm thread 0 is the main thread...

But at [1] we still call runnable->RunOnTargetThread().

still occurs on the aurora nightly 02/07/2012
Happens since re-implementation 12/22/2012: bug 700527; there's no way to get to the dialog before that.
This issue is still reproducing on the latest Nightly (03/26). Imo, there are enough changes that an user to tap just OK button when Master Password prompt dialog is triggered. We should avoid this kind of crashes when security elements are involved such Master Password. It feels like a vulnerability
blocking-fennec1.0: --- → ?
Version: 12 Branch → Trunk
Assignee: nobody → wjohnston
blocking-fennec1.0: ? → +
It derefrences a null pointer.  4 crashes in 4 weeks.  Re-noming to see if we should unblock.
blocking-fennec1.0: + → ?
Whiteboard: [native-crash:P4] → [native-crash:P4], startupcrash
Whiteboard: [native-crash:P4], startupcrash → [native-crash:P4]
This crash still occurs on the latest Nightly and it's always reproducible if steps from comment #1 are performed. 

Firefox 14.0a1 (2012-04-04)
Device: HTC Desire Z
OS: Android 2.3.3
blocking-fennec1.0: ? → soft
Priority: -- → P2
Whiteboard: [native-crash:P4] → [native-crash:P4][QA^]
I believe you have to have a blank dialog for the master password in order to get this crash.  Even though it's reproducible, the chances of someone placing in a blank master password is a lot lower.  (though, the fix should just be a null check right?)
(In reply to Naoki Hirata :nhirata from comment #11)
> I believe you have to have a blank dialog for the master password in order
> to get this crash.  Even though it's reproducible, the chances of someone
> placing in a blank master password is a lot lower.

FTR, changing an existing master password to blank is the preferred method to removing the master password, so this is a case that should work.
blocking-fennec1.0: soft → -
This is a low volume crasher, so I'm curious why we think this needs to track for 15/16 release. Is this affecting testing or our users significantly? It's not clear to me that comment 1 is a common user scenario.
(In reply to Alex Keybl [:akeybl] from comment #13)
> This is a low volume crasher, so I'm curious why we think this needs to
> track for 15/16 release. Is this affecting testing or our users
> significantly? It's not clear to me that comment 1 is a common user scenario.

Yes, it is. Actually, when master password popup is triggered, just tap the OK button to reproduce this crash.
It just means that this feature is not used much. But it is ridiculously easy to hit this crash when using this feature.
Assignee: wjohnston → nobody
Wes - any updates here? We'd like to get a fix into all branches asap.
Assignee: nobody → wjohnston
Attached patch PatchSplinter Review
Sorry! Hadn't seen this. This should fix this, plus some other potential problems. When users cancel out of an dialog, we should hit onCancel:

which should mean we don't return anything, but we don't change the entered values in that case anyway.

If they hit OK, with this we will return the values entered regardless of what they are. Before we only returned them if they were truthy.

CheckState.value is independent of that I think? i.e. If it says "Remember this choice" callers likely want to remember that you hit cancel.
Attachment #643700 - Flags: review?(mark.finkle)
Attachment #643700 - Flags: review?(mark.finkle) → review+
I'm nervous about pushing this forward until its had a little bake time and QA has looked at it. I want to hold off on nom'ing.
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment on attachment 643700 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 694455.
User impact if declined: Crashes in this case because we're talking to Gecko code that hates the null response. Most cases web pages will just receive erroneous information.
Testing completed (on m-c, etc.): Been on mc a week.
Risk to taking this patch (and alternatives if risky): Mobile prompt service only. Involves correcting the prompt service behavior and since that interacts with lots of Gecko and web pages, its slightly risky IMO.
String or UUID changes made by this patch: None.
Attachment #643700 - Flags: approval-mozilla-beta?
Attachment #643700 - Flags: approval-mozilla-aurora?
Comment on attachment 643700 [details] [diff] [review]

[Triage Comment]
Since this is slightly risky, let's start on Aurora 16 and then evaluate for Beta 15 later this week.
Attachment #643700 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Adding qawanted - can someone run the steps in comment 1 on Aurora so we can determine if this is verified fixed before consideration for Beta?
Keywords: qawanted
Cannot access Firefox Settings due to Bug 776909. I will verify this bug when the patch for the mentioned crash will land.
(In reply to Lukas Blakk [:lsblakk] from comment #23)
> Adding qawanted - can someone run the steps in comment 1 on Aurora so we can
> determine if this is verified fixed before consideration for Beta?

I cannot reproduce this crash on the latest Aurora build.

Firefox 16.0a2 (2012-07-25)
Device: Acer Iconia TAB A500
OS: Android 3.2
Comment on attachment 643700 [details] [diff] [review]

[Triage Comment]
Given positive feedback on Aurora, uplifting to Beta 15.
Attachment #643700 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.