339754 - Threadsafety asserts from chrome registry when installing extensions

Status: RESOLVED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Current trunk build

STEPS TO REPRODUCE: Install http://downloads.mozdev.org/flashblock/flashblock-1.5.unstable.xpi

ACTUAL RESULTS:  Asserts about the chrome registry not being threadsafe.

EXPECTED RESULTS:  No asserts, propert functioning.

Comment 1

[Robert Strong (they/them - no direct email)]

Is this asserting immediately before clicking install or after the EM window is displayed?

Comment 2

[James Ross]

I don't get anything mentioning the chrome registry, but I do get 4 assertion failures (all thread-related) when the XPInstall engine finished (basically the point the install is done and it displays the "Restart to complete" message).

Boris, could you include the actual assets even if you don't have any stacks, so it's clear what you are concerned about in this particular case?

Comment 3

[Robert Strong (they/them - no direct email)]

Those ones I've seen and unless I'm mistaken are in XPInstall.

Comment 4

[Boris Zbarsky [:bzbarsky]]

James, I would if I could, but I can't uninstall the extension (that crashes in current trunk debug builds), so I can't try installing it.  I have _not_ seen these same asserts with other extensions.

Robert, this happens after I click install and _maybe_ after the EM window is displayed.  It's hard to tell, because Firefox hung (maybe deadlock, maybe not; 0-cpu hang) right around the same time.  This was also about 2 hours ago and I was debugging the uninstall crash in between, so I just don't recall the details.  I should have filed this right away while I still had the asserts in my scrollback.  :(

Comment 5

[James Ross]

(In reply to comment #3)
> Those ones I've seen and unless I'm mistaken are in XPInstall.

Yeah, the 4 I saw here are all XPInstall itself. Unfortunately, that's all I got when installing said extension into my debug trunk build.

You should be able to uninstall the extension by nuking its folder under profile/extensions? Or does that trigger the same crash?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, nuking works.  The exact assert is:

###!!! ASSERTION: nsChromeRegistry not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file ../../../mozilla/chrome/src/nsChromeRegistry.cpp, line 445

#0  Break (
    aMsg=0xb056ec20 "###!!! ASSERTION: nsChromeRegistry not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file ../../../mozilla/chrome/src/nsChromeRegistry.cpp, line 445") at ../../../mozilla/xpcom/base/nsDebugImpl.cpp:471
#1  0xb7e40566 in NS_DebugBreak_P (aSeverity=1, 
    aStr=0xb60948e0 "nsChromeRegistry not thread-safe", 
    aExpr=0xb60948ac "_mOwningThread.GetThread() == PR_GetCurrentThread()", 
    aFile=0xb6094814 "../../../mozilla/chrome/src/nsChromeRegistry.cpp", aLine=445)
    at ../../../mozilla/xpcom/base/nsDebugImpl.cpp:354
#2  0xb6079b49 in nsChromeRegistry::AddRef (this=0x8198cb0)
    at ../../../mozilla/chrome/src/nsChromeRegistry.cpp:445
#3  0xb6079aa0 in nsChromeRegistry::QueryInterface (this=0x8198cb0, aIID=@0xb7fe4c80, 
    aInstancePtr=0xb056f0b4) at ../../../mozilla/chrome/src/nsChromeRegistry.cpp:443
#4  0xb7daa738 in nsQueryInterface::operator() (this=0xb056f0bc, aIID=@0xb7fe4c80, 
    answer=0xb056f0b4) at nsCOMPtr.cpp:47
#5  0xb7daa8c8 in nsCOMPtr_base::assign_from_qi (this=0xb056f140, qi=
      {mRawPtr = 0x8198cb0}, iid=@0xb7fe4c80) at nsCOMPtr.cpp:96
#6  0xb7dab3c5 in nsCOMPtr<nsISupports>::nsCOMPtr ()
    at ../../../../dist/include/xpcom/nsID.h:73
#7  0xb7e3ca0c in ~nsProxyEventObject (this=0x9569460)
    at ../../../../mozilla/xpcom/proxy/src/nsProxyEventObject.cpp:449
#8  0xb7e3ccb5 in nsProxyEventObject::Release (this=0x9569460)
    at ../../../../mozilla/xpcom/proxy/src/nsProxyEventObject.cpp:500
#9  0xb7e3cb15 in ~nsProxyEventObject (this=0x9556ce8)
    at ../../../../mozilla/xpcom/proxy/src/nsProxyEventObject.cpp:467
#10 0xb7e3ccb5 in nsProxyEventObject::Release (this=0x9556ce8)
    at ../../../../mozilla/xpcom/proxy/src/nsProxyEventObject.cpp:500
#11 0xb05b2ec0 in nsCOMPtr<nsIToolkitChromeRegistry>::~nsCOMPtr ()
    at ../../dist/include/string/nsTSubstring.h:173
#12 0xb05abc97 in ~nsInstallInfo (this=0x956bca0)
    at ../../../mozilla/xpinstall/src/nsInstall.cpp:232
#13 0xb05ca449 in nsSoftwareUpdate::InstallJarCallBack (this=0x8cbaf30)
    at ../../../mozilla/xpinstall/src/nsSoftwareUpdate.cpp:381
#14 0xb05cd312 in RunInstallOnThread (data=0x956bca0)
    at ../../../mozilla/xpinstall/src/nsSoftwareUpdateRun.cpp:611
#15 0xb7d0b714 in _pt_root (arg=0x9557500)
    at ../../../../../mozilla/nsprpub/pr/src/pthreads/ptthread.c:220
