163228 - Trunk startup crashes [@ nsPluginHostImpl::WritePluginInfo]

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[greer]

Starting with the 2002081508 build, trunk topcrash reports are showing startup
crashes at nsPluginHostImpl::WritePluginInfo.

(cc'ing anthonyd, looks like this might be a result of his checkin for bug
135292.  Also cc'ing Darin who looks like he has been in the code recently and
may be able to assist.)

     First  Build ID : 2002081508
     Latest Build ID : 2002081612

Stack Trace: 

	 nsPluginHostImpl::WritePluginInfo
[c:/builds/seamonkey/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp  line
5342]
	 nsPluginHostImpl::FindPlugins
[c:/builds/seamonkey/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp  line
5216]
	 nsPluginHostImpl::LoadPlugins
[c:/builds/seamonkey/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp  line
5044]
	 nsPluginHostImpl::GetPluginFactory
[c:/builds/seamonkey/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp  line
4559]
	 nsJVMManager::StartupJVM
[c:/builds/seamonkey/mozilla/modules/oji/src/nsJVMManager.cpp  line 671]
	 nsJVMManager::MaybeStartupLiveConnect
[c:/builds/seamonkey/mozilla/modules/oji/src/nsJVMManager.cpp  line 902]
	 nsJVMManager::StartupLiveConnect
[c:/builds/seamonkey/mozilla/modules/oji/src/nsJVMManager.h  line 144]
	 nsJSEnvironment::Init
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp  line 1702]
	 NS_CreateScriptContext
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp  line 1748]
	 nsDOMSOFactory::NewScriptContext
[c:/builds/seamonkey/mozilla/dom/src/build/nsDOMFactory.cpp  line 156]
	 nsDocShell::EnsureScriptEnvironment
[c:/builds/seamonkey/mozilla/docshell/base/nsDocShell.cpp  line 6407]
	 nsWebShell::GetInterface
[c:/builds/seamonkey/mozilla/docshell/base/nsWebShell.cpp  line 303]
	 nsGetInterface::operator()
[c:/builds/seamonkey/mozilla/xpcom/glue/nsIInterfaceRequestorUtils.cpp  line 55]
	 nsCOMPtr_base::assign_from_helper
[c:/builds/seamonkey/mozilla/xpcom/glue/nsCOMPtr.cpp  line 81]
	 nsDocShell::InternalLoad
[c:/builds/seamonkey/mozilla/docshell/base/nsDocShell.cpp  line 4675]
	 nsDocShell::LoadURI
[c:/builds/seamonkey/mozilla/docshell/base/nsDocShell.cpp  line 687]
	 nsDocShell::LoadURI
[c:/builds/seamonkey/mozilla/docshell/base/nsDocShell.cpp  line 2387]
	 nsWebShellWindow::Initialize
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsWebShellWindow.cpp  line 343]
	 nsAppShellService::JustCreateTopWindow
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp  line 717]
	 nsAppShellService::CreateHiddenWindow
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp  line 419]
	 main1
[c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp  line 1480]
	 main
[c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp  line 1876]
	 WinMain
[c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp  line 1894]
	 WinMainCRTStartup()
	 KERNEL32.DLL + 0x1b537 (0xbff8b537)
	 KERNEL32.DLL + 0x1b3e9 (0xbff8b3e9)
	 KERNEL32.DLL + 0x19dac (0xbff89dac)

Comment 1

[greer]

71 incidents on the Trunk in the past 72 hrs. 
keywords -> topcrash, zt4newcrash
hence,
severity -> blocker

Comment 2

[Olivier Cahagne]

*** Bug 163431 has been marked as a duplicate of this bug. ***

Comment 3

[Olivier Cahagne]

*** Bug 163378 has been marked as a duplicate of this bug. ***

Comment 4

[rubydoo123]

assigning to anthonyd

Comment 5

[serge (gone)]

this is regression from 109739.
--> to myself.

Comment 6

[serge (gone)]

Attachment: patch v1

check for empty strings in plugin's tag

Comment 7

[serge (gone)]

Av, could you please r= on this? Thanks.

Comment 8

[av (gone)]

Comment on attachment 95873 [details] [diff] [review]
patch v1

