Closed Bug 721817 Opened 10 years ago Closed 4 years ago

xulrunner-stub can't find plugin-container.exe (dom.ipc.plugins.enabled = true)


(Core :: IPC, defect)

9 Branch
Not set





(Reporter: kylo_kit, Assigned: kylo_kit)



(1 file, 2 obsolete files)

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

Steps to reproduce:

Run a Windows application based on XULRunner SDK using the following (recommended) directory structure:

- chrome
- components
- defaults
- xulrunner
    - ...
    - plugin-container.exe
    - xulrunner.exe

Under prefs, set "dom.ipc.plugins.enabled" to true.

Run the app through xulrunner-stub.exe

Actual results:

App hangs/crashes when any pages containing Flash, Silverlight, Quicktime, etc. are loaded into a browser.

Watching the list of running processes shows that the plugin-container.exe executable fails to load.

When the app is run with xulrunner.exe (ie. ./xulrunner/xulrunner.exe -app application.ini), plugin-container.exe shows in the process list, the content properly loads and the app does not hang.

Expected results:

Looks like the culprit is some bad logic in ipc/glue/GeckoChildProcessHost.cpp

in the GetPathToBinary function, the location of the plugin-container.exe (MOZ_CHILD_PROCESS_NAME) is put together from the path of the CURRENT process (ie. xulrunner-stub.exe) instead of looking for the GRE path. So it works only if everything happens to be in the same directory.

I was able to fix the problem for a local build by copying the logic under the OS_POSIX ifdef which does the proper thing and uses the directoryService to find GreD. I had to adjust for differences in windows path strings. My solution is kind of kludgy (I'm not much of a C++ guy), so a professional should probably take a look at this.

I attached a tiny xulrunner app complete with flash, quicktime, and silverlight content to test.
Sorry, go here for test case xulrunner app:
Component: XULRunner → IPC
Product: Toolkit → Core
QA Contact: xulrunner → ipc
Benjamin/Josh, any idea why the Windows logic in GeckoChildProcessHost is the way it is? HG spelunking shows that the code has basically remained the same since the original electrolysis merge.
No, I suspect that this was always incorrect but so few people use XR+plugins and XR on Windows that it never came up.
Kit, would you like to attach a patch that adds the windows check to the POSIX code path and removes the current Windows implementation?
I've only tested this locally on a Win7 machine.

It seems like we still need to test for POSIX/Win to determine the string type for path (nsCString or nsString) - but I may have the wrong approach here.

I also changed "GetNativePath" to "GetPath" to help get the right string type for the path on Windows. Hopefully, that's kosher.
Ugh, gross. I don't think this is the correct way to get around that. Instead, I would try using nsCString and GetNativePath on both platforms, but on Windows create the FilePath from NS_ConvertUTF8toUTF16(path).get(), and for POSIX use the existing code. Benjamin, does that sound sensible?
I had read a google groups post from Benjamin recommending not calling GetNativePath on an nsiFile in windows, but it is very likely that I misunderstood the context.
Yes, you don't ever want to be calling GetNativePath on a Windows nsIFile. I'm having trouble reading the code (both before and after!), but typically what we do on Windows is convert the wide-string path to UTF8 to pass it across the wire, and then convert it back.
Attached patch Second attempt (obsolete) — Splinter Review
My second attempt - not much different from the first.

I'm now switching between nsAutoString & getPath and nsCAutoString & getNativePath. There seems to be some precedent for this logic in at least toolkit/xre/nsUpdateDriver.cpp if not elsewhere.

FilePath should handle the wide/narrow conversions... (defined in ipc/chromium/src/base/file_path)

Let me know if I'm still off base.
Attachment #592849 - Attachment is obsolete: true
Comment on attachment 593859 [details] [diff] [review]
Second attempt

That seems more reasonable to me. Benjamin, what say you?
Attachment #593859 - Flags: review?(benjamin)
Assignee: nobody → kitwood
Ever confirmed: true
Comment on attachment 593859 [details] [diff] [review]
Second attempt

I think this is correct, and I hate to mark r-, but I need a version of this patch which uses spaces instead of tabs for indentation. That will make it easier to review, and our code style guide specifies no hard tabs in sources (partly because Mozilla coders use all kinds of editors and getting the tab behavior the same in all of them is basically impossible).
Attachment #593859 - Flags: review?(benjamin) → review-
Oops. I replaced tabs with spaces.
Attachment #593859 - Attachment is obsolete: true
xulrunner is deprecated.!msg/
Closed: 4 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.