702487 - crash [@ PK11PasswordPromptRunnable::RunOnTargetThread]

Status: RESOLVED FIXED

(Core :: Security, defect, P2)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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 	libxul.so 	PK11PasswordPromptRunnable::RunOnTargetThread 	nsCharTraits.h:378
1 	libxul.so 	PK11PasswordPrompt 	security/manager/ssl/src/nsNSSCallbacks.cpp:827
2 	libnss3.so 	PK11_DoPassword 	pk11auth.c:535
3 	libnss3.so 	PK11_Authenticate 	pk11auth.c:334
4 	libxul.so 	nsSecretDecoderRing::Decrypt 	security/manager/ssl/src/nsSDR.cpp:147
5 	libxul.so 	nsSecretDecoderRing::DecryptString 	security/manager/ssl/src/nsSDR.cpp:212
6 	libxul.so 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:194
7 	libxul.so 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2907
8 	libxul.so 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1553
9 	libxul.so 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
10 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:3948
11 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:584
12 	libxul.so 	js::Invoke 	js/src/jsinterp.cpp:647
13 	libxul.so 	JS_CallFunctionValue 	js/src/jsapi.cpp:5144
14 	libxul.so 	nsXPCWrappedJSClass::CallMethod 	js/xpconnect/src/XPCWrappedJSClass.cpp:1533
15 	libxul.so 	nsXPCWrappedJS::CallMethod 	js/xpconnect/src/XPCWrappedJS.cpp:553
16 	libxul.so 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:131
17 	libxul.so 	libxul.so@0x947d0b 	
18 	libxul.so 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:194
19 	libxul.so 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2907
20 	libxul.so 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1553
21 	libxul.so 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
22 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:3948
23 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:584
24 	libxul.so 	js::Invoke 	js/src/jsinterp.cpp:647
25 	libxul.so 	JS_CallFunctionValue 	js/src/jsapi.cpp:5144
26 	libxul.so 	nsXPCWrappedJSClass::CallMethod 	js/xpconnect/src/XPCWrappedJSClass.cpp:1533
27 	libxul.so 	nsXPCWrappedJS::CallMethod 	js/xpconnect/src/XPCWrappedJS.cpp:553
28 	libxul.so 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:131
29 	libxul.so 	libxul.so@0x947d0b 	
30 	libxul.so 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:194
31 	libxul.so 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2907
32 	libxul.so 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1553
33 	libxul.so 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
34 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:3948
35 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:584
36 	libxul.so 	js::Invoke 	js/src/jsinterp.cpp:647
37 	libxul.so 	JS_CallFunctionValue 	js/src/jsapi.cpp:5144
38 	libxul.so 	nsXPCWrappedJSClass::CallMethod 	js/xpconnect/src/XPCWrappedJSClass.cpp:1533
39 	libxul.so 	nsXPCWrappedJS::CallMethod 	js/xpconnect/src/XPCWrappedJS.cpp:553
40 	libxul.so 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:131
41 	libxul.so 	libxul.so@0x947d0b 	
42 	libxul.so 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:194
43 	libxul.so 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2907
44 	libxul.so 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1553
45 	libxul.so 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
46 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:3948
47 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:584
48 	libxul.so 	js::InvokeGetterOrSetter 	js/src/jsinterp.cpp:647
49 	libxul.so 	js_GetPropertyHelper 	js/src/jsscopeinlines.h:279
50 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:3478
51 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:584
52 	libxul.so 	js::InvokeGetterOrSetter 	js/src/jsinterp.cpp:647
53 	libxul.so 	js_GetPropertyHelper 	js/src/jsscopeinlines.h:279
54 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:3478
55 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:584
56 	libxul.so 	js::InvokeGetterOrSetter 	js/src/jsinterp.cpp:647
57 	libxul.so 	js_GetPropertyHelper 	js/src/jsscopeinlines.h:279
58 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:3478
59 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:584
60 	libxul.so 	js::InvokeGetterOrSetter 	js/src/jsinterp.cpp:647
61 	libxul.so 	js_GetPropertyHelper 	js/src/jsscopeinlines.h:279
62 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:3478
63 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:584
64 	libxul.so 	js::InvokeKernel 	js/src/jsinterp.cpp:647
65 	libxul.so 	js_fun_call 	js/src/jsinterp.h:148
66 	libxul.so 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
67 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:3948
68 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:584
69 	libxul.so 	js::InvokeKernel 	js/src/jsinterp.cpp:647
70 	libxul.so 	js_fun_call 	js/src/jsinterp.h:148
71 	libxul.so 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
72 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:3948
73 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:584
74 	libxul.so 	js::InvokeKernel 	js/src/jsinterp.cpp:647
75 	libxul.so 	js_fun_call 	js/src/jsinterp.h:148
76 	libxul.so 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
77 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:3948
78 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:584
79 	libxul.so 	js::InvokeKernel 	js/src/jsinterp.cpp:647
80 	libxul.so 	js_fun_call 	js/src/jsinterp.h:148
81 	libxul.so 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
82 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:3948
83 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:584
84 	libxul.so 	js::InvokeKernel 	js/src/jsinterp.cpp:647
85 	libxul.so 	js_fun_call 	js/src/jsinterp.h:148
86 	libxul.so 	js::InvokeKernel 	js/src/jscntxtinlines.h:297
87 	libxul.so 	js::Interpret 	js/src/jsinterp.cpp:3948
88 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:584
89 	libxul.so 	js::InvokeKernel 	js/src/jsinterp.cpp:647
90 	libxul.so 	js::CallOrConstructBoundFunction 	js/src/jsinterp.h:148
91 	libxul.so 	js::Invoke 	js/src/jscntxtinlines.h:297
92 	libxul.so 	JS_CallFunctionValue 	js/src/jsapi.cpp:5144
93 	libxul.so 	nsXPCWrappedJSClass::CallMethod 	js/xpconnect/src/XPCWrappedJSClass.cpp:1533
94 	libxul.so 	nsXPCWrappedJS::CallMethod 	js/xpconnect/src/XPCWrappedJS.cpp:553
95 	libxul.so 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:131
96 	libxul.so 	libxul.so@0x947d0b 	
97 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:631

