881237 - Firefox 22b4 crash on load multi-version extension with xpcom component

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Igor]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36

Steps to reproduce:

Create extension with xpcom component (21 and 22 for example).
Couple versions xpcom are supported using "components.manifest" file.
Attach dump file.



Actual results:

Firefox crash on start


Expected results:

Firefox work fine

Comment 1

[Igor]

link to dump https://dl.dropboxusercontent.com/u/97043962/moz_dump_2.rar

Comment 2

[Scoobidiver (away)]

Please provide a testcase as an attachment and a crash ID from the about:crashes page.

Comment 3

[Igor]

Problem is reproduced if 
1. you have any extension with two xpcom for different versions of gecko.
2. you write components.manifest with little error like this
interfaces gecko22/type_lib.xpt appversion>=22
binary-component gecko22/xpcom.dll appversion>=22
interfaces gecko21/type_lib.xpt appversion>=21
binary-component gecko21/xpcom.dll appversion>=21

you write >= for 21 and for 22 version.

3. on firefox start you get crash

Stack from my dump file
0038e114 5fda44f5 0243d204 60c45d94 60c45dec xul!SafeMutex::AssertNotCurrentThreadOwns+0x4e2cb7 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\xpcom\components\nscomponentmanager.h @ 108]
0038e130 5fc36b03 60c45d94 058908b8 0038e15c xul!nsCOMPtr_base::assign_from_gs_contractid+0x25 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\obj-firefox\xpcom\build\nscomptr.cpp @ 92]
0038e140 5fc584c9 60c45d94 058908b8 00000000 xul!nsCOMPtr<nsIConsoleService>::nsCOMPtr<nsIConsoleService>+0x15 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\obj-firefox\dist\include\nscomptr.h @ 589]
0038e200 603d9c28 60d506b8 058909c8 0038e240 xul!LogMessage+0x2b [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\xpcom\components\manifestparser.cpp @ 132]
0038e268 5fe68749 0243d200 058dcca0 0038e424 xul!nsComponentManagerImpl::RegisterCIDEntryLocked+0x571368 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\xpcom\components\nscomponentmanager.cpp @ 497]
0038e2d4 5fc4029b 0243d200 0038e3cc 60e23290 xul!nsComponentManagerImpl::RegisterModule+0xb9 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\xpcom\components\nscomponentmanager.cpp @ 453]
0038e30c 5fe66b88 0038e420 0000000b 0038e39c xul!nsComponentManagerImpl::ManifestBinaryComponent+0x81 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\xpcom\components\nscomponentmanager.cpp @ 599]
0038e908 5fc7dc51 00000000 0038e968 058d1000 xul!ParseManifest+0x638 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\xpcom\components\manifestparser.cpp @ 488]
0038e94c 5fc82e7a 00000000 0038e968 00000000 xul!nsComponentManagerImpl::RegisterManifest+0x70 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\xpcom\components\nscomponentmanager.cpp @ 561]
0038e97c 5fe66b88 0038ea90 00000007 0038ea0c xul!nsComponentManagerImpl::ManifestManifest+0x2c [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\xpcom\components\nscomponentmanager.cpp @ 570]
0038ef78 5fc7dc51 00000000 0587ec84 056e47c0 xul!ParseManifest+0x638 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\xpcom\components\manifestparser.cpp @ 488]
0038efbc 5fc15c5c 00000000 0587ec84 00000000 xul!nsComponentManagerImpl::RegisterManifest+0x70 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\xpcom\components\nscomponentmanager.cpp @ 561]
0038efd8 5fc7b56d 00000000 056c9c40 00000000 xul!XRE_AddManifestLocation+0x42 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\xpcom\components\nscomponentmanager.cpp @ 1930]
0038f0f4 5fc51773 0038f12c 60c4603c 00000000 xul!LoadExtensionDirectories+0x1b9 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\toolkit\xre\nsxredirprovider.cpp @ 577]
0038f150 5fc51867 0038f2a4 0038f288 0038f19c xul!nsXREDirProvider::LoadExtensionBundleDirectories+0xb4 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\toolkit\xre\nsxredirprovider.cpp @ 605]
0038f17c 5fc80fcc 0038f2a8 640910a0 0038f288 xul!nsXREDirProvider::DoStartup+0xa3 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\toolkit\xre\nsxredirprovider.cpp @ 816]
0038f250 5fc2df7a 0038f3b4 0038f288 02417100 xul!XREMain::XRE_mainRun+0x1cc [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\toolkit\xre\nsapprunner.cpp @ 3764]
0038f268 5feb2ec8 0038f288 00000001 023b2dc8 xul!XREMain::XRE_main+0xdc [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\toolkit\xre\nsapprunner.cpp @ 3935]
0038f380 00b7157e 00000001 023b2dc8 0038f3b4 xul!XRE_main+0x30 [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\toolkit\xre\nsapprunner.cpp @ 4140]
0038f614 00b71eec 00000001 024170a0 00b7548c firefox!do_main+0x57e [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\browser\app\nsbrowserapp.cpp @ 273]
0039fef4 00b720c4 00000001 023b2d40 023b1228 firefox!wmain+0x7ec [e:\builds\moz2_slave\rel-m-beta-w32_bld-00000000000\build\toolkit\xre\nswindowswmain.cpp @ 105]
0039ff38 77523677 fffde000 0039ff84 77b19d72 firefox!__tmainCRTStartup+0x122 [f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c @ 552]