r=av, we need to increment version and to implement refreshing the whole
registry if versions do not match. This will help to avoid problems for those
who use nightly builds in the future additions/fixes. I filed bug 163517 on
this issue.

Comment 9

[serge (gone)]

Attachment: patch v2 the sane as v1 but with VERSION_MINOR increased

-#define PLUGIN_REGISTRY_VERSION_MINOR 6
+#define PLUGIN_REGISTRY_VERSION_MINOR 7

Comment 10

[serge (gone)]

Comment on attachment 95895 [details] [diff] [review]
patch v2  the sane as v1 but  with VERSION_MINOR increased

carry over av's r=

Comment 11

[serge (gone)]

ccing [e-mail] fro possible sr=

Comment 12

[av (gone)]

Comment on attachment 95895 [details] [diff] [review]
patch v2  the sane as v1 but  with VERSION_MINOR increased

carrying over r=av

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 163731 has been marked as a duplicate of this bug. ***

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 95895 [details] [diff] [review]
patch v2  the sane as v1 but  with VERSION_MINOR increased

> #define PLUGIN_REGISTRY_MAX_MIMETYPES_PER_PLUGIN 4

plugins are limited to 4 types? Is that why Quicktime comes in multiple .dll's?
Seems pretty arbitrary. (just curious, not part of the review)

>+#define PLUGIN_REGISTRY_END_OF_LINE_MARKER '$'

How does this help? You've already got a real EOL that apparently wasn't used
correctly, adding another marker bloats the file, complicates things, and does
nothing in itself to ensure you'll not make mistakes detecting it.

Are you finding the field delimiter in real data and getting confused (my fears
in the original patch)? What was the original crash and how does this help?

Comment 15

[serge (gone)]

>#define PLUGIN_REGISTRY_MAX_MIMETYPES_PER_PLUGIN 4
is the size of char *stackalloced[PLUGIN_REGISTRY_MAX_MIMETYPES_PER_PLUGIN * 3];
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp&rev=1.415&root=/cvsroot#5480
if there is more than 4 mimetype per plugin different array will be malloced

>+#define PLUGIN_REGISTRY_END_OF_LINE_MARKER '$'
>How does this help?
nsManifestLineReader.NextLine()
does eat all "\n\n\n"
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/plugin/base/src/nsPluginManifestLineReader.h&rev=1.1&root=/cvsroot#86
and if we cannot read an empty string at all:(

Comment 16

[serge (gone)]

>Are you finding the field delimiter in real data and getting confused (my fears
>in the original patch)?
no, the problem is with an empty plugin descriptor

>What was the original crash and how does this help?
the crash occurred on dereferencing of null tag->mMimeDescriptionArray
here is the fix:
+          (tag->mMimeTypeArray && tag->mMimeTypeArray[i] ?
tag->mMimeTypeArray[i] : ""),
+          PLUGIN_REGISTRY_FIELD_DELIMITER,
+          (tag->mMimeDescriptionArray && tag->mMimeDescriptionArray[i] ?
tag->mMimeDescriptionArray[i] : ""),
+          PLUGIN_REGISTRY_FIELD_DELIMITER,
+          (tag->mExtensionsArray && tag->mExtensionsArray[i] ?
tag->mExtensionsArray[i] : ""),

Comment 17

[Daniel Veditz [:dveditz]]

>>+#define PLUGIN_REGISTRY_END_OF_LINE_MARKER '$'
>>How does this help?
>nsManifestLineReader.NextLine() does eat all "\n\n\n"

Sure, but for each one you've also added a field delimiter char at the end so I
don't see why you need both. Thanks for pointing out the added ?: operators, I
missed that the first time while trying to match format specifiers and arguments.

sr=dveditz 

Comment 18

[serge (gone)]

>but for each one you've also added a field delimiter char at the end so I
>don't see why you need both
I'm adding '$' only to the lines which suppose to have delimiters,
not to the section headers, or counters.
thanks.

Comment 19

[serge (gone)]

on the trunk
nsPluginManifestLineReader.h;
new revision: 1.2; previous revision: 1.1
nsPluginHostImpl.cpp;
new revision: 1.418; previous revision: 1.417

Comment 20

[Jay Patel [:jay]]

No crashes in Talkback data since the checkin on 8/22.  Marking verified.