Closed Bug 751541 Opened 8 years ago Closed 8 years ago

Review and land metro specific toolkit/library changes

Categories

(Toolkit Graveyard :: Build Config, defect)

x86_64
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla15

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(1 file, 3 obsolete files)

We haven't added much here but we did tweak a few things.
Depends on: 732124
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.
Attachment #622709 - Attachment is obsolete: true
Attachment #624028 - Flags: review?(ehsan)
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.
Attachment #624028 - Flags: review?(ehsan) → review+
(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.
Assignee: nobody → jmathies
Attachment #624028 - Attachment is obsolete: true
Attachment #624723 - Flags: superreview?(khuey)
Attachment #624723 - Flags: review+
Just noticed, the 'uiautomationcore' dll changes are from another patch. I'll strip that out in the final.
Updated to remove the uiautomation core delay load code, and fixed the assertion which had a reversed test.
Attachment #624723 - Attachment is obsolete: true
Attachment #624723 - Flags: superreview?(khuey)
Attachment #625084 - Flags: superreview?(khuey)
Attachment #625084 - Flags: review+
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.
Attachment #625084 - Flags: superreview?(khuey) → superreview?(benjamin)
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.
Attachment #625084 - Flags: superreview?(benjamin) → superreview+
(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.
https://hg.mozilla.org/mozilla-central/rev/40ba9d985ba3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.