Bugzilla (bugzilla.redhat.com) will be under maintenance for infrastructure upgrades and will not be available on July 31st between 12:30 AM - 05:30 AM UTC. We appreciate your understanding and patience. You can follow status.redhat.com for details.
Bug 459297 - (curl NSS) Firefox crash during Flash 10 teardown
Summary: (curl NSS) Firefox crash during Flash 10 teardown
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: firefox
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Warren Togami
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-08-15 20:05 UTC by Warren Togami
Modified: 2018-04-11 17:09 UTC (History)
8 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-09-11 17:16:38 UTC
Type: ---


Attachments (Terms of Use)
Add locking around NSS_Initialize() (1.54 KB, patch)
2008-09-03 13:05 UTC, Rob Crittenden
no flags Details | Diff
Also lock during cleanup to make it thread-safe (1.68 KB, patch)
2008-09-03 20:39 UTC, Rob Crittenden
no flags Details | Diff

Description Warren Togami 2008-08-15 20:05:16 UTC
Firefox can crash during Flash 10 teardown, either reloading the page or closing the browser.  Please help to isolate the bug to firefox or Flash.  If the bug is in Flash, then assign it to me.

firefox-3.0.1-1.fc9.i386
xulrunner-1.9.0.1-1.fc9.windowless.i386
flash-plugin-10.0.0.569-release.i386
(NO NSPLUGINWRAPPER)

1) mkdir /etc/adobe
2) echo "WindowlessDisable=true" > /etc/adobe/mms.cfg
This disables the WMODE that can cause firefox to crash due to Mozilla Bug 435764.  This other crash seems to be unrelated.
3) http://www.intel.com/business/business-pc/demo/demo.htm?iid=itopia+cmt+security+vpro_demo
4) "Close" the window, and firefox crashes on the way down.

Crashes in exactly the same place in upstream's firefox-3.0.1 binary.

#0  NSSRWLock_LockRead_Util (rwlock=<value optimized out>) at nssrwlk.c:177
#1  0x021052cd in SECMOD_GetReadLock (modLock=Could not find the frame base for "SECMOD_GetReadLock".
) at pk11list.c:71
#2  0x0211d972 in SECMOD_FindModule (name=<value optimized out>) at pk11util.c:226
#3  0x02c5a190 in nsNSSComponent::UnloadLoadableRoots (this=<value optimized out>) at nsNSSComponent.cpp:838
#4  0x02c5b051 in nsNSSComponent::ShutdownNSS (this=<value optimized out>) at nsNSSComponent.cpp:1686
#5  0x02c5b0fe in nsNSSComponent::DoProfileBeforeChange (this=<value optimized out>, aSubject=<value optimized out>) at nsNSSComponent.cpp:2388
#6  0x02c5b387 in nsNSSComponent::Observe (this=<value optimized out>, aSubject=<value optimized out>, aTopic=<value optimized out>, someData=<value optimized out>)
    at nsNSSComponent.cpp:1972
#7  0x02e0d79d in nsObserverList::NotifyObservers (this=<value optimized out>, aSubject=<value optimized out>, aTopic=<value optimized out>, someData=<value optimized out>)
    at nsObserverList.cpp:128
#8  0x02e0db0e in nsObserverService::NotifyObservers (this=<value optimized out>, aSubject=Could not find the frame base for "nsObserverService::NotifyObservers(nsISupports*, char const*, unsigned short const*)".
) at nsObserverService.cpp:181
#9  0x025ffdac in nsXREDirProvider::DoShutdown (this=<value optimized out>) at nsXREDirProvider.cpp:853
#10 0x025fa8bb in ~ScopedXPCOMStartup (this=<value optimized out>) at nsAppRunner.cpp:905
#11 0x025fcfa9 in XRE_main (argc=<value optimized out>, argv=<value optimized out>, aAppData=<value optimized out>) at nsAppRunner.cpp:3211
#12 0x08049169 in main (argc=1, argv=0xbf9b1474) at nsXULStub.cpp:364

With NSPLUGINWRAPPER
====================
No crash during firefox close, but if you click reload a few times you can see the plugin viewer fail.

*** CLICK RELOAD ***
(npviewer.bin:7223): Gtk-CRITICAL **: gtk_style_detach: assertion `style->attach_count > 0' failed
(npviewer.bin:7223): Gtk-CRITICAL **: gtk_widget_destroy: assertion `GTK_IS_WIDGET (widget)' failed

*** CLICK RELOAD ***
(npviewer.bin:7223): Gtk-CRITICAL **: gtk_style_detach: assertion `style->attach_count > 0' failed
(npviewer.bin:7223): Gtk-CRITICAL **: gtk_widget_destroy: assertion `GTK_IS_WIDGET (widget)' failed

*** CLICK RELOAD ***
(npviewer.bin:7223): Gtk-CRITICAL **: gtk_style_detach: assertion `style->attach_count > 0' failed
(npviewer.bin:7223): Gtk-CRITICAL **: gtk_widget_destroy: assertion `GTK_IS_WIDGET (widget)' failed
*** NSPlugin Wrapper *** ERROR: passing an unknown instance
*** NSPlugin Wrapper *** ERROR: NPP_NewStream() invoke: Connection reset by peer

Comment 1 Matěj Cepl 2008-09-01 11:22:31 UTC
Do we support flash without nspluginwrapper?

Comment 2 Warren Togami 2008-09-01 17:18:02 UTC
As an option yes.  The distro allows you to use firefox with native plugins if you uninstall nspluginwrapper.

