534863 - OOPP: Adobe Reader silent crash (any plugin with spaces in the path)

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

Enable the 'IPC' options, restart the browser, then visit a site that has 'pdf's and attempt to open any in the browser.  Minefield will silently crash, with no crash-reporter, or MS error dialog, browser just closes.

No errors noted in Console2.

Using latest hourly build:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091215 Minefield/3.7a1pre Firefox/3.6 ID:20091215021820

cset: http://hg.mozilla.org/mozilla-central/rev/c39745ba3c9e

Win7 64bit
Adobe Reader 9.1.0.163

Tested using this page:
http://www.in.gov/judiciary/  
Scroll down and select any PDF file, note after a few seconds the browser closes.

Comment 1

[timeless]

https://developer.mozilla.org/en/How_to_get_a_stacktrace_with_WinDbg

Comment 2

[Benjamin Smedberg]

ASSERTION: plugin file ain't there: 'exists', file c:/builds/electrolysis/src/dom/plugins/PluginModuleChild.cpp, line 129

The child crashes in PR_FindFunctionSymbol(PRLibrary* lib = NULL) -> PR_Assert soon after, and then the parent crashes with this stacktrace:

 	xul.dll!NS_DebugBreak_P(unsigned int aSeverity=0x00000003, const char * aStr=0x6c9ea9d0, const char * aExpr=0x00000000, const char * aFile=0x6c9ea980, int aLine=0x00000087)  Line 326	C++
>	xul.dll!mozilla::ipc::AsyncChannel::Close()  Line 135 + 0x18 bytes	C++
 	xul.dll!mozilla::plugins::PPluginModuleParent::Close()  Line 55	C++
 	xul.dll!mozilla::plugins::PluginModuleParent::NP_Shutdown(short * error=0x0031e678)  Line 581	C++
 	xul.dll!mozilla::plugins::PluginModuleParent::~PluginModuleParent()  Line 88	C++
 	xul.dll!mozilla::plugins::PluginModuleParent::`scalar deleting destructor'()  + 0xf bytes	C++
 	xul.dll!nsNPAPIPlugin::~nsNPAPIPlugin()  Line 291 + 0x23 bytes	C++
 	xul.dll!nsNPAPIPlugin::`scalar deleting destructor'()  + 0xf bytes	C++
 	xul.dll!nsNPAPIPlugin::Release()  Line 221 + 0xd6 bytes	C++

which is NS_RUNTIMEABORT("Close() called on closed channel!");

cjones, if you could take the parent channel part of this bug, I'll look at the child-side error.

Comment 3

[Benjamin Smedberg]

PluginModuleChild::Init aPluginFilename is "C:\Program"

in PluginModuleParent::LoadModule aFilePath is "C:\Program Files (x86)\Adobe\Reader 9.0\Reader\browser\nppdf32.dll"

The bug appears to be that we're calling cmdLine.AppendLooseValue at http://hg.mozilla.org/mozilla-central/annotate/96e8d529b2d3/ipc/glue/GeckoChildProcessHost.cpp#l209 which doesn't quote the argument properly.

in command_line.cc CommandLine::AppendLooseValue is // TODO(evan): quoting?

Perhaps we should be using AppendSwitchWithValue and read the switch in mozilla-runtime instead of a direct argument... switches are quoted correctly.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pushed http://hg.mozilla.org/projects/electrolysis/rev/69674697dbce

Comment 5

[u88484]

Im curious as to what bug # the patch for this lives?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The review was fast tracked, done on IRC.  See the hg changeset and commit message.

Comment 7

[Jim Jeffery not reading bug-mail 1/2/11]

So, I guess the OOPP stuff is being handled like TraceMonkey, in that the fixes land on electrolysis builds, then at some point merged back into m-c ?  How often are those merges planned, as this, and other OOPP bugs related to plugin's is a deal breaker for some testers and waiting for a long period of time to see the issue fixed on m-c is not good IMO.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

You can always unset the ipc pref until we merge again.

Comment 9

[IU]

(In reply to comment #8)
> You can always unset the ipc pref until we merge again.

This one should be pushed to m-c before the merge, because it appears to be responsible for breaking all other non-flash plug-ins, regardless of the ipc pref setting.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The pushed change doesn't affect in-process plugins.  If they're silently failing it's due to some other problem.

Comment 11

[IU]

See Benjamin Smedberg's comments in bug 534866

Comment 12

[Benjamin Smedberg]

Merged to mozilla-central at http://hg.mozilla.org/mozilla-central/rev/69674697dbce

cjones, could you file followups for this? In particular, the windows-only hack here sucks, because we certainly could end up with "Some Plugin.dylib" on mac/linux. Also, we still need a bug to track the fact that the parent process is crashing (runtimeabort) if the plugin process crashes on load/initialize.

Comment 13

[(not currently active) Ted Mielczarek]

Yeah, like: /Library/Internet Plug-Ins/Flash Player.plugin/ :)

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #12)
> In particular, the windows-only hack
> here sucks, because we certainly could end up with "Some Plugin.dylib" on
> mac/linux.

It's not a problem for mac/linux because we use exec() there.

> Also, we still need a bug to track the fact that the parent process
> is crashing (runtimeabort) if the plugin process crashes on load/initialize.

There's bug 535077 for a related issue.  I'll file the bad-|Close()| bug that was causing this abort.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Filed bug 535207 for the failed-init-ABORT.

Comment 16

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=11182 has been updated and added to the BFT to cover this scenario