1515982 - Invalid mozglue manifest resources

Status: RESOLVED FIXED

(Core :: mozglue, enhancement)

Description

[Jacek Caban]

I hit the problem with clang build that no longer work after bug 1496179 due to missing mozglue.dll manifest (we were missing all resources, but with bug 1506450 fixed that's no longer the case). The problem I found is, however, not exactly limited to mingw clang builds.

mozglue.rc includes manifest using RT_MANIFEST as resource type, but it doesn't include any system headers. RT_MANIFEST is not a RC script keyword, it's a define inside winuser.rh to integer constant (24). When winuser.rh is not included, resource compiler considers it as a custom string identifier and adds it as resource of type (string) "RT_MANIFEST" instead. Windows expects resources of type (int) 24 to be manifests, so it ignores the manifest.

The above is the main problem, but it does not explain why it works on clang-cl build (on which this manifest is ignored as well). I can see that clang-cl builds have two manifests. It contains both the problematic manifest and a second, slightly different, one. I don't see where does the second one come from.


Here is a try push of a trivial fix so that mozglue.rc works as intended:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce54ef5a995c790430cb839a2f60bc47046986ed&selectedJob=218355768

Comment 1

[Masatoshi Kimura [:emk]]

I guess the second mafest is embedded by MSMANIFEST_TOOL if it is avilable (e.g. clang-cl builds):
https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/config/rules.mk#718-733

The change looks good if it does not break clang-cl builds. It would be better to replace `RT_MANIFEST` with `24` to remove dependency on header files:
https://dxr.mozilla.org/mozilla-central/search?q=RT_MANIFEST

Comment 2

[Jacek Caban]

(In reply to Masatoshi Kimura [:emk] from comment #1)
> I guess the second mafest is embedded by MSMANIFEST_TOOL if it is avilable
> (e.g. clang-cl builds):
> https://dxr.mozilla.org/mozilla-central/rev/
> c2593a3058afdfeaac5c990e18794ee8257afe99/config/rules.mk#718-733

I see. LLVM provides llvm-mt that we could in theory try to use for mingw builds. I tried that and found that it doesn't support OUTPUTRESOURCE argument, so it would require adding support for that to LLVM first.

Adding manifest resources doesn't really seem like something we need a dedicated tool through. .rc files for those are rather trivial, we could generate them from build system and link like any other resources (instead of depending on mt.exe to modify binaries after linking).

> The change looks good if it does not break clang-cl builds. It would be
> better to replace `RT_MANIFEST` with `24` to remove dependency on header
> files:
> https://dxr.mozilla.org/mozilla-central/search?q=RT_MANIFEST

I will attach a patch doing that for mozglue (I'm not sure about some of other places).

Comment 3

[Jacek Caban]

Attachment: Bug 1515982 - Don't use RT_MANIFEST in mozglue.rc file.

It's a define that needs winuser.rh to be included.

MozReview-Commit-ID: LPfJOwnNm6V

Comment 4

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I seem to be hitting this problem still; even with the patches. I tried the constant (https://treeherder.mozilla.org/#/jobs?repo=try&revision=12e470f81af6733ce1f76fedd27f3d60ba0d9318) and the include (https://treeherder.mozilla.org/#/jobs?repo=try&revision=88dc032126655c8669b212f0056d0031353167be)

I tested the x64 Opt build and both builds fail for me on Windows 10. I'm not sure how to investigate the resulting builds to inspect the dll.

Comment 5

[Jacek Caban]

Here is an example of try push from December that worked:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e93871067f1aec8eadff7b32badc13b8f80b2c88
I noticed that your pushes included accessibility changes, maybe that reveals more problems. Let's try m-c with only this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4e67bac29c6502a1578f98e2f15c67edb7dc531

Comment 6

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(In reply to Jacek Caban from comment #5)

I noticed that your pushes included accessibility changes, maybe that reveals more problems. Let's try m-c with only this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4e67bac29c6502a1578f98e2f15c67edb7dc531

The two patches in question have been commited so I think this push includes them. I sent in a try run that backs them out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f5e28bbf5a7254b30b0ba388dfb12c1464aae07

Nonetheless, Neither your try run nor mine here run for me.

Here is an example of try push from December that worked:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e93871067f1aec8eadff7b32badc13b8f80b2c88

But most interestingly - this build does not run for me either. I'm testing the x64 opt build. I'll have to look into this more tomorrow...

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to Tom Ritter [:tjr] from comment #6)

Here is an example of try push from December that worked:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e93871067f1aec8eadff7b32badc13b8f80b2c88

But most interestingly - this build does not run for me either. I'm testing the x64 opt build. I'll have to look into this more tomorrow...

Windows Event Log would have some information if your problem is related with SxS assemblies.

Comment 8

[(Away)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e93871067f1aec8eadff7b32badc13b8f80b2c88

But most interestingly - this build does not run for me either. I'm testing the x64 opt build. I'll have to look into this more tomorrow...

The x64 opt build from that link works for me, but I do get a system ding and 5-second delay (similar to bug 1515826) that I can work around by disabling media.rdd-process.enabled.

Comment 9

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(In reply to David Major [:dmajor] from comment #8)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e93871067f1aec8eadff7b32badc13b8f80b2c88

But most interestingly - this build does not run for me either. I'm testing the x64 opt build. I'll have to look into this more tomorrow...

The x64 opt build from that link works for me, but I do get a system ding and 5-second delay (similar to bug 1515826) that I can work around by disabling media.rdd-process.enabled.

So that white-screen no chrome or tabs is exactly what I'm experiencing. But it doesn't go away for me after 5 second or 5 minutes.

The RDD pointer narrowed this down somewhat...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4c12777c8354f290a7a21bf083f29d26ac8f021 is that build + disabling RDD entirely. It does work for me. And an av1 demo I found does play for me.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e352b1ed953460f5f9d0d62d610b6aa263d2cc99 is a rebased to central version that includes the RDD delayed launch (I needed to backout the delayed launch backout). It works for me; except rhe av1 demo (http://demo.bitmovin.com/public/firefox/av1/ ) does not play and causes the browser to freeze.

So now that I've figured out that this is unrelated to the mozglue manifest resources, we can unstick this patch and land it and figure out the av1 issue elsewhere...

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87a1eddf108d
Don't use RT_MANIFEST in mozglue.rc file. r=froydnj

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/87a1eddf108d