Bug 459297 - (curl NSS) Firefox crash during Flash 10 teardown
(curl NSS) Firefox crash during Flash 10 teardown
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: firefox (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Warren Togami
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-15 16:05 EDT by Warren Togami
Modified: 2008-09-11 13:16 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-09-11 13:16:38 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


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

  None (edit)
Description Warren Togami 2008-08-15 16:05:16 EDT
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 07:22:31 EDT
Do we support flash without nspluginwrapper?
Comment 2 Warren Togami 2008-09-01 13:18:02 EDT
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) 2008-09-02 19:16:16 EDT
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) 2008-09-02 19:18:03 EDT
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-02 20:18:52 EDT
> 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-02 21:35:26 EDT
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 04:17:25 EDT
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 08:02:51 EDT
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 09:05:44 EDT
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 12:13:09 EDT
> -  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 16:39:38 EDT
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 17:43:34 EDT
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 17:46:35 EDT
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 10:16:05 EDT
Can I just remind you that the upstream project appreciate getting patches...
Comment 15 Rob Crittenden 2008-09-05 10:22:05 EDT
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 13:12:57 EDT
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 13:16:35 EDT
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.