350616 - Need to figure out VC8 CRT issues with the XULRunner stub (and with embedding)

Status: RESOLVED FIXED

(Toolkit Graveyard :: XULRunner, defect)

Description

[Benjamin Smedberg]

Followup to bug 334528: even after that lands, the XULRunner stub can't load gecko correctly. The situation is something like this:

c:\Program Files\MyApplication\xulrunner-stub.exe, when launched, tries to dynamically load
c:\Program Files\Mozilla\XULRunner 1.9a1\xul.dll (and xpcom.dll, and other things). But because the CRT is at c:\Program Files\Mozilla\XULRunner 1.9a1 and not at c:\Program Files\MyApplication, the loader doesn't find it and the app doesn't start.

Ted, I need your help with this one. Is there any way to embed a manifest in xul.dll (and nspr4.dll and the other DLLs in question) which tells the loader to look in the same directory as the DLL for the CRT assembly, instead of looking in the application directory? I'd like to avoid the Activation Context goop that we did for the installer in bug 328579

Comment 1

[(not currently active) Ted Mielczarek]

AIUI, if you embed a manifest at resource ID 2, the system will look for the CRT assembly in the same directory as the DLL.  As long as you don't intend to use these DLLs in any other fashion, that's ok.

The problem we had with the installer bug was that we used the same DLLs in two different fashions: once for the installer, where they were extracted to a temp dir with the CRT manifest/assemblies in the same directory, and then again in appdir/components/, where the CRT was in appdir/.

Reference: http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sbscs/setup/searching_for_assembly_files.asp (specifically the last paragraph)

Comment 2

[(not currently active) Ted Mielczarek]

I forgot to mention, the patch from bug 328579 gives you the ability to say:
EMBED_MANIFEST_AT = 2

In your Makefile.in before you include rules.mk.

Comment 3

[Benjamin Smedberg]

No component libraries are involved, only library DLLS that would live next to the CRT.

Comment 4

[(not currently active) Ted Mielczarek]

Then you can do this safely by embedding the manifest at ID 2, and XULRunner app authors including binary components will have to make sure not to embed a manifest at all in their components.

Comment 5

[Benjamin Smedberg]

What about NSPR]NSS? Do they currently embed a manifest?

Comment 6

[(not currently active) Ted Mielczarek]

Nope.  Currently, EXEs get a manifest at resource ID 1, and DLLs don't get a manifest at all unless they ask for it with that Makefile define.  The only places that happens right now are in XPInstall and MSAA (from bug 338807 ).

http://lxr.mozilla.org/seamonkey/search?string=EMBED_MANIFEST_AT

Comment 7

[(not currently active) Ted Mielczarek]

Oh, one more note:  I believe that once one DLL gets loaded with an activation context, then that becomes the current activation context, so any other DLLs loaded without one will use the existing one.  If you can guarantee that xul.dll or xpcom.dll or whatever will get loaded first, then I think you can get away with just embedding the manifest in that one DLL.

I could be off base, but it's easy enough to test.  Just use mt.exe from the commandline and embed a manifest in your dll, and see if it works.

Comment 8

[(not currently active) Ted Mielczarek]

Additionally, if I'm wrong on that last point, and you do need to embed a manifest in every single DLL to make this work, and getting that into NSPR/NSS is a problem, I think copying the activation context magic from xpinstall wouldn't be too bad.  You would basically need to stick this block of code: (modified a bit)
http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/installer/windows/wizard/setup/xpi.c#132
Right before here:
http://lxr.mozilla.org/seamonkey/source/xulrunner/stub/nsXULStub.cpp#269

Comment 9

[Benjamin Smedberg]

Yeah... I'd prefer a solution that didn't require that, because that require every embedder to do it, as well as every future version of the stub. I'm going to see what we can do to get the manifests embedded in NSPR, because everything else depends on the NSPR libs being loaded first.

Comment 10

[Ludovic Hirlimann [:Usul]]