This is MOZ_CRASH in function SafeMutex::AssertNotCurrentThreadOwns
    MOZ_NEVER_INLINE void AssertNotCurrentThreadOwns() const
    {
        // This method is a release-mode check
        if (PR_GetCurrentThread() == mOwnerThread) {
            MOZ_CRASH();
        }
    }
Object is
0:000> dt this -b
Local var @ 0x38e118 Type SafeMutex*
0x5fda44f5 
   +0x000 mMutex           : mozilla::Mutex
      =60aef2d4 mozilla::BlockingResourceBase::kResourceTypeName : 
       [00] 0x60d4dcf0  "Mutex"
       [01] 0x60d4dcdc  "ReentrantMonitor"
       [02] 0x60d4dcd4  "CondVar"
      +0x000 mLock            : 0x00000025 
   +0x004 mOwnerThread     : 0xdc850f80

Comment 4

[Scoobidiver (away)]

We still need a real testcase as an attachment.

Comment 5

[Benjamin Smedberg]

Actually no, I don't need a testcase, this is pretty straightforward.

http://hg.mozilla.org/mozilla-central/annotate/9ca690835a5e/xpcom/components/nsComponentManager.cpp#l494 is being called from within the component manager lock, which is no longer reentrant as of bug 684887 and will therefore crash.

You can work around this problem by not doing the thing which is causing the warning, which in this case is trying to register a component CID for something which is already registered. Perhaps you tried to reuse a CID or something?

Comment 6

[Benjamin Smedberg]

Since there is an obvious workaround, I don't think we need to rush a fix for this, although I'll happily mentor somebody who wants to fix it. I think that we can simply scoped-unlock the component manager lock while issuing this warning. Writing a test should be fairly simple, based on the existing test binary component at http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/component/TestComponent.cpp

Comment 7

[Igor]

Hello 
We trying to support new Firefox and can't do it.
After I found this issue we change our component.manifest to

interfaces gecko21/type_lib.xpt appversion<21.*
binary-component gecko21/xpcom.dll appversion<21.*
interfaces gecko22/type_lib.xpt appversion<22.*
binary-component gecko22/xpcom.dll appversion<22.*

Firefox does not load component from 21 version to 22 version.
But now we add 23 and 24 versions

interfaces gecko21/type_lib.xpt appversion<21.*
binary-component gecko21/xpcom.dll appversion<21.*
interfaces gecko22/type_lib.xpt appversion<22.*
binary-component gecko22/xpcom.dll appversion<22.*
interfaces gecko23/type_lib.xpt appversion<23.*
binary-component gecko23/xpcom.dll appversion<23.*
interfaces gecko24/type_lib.xpt appversion<24.*
binary-component gecko24/xpcom.dll appversion<24.*

