203362 - Turn on building IDispatch code by default

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[David Bradley]

This only turn on the building of the code. It will not actually provide
IDispatch support to Mozilla builds.

Comment 1

[David Bradley]

We need to get this into 1.4

Comment 2

[hacker formerly known as [e-mail]]

Strictly speaking, that's not true, right?  IDispatch will be built into
xpconnect so it will be available to Mozilla builds.  What won't be available is
the activex plugin.

Comment 3

[Adam Lock]

The patch would have to set --enable-activex-scripting in the configure script,
but also supply patches to lock down the default settings for the functionality
so it did nothing at all. That means activex.js & nsAxSecurityPolicy.js as well
as any internal settings need to assume the strictest values by default.

I think the plugin should still be built if the switch is on, but the package
lists can be changed so it is not actually packaged and shipped.

Comment 4

[David Bradley]

>IDispatch will be built into xpconnect so it will be available to Mozilla builds.

Yes and no. To clarify, IDispatch code will be there, but there will be no way
to instantiate an ActiveX/COM component via JS code since default security
policty will block any such attempt.

And I agree the patch for activex.js & nsAxSecurityPolicy.js should go here.

As far as how ActiveX plugin is not shiped, either not built or not packaged, I
don't have a preference/opinoin

Comment 5

[hacker formerly known as [e-mail]]

Attachment: v1.0

This patch turns activex-scripting support (IDispatch) on for win32 by default.
 It builds but does not export the activex plugin files to dist/.

Comment 6

[hacker formerly known as [e-mail]]

Attachment: IsEqualGUID v1.0

This patch implements the workaround proposed in bug 127982 so that I can build
with the MS Platform SDK installed.

Comment 7

[David Bradley]

Comment on attachment 121764 [details] [diff] [review]
IsEqualGUID v1.0

r=dbradley

Thanks for fixing this. There's a couple of instances in the ActiveX code as
well http://lxr.mozilla.org/seamonkey/search?string=InlineIsEqualGUID

Comment 8

[Adam Lock]

We still need to ensure activex.js and nsAxSecurityPolicy.js are exported from
plugin/ even if npmozax.dll and nsIMozAxPlugin.xpt are not.

Comment 9

[David Bradley]

activex.js probably doesn't matter, since if it's not there, the result is a
blank whitelist.

Comment 10

[David Bradley]

Actually I wonder if it would be better if we could do without
nsAxSecurityPolicy.js as well. If it's not there, we just default to hosting
nothing right?

Comment 11

[Adam Lock]

I will have to check the code. Certain places hardcode values for hosting flags
in the absence of a service or pref to tell them otherwise. I am sure the
default hosting flags are not absolute and I'll have to check places where the
security service is called to make sure it doesn't fall over with errors when it
is absent.

Comment 12

[Adam Lock]

We seem to be safe concerning the absence of nsAxSecurityPolicy.js, the
nsDispatchSupport object calls it to return the hosting flags but returns 0 if
there is no service.

http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/nsDispatchSupport.cpp#323

The plugin should also be safe since it defaults to 0 when there is no pref but
it needs some work. It asks for the hosting flags directly from the prefs which
is wrong and allows the value to be overridden. Since it grabs the
nsDispatchSupport object nearby it should be calling that for the values. I'll
work a patch to fix this behaviour. I don't think it needs to care about
monitoring the prefs at all in COM connect mode.

http://lxr.mozilla.org/seamonkey/source/embedding/browser/activex/src/plugin/PrefObserver.cpp#59

Comment 13

[David Bradley]

Since this deals with not shipping DLL, this bug wouldn't be dependant on fixing
issues in the DLL, right? We should be able to go forward with this.

Comment 14

[Adam Lock]

Yes, the issues in the plugin can be fixed in bug 203704. It looks like the
dispatch support service is happy to return 0 in the hosting flags if the
nsAxSecurityPolicy is not present.

Comment 15

[hacker formerly known as [e-mail]]

Attachment: v1.1

Fixes mingw builds.

Comment 16

[David Bradley]

So are we good to go with this, reviews?

Comment 17

[Adam Lock]

Comment on attachment 121938 [details] [diff] [review]
v1.1

Does there need to be an entry for MOZ_USE_ACTIVEX_PLUGIN in autoconf.mk.in?

Otherwise r=adamlock

Comment 18

[David Bradley]

Seeking 1.4b blocking status.

We want to get this into beta so that we have some time running with it before
1.4 final.

Comment 19

[David Bradley]

Seeking 1.4b blocking status.

We want to get this into beta so that we have some time running with it before
1.4 final.

(collision happened and the blocking status got reverted when I went back, sorry
for the extra comment)

Comment 20

[David Bradley]

Comment on attachment 121764 [details] [diff] [review]
IsEqualGUID v1.0

Seeking sr/a for small mingw patch so we don't have problems when IDispatch is
built by default

Comment 21