(In reply to comment #9)
>  I'm going
> to see what we can do to get the manifests embedded in NSPR, because everything
> else depends on the NSPR libs being loaded first.
> 

Benja;in, did you progress on that ?

Comment 11

[Benjamin Smedberg]

No, it's P3 which means that it's well below my radar right now.

Comment 12

[Jonathan Watt [:jwatt]]

Seems that NSS does now embed a manifest. See bug 353475.

Comment 13

[Jonathan Watt [:jwatt]]

Embedding a manifest in nspr4.dll simply reduced the number of:

  This application has failed to start because MSVCR80D.dll was not found.
  Re-installing the application may fix this problem.

popups from six to five (non-libxul build). All six popups are eliminated when manifests are embedded in each of:

  nsprpub/lib/ds/plds4.dll
  nsprpub/lib/libc/src/plc4.dll
  nsprpub/pr/src/nspr4.dll
  toolkit/library/xul.dll
  xpcom/build/xpcom_core.dll
  xpcom/stub/xpcom.dll

but then DLLs from the xulrunner/components directory fail to load. I get lots of:

  nsNativeModuleLoader::LoadModule("...\xulrunner\components\caps.dll") - load
  FAILED, rv: 80004003, error:
          error 126

for DLLs such as caps.dll, necko.dll, chrome.dll and tkitcmps.dll before the process dies. Embedding a manifest at resource ID 2 in caps.dll seems to get it to load even though the CRT isn't beside that DLL. I'm not sure how to proceed other than to embed a manifest in all DLLs the app can load. Thoughts?

Comment 14

[Jonathan Watt [:jwatt]]

I believe that http://msdn.microsoft.com/library/en-us/sbscs/setup/using_side-by-side_assemblies_as_a_resource.asp confirms my observations that this isn't as simple as embedding a manifest once in nspr4.dll (only embedding at resource ID 1 sets the process default, but embedding at resource ID 1 isn't valid for DLLs).

Comment 15

[Jonathan Watt [:jwatt]]

Doing as Ted suggests in comment 8 doesn't seem to be enough. It appears to get all the DLLs in the xulrunner directory loading correctly, but all the DLLs in xulrunner/components fail.

Comment 16

[Jonathan Watt [:jwatt]]

Programmatically making the xulrunner directory the current directory as well as doing the activation context stuff seems to fix things. DLLs from appdir/components seem to load fine too. I'm going to play with this some more and then I'll attach a patch.

Comment 17

[Benjamin Smedberg]

You must not change the current directory. It will break all kinds of commandline handling behaviors.

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: hack

In that case I won't bother trying to make the setting of the directory cross platform. Here's what I have, nevertheless. I embedded the manifest in xpcom.dll instead of nspr4.dll simply because I couldn't figure out how to use the build system of the latter to do the embedding.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 244810 [details] [diff] [review]
hack


>+  // change the current directory to the GRE directory
>+  SetCurrentDirectory(greDir);
>+  // now put XPCOM_DLL back on the end of greDir
>+  if (lastSlash) {
>+    *lastSlash = PATH_SEPARATOR_CHAR;
>+  }
>+  ACTCTXA actctx;
>+  HANDLE hActCtx = INVALID_HANDLE_VALUE;
>+  ULONG_PTR ulpActivationCookie;
>+
>+  /* Windows XP + Visual C++ 8 requires proper
>+   * Side by Side configuration, which means
>+   * we have to have a manifest to create an
>+   * Activation Context so we can locate the
>+   * VC8 runtime.
>+   */
>+  memset(&actctx, 0, sizeof(actctx));
>+  actctx.cbSize = sizeof(actctx);
>+  actctx.lpSource = (LPCSTR)greDir;
>+  actctx.lpResourceName = MAKEINTRESOURCE(17);

I think instead of using SetCurrentDirectory, you may be able to do like:

actctx.lpAssemblyDirectory = greDir;
actctx.dwFlags = ACTCTX_FLAG_RESOURCE_NAME_VALID | ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID;

Can you give that a try?

Comment 20

[(not currently active) Ted Mielczarek]

Of course that code isn't completely correct, you'll probably need to strcpy the greDir where you do the SetCurrentDirectory there.

Comment 21

[Jonathan Watt [:jwatt]]

Still the failures trying to load anything from xulrunner/components I'm afraid.

Comment 22

[(not currently active) Ted Mielczarek]

Just for completeness, can you post your updated patch you tried?

Comment 23

[Jonathan Watt [:jwatt]]

Attachment: hack with lpAssemblyDirectory instead of SetCurrentDirectory (components fail to load)

Sure, best to check I'm not doing something silly. ;-) Stepping through the code in the debugger everything looks fine (paths look good). However, at this point in the code:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/linking/prlink.c&rev=3.51.2.37&mark=895,908#892

although the absolute path to the DLL in wname looks fine, h gets set to NULL with error 126 (ERROR_MOD_NOT_FOUND). See

http://msdn.microsoft.com/library/en-us/debug/base/system_error_codes__0-499_.asp

Adding the SetCurrentDirectory change "fixes" things so that the DLLs in xulrunner/components load again. Since the ActivateActCtx call fixes the loading of DLLs from the xulrunner directory, I'm not sure what the problem with the DLLs in the components directory is. I don't believe they're failing because the CRT isn't found though. I think it must be something else.

Comment 24

[Jonathan Watt [:jwatt]]

Ah. If I replace SetCurrentDirectory with SetDllDirectory, things work too. This is even though I pass SetDllDirectory the xulrunner directory, and not the xulrunner/components directory.

Comment 25

[(not currently active) Ted Mielczarek]

You are running on WinXP, right?

Comment 26

[Jonathan Watt [:jwatt]]

Note that to build with SetDllDirectory you need to add the following line before the windows.h include:

#define _WIN32_WINNT 0x0502

Comment 27

[Jonathan Watt [:jwatt]]

WinXP SP2, right.

Comment 28

[Benjamin Smedberg]

Since SetDllDirectory is not available in win2k, we'd need to dynamically load it (but that's ok, since we don't have this problem except in winxp+). We could even do it from the app itself, so that we don't have to change the stub.

Comment 29

[Jonathan Watt [:jwatt]]

Great. It would be really nice to get a handle on _why_ the above attempts work/fail first though. :-)

