274542 - Official Firefox installer contains evil dead AccessibleMarshal.dll

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[Aaron Leventhal]

The AccessibleMarshal.dll's job is to let external Windows apps use some COM API
extensions such as ISimpleDOMNode and ISimpleDOMText, which allow screen readers
to get DOM info.
The good version of this dll comes with Seamonkey installers, in official
Firefox zip files, and installers that I build on my system. I use 7zip and uxp.

Good AccessibleMarshal.dll: 8704 bytes
Evil AccessibleMarshal.dll: 30835 bytes and marshalling is broken.

Some debugging ideas:
* debug lines in the official installer scripts to dump the filesize of
AccessibleMarshal.dll after each step to determine what's bloating/hosing it.
* temporarily turn off MOZ_INSTALLER_USE_7ZIP to see if 7zip or uxp is doing it
* temporarily turn off rebasing/stripping to see if they're doing it

Comment 1

[Aaron Leventhal]

Chase, do you have to look at this to see what build step is creating the dead
bloated version of AccessibleMarshal.dll?

Comment 2

[Aaron Leventhal]

*** Bug 273896 has been marked as a duplicate of this bug. ***

Comment 3

[Chase Phillips]

Attachment: Do not rebase AccessibleMarshal.dll, output rebase list

Keep AccessibleMarshal.dll from being rebased and output what files will be
rebased in the process.

Comment 4

[Aaron Leventhal]

Comment on attachment 171696 [details] [diff] [review]
Do not rebase AccessibleMarshal.dll, output rebase list

I'd put a comment in there something like:
# Remove AccessibleMarshal from rebase list, because it is a COM dll. Rebasing
would break it.

Comment 5

[Benjamin Smedberg]

Why is rebasing screwing up AccessibleMarshal? There is no reason why COM DLLs
cannot be rebased in general.

Comment 6

[Aaron Leventhal]