Comment 1

[Cristian Nicolae (:xti)]

Attachment: 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 litmus.mozilla.org > 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: https://crash-stats.mozilla.com/report/index/bp-c2da091e-1f0c-48a2-b67f-fe63c2120109

--
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

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Still occurs in 1/21 build

https://crash-stats.mozilla.com/report/list?product=FennecAndroid&query_search=signature&query_type=contains&query=PK11PasswordPromptRunnable%3A%3ARunOnTargetThread&reason_type=contains&date=01%2F24%2F2012%2015%3A49%3A50&range_value=30&range_unit=days&hang_type=any&process_type=any&do_query=1&signature=PK11PasswordPromptRunnable%3A%3ARunOnTargetThread

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Need regression range; still reproducible.

Comment 4

[Honza Bambas (:mayhemer)]

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().

[1] http://hg.mozilla.org/mozilla-central/file/22f16ec4052a/security/manager/ssl/src/nsNSSCallbacks.cpp#l823

Comment 5

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

still occurs on the aurora nightly 02/07/2012

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Happens since re-implementation 12/22/2012: bug 700527; there's no way to get to the dialog before that.

Comment 7

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Still occurs:

https://crash-stats.mozilla.com/report/list?product=Fennec&product=FennecAndroid&query_search=signature&query_type=contains&query=PK11PasswordPromptRunnable%3A%3ARunOnTargetThread&reason_type=contains&date=02%2F28%2F2012%2023%3A57%3A15&range_value=30&range_unit=days&hang_type=any&process_type=any&do_query=1&admin=1&signature=PK11PasswordPromptRunnable%3A%3ARunOnTargetThread

Comment 8

[Cristian Nicolae (:xti)]

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

Comment 9

[JP Rosevear [:jpr]]

It derefrences a null pointer.  4 crashes in 4 weeks.  Re-noming to see if we should unblock.

Comment 10

[Cristian Nicolae (:xti)]

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

Comment 11

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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?)

Comment 12

[Robert Kaiser]

(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.

Comment 13

[Alex Keybl [:akeybl]]

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.

Comment 14

[Cristian Nicolae (:xti)]

(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.

Comment 15

[Martijn Wargers (dead)]

It just means that this feature is not used much. But it is ridiculously easy to hit this crash when using this feature.

Comment 16

[Alex Keybl [:akeybl]]

Wes - any updates here? We'd like to get a fix into all branches asap.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch

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:

http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/PromptService.java#320

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.

Comment 18

[Wesley Johnston (:wesj)]

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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5191b82f2976

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5191b82f2976

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 643700 [details] [diff] [review]
Patch

[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.

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 643700 [details] [diff] [review]
Patch

[Triage Comment]
Since this is slightly risky, let's start on Aurora 16 and then evaluate for Beta 15 later this week.

Comment 22

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8029ce3d62af

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

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?

Comment 24

[Cristian Nicolae (:xti)]

Cannot access Firefox Settings due to Bug 776909. I will verify this bug when the patch for the mentioned crash will land.

Comment 25

[Cristian Nicolae (:xti)]

(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 26

[Alex Keybl [:akeybl]]

Comment on attachment 643700 [details] [diff] [review]
Patch

[Triage Comment]
Given positive feedback on Aurora, uplifting to Beta 15.

Comment 27

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-beta/rev/fb51324efddf