Comment 30

[(not currently active) Ted Mielczarek]

What I assume is that the CRT DLLs are not in the search path unless a) the dll you're trying to load is in that same dir, or b) you've called SetDllDirectory.

However, the more I read about the SxS stuff, the less I understand.

It looks like my suggestion from comment #19 has no effect, so you can just remove that and use your SetDllDirectory fix along with the original activation context magic, and things should work.

Comment 31

[Jonathan Watt [:jwatt]]

I'm not convinced that finding the CRT DLLs is the problem. Maybe it is, but then I'd have expected similar errors loading the DLLs under the components directory as we get when loading the DLLs under the xulrunner directory without the activation context stuff. I'll investigate further tomorrow.

I certainly sympathize with your feelings on the SxS stuff. I can't get anything more than a fuzzy grip on how this stuff works from my reading of the docs.

Regarding your suggestion in comment 19, I think whether it works or not, it's the right thing to do so I'd rather leave it in.

Benjamin, assuming doing the activation and dll path stuff is all good, where do you want the various pieces put? I was thinking putting them in XPCOMGlueStartup should get things working and save embedders from having to know about this. That files doesn't reach out to system.dll right now, but hopefully making it do so isn't a problem.

Comment 32

[Jonathan Watt [:jwatt]]

Attachment: current diff (activation context + SetDllDirectory)

A couple of people have mailed me privately to ask for my current diff. It's not hard to derive it from the comments above, but I might as well attach it I guess.

Comment 33

[Jonathan Watt [:jwatt]]

Gah! So because the stub is inexplicably broken even for people with the CRT in WinSxS, when I got it to work on my machine (which has the CRT in WinSxS) I forgot to test on a machine without Visual Studio installed. My current efforts only manage to get the CRT in WinSxS recognized. Even though it's all about setting search paths/activation contexts to the xulrunner directory! The CRT bundled with XULRunner still isn't found, so this doesn't fix things for the general populous. Now I'm even more confused than before.

