The default bug view has changed. See this FAQ.

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

ASSIGNED
Assigned to

Status

()

Core
IPC
ASSIGNED
5 years ago
5 years ago

People

(Reporter: kylo_kit, Assigned: kylo_kit)

Tracking

9 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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
application.ini
...
xulrunner-stub.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.
(Assignee)

Comment 1

5 years ago
Sorry, go here for test case xulrunner app:
http://kylo2.s3.amazonaws.com/test_crash_app.zip

Updated

5 years ago
Component: XULRunner → IPC
Product: Toolkit → Core
QA Contact: xulrunner → ipc

Comment 2

5 years ago
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.

Comment 4

5 years ago
Kit, would you like to attach a patch that adds the windows check to the POSIX code path and removes the current Windows implementation?
(Assignee)

Comment 5

5 years ago
Created attachment 592849 [details] [diff] [review]
Patch to GeckoChildProcessHost.cpp to fix xulrunner-stub not finding plugin-container

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.

Comment 6

5 years ago
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?
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Comment 9

5 years ago
Created attachment 593859 [details] [diff] [review]
Second attempt

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)

Updated

5 years ago
Assignee: nobody → kitwood
Status: UNCONFIRMED → ASSIGNED
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-
(Assignee)

Comment 12

5 years ago
Created attachment 595035 [details] [diff] [review]
Third attempt - fix spaces vs. tabs

Oops. I replaced tabs with spaces.
Attachment #593859 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.