767639 - Remove remaining ActiveX stub

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[John Schoenick [:johns]]

nsObjectLoadingContent has code related to trying to load an ActiveX plugin if we have classid="clsid:blah" and a helper-plugin that can actually support activeX. This should just go away as we've not had such a helper plugin in a long time

Comment 1

[John Schoenick [:johns]]

Attachment: Remove activeX stubs and simpify classid/java handling

Comment 2

[John Schoenick [:johns]]

Comment on attachment 649797 [details] [diff] [review]
Remove activeX stubs and simpify classid/java handling

This simplifies handling java a bit, and removes the special cases for classid now that classid can only be 'java:' URIs

In particular, objects like:
> <object type="application/x-java-vm" data="my.class"></object>

Wont spuriously open data streams that will never be used and possibly generate meaningless errors

Comment 3

[John Schoenick [:johns]]

Comment on attachment 649797 [details] [diff] [review]
Remove activeX stubs and simpify classid/java handling

https://hg.mozilla.org/integration/mozilla-inbound/rev/471f34ddcceb

try:
https://tbpl.mozilla.org/?tree=Try&rev=02f602af452c

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/471f34ddcceb

Comment 5

[Jacek Caban]

(In reply to John Schoenick [:johns] from comment #0)
> This should just go away as we've not had such a helper plugin in a long time

We re-used this functionality in Wine. We have MSHTML implementation based on our forked (but close to upstream) Gecko fork and our own pseudo-plugin that, in combination with our MSHTML, adds support for ActiveX components. All we need is Gecko to recognize classid="clsid:..." as application/x-oleobject MIME. Thus, the patch from this bug causes big regression for us. We may deal with it in our fork, but it's always nicer to not have downstream changes. And given that we need very little from Gecko, the maintenance cost of having such code is pretty low for Mozilla.

Would you please consider r+ for a patch reverting relevant parts of the patch? I will take care of that if you'd agree.

Comment 6

[John Schoenick [:johns]]

(In reply to Jacek Caban from comment #5)
> (In reply to John Schoenick [:johns] from comment #0)
> > This should just go away as we've not had such a helper plugin in a long time
> 
> We re-used this functionality in Wine. We have MSHTML implementation based
> on our forked (but close to upstream) Gecko fork and our own pseudo-plugin
> that, in combination with our MSHTML, adds support for ActiveX components.
> All we need is Gecko to recognize classid="clsid:..." as
> application/x-oleobject MIME. Thus, the patch from this bug causes big
> regression for us. We may deal with it in our fork, but it's always nicer to
> not have downstream changes. And given that we need very little from Gecko,
> the maintenance cost of having such code is pretty low for Mozilla.
> 
> Would you please consider r+ for a patch reverting relevant parts of the
> patch? I will take care of that if you'd agree.

The code necessary to support this is pretty minimal, so it comes down to whether or not we want this hook present. If it's serving a useful purpose somewhere I have no issue with it, so I'll leave it up to josh

Comment 7

[John Schoenick [:johns]]

Attachment: Restore activex classid hook for object tags

This restores the activex hook while maintaining the java/uri-handling cleanup I snuck into the patch.
ActiveX similarly uses 'codebase' as its actual-URI, so it can just piggyback on the java logic here

Comment 8

[Josh Aas]

Comment on attachment 655132 [details] [diff] [review]
Restore activex classid hook for object tags

Review of attachment 655132 [details] [diff] [review]:
-----------------------------------------------------------------

I have a strong desire to be helpful here but maintaining code we don't need in a notoriously touchy area for the sake of one particular project that already maintains a fork of Gecko doesn't seem like the right move. I don't want to take on this added compatibility concern for Wine when Wine already has the code (John's patch here is probably good) and a delivery mechanism in place. If the maintenance cost for us is low, then it is probably also low for Wine.

I don't expect this code to change a lot after John's project to clean it up is over in a week or so, and I'd be happy to help answer questions you guys might have in the future.

Comment 9

[John Schoenick [:johns]]

Remarking as fixed, in that case. I did push this patch to try though, to make sure it doesn't introduce any issues:

https://tbpl.mozilla.org/?tree=Try&rev=fbe4d149be3f

Feel free to ping josh or myself if issues with this arise

Comment 10

[Jacek Caban]

Thanks a lot for the patch and feedback! It looks good on try and passes all Wine tests, so I've committed it to our repository.