774964 - calling nsIX509CertDB.importPKCS12File from addon causes hang on Android Native UI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Makoto Kato [:m_kato]]

This nsIX509CertDB.importPKCS12File method worked well on XUL version. But this causes hang on Native UI version.

Although Certification Dialogs is implemented by XUL, it cannot open as modal.  We should remove Certification XUL UIs from Native UI version to be smaller packages. (until implementing Certification Native UI)



Thread 10 (Thread 26458):
#0  0xafd0c868 in __futex_syscall3 ()
   from /home/makoto/android/system/lib/libc.so
#1  0xafd11284 in __pthread_cond_timedwait_relative ()
   from /home/makoto/android/system/lib/libc.so
#2  0xafd11358 in __pthread_cond_timedwait ()
   from /home/makoto/android/system/lib/libc.so
#3  0x48e42aa6 in PR_WaitCondVar (cvar=0x48aadde0, timeout=4294967295)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/nsprpub/pr/src/pthreads/ptsynch.c:385
#4  0x4bccc80a in mozilla::CondVar::Wait (this=0x4ac0e38c, interval=4294967295)
    at /home/makoto/Development/hg.mozilla.org/objdir-mobile-arm/xpcom/build/BlockingResourceBase.cpp:340
#5  0x4bbb1408 in mozilla::ipc::SyncChannel::WaitForNotify (this=0x48b80288)
    at ../../dist/include/mozilla/Monitor.h:47
#6  0x4bbb18b8 in mozilla::ipc::SyncChannel::Send (this=0x48b80288, 
    _msg=<value optimized out>, reply=0x45a3d068)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/ipc/glue/SyncChannel.cpp:95
#7  0x4bbaea6a in mozilla::ipc::RPCChannel::Send (this=0x48b80288, 
    msg=0x48afe280, reply=0x45a3d068)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/ipc/glue/RPCChannel.cp---Type <return> to continue, or q <return> to quit---
p:118
#8  0x4bc68ef6 in mozilla::layers::PCompositorChild::SendPause (
    this=0x48b80280)
    at /home/makoto/Development/hg.mozilla.org/objdir-mobile-arm/ipc/ipdl/PCompositorChild.cpp:213
#9  0x4baef1f8 in nsWindow::OnGlobalAndroidEvent (ae=0x479b5030)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/widget/android/nsWindow.cpp:913
#10 0x4badf7da in nsAppShell::ProcessNextNativeEvent (this=0x45d62660, 
    mayWait=<value optimized out>)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/widget/android/nsAppShell.cpp:618
#11 0x4baf270c in nsBaseAppShell::DoProcessNextNativeEvent (this=0xfffffe00, 
    mayWait=128, recursionDepth=4294967292)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/widget/xpwidgets/nsBaseAppShell.cpp:139
#12 0x4baf292c in nsBaseAppShell::OnProcessNextEvent (this=0x45d62660, 
    thr=0x45d510f0, mayWait=128, recursionDepth=0)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/widget/xpwidgets/nsBaseAppShell.cpp:298
#13 0x4bd03d7c in nsThread::ProcessNextEvent (this=0x45d510f0, mayWait=true, 
    result=0x45a3d41f)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/xpcom/threads/nsThread---Type <return> to continue, or q <return> to quit---
.cpp:586
#14 0x4bccb054 in NS_ProcessNextEvent_P (thread=0xfffffe00, mayWait=true)
    at /home/makoto/Development/hg.mozilla.org/objdir-mobile-arm/xpcom/build/nsThreadUtils.cpp:217
#15 0x4b98af70 in nsXULWindow::ShowModal (this=0x49780d70)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/xpfe/appshell/src/nsXULWindow.cpp:378
#16 0x4b98394e in nsContentTreeOwner::ShowAsModal (this=<value optimized out>)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/xpfe/appshell/src/nsContentTreeOwner.cpp:529
#17 0x4b95e5ee in nsWindowWatcher::OpenWindowJSInternal (
    this=<value optimized out>, aParent=<value optimized out>, 
    aUrl=<value optimized out>, aName=<value optimized out>, 
    aFeatures=0x4c5f3284 "centerscreen,chrome,modal,titlebar", aDialog=true, 
    argv=0x5223e100, aCalledFromJS=false, _retval=0x45a3d7f8)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:999
