Last Comment Bug 702487 - crash [@ PK11PasswordPromptRunnable::RunOnTargetThread]
: crash [@ PK11PasswordPromptRunnable::RunOnTargetThread]
: crash, qawanted, reproducible
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: ARM Android
P2 critical (vote)
: mozilla17
Assigned To: Wesley Johnston (:wesj)
: David Keeler [:keeler] (use needinfo?)
Depends on:
  Show dependency treegraph
Reported: 2011-11-14 16:59 PST by Naoki Hirata :nhirata (please use needinfo instead of cc)
Modified: 2012-07-26 13:33 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

log (91.22 KB, text/plain)
2012-01-09 01:53 PST, Cristian Nicolae (:xti)
no flags Details
Patch (4.36 KB, patch)
2012-07-18 18:56 PDT, Wesley Johnston (:wesj)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-11-14 16:59:12 PST
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
Comment 1 User image Cristian Nicolae (:xti) 2012-01-09 01:53:32 PST
Created attachment 586932 [details]

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
Comment 3 User image Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-01-24 16:01:08 PST
Need regression range; still reproducible.
Comment 4 User image Honza Bambas (:mayhemer) 2012-01-24 16:33:42 PST
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().

Comment 5 User image Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-02-07 09:39:03 PST
still occurs on the aurora nightly 02/07/2012
Comment 6 User image Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-02-09 15:09:47 PST
Happens since re-implementation 12/22/2012: bug 700527; there's no way to get to the dialog before that.
Comment 8 User image Cristian Nicolae (:xti) 2012-03-26 08:08:18 PDT
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 User image JP Rosevear [:jpr] 2012-04-04 19:29:43 PDT
It derefrences a null pointer.  4 crashes in 4 weeks.  Re-noming to see if we should unblock.
Comment 10 User image Cristian Nicolae (:xti) 2012-04-05 00:45:31 PDT
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 User image Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-05-15 08:06:41 PDT
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 User image Robert Kaiser 2012-05-15 12:13:13 PDT
(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 User image Alex Keybl [:akeybl] 2012-06-19 11:19:28 PDT
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 User image Cristian Nicolae (:xti) 2012-06-20 01:49:43 PDT
(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 User image Martijn Wargers [:mwargers] 2012-06-20 02:06:12 PDT
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 User image Alex Keybl [:akeybl] 2012-07-16 08:07:57 PDT
Wes - any updates here? We'd like to get a fix into all branches asap.
Comment 17 User image Wesley Johnston (:wesj) 2012-07-18 18:56:30 PDT
Created attachment 643700 [details] [diff] [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.
Comment 18 User image Wesley Johnston (:wesj) 2012-07-19 12:10:55 PDT
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.
Comment 19 User image Ed Morley [:emorley] 2012-07-20 06:45:19 PDT
Comment 20 User image Wesley Johnston (:wesj) 2012-07-23 15:29:47 PDT
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.
Comment 21 User image Alex Keybl [:akeybl] 2012-07-23 15:34:38 PDT
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.
Comment 22 User image Wesley Johnston (:wesj) 2012-07-23 17:59:49 PDT
Comment 23 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-07-24 13:51:20 PDT
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 User image Cristian Nicolae (:xti) 2012-07-25 02:03:53 PDT
Cannot access Firefox Settings due to Bug 776909. I will verify this bug when the patch for the mentioned crash will land.
Comment 25 User image Cristian Nicolae (:xti) 2012-07-26 00:46:20 PDT
(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 User image Alex Keybl [:akeybl] 2012-07-26 10:00:25 PDT
Comment on attachment 643700 [details] [diff] [review]

[Triage Comment]
Given positive feedback on Aurora, uplifting to Beta 15.
Comment 27 User image Wesley Johnston (:wesj) 2012-07-26 12:28:12 PDT

Note You need to log in before you can comment on or make changes to this bug.