(In reply to comment #5)
> Why is rebasing screwing up AccessibleMarshal? There is no reason why COM DLLs
> cannot be rebased in general.

Doesn't the rebasing act on the dll's as a group?

I don't know much about rebasing, but I would think that the XPCOM and COM dll's
should be treated as separate groups.

Comment 7

[Aaron Leventhal]

Hmm, a strange thing is that when I enable rebasing and build an installer on my
machine, there is no problem.

But, it would be nice even to have a temporary fix to this problem as it is
preventing me from working with screen reader developers on DOM support.

Comment 8

[Benjamin Smedberg]

cmp, I rather don't like this patch as a long-term solution, but I think that we
perhaps should apply it to the build machine as a temporary fix. Can you tell me
what "rebase" you're using on the build machine? We might want to try a newer
version to see if it stops corruption. It is also possible that there is a
non-MSVC "rebase" command in the path; google says that there is a bogus
"rebase" that comes with cygwin apache.

http://www.cygwin.com/ml/cygwin/2003-04/msg00747.html

Comment 9

[Aaron Leventhal]

Chase, bsmedberg also mentioned that rebase should never change the size of the
target dll. It's only modifying preset address ranges.

Comment 10

[Chase Phillips]

Running 'dumpbin' on AccessibleMarshal.dll from beast shows:

  Summary
    1000 .data
    5000 .orpc
    2000 .rdata
    1000 .reloc
    1000 .rsrc
    1000 .text

'which rebase' on beast is:

  /cygdrive/c/Program Files/Microsoft Visual Studio/VC98/bin/rebase

Comment 11

[Chase Phillips]

In your report you mention:

  The good version of this dll comes with Seamonkey installers, in official
  Firefox zip files, and installers that I build on my system. I use 7zip and uxp.

When was the last time you tested AccessibleMarshal.dll in an official Firefox
zip file?  I've looked closely at sizes and MD5 sums from my latest build and
have seen some interesting results.

The version in mozilla/accessible/public/msaa/:

  Size = 30836 bytes
  MD5 = 63e8d0d804b1424c62d42964979d606f

The version in mozilla/dist/bin/:

  Size = 30836
  MD5 = 805ac727251305514e9390e0dd21e1dc

The version in the zip file:

  Size = 30836 bytes
  MD5 = 805ac727251305514e9390e0dd21e1dc

The version in browser.xpi:

  Size = 30836 bytes
  MD5 = 805ac727251305514e9390e0dd21e1dc

This suggests that the version in the zip file is as broken as the version in
the installer.  Can you help me confirm this?

The files I'm testing can be found at:

  http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2005-01-19-16-trunk/

Comment 12

[Aaron Leventhal]

Yes, the zip version is broken too. I was certain when I tested last month that
the zip version of the AccessibleMarshal.dll was okay.

Comment 13

[Chase Phillips]

Aaron,

Could you try out the zip build at:

  http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2005-01-19-17-trunk/

I've replaced the normal AccessibleMarshal.dll with a "virgin" copy.  I'd be
interested to know if this version (which is taken directly from beast's
accessible/public/msaa/ directory) produces positive results for you.

Comment 14

[Aaron Leventhal]

(In reply to comment #13)
> I'd be interested to know if this version (which is taken directly from beast's
> accessible/public/msaa/ directory) produces positive results for you.
I've actually done that test already. In fact, using the "virgin" dll does
produce positive results. What does that tell us? That whatever in the install
process is bloating the dll is in fact screwing it up. What's doing that?

Comment 15

[Aaron Leventhal]

Comment on attachment 171696 [details] [diff] [review]
Do not rebase AccessibleMarshal.dll, output rebase list

The AccessibleMarshal.dll from beast's accessible/public/msaa directory is also
broken, so it's not rebasing or anything in the installer build process. The
dll is broken as soon as it's made.

Comment 16

[Chase Phillips]

Aaron reports that a version of AccessibleMarshal.dll built from beast with
MOZ_DEBUG_SYMBOLS undefined (off) also does not work.

Comment 17

[Aaron Leventhal]

I'm a little embarrassed, but I think I've been giving wrong info.

It doesn't look like any of the AccessibleMarshal.dll's produced by mozilla.org
have ever worked.

I was fooled because of the way regsvr32 works. All the official builds I used
were actually using the AccessibleMarshal.dll I register when I build my own.

Comment 18

[Aaron Leventhal]

Attachment: dumpbin /symbols for the good dll

Comment 19

[Aaron Leventhal]

Attachment: dumpbin /symbols for the bad dll

Comment 20

[Chase Phillips]

Changing the summary from
  "Official Firefox installer contains evil bloated dead AccessibleMarshal.dll"
to
  "Official Firefox installer contains evil dead AccessibleMarshal.dll"

This helps to reflect that we aren't certain that bloating of
AccessibleMarshal.dll factors into this particular bug.

Comment 21

[Chase Phillips]

Comment on attachment 171947 [details]
dumpbin /symbols for the good dll

Noting differences in this output compared to the output of dumpbin on the bad
dll, the difference in MSVCRT.DLL and MSVCR71.DLL should be due to the
difference in compilers.

beast uses vc6 and the machine from which the good dll is made uses vc71.

Here's the output of 'cl' on beast:

Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 12.00.8804 for 80x86
Copyright (C) Microsoft Corp 1984-1998. All rights reserved.

Comment 22

[Aaron Leventhal]

Attachment: Good build log from Aaron's machine

Comment 23

[Aaron Leventhal]

Attachment: Log from accessible/public on Aaron's machine, builds a good version of the dll

Comment 24

[Aaron Leventhal]

Here's a difference:
Good:
/cygdrive/c/moz/mozilla/build/cygwin-wrapper midl /no_robust
/cygdrive/c/moz/mozilla/accessible/public/msaa/ISimpleDOMDocument.idl
Microsoft (R) 32b/64b MIDL Compiler Version 6.00.031

Bad:
/cygdrive/c/builds/tinderbox/Fx-Trunk/WINNT_5.0_Clobber/mozilla/build/cygwin-wrapper
midl 
/cygdrive/c/builds/tinderbox/Fx-Trunk/WINNT_5.0_Clobber/mozilla/accessible/public/msaa/ISimpleDOMNode.idl
Microsoft (R) MIDL Compiler Version 5.01.0164  

So, the official builds use an older version of midl.
Because of this, configure decides not to use /no_robust

http://lxr.mozilla.org/seamonkey/source/configure#2851

Comment 25

[Chase Phillips]

Bug 208461 deals with a change that occurred in midl semantics in later versions
of VC.  That bug includes some history on patches made to support midl's
"/no_robust" argument that appeared in versions of midl beginning with 6.0.359.

Comment 26

[Chase Phillips]

Per suggestion from Aaron I set MIDL_FLAGS to "/Oicf".  The resulting build can
be found at:

  http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2005-01-20-22-trunk/

The zip distribution has an AccessibleMarshal.dll that is 8308 bytes.  Let us
know if this file works where the others didn't.

Comment 27

[Aaron Leventhal]

(In reply to comment #26)
> Per suggestion from Aaron I set MIDL_FLAGS to "/Oicf".  The resulting build can
> be found at: ...

This one works both for me and Sylvain, who is CC'd in this bug.

Perhaps we should be using /Oicf across the board. It makes the resulting dll
smaller, at least in this case. Not to mention the marshalling now works.

Comment 28

[Aaron Leventhal]

MSDN on /Oicf:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/midl/midl/_oi.asp

"Oif or Oicf
    Specifies the codeless proxy method of marshaling that includes all the
features provided by /Oi and /Oic but uses a new interpreter (fast format
strings) that provides better performance than /Oi or /Oic."

"Starting with MIDL version 6.0.359, the MIDL compiler generates /Oicf /robust
stubs by default. Some language features are not supported in some modes. In
this case, the compiler automatically switches to the appropriate mode and
issues a warning."

"/Oic is supported on Windows NT 3.51 or later and Windows 95 or later.
/Oif is supported on Windows NT 4.0"

That's enough compatibility for me.

Comment 29

[Chase Phillips]

(In reply to comment #27)
> Perhaps we should be using /Oicf across the board. It makes the resulting dll
> smaller, at least in this case. Not to mention the marshalling now works.

The safest fix for this bug is to change just the midl invocations within
public/accessible/.  I'll come up with a patch for that and attach it here.

Comment 30

[Chase Phillips]

Attachment: Add /Oicf to accessible/public/msaa's midl invocations

Comment 31

[Aaron Leventhal]

Comment on attachment 172038 [details] [diff] [review]
Add /Oicf to accessible/public/msaa's midl invocations

I still think it's better to use /Oicf throughout the code, after reading what
msdn says. But okay with me, as long as AccessibleMarshal.dll works :)

Comment 32

[Benjamin Smedberg]

Comment on attachment 172038 [details] [diff] [review]
Add /Oicf to accessible/public/msaa's midl invocations

We should talk with adamlock about changing to /Oicf globally.

Comment 33

[Chase Phillips]

Comment on attachment 172038 [details] [diff] [review]
Add /Oicf to accessible/public/msaa's midl invocations

Checking in Makefile.in;
/cvsroot/mozilla/accessible/public/msaa/Makefile.in,v  <--  Makefile.in
new revision: 1.10; previous revision: 1.9
done

Comment 34

[timeless]

aaronl: is enough of accessibility available on nt3.51 for me to worry about
this change? my guess is it isn't, but i don't have the time to pull our my MSAA
disks...

Comment 35

[[e-mail]]

I don't think W95 supports accessibility out of the box either.

Comment 36

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381