Anyway, this hack allows our guys to continue with their work on their VS installed machines for now, and I have too many other pressing things to look into this further. If someone has ideas or insight I'll happily run tests for you, or if someone wants to pick this up and run with it, feel free.

Comment 34

[Tom Clowers]

Is there anything new to add on this? Will this need to be solved in order to build Firefox running on XULRunner?

Comment 35

[Tom Brunet]

I'm doing accessibility work for a xulrunner based app and wanted to take 1.9a4 for a spin to see what's been fixed/broken.  Unfortunately, I immediately received the R6034 error.  Is there an update to the target milestone (still listed as 1.9a1)?  And doesn't this qualify for 'Blocker' or at least 'Critical' severity since the application can't run at all?

Comment 36

[(not currently active) Ted Mielczarek]

Ok, I've done some investigation.  XULRunner on windows doesn't ship any binary components in components/.  Therefore, if we just embed a manifest in all of our DLLs, we should be in good shape.  I tested this with the JavaXPCOM tests and the XR stub.  I will have to patch NSPR to ensure that it gets manifests in everything.  XR app authors and extension authors will have to statically link to the CRT in any components they author, but I think this is a workable situation.

Comment 37

[(not currently active) Ted Mielczarek]

Attachment: embed manifests in everything

Comment 38

[(not currently active) Ted Mielczarek]

Attachment: embed manifests in nspr libs

And for NSPR as well.

Comment 39

[Wan-Teh Chang]

Comment on attachment 262549 [details] [diff] [review]
embed manifests in nspr libs

This patch looks okay.  I'd prefer a patch that's similar to the NSS patch, which uses a variable named MT whose value is mt.exe.

Comment 40

[(not currently active) Ted Mielczarek]

Attachment: embed manifests in nspr libs, use $(MT)

Is this better?

Comment 41

[Wan-Teh Chang]

Comment on attachment 267580 [details] [diff] [review]
embed manifests in nspr libs, use $(MT)

r=wtc.

Comment 42

[Wan-Teh Chang]

Comment on attachment 267580 [details] [diff] [review]
embed manifests in nspr libs, use $(MT)

Ted, just curious: what do the ;1 and ;2 at the end of the
-OUTPUTRESOURCE option mean?  I noticed that
mozilla/xpinstall/stub/Makefile.in were using ;17 before.

Comment 43

[(not currently active) Ted Mielczarek]

That's the resource ID where the manifest is embedded.  Windows expects to read manifests at ID 1 for executables, and 2 for DLLs (usually, although you can do other things).  Anything above 16 is left for user-defined activities.  We were using 17 because we had some code in xpistub to manually create an activation context and load the manifest into it.  Since we're no longer using the xpinstaller, we don't need that anymore.  If anyone still needed it, they could make it load from resource ID 2 anyway.

Thanks for the review!

Comment 44

[(not currently active) Ted Mielczarek]

Comment on attachment 262548 [details] [diff] [review]
embed manifests in everything

Checked in on trunk.

Comment 45

[Wan-Teh Chang]

Attachment: embed manifests in nspr libs, use $(MT) (as checked in)

I made some minor changes.  I've checked in this patch on the
NSPR trunk (NSPR 4.7) and will check this in on the
NSPRPUB_PRE_4_2_CLIENT_BRANCH when the Mozilla trunk reopens.

Comment 46

[Wan-Teh Chang]

Attachment: embed manifests in nspr tests

We missed the NSPR test programs, which are built with a
rule in mozilla/nsprpub/pr/tests/Makefile.in.  This patch
takes care of that makefile rule.

Comment 47

[Wan-Teh Chang]

Comment on attachment 267803 [details] [diff] [review]
embed manifests in nspr tests

Note that I use -OUTPUTRESOURCE:$@\;1 to build the NSPR
test programs.

