Closed
Bug 339754
Opened 19 years ago
Closed 17 years ago
Threadsafety asserts from chrome registry when installing extensions
Categories
(Core Graveyard :: Installer: XPInstall Engine, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9beta1
People
(Reporter: bzbarsky, Assigned: mossop)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
dveditz
:
review+
dveditz
:
superreview+
ted
:
approval1.9+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dveditz
:
review+
dveditz
:
superreview+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
Is this asserting immediately before clicking install or after the EM window is displayed?
Comment 2•19 years ago
|
||
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•19 years ago
|
||
Those ones I've seen and unless I'm mistaken are in XPInstall.
Reporter | ||
Comment 4•19 years ago
|
||
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•19 years ago
|
||
(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?
Reporter | ||
Comment 6•19 years ago
|
||
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•19 years ago
|
||
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).
Reporter | ||
Comment 8•19 years ago
|
||
Ah, yes. That patch fixes this.
Comment 9•19 years ago
|
||
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.)
Reporter | ||
Comment 10•19 years ago
|
||
I don't know offhand; I don't recall seeing this exact assert even on trunk a month or two ago...
Assignee | ||
Comment 11•17 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•17 years ago
|
Status: REOPENED → NEW
Component: Extension/Theme Manager → Installer: XPInstall Engine
Product: Firefox → Core
QA Contact: extension.manager → xpi-engine
Reporter | ||
Comment 12•17 years ago
|
||
> Apparently bug 337492 relies on the underlying object having a threadsafe
> nsISupports implementation
It does? Where?
Comment 13•17 years ago
|
||
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.
Assignee | ||
Comment 14•17 years ago
|
||
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).
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #282550 -
Flags: superreview?(dveditz)
Attachment #282550 -
Flags: review?(benjamin)
Comment 15•17 years ago
|
||
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.
Attachment #282550 -
Flags: superreview?(dveditz)
Attachment #282550 -
Flags: superreview+
Attachment #282550 -
Flags: review?(benjamin)
Attachment #282550 -
Flags: review+
Assignee | ||
Comment 16•17 years ago
|
||
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.
Attachment #283006 -
Flags: review?(dveditz)
Comment 17•17 years ago
|
||
Comment on attachment 283006 [details] [diff] [review]
use runnable macro
r/sr=dveditz
This works too
Attachment #283006 -
Flags: superreview+
Attachment #283006 -
Flags: review?(dveditz)
Attachment #283006 -
Flags: review+
Assignee | ||
Updated•17 years ago
|
Attachment #283006 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #283006 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 18•17 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 19 years ago → 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9
Assignee | ||
Updated•17 years ago
|
Attachment #282550 -
Attachment is obsolete: true
Assignee | ||
Comment 19•17 years ago
|
||
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.
Attachment #284492 -
Flags: superreview?(dveditz)
Attachment #284492 -
Flags: review?(dveditz)
Reporter | ||
Comment 20•17 years ago
|
||
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?
Assignee | ||
Comment 21•17 years ago
|
||
(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
Assignee | ||
Comment 22•17 years ago
|
||
From mozilla/xpinstall/src/nsSoftwareUpdateRun.cpp(595)
Reporter | ||
Comment 23•17 years ago
|
||
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...
Assignee | ||
Comment 24•17 years ago
|
||
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•17 years ago
|
||
Comment on attachment 284492 [details] [diff] [review]
bustage fix
r/sr=dveditz
Attachment #284492 -
Flags: superreview?(dveditz)
Attachment #284492 -
Flags: superreview+
Attachment #284492 -
Flags: review?(dveditz)
Attachment #284492 -
Flags: review+
Updated•9 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•