751541 - Review and land metro specific toolkit/library changes

Status: RESOLVED FIXED

(Toolkit Graveyard :: Build Config, defect)

Description

[Jim Mathies [:jimm]]

We haven't added much here but we did tweak a few things.

Comment 1

[Jim Mathies [:jimm]]

Attachment: vcorlib shim for non-win8 platforms v.1

Comment 2

[Jim Mathies [:jimm]]

Attachment: vcorlib shim for non-win8 platforms v.1

Ehsan since you've worked with these hooks before seems like you would be a good reviewer for this code.

This addresses an issue with component extensions and vc11 on non-win8 platforms. The comment in dll main explains the situation pretty well. Assuming we go with vc11 for our builds, which seems likely.

Comment 3

[(no longer active)]

Comment on attachment 624028 [details] [diff] [review]
vcorlib shim for non-win8 platforms v.1

Review of attachment 624028 [details] [diff] [review]:
-----------------------------------------------------------------

This looks generally good.  I have made a fair number of nits, and some actual comments.  r=me with those addressed.  Also, please ask review from a build system peer.  The build system changes look benign, but one never knows.  ;-)

::: toolkit/library/nsDllMain.cpp
@@ +100,5 @@
> +  osInfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX);
> +  ::GetVersionEx((OSVERSIONINFO*)&osInfo);
> +  version =
> +    (osInfo.dwMajorVersion & 0xff) << 8 | (osInfo.dwMinorVersion & 0xff);
> +  return (version >= 0x602);

This matches both Windows 8 and Windows 8 server (whatever Microsoft ends up calling it.)  I'm assuming that this OK, but please add a comment mentioning this explicitly.

@@ +106,5 @@
> +
> +const char* kvccorlib = "vccorlib";
> +const char* kwinrtprelim = "api-ms-win-core-winrt";
> +
> +static bool shouldHandleLib(PDelayLoadInfo pdli, const char* aLibToken)

Nit: please use a better name, such as IsWinRTDLLPresent.  Also, please use CapsCamel.  :-)

@@ +109,5 @@
> +
> +static bool shouldHandleLib(PDelayLoadInfo pdli, const char* aLibToken)
> +{
> +  if(!IsWin8OrHigher() && pdli->szDll &&
> +     !strnicmp(pdli->szDll, aLibToken, strlen(aLibToken))) {

Can we also include the .dll suffix here?  This will potentially also match vccorlibcustom.dll...

@@ +111,5 @@
> +{
> +  if(!IsWin8OrHigher() && pdli->szDll &&
> +     !strnicmp(pdli->szDll, aLibToken, strlen(aLibToken))) {
> +    return true;
> +  }

Nit: just return the if condition.

@@ +124,5 @@
> +    }
> +#ifdef DEBUG
> +    if (shouldHandleLib(pdli, kwinrtprelim)) {
> +      NS_WARNING("Attempting to load winrt libs in non-metro environment. "
> +                 "(Winrt variable type placed in global scope?)");

This should be a fatal assertion IMO.  Also, why is this check only enabled in debug builds?

@@ +131,5 @@
> +  }
> +  return NULL;
> +}
> +
> +PfnDliHook __pfnDliNotifyHook2 = delayDllLoadHook;

Hmm, this should be EXTERN_C, right?

::: toolkit/library/winvccorlib/dummyvccorlib.cpp
@@ +7,5 @@
> +
> +// A dummy vccorlib.dll for shunting winrt initialization calls when we are not
> +// using the winrt library. For a longer explanantion see nsDllMain.cpp.
> +
> +BOOL APIENTRY DllMain( HMODULE hModule,

Nit: no whitespace after ( or before ).

@@ +12,5 @@
> +                       DWORD  ul_reason_for_call,
> +                       LPVOID lpReserved
> +					 )
> +{
> +	switch (ul_reason_for_call)

Nit: please use spaces.

@@ +21,5 @@
> +	case DLL_PROCESS_DETACH:
> +		break;
> +	}
> +	return TRUE;
> +}

