Turn on building IDispatch code by default

VERIFIED FIXED in mozilla1.4beta

Status

()

VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: dbradley, Assigned: netscape)

Tracking

Trunk
mozilla1.4beta
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

16 years ago
This only turn on the building of the code. It will not actually provide
IDispatch support to Mozilla builds.
(Reporter)

Updated

16 years ago
Blocks: 190852
(Reporter)

Comment 1

16 years ago
We need to get this into 1.4
Target Milestone: --- → mozilla1.4beta
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.
Status: NEW → ASSIGNED
Flags: blocking1.4?

Comment 3

16 years ago
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.
(Reporter)

Comment 4

16 years ago
>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
Created attachment 121763 [details] [diff] [review]
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/.
Created attachment 121764 [details] [diff] [review]
IsEqualGUID v1.0

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

Comment 7

16 years ago
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
Attachment #121764 - Flags: review+

Comment 8

16 years ago
We still need to ensure activex.js and nsAxSecurityPolicy.js are exported from
plugin/ even if npmozax.dll and nsIMozAxPlugin.xpt are not.
(Reporter)

Comment 9

16 years ago
activex.js probably doesn't matter, since if it's not there, the result is a
blank whitelist.
(Reporter)

Comment 10

16 years ago
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

16 years ago
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

16 years ago
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
(Reporter)

Comment 13

16 years ago
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

16 years ago
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.
Created attachment 121938 [details] [diff] [review]
v1.1

Fixes mingw builds.
Attachment #121763 - Attachment is obsolete: true
(Reporter)

Comment 16

16 years ago
So are we good to go with this, reviews?

Comment 17

16 years ago
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
Attachment #121938 - Flags: review+
(Reporter)

Comment 18

16 years ago
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.
(Reporter)

Comment 19

16 years ago
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)
Flags: blocking1.4b?
(Reporter)

Comment 20

16 years ago
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
Attachment #121764 - Flags: superreview?(alecf)
Attachment #121764 - Flags: approval1.4b?
(Reporter)

Comment 21

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

Seeking sr to build IDispatch code as part of the default build.
Attachment #121938 - Flags: superreview?(alecf)

Comment 22

16 years ago
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
Attachment #121938 - Flags: superreview?(alecf)
Attachment #121938 - Flags: superreview+
Attachment #121938 - Flags: review?(seawood)
Attachment #121938 - Flags: review+

Comment 23

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

sr=alecf
Attachment #121764 - Flags: superreview?(alecf) → superreview+
(Reporter)

Comment 24

16 years ago
He created the patch, so wasn't sure who would need to review.
(Reporter)

Comment 25

16 years ago
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
Attachment #121938 - Flags: review?(seawood) → review?(bryner)

Comment 26

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

a=asa (on behalf of drivers) for checkin to 1.4b
Attachment #121764 - Flags: approval1.4b? → approval1.4b+
Attachment #121938 - Flags: review?(bryner) → review+
(Reporter)

Comment 27

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

Seeking approval for this second patch, this actually turns on building
IDispatch support by default.
Attachment #121938 - Flags: approval1.4b?
Created attachment 122053 [details] [diff] [review]
v2.0

Small add-on to v1.1 to remove bogus dependency from $(SHARED_LIBRARY) rule.
Comment on attachment 121938 [details] [diff] [review]
v1.1

a=sspitzer
Attachment #121938 - Flags: approval1.4b? → approval1.4b+
(Reporter)

Comment 30

16 years ago
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.
(Reporter)

Comment 31

16 years ago
Ok, after cls set me straight on configure.in and configure, this is now checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 32

16 years ago
Checkin verified -
Status: RESOLVED → VERIFIED
(Reporter)

Comment 33

16 years ago
Removing blocking 1.4b since this is checked in
Flags: blocking1.4b?
Flags: blocking1.4?

Comment 34

16 years ago
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? 
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

16 years ago
Created attachment 122253 [details] [diff] [review]
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

16 years ago
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

16 years ago
Created attachment 122255 [details] [diff] [review]
Configure patch

Clean patch
Attachment #122253 - Attachment is obsolete: true

Comment 39

16 years ago
Created attachment 122302 [details] [diff] [review]
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.
Attachment #122255 - Attachment is obsolete: true

Comment 40

16 years ago
Comment on attachment 122302 [details] [diff] [review]
Configure patch

Chris, can you review this patch to improve the build flags please? Thanks
Attachment #122302 - Flags: review?(seawood)
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.
Attachment #122302 - Flags: review?(seawood) → review-
(Reporter)

Comment 42

16 years ago
Do we want to reopen this or should we move further patches to the bryner's bug
204292?
You need to log in before you can comment on or make changes to this bug.