Firefox 23 try to load 24 componens and crash.
I don't know how to fix this problem in component.manifest and I change priority to P1

Comment 8

[Aroon]

Attachment: Proposed fix 881237

Comment 9

[Benjamin Smedberg]

Comment on attachment 809336 [details] [diff] [review]
Proposed fix 881237

Overall, this is correct. Thank you for taking this! The notes below are entirely about whitespace/formatting issues with your patch. Could you please fix this issues and attach a new patch?

Could you please add your information to this patch for checkin? See https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F for instructions on how to do that.

>diff --git a/xpcom/components/nsComponentManager.cpp b/xpcom/components/nsComponentManager.cpp

> void
> nsComponentManagerImpl::RegisterCIDEntryLocked(

>         else
>             existing = "<unknown module>";
>+		mLock.Unlock();
>         LogMessage("While registering XPCOM module %s, trying to re-register CID '%s' already registered by %s.",

You have hard tabs in here which are causing spacing issues. Please replace your tabs with spaces (most editors can be configured to not use hard tabs).

>diff --git a/xpcom/tests/component/TestComponent.cpp b/xpcom/tests/component/TestComponent.cpp

> static const mozilla::Module::CIDEntry kTestCIDs[] = {
>   { &kNS_TESTING_CID, false, NULL, DummyConstructorFunc },
>-  { NULL }
>+  { &kNS_TESTING_CID, false, NULL, DummyConstructorFunc },
>+   {NULL}

Nit, please restore the { NULL } whitespace the way it was.


> NSMODULE_DEFN(dummy) = &kTestModule;
> 
>+
>+

Nit, please don't add this extra whitespace at the end.

Comment 10

[Aroon]

Attachment: 881237

Comment 11

[Benjamin Smedberg]

I'm marking checkin-needed so that sheriffs will push this for you.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

patching file xpcom/components/nsComponentManager.cpp
bad hunk #1 @@ -475,34 +475,39 @@
 (38 34 39 39)
patch failed, rejects left in working dir

Comment 13

[Benjamin Smedberg]

Aroon, for some reason this patch isn't valid. Did you try to hand-edit it? Can you please re-create it using hg queues so that "hg import" can import it properly?

Comment 14

[Aroon]

Attachment: 881237.patch

Comment 15

[Benjamin Smedberg]

Comment on attachment 810166 [details] [diff] [review]
881237.patch

Something is going wrong with your patch generation. Firstly the tabs are back in nsComponentManager.cpp. And in the TestComponent.cpp hunk something is going wrong so that hg refuses to apply the patch: probably something to do with windows line endings.

Comment 16

[Aroon]

Attachment: patch.diff

Comment 17

[Aroon]

Attachment: patch_881237.diff

Comment 18

[Benjamin Smedberg]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cc4d49b29b0

Comment 19

[Benjamin Smedberg]

Comment on attachment 810761 [details] [diff] [review]
patch_881237.diff

This patch still has a hard tab in it, but I fixed that locally and pushed. Thank you.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/20855adc0ef1 for xpcshell test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=28430847&tree=Mozilla-Inbound

Comment 21

[Benjamin Smedberg]

Aroon, this failed because I gave you bad advice! I'm sorry about that. It's not safe to simply unlock mLock in the method, because the method starts out with the lock locked. That means that the caller will try to unlock the lock again and assert/crash.

What we need to do is, instead of just calling .Unlock, use MutexAutoUnlock around the logging functions, so that we re-enter the lock before returning.

Let me know if you need help. This test should have failed locally for you if you were building a debug build; you should do a debug build of your new patch to make sure that the test passes without assertions.

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9cc4d49b29b0 will show you all the results, including the xpcshell failures across all the debug builds.

Comment 22

[Aroon]

Attachment: patch_881237.patch

Comment 23

[Benjamin Smedberg]

Comment on attachment 812815 [details] [diff] [review]
patch_881237.patch

I'll go a local test to verify and push this if it's green.

Comment 24

[Benjamin Smedberg]

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

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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