Comment 48

[Robert Kaiser]

I heavily suspect this has caused SeaMonkey Ts to increase from ~760ms to ~880ms on Windows (see http://build-graphs.mozilla.org/graph/query.cgi?testname=startup&units=ms&tbox=sea-win32-tbox&autoscale=1&days=7&avg=1 )

We currently have no test data for Firefox, but one reason for that may again be this checkin (that is the one in comment #44) as suggested in bug 383875

Comment 49

[Robert Kaiser]

Sorry for yet another comment, but that SeaMonkey perf data is backed by the other Windows tinderbox in SeaMonkey-Ports which went from ~2200ms to ~2600 on Ts, while the Mac box there and the Linux both on the SeaMonkey tinderbox page all stayed unchanged. Also other perf data than Ts stayed unchanged.

So overall, we have a 15-20% Windows-only Ts regression for SeaMonkey.

Comment 50

[:Gavin Sharp [email: [e-mail]]]

I backed out attachment 262548 [details] [diff] [review] to see if it would fix the perf test machine bustage that's keeping the tree closed (bl-bldxp01). It cycled once, and still crashed. I've triggered a clobber to see if that will fix it, but won't be able to stay around until we get the results (the cycle time is at least a couple of hours).

Comment 51

[Phil Ringnalda (:philor)]

And while the backout didn't make bl-bldxp01 happy, it did drop Ts on sea-win32-tbox from 891 the run before to 750 the run after, so more than "heavily suspect."

Comment 52

[Joe Sabash [:JoeS1]]

The backout seems to fixed Thunderbird trunk also.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070610 Thunderbird/3.0a1pre ID:2007061003 [cairo]
Starts and runs fine here, winxp sp2 (previously crashed on startup)

Comment 53

[(not currently active) Ted Mielczarek]

Ok, so there are two separate issues here, both of which suck.  There's some sort of startup crash, and there's a Ts hit.  I'm not entirely surprised by a Ts hit, since the DLL loader probably has to parse the manifest out of every DLL now.  It'd be interesting to compare the effect on Firefox since it's a static build.  The crash is strange, I was able to reproduce it in a VM, and I submitted a crash report here:
https://crash-reports.mozilla.com/reports/report/index/27a59de6-1758-11dc-a54e-001a4bd43ed6

It's crashing in nsFrame, but lower in the stack it looks like we're doing XPCOM shutdown.  I don't understand that.

Comment 54

[Kai Engert (:KaiE:)]

With regards to NSPR, the patches from this bug are the only difference between HEAD and NSPRPUB_PRE_4_2_CLIENT_BRANCH (does not yet have the patch).

Note that we would like to obsolete the NSPRPUB_PRE_4_2_CLIENT_BRANCH, see bug 387992 and switch to snapshots of HEAD.

Is it ok for Mozilla trunk to pick up the changes from this bug, as checked in to NSPR HEAD?

Comment 55

[Benjamin Smedberg]

That should be fine. If it's not, the tinderbox will let us know ;-)

Comment 56

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

I think this is wanted-1.9, right? Remove it if not.

Comment 57

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

Actually, let's see if drivers feel that this is blocking-1.9.

Comment 58

[Benjamin Smedberg]

Well, I'd like for ted to try and reland it.

Comment 59

[(not currently active) Ted Mielczarek]

Comment on attachment 262548 [details] [diff] [review]
embed manifests in everything

Ok, I've rebuilt with this patch applied, and tested the resulting build on Vista and XPSP2 with no problems.  I'd like to check this back in.

Comment 60

[(not currently active) Ted Mielczarek]

Checked in.  Hopefully it sticks this time!

Comment 61

[Robert Kaiser]

SeaMonkey Ts regressed again, like in comment #48 and comment #49 - I filed that as bug 350616 so that it's tracked.
I guess SeaMonkey will need to switch to libxul to fix that again, which is a target anyways.

Comment 62

[Robert Kaiser]

Umm, sorry, wrong bug number. The Ts regression is bug 394470, actually.