#18 0x4b95ea0a in nsWindowWatcher::OpenWindow (this=<value optimized out>, 
    aParent=<value optimized out>, aUrl=<value optimized out>, 
    aName=<value optimized out>, 
    aFeatures=0x4c5f3284 "centerscreen,chrome,modal,titlebar", 
    aArguments=0x4a0faac0, _retval=0x45a3d7f8)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/embedding/components/w---Type <return> to continue, or q <return> to quit---
indowwatcher/src/nsWindowWatcher.cpp:381
#19 0x4bad8728 in nsNSSDialogHelper::openDialog (window=<value optimized out>, 
    url=0x4c5f306e "chrome://pippki/content/getp12password.xul", 
    params=0x4a0faac0, modal=<value optimized out>)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/security/manager/pki/src/nsNSSDialogHelper.cpp:44
#20 0x4bad7002 in nsNSSDialogs::GetPKCS12FilePassword (
    this=<value optimized out>, ctx=<value optimized out>, 
    _password=@0x45a3d888, _retval=0x45a3d927)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/security/manager/pki/src/nsNSSDialogs.cpp:404
#21 0x4b9cf0c8 in nsPKCS12Blob::getPKCS12FilePassword (this=0x45a3da18, 
    unicodePw=0x45a3d9b0)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/security/manager/ssl/src/nsPKCS12Blob.cpp:536
#22 0x4b9cf676 in nsPKCS12Blob::ImportFromFileHelper (this=0x45a3da18, 
    file=0x518fc280, aImportMode=nsPKCS12Blob::im_standard_prompt, 
    aWantRetry=@0x45a3d9f0)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/security/manager/ssl/src/nsPKCS12Blob.cpp:161
#23 0x4b9cf8ba in nsPKCS12Blob::ImportFromFile (this=0x45a3da18, 
    file=0x518fc280)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/security/manager/ssl/s---Type <return> to continue, or q <return> to quit---
rc/nsPKCS12Blob.cpp:125
#24 0x4b9e1afe in nsNSSCertificateDB::ImportPKCS12File (
    this=<value optimized out>, aToken=0x0, aFile=0x518fc280)
    at /home/makoto/Development/hg.mozilla.org/mobile-arm/security/manager/ssl/src/nsNSSCertificateDB.cpp:1181

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Bug 807606 implements the NSS dialog interface on android, but I didn't implement this particular function because I mistakenly thought it would never be invoked on mobile. It shouldn't hang/crash any more, but the get password call will just fail. It shouldn't be too hard to implement the function to pop up a password prompt.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Here is a patch that should work but I have no way of testing it. I have pushed a try build to [1]. Makoto, could you try out the build and let me know if it fixes the behaviour for the add-on?

Also, does the setPKCS12FilePassword function need to be implemented?

[1] https://tbpl.mozilla.org/?tree=Try&rev=cf3c3e0836b8

Comment 3

[zebardy]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Created attachment 706881 [details] [diff] [review]
> Patch
> 
> Here is a patch that should work but I have no way of testing it. I have
> pushed a try build to [1]. Makoto, could you try out the build and let me
> know if it fixes the behaviour for the add-on?
> 
> Also, does the setPKCS12FilePassword function need to be implemented?
> 
> [1] https://tbpl.mozilla.org/?tree=Try&rev=cf3c3e0836b8