https://bugzilla.mozilla.org/show_bug.cgi?id=427715
https://bugzilla.mozilla.org/show_bug.cgi?id=450468
These two NSS bugs seem to be related to this particular crash.  It fails similarly with native Firefox and nspluginwrapper.  So this might be a bug not in Flash.

Comment 3 Kai Engert (:kaie) (inactive account) 2008-09-02 23:16:16 UTC
This seems to be caused by multiple calls to NSS_Init, NSS_Shutdown, partly triggered by the flash plugin (init), partly triggered by libcurl, which always calls NSS_Shutdown unconditionally during cleanup.

Comment 4 Kai Engert (:kaie) (inactive account) 2008-09-02 23:18:03 UTC
Rob C: 

Does libcurl detect, whether NSS has already been initialized?
If it's already initialized, does it avoid the call to NSS_Init* ?

That would be good.

In addition, it would be helpful if the same bool were used to avoid calling NSS_Shutdown.

Comment 5 Warren Togami 2008-09-03 00:18:52 UTC
> In addition, it would be helpful if the same bool were used to avoid calling
> NSS_Shutdown.

/* Global cleanup */
void Curl_nss_cleanup(void)
{
  NSS_Shutdown();
  initialized = 0;
}

If a call to NSS_IsInitialized() is added prior to these two lines, is there a way to lock to prevent the tiny potential of another thread racing between?

Comment 6 Rob Crittenden 2008-09-03 01:35:26 UTC
libcurl is actually not smart enough to detect that NSS is already initialized. NSS is detecting this itself in nss_Init():

I think this is the right fix (untested). It will call NSS_Shutdown() only if libcurl did the initialization:

--- curl-7.18.2.orig/lib/nss.c  2008-05-26 11:02:49.000000000 -0400
+++ curl-7.18.2/lib/nss.c       2008-09-02 21:32:27.000000000 -0400
@@ -730,7 +730,8 @@
 /* Global cleanup */
 void Curl_nss_cleanup(void)
 {
-  NSS_Shutdown();
+  if (initialized)
+    NSS_Shutdown();
   initialized = 0;
 }
 
@@ -805,7 +806,7 @@
   curlerr = CURLE_SSL_CONNECT_ERROR;
 
   /* FIXME. NSS doesn't support multiple databases open at the same time. */
-  if(!initialized) {
+  if(!initialized && !NSS_IsInitialized()) {
     initialized = 1;
 
     certDir = getenv("SSL_DIR"); /* Look in $SSL_DIR */

Comment 7 Warren Togami 2008-09-03 08:17:25 UTC
This is exactly what I was hinting at.  Doing an if conditional prior to NSS_Shutdown() in libcurl does not guarantee that initialized could change between testing it and NSS_Shutdown().  Isn't there still the potential for racy behavior here?

It seems that NSS internally must do locking, so NSS_Init and NSS_Shutdown cannot happen simultaneously in multiple threads?

Hmm, looking at libcurl-*/lib/ssluse.c it appears that the old OpenSSL equivalent could be racy as well?  (This is important for RHEL5 support.)

Comment 8 Rob Crittenden 2008-09-03 12:02:51 UTC
Probably only in a misbehaving libcurl app. curl_global_cleanup() which ends up calling NSS_Shutdown() is only supposed to be called when an an application exits, not between SSL requests.

NSS_Shutdown() isn't equivalent to the OpenSSL SSL_shutdown() function. 

SSL_shutdown is a per-request function that closes down an SSL connection. 

NSS_Shutdown() unloads NSS.

I think the bigger threat is multiple calls to NSS_Initialize() in a threaded application.

I'll take a crack at creating a lock in Curl_nss_init() that we can use.

Comment 9 Rob Crittenden 2008-09-03 13:05:44 UTC
Created attachment 315638 [details]
Add locking around NSS_Initialize()

curl_global_init() and curl_global_cleanup() are both documented as not thread-safe so these new conditionals in Curl_nss_init() and Curl_nss_cleanup() are ok.

Note that there is a somewhat subtle side-effect here.

If NSS is initialized outside of libcurl then it won't be re-initialized here even if if the global variable initialized is not set because we also check NSS_IsInitialized().

NSS_Shutdown() will not be called if the global initialized is not set. So if NSS is initialized outside of libcurl it needs to be shut down there as well.

Comment 10 Warren Togami 2008-09-03 16:13:09 UTC
> -  NSS_Shutdown();
> +  if (initialized)
> +    NSS_Shutdown();

This isn't protected by the lock.  Isn't there a chance of it being pre-empted between the if conditional and NSS_Shutdown()?

Comment 11 Rob Crittenden 2008-09-03 20:39:38 UTC
Created attachment 315685 [details]
Also lock during cleanup to make it thread-safe

Make the cleanup function thread-safe too for safety purposes

Comment 12 Fedora Update System 2008-09-04 21:43:34 UTC
curl-7.18.2-5.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/curl-7.18.2-5.fc9

Comment 13 Fedora Update System 2008-09-04 21:46:35 UTC
curl-7.18.2-5.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/curl-7.18.2-5.fc8

Comment 14 Daniel Stenberg 2008-09-05 14:16:05 UTC
Can I just remind you that the upstream project appreciate getting patches...

Comment 15 Rob Crittenden 2008-09-05 14:22:05 UTC
Hold your horses, I'm working on it :-) I need to make sure it applies cleanly to the source tip before submitting it.

Comment 16 Fedora Update System 2008-09-11 17:12:57 UTC
curl-7.18.2-5.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 17 Fedora Update System 2008-09-11 17:16:35 UTC
curl-7.18.2-5.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.


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