Also, this DllMain seems to be completely redundant.

Comment 4

[Jim Mathies [:jimm]]

Attachment: vcorlib shim for non-win8 platforms v.2

(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> Comment on attachment 624028 [details] [diff] [review]
> vcorlib shim for non-win8 platforms v.1
> 
> Review of attachment 624028 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks generally good.  I have made a fair number of nits, and some
> actual comments.  r=me with those addressed.  Also, please ask review from a
> build system peer.  The build system changes look benign, but one never
> knows.  ;-)

Thanks!

> ::: toolkit/library/nsDllMain.cpp
> @@ +100,5 @@
> > +  osInfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX);
> > +  ::GetVersionEx((OSVERSIONINFO*)&osInfo);
> > +  version =
> > +    (osInfo.dwMajorVersion & 0xff) << 8 | (osInfo.dwMinorVersion & 0xff);
> > +  return (version >= 0x602);
> 
> This matches both Windows 8 and Windows 8 server (whatever Microsoft ends up
> calling it.)  I'm assuming that this OK, but please add a comment mentioning
> this explicitly.

I would assume the new runtime will be present on all os greater to or equal to win8, including server. I've added a comment. 

> 
> Can we also include the .dll suffix here?  This will potentially also match
> vccorlibcustom.dll...

No, there's a version number on the end of the filename. The dummy lib is dummyvccorlib so it's safe. It would never result in a call here anyway because we load it directly with LoadLibrary.

> This should be a fatal assertion IMO.  Also, why is this check only enabled
> in debug builds?

Didn't want to add the overhead to release builds. Not a big deal though. I've taken the ifdefing out in the updated patch.

> @@ +131,5 @@
> > +  }
> > +  return NULL;
> > +}
> > +
> > +PfnDliHook __pfnDliNotifyHook2 = delayDllLoadHook;
> 
> Hmm, this should be EXTERN_C, right?

ExternC I guess based on the header, although this works just fine without it. Updated.

> @@ +21,5 @@
> > +	case DLL_PROCESS_DETACH:
> > +		break;
> > +	}
> > +	return TRUE;
> > +}
> 
> Also, this DllMain seems to be completely redundant.

I nixed all the default project code.

Comment 5

[Jim Mathies [:jimm]]

Just noticed, the 'uiautomationcore' dll changes are from another patch. I'll strip that out in the final.

Comment 6

[Jim Mathies [:jimm]]

Attachment: vcorlib shim for non-win8 platforms v.2

Updated to remove the uiautomation core delay load code, and fixed the assertion which had a reversed test.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 625084 [details] [diff] [review]
vcorlib shim for non-win8 platforms v.2

Review of attachment 625084 [details] [diff] [review]:
-----------------------------------------------------------------

I want bsmedberg to look at this delay loading stuff.  We've had trouble with that and the dependent libs list in the past.

::: configure.in
@@ +677,5 @@
> +    # toolkit/library/makefile.in needs these, see nsDllMain.
> +    CRTDLLVERSION=110
> +    CRTEXPDLLVERSION=1-1-0
> +    AC_SUBST(CRTDLLVERSION)
> +    AC_SUBST(CRTEXPDLLVERSION)

Please AC_SUBST these variables unconditionally.

Comment 8

[Benjamin Smedberg]

Comment on attachment 625084 [details] [diff] [review]
vcorlib shim for non-win8 platforms v.2

I'm not sure why we need the variables CRTDLLVERSION and such in configure.in instead of just putting them in toolkit/library/Makefile.in, but I also don't think it really matters.

Comment 9

[Jim Mathies [:jimm]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> Comment on attachment 625084 [details] [diff] [review]
> vcorlib shim for non-win8 platforms v.2
> 
> I'm not sure why we need the variables CRTDLLVERSION and such in
> configure.in instead of just putting them in toolkit/library/Makefile.in,
> but I also don't think it really matters.

I put them there because when we update WINSDK_TARGETVER we'll also need to update the crt version numbers.

Comment 10

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40ba9d985ba3

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/40ba9d985ba3