This bug seems to be what was causing the native port of the cert manager plugin (https://github.com/oernii/cert-manager/tree/native) to crash. Testing the plugin with your patched build solves the crash issue.

Only problem is that the chose your certificate dialogue presented when trying to use the client cert is unresponsive. However I guess that is a different bug.

Comment 4

[Makoto Kato [:m_kato]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Created attachment 706881 [details] [diff] [review]
> Patch
> 
> Here is a patch that should work but I have no way of testing it. I have
> pushed a try build to [1]. Makoto, could you try out the build and let me
> know if it fixes the behaviour for the add-on?
> 
> Also, does the setPKCS12FilePassword function need to be implemented?
> 
> [1] https://tbpl.mozilla.org/?tree=Try&rev=cf3c3e0836b8

Sorry for delay.

When XUL version (until version 10), certificate XPCOM interface and dialog works on android version.  So some developers (includes security solution vendors) created addon for their service.  But after NativeUI version, this API doesn't work and it causes hang up UI.

I (as Mozilla Japan) suggest to overwrite some XPCOM interfaces for certificate for dialogs by their addon.  So their resolve this issue by addon.

If returning NS_ERROR_NOT_IMPLEMENTED on default now, you can resolve as WORKSFORME or WONTFIX due to not hang up.

I think that Fennec (and GAIA) UX team should design mobile UI for certification.  After that, we should implement it for certificate.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Closing as WFM based on comment 4. If I understand correctly the addons in question have been updated to not use the old XUL interface and so this bug doesn't need fixing. Please re-open if this is not the case.

As for comment 3, I don't see anything in the plugin that would cause the attached patch to change behaviour; it might have been fixed by bug 807606 though. If you would like any further changes to the certificate UI implementation over what is in FF21 already please file a bug with clear steps to reproduce and CC me. Thanks.

Comment 6

[zebardy]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> Closing as WFM based on comment 4. If I understand correctly the addons in
> question have been updated to not use the old XUL interface and so this bug
> doesn't need fixing. Please re-open if this is not the case.
> 
> As for comment 3, I don't see anything in the plugin that would cause the
> attached patch to change behaviour; it might have been fixed by bug 807606
> though. If you would like any further changes to the certificate UI
> implementation over what is in FF21 already please file a bug with clear
> steps to reproduce and CC me. Thanks.

When attempting to import a client cert the native fork of the plugin calls importPKCS12File with the relevant cert file against the nsIX509CertDB service. Previous to your patch this caused firefox to crash. With the patch the dialog implemented by this patch is displayed requesting the cert password, and no crash. 

I can see the sense in holding off on the proper implementation of client cert UI components for mobile android until a proper user experience for client certs is designed. 

Until then I'm investigating the suggested solution of overwriting the XPCOM interfaces to supply the UI component (possibly as a Doorhanger) implemented in the patch as part of a plugin. I would be grateful for suggestions (or pointers to a similar example/documentation) for how to override just the getPKCS12FilePassword function within the XPCOM component. I'm guessing create a new component which inherits from original, override the relevant function and register the new component over the original in XPCOM?

I will raise a separate bug for the issue I observed regarding the client certificate selection component.

Thanks for your help :).

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Ah, I misunderstood then. If this patch fixes the crash on the plugin then we should land it. I don't think we should block on a "proper user experience for client certs" because I don't know if such a thing is even being planned.

(In reply to zebardy from comment #3)
> Only problem is that the chose your certificate dialogue presented when
> trying to use the client cert is unresponsive. However I guess that is a
> different bug.

Is this still happening with the patch on this bug? It might be that I messed up the implementation since I wasn't able to test it. Can you provide specific steps to reproduce the behaviour using the addon (i.e. what to do to trigger the call to importPKCS12File)?

Comment 8

[zebardy]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> Ah, I misunderstood then. If this patch fixes the crash on the plugin then
> we should land it. I don't think we should block on a "proper user
> experience for client certs" because I don't know if such a thing is even
> being planned.
> 
> (In reply to zebardy from comment #3)
> > Only problem is that the chose your certificate dialogue presented when
> > trying to use the client cert is unresponsive. However I guess that is a
> > different bug.
> 
> Is this still happening with the patch on this bug? It might be that I
> messed up the implementation since I wasn't able to test it. Can you provide
> specific steps to reproduce the behaviour using the addon (i.e. what to do
> to trigger the call to importPKCS12File)?

This issue is not directly related to the call to importPKCS12File. With the patch the call to importPKCS12File completes successfully. However following a successful import, when browsing to a site which requires the use of the installed client certificate, triggers the browser to display a "This site has requested that you identify yourself with a certificate" component (security/manager/pki/resources/content/clientauthask.xul). This cannot be scrolled to reach the accept button, and the certificate chooser drop down is unresponsive. I'll attach a screenshot as this may help to explain.

Comment 9

[zebardy]

Attachment: Screenshot of client cert selection ui component

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Ah I see. Yes, please file a new bug for that. We should implement a native version of that XUL. I'll try to get this patch landed to keep the ball rolling forward.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

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

Requesting review of this patch; it has been shown to work by zebardy; see discussion above.

Comment 12

[zebardy]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> Ah I see. Yes, please file a new bug for that. We should implement a native
> version of that XUL. I'll try to get this patch landed to keep the ball
> rolling forward.

Thanks,

I have raised https://bugzilla.mozilla.org/show_bug.cgi?id=845375 to cover the issue as suggested.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9873a552eda

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a9873a552eda