#16 0x00d57341 in start_thread () from /lib/tls/libpthread.so.0
#17 0xb7c75fee in clone () from /lib/tls/libc.so.6

Comment 7

[James Ross]

I'm suspicious that bug 337492 may have fixed this; I know my build is new enough to contain the fix for that. That was landed almost exactly 5 hours ago (16:07 PDT).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  That patch fixes this.

Comment 9

[Darin Fisher]

so, do we want bug 337492 to be fixed on the 1.8 branch?  did this bug happen with non-trunk builds?  (i think it should have.)

Comment 10

[Boris Zbarsky [:bzbarsky]]

I don't know offhand; I don't recall seeing this exact assert even on trunk a month or two ago...

Comment 11

[Dave Townsend [:mossop]]

This isn't fixed, I still see this in my debug builds today.

Apparently bug 337492 relies on the underlying object having a threadsafe nsISupports implementation which the chrome registry doesn't.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Apparently bug 337492 relies on the underlying object having a threadsafe
> nsISupports implementation

It does?  Where?

Comment 13

[Benjamin Smedberg]

XPCOM thread proxies have always relied on the underlying object having threadsafe addref/release. QueryInterface and Release are both called from arbitrary threads... we only guarantee that the *last* Release will occur on the target thread.

Comment 14

[Dave Townsend [:mossop]]

Attachment: proxy call to InstallJarCallBack

So the reason we are getting assertions is because we destroy the nsInstallInfo holing the chrome registry on a background thread, so if we proxy the call to InstallJarCallBack then we will destroy on the main thread and get no assertion.

This does that by creating a simple nsIRunnable implementation that we can dispatch to the main thread. We can't use a simple object proxy since nsISoftwareUpdate isn't a true xpcom interface. We also can't use simple NS_NEW_RUNNABLE_METHOD since the callback function is non-void (though we could opt to change that in the nsISoftwareUpdate interface if preferable).

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 282550 [details] [diff] [review]
proxy call to InstallJarCallBack

r/sr=dveditz

There's no good reason for InstallJarCallBack to be non-void. If you want to fix it the other way feel free.

Comment 16

[Dave Townsend [:mossop]]

Attachment: use runnable macro

This is the alternate, uses the regular runnable macro to create the proxy call. A bit cleaner on xpinstall I think even with changing the interface slightly.

This also adds a slight change of behaviour, in the event that the install wasn't run on a background thread there is no need to proxy the callback and doing so would I think change the current behaviour.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 283006 [details] [diff] [review]
use runnable macro

r/sr=dveditz

This works too

Comment 18

[Dave Townsend [:mossop]]

Checking in xpinstall/public/nsISoftwareUpdate.h;
/cvsroot/mozilla/xpinstall/public/nsISoftwareUpdate.h,v  <--  nsISoftwareUpdate.h
new revision: 1.40; previous revision: 1.39
done
Checking in xpinstall/src/nsSoftwareUpdate.cpp;
/cvsroot/mozilla/xpinstall/src/nsSoftwareUpdate.cpp,v  <--  nsSoftwareUpdate.cpp
new revision: 1.120; previous revision: 1.119
done
Checking in xpinstall/src/nsSoftwareUpdate.h;
/cvsroot/mozilla/xpinstall/src/nsSoftwareUpdate.h,v  <--  nsSoftwareUpdate.h
new revision: 1.43; previous revision: 1.42
done
Checking in xpinstall/src/nsSoftwareUpdateRun.cpp;
/cvsroot/mozilla/xpinstall/src/nsSoftwareUpdateRun.cpp,v  <--  nsSoftwareUpdateRun.cpp
new revision: 1.110; previous revision: 1.109
done

Comment 19

[Dave Townsend [:mossop]]

Attachment: bustage fix

Had to add this to fix bustage on windows, can you just retroactively confirm you're ok with it. Windows was complaining about the method being stdcall rather than thiscall.

Comment 20

[Boris Zbarsky [:bzbarsky]]

That looks wrong.  You should be using NS_IMETHOD* in an nsIFoo like this, last I checked...

And since your header and impl matched it... what was the problem, exactly?  what was the exact error message?

Comment 21

[Dave Townsend [:mossop]]

(In reply to comment #20)
> And since your header and impl matched it... what was the problem, exactly? 
> what was the exact error message?

error C2664: 'nsRunnableMethod<T>::nsRunnableMethod(T *,void (__thiscall nsISoftwareUpdate::* )(void))' : cannot convert parameter 2 from 'void (__stdcall nsISoftwareUpdate::* )(void)' to 'void (__thiscall nsISoftwareUpdate::* )(void)'
        with
        [
            T=nsISoftwareUpdate
        ]
        Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

Comment 22

[Dave Townsend [:mossop]]

From mozilla/xpinstall/src/nsSoftwareUpdateRun.cpp(595)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Oh.  Yeah, you can't use nsRunnableMethod with a COM interface signature...  So either this isn't a COM interface (and then the patch is correct), or it is and you need to be calling a non-COM function from the runnable...

Comment 24

[Dave Townsend [:mossop]]

Well I think the particular method isn't really part of the COM interface (there is even a comment in nsISoftwareUpdate.h about moving it to a private interface) so I believe the patch may be correct.

If not however we can just go back with attachment 282550 [details] [diff] [review] instead, though I would want to make the change such that InstallJarCallBack is called directly when no thread dispatch was necessary I think.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 284492 [details] [diff] [review]
bustage fix

r/sr=dveditz