Last Comment Bug 804592 - CreatePipe from Chromium IPC requires Logon Session, fails otherwise
: CreatePipe from Chromium IPC requires Logon Session, fails otherwise
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: 14 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla19
Assigned To: rmkn85
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-23 07:49 PDT by rmkn85
Modified: 2012-11-02 17:23 PDT (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove GetLogonSessionOnlyDACL from ipc_channel_win (2.01 KB, patch)
2012-10-24 08:11 PDT, rmkn85
cjones.bugs: review+
Details | Diff | Splinter Review

Description rmkn85 2012-10-23 07:49:11 PDT
When running XulRunner based application (using GeckoFX https://bitbucket.org/geckofx/geckofx-14.0) from a Windows Service, on Windows Server 2008 R2 Datacenter (meaning it's without a Local Logon Session), it crashes.

The last error from Mozilla is:
WARNING: file e:/builds/moz2_slave/rel-m-rel-xr-w32-bld/build/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 153

According to Mozilla IPC code imported from Chromium (2006-2008), it requires a Security Descriptor, and gets that through GetLogonSessionOnlyDACL(), which fails when there's no Logon Session:
http://lxr.mozilla.org/mozilla-central/source/ipc/chromium/src/chrome/common/ipc_channel_win.cc#142

The latest Chromium IPC code (2012) is different and doesn't require a Security Descriptor to create a pipe:
http://src.chromium.org/svn/trunk/src/ipc/ipc_channel_win.cc

Can latest Chromium IPC code be back-ported to Mozilla repository?
Comment 1 Josh Matthews [:jdm] 2012-10-23 07:54:44 PDT
My suspicion is that if this isn't biting us in released versions of Firefox, you're going to need to do the work required here. The Chromium code is updated one absolutely-required bugfix at a time, these days.
Comment 2 rmkn85 2012-10-23 09:36:00 PDT
Possibly related to these Mozilla Crash Reports
https://crash-stats.mozilla.com/report/list?signature=win_util%3A%3AGetLogonSessionOnlyDACL(_SECURITY_DESCRIPTOR**)

@Josh why isn't the Chromium code updated more regularly, including backporting fixes done in their repository?
Comment 3 Josh Matthews [:jdm] 2012-10-23 10:32:47 PDT
Effort vs. reward. At this point we are multiple years behind, and what we have seems to be working well. It was never intended to be a permanent dependency, but at some point we gave up pretending that it was anything else.
Comment 4 rmkn85 2012-10-24 01:52:20 PDT
The main difference relevant to this bug is the last parameter of CreateNamedPipeW().
In the code in Mozilla, it's &security_attributes, which uses GetLogonSessionOnlyDACL, which may fail (when there's no Logon Session).
In the code in Chromium, they are just passing NULL to that parameter.

According to Microsoft Documentation, passing NULL gets default security descriptor.
http://msdn.microsoft.com/en-us/library/windows/desktop/aa365150(v=vs.85).aspx

Is there any reason not to do this modification?
Comment 5 Josh Matthews [:jdm] 2012-10-24 06:41:34 PDT
We've approved similar patches before. Would you mind creating one?
Comment 6 rmkn85 2012-10-24 07:01:50 PDT
I haven't submitted any patches to Mozilla before, does it mean I need to download the entire source code, build it and run all the tests, to submit this patch for approval?
Comment 7 Josh Matthews [:jdm] 2012-10-24 07:07:28 PDT
As long as the patch applies and builds, nobody will question how it was created. That being said, a patch against mozilla-central is certainly the easiest way forward. With regards to testing, we can throw it at a server to run them for us.
Comment 8 rmkn85 2012-10-24 08:00:30 PDT
I'm porting part of the following commit from Chromium, will prepare a patch in a moment:

http://src.chromium.org/viewvc/chrome?view=rev&revision=84461
http://src.chromium.org/viewvc/chrome/trunk/src/ipc/ipc_channel_win.cc?r1=83362&r2=84461
Comment 10 Josh Matthews [:jdm] 2012-10-24 08:21:37 PDT
Pushed to our try server for testing: https://tbpl.mozilla.org/?tree=Try&rev=26f7da157aef
Comment 11 rmkn85 2012-10-24 08:29:51 PDT
Did you run it against latest trunk source code, and there will be a build to download when it finishes?
Can you also make it build a patched XulRunner 14, so I can confirm it fixes the issue in my environment?
Comment 12 Josh Matthews [:jdm] 2012-11-02 09:00:58 PDT
Sorry for the radio silence here. I've committed your patch in https://hg.mozilla.org/integration/mozilla-inbound/rev/546e2b61da57. Getting tryserver to produce a xulrunner build (even forgetting about the requirement for 14) is an uncertain process that I don't really have time to dig into at the moment, unfortunately.
Comment 13 Josh Matthews [:jdm] 2012-11-02 09:01:20 PDT
However, the try link above does have Firefox builds available, if that is of any use to you.
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-11-02 17:23:05 PDT
https://hg.mozilla.org/mozilla-central/rev/546e2b61da57

Note You need to log in before you can comment on or make changes to this bug.