[David Bradley]

Comment on attachment 121938 [details] [diff] [review]
v1.1

Seeking sr to build IDispatch code as part of the default build.

Comment 22

[Alec Flett]

Comment on attachment 121938 [details] [diff] [review]
v1.1

sr=alecf but seawood needs to be the reviewer for build-level stuff like
configure.in

Comment 23

[Alec Flett]

Comment on attachment 121764 [details] [diff] [review]
IsEqualGUID v1.0

sr=alecf

Comment 24

[David Bradley]

He created the patch, so wasn't sure who would need to review.

Comment 25

[David Bradley]

Comment on attachment 121938 [details] [diff] [review]
v1.1

bryner, would you be able to review this. We need to get this reviewed ASAP so
we can get it checked in for 1.4b

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 121764 [details] [diff] [review]
IsEqualGUID v1.0

a=asa (on behalf of drivers) for checkin to 1.4b

Comment 27

[David Bradley]

Comment on attachment 121938 [details] [diff] [review]
v1.1

Seeking approval for this second patch, this actually turns on building
IDispatch support by default.

Comment 28

[hacker formerly known as [e-mail]]

Attachment: v2.0

Small add-on to v1.1 to remove bogus dependency from $(SHARED_LIBRARY) rule.

Comment 29

[(not reading, please use [e-mail] instead)]

Comment on attachment 121938 [details] [diff] [review]
v1.1

a=sspitzer

Comment 30

[David Bradley]

I checked in the patch, but now I'm not sure we're quite where we need to be. I
don't think XPC_IDISPATCH_SUPPORT is getting defined in the default build.
Unfortunately I have that defined in my environment so didn't notice when I was
building with the patch and thought things were working ok.

Comment 31

[David Bradley]

Ok, after cls set me straight on configure.in and configure, this is now checked in.

Comment 32

[Phil Schwartau]

Checkin verified -

Comment 33

[David Bradley]

Removing blocking 1.4b since this is checked in

Comment 34

[Scott MacGregor]

Comment from the peanut gallery...I currently build with activex disabled. This
patch now causes my build to break. I get an error from configure.in about
enabling active x scripting support when active x support is disabled.

That seems weird. If I disable activex then activex scripting should be disabled
automatically. Do we really need to say if you don't want active X you need to
set two separate build config options? That doesn't seem efficient.

Why can't disable-activex implicitly disable active-x scripting? Otherwise, what
good is disable-activex? 

Comment 35

[hacker formerly known as [e-mail]]

Because apparently, --disable-activex is only supposed to apply to the activex
control and not everything activex related.  We're still hashing out the details
outside of bugzilla.

Comment 36

[Adam Lock]

Attachment: Configure patch

This is a new configure patch to make the activex behaviour easier to
understand. Default behaviour is

--enable-activex-control
--enable-activex-scripting
--disable-activex-plugin

The plugin will not be built at all by default, but may be enabled with
--enable-activex-plugin. The --disable-activex-control switch replaces
--disable-activex which was non-obvious with the plugin and scripting
potentially being built as well.

The configure script attempts to check for odd combinations, such as asking for
activex on a unsupported platform.

The plugin's Makefile.in must remove the ifdef MOZ_USE_ACTIVEX_PLUGIN but that
isn't included in the patch.

Comment 37

[Adam Lock]

Ignore the non ActiveX related changes in that that configure.in file, something
screwed up in my CVS, I'll try and submit a clean patch

Comment 38

[Adam Lock]

Attachment: Configure patch

Clean patch

Comment 39

[locka]

Attachment: Configure patch

Minor tweak after testing on Linux, to move a Windows specific test before the
plugin dependency on scripting test to ensure a more meaningful error.

Patch also removes the ifdef MOZ_USE_ACTIVEX_PLUGIN in the plugin which should
not be needed any more with this patch.

Comment 40

[Adam Lock]

Comment on attachment 122302 [details] [diff] [review]
Configure patch

Chris, can you review this patch to improve the build flags please? Thanks

Comment 41

[hacker formerly known as [e-mail]]

Comment on attachment 122302 [details] [diff] [review]
Configure patch

Major confusion here.  First, get rid of the MOZ_NO_feature variables. 
MOZ_feature is preferred.  Second, why is MOZ_NO_ACTIVEX_SUPPORT still around
if the option which controlled it was replaced by one that used
MOZ_NO_ACTIVEX_CONTROL_SUPPORT?  I don't see the need for that whole series of
checks just to set MOZ_NO_ACTIVEX_SUPPORT since it's only used in one other
place. In embedding/browser/Makefile.in, it should check to see if any of the
activex variables are set, not just MOZ_NO_ACTIVEX_SUPPORT.  
In embedding/browser/activex/src/Makefile.in, you're using the plugin support
variable to check whether to build the control.

Comment 42

[David Bradley]

Do we want to reopen this or should we move further patches to the bryner's bug
204292?