909656 - Can we remove the legacycaller on <object>/<embed>?

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now we support pages invoking an <object> or <embed> as a function; if that's done we call into plug-in code somehow, I think.

Could we drop support for this?  jst and bsmedberg seemed to think so....

If we do, we should raise this as a spec issue as well: right now it requires support for legacycaller on these elements.

Thoughts?

Comment 1

[taxilian]

While my initial reaction to any push to remove functionality from npapi is "No! You'll break existing plugins!" I'm fighting the knee-jerk reaction and would like to ask a few clarifying questions.

1) Would this affect all NPObjects, removing the ability to call them with InvokeDefault, or just the one returned to the page to provide the brains for the object/embed tag

2) What problems is this solving? Are you actually fixing a problem, or just removing something that you perceive as unusual?

I know for a fact that there are a lot of NPObjects out there that like to be called by InvokeDefault; I'm less certain that they are the primary NPObject for an embed or object tag.  I can think of many use cases for this, but I don't have any hard data as to how often this is used.

- Richard Bateman
FireBreath author

Comment 2

[Boris Zbarsky [:bzbarsky]]

> "No! You'll break existing plugins!"

That's why I haven't done it yet; do existing plugins use this stuff?

> Would this affect all NPObjects

This is not talking about any changes to NPAPI or NPObject per se.

This is specifically talking about the JSObject for the <embed> or <object> and whether it has a [[Call]] method or not.  Right now it does, and invoking that [[Call]] from JS will, as far as I can tell, get some other JS object (whatever nsNPAPIPluginInstance::GetJSObject) returns and invoke its [[Call]].  What I don't know is what that ends up doing or can end up doing.  Presumably it ends up trying to InvokeDefault on some NPObject, but which one(s)?

> 2) What problems is this solving?

The problem that |typeof some-embed-element == "function"| in JS, which confuses web developers and screws up their type tests.  The JS spec requires this for objects that have a [[Call]].  There is the secondary consideration of simplifying code, etc, but the web-facing API problem is what I'd like to resolve.

Comment 3

[Benjamin Smedberg]

1) No, this is limited to the prototype object installed on the plugin element itself (NPPVpluginScriptableNPObject). All other NPObjects would remain callable.

2) Making an object callable significantly changes some of its behaviors, especially when it is also a DOM object. We'd like to avoid changing whether an object can be callable during its lifetime, which is what happens now when a plugin is instantiated (and again later if the plugin is destroyed).

Last thing I heard is we added telemetry to check whether this ever happened in the wild (ISTR a patch, can't find it right now). I think that unless telemetry shows significant cases of this, we should just do it.

Comment 4

[Benjamin Smedberg]

Indeed, bz added the telemetry in bug 851917: http://hg.mozilla.org/mozilla-central/annotate/6ce16afcc8bd/content/base/src/nsObjectLoadingContent.cpp#l3199

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, so we did:  Telemetry::Accumulate(Telemetry::PLUGIN_CALLED_DIRECTLY, true), recorded when the call on the pi_obj doesn't throw.

That landed in Firefox 23, so it's been shipping to actual users for a while now.

Comment 6

[taxilian]

My primary concern would be if it modified the ability to call InvokeDefault on the NPObject; given that it doesn't, I still cringe a bit about changing established behavior, but I don't actually know of anybody in the FireBreath community that is using this feature on an object/embed tag. 

To answer Boris' question, currently InvokeDefault is called on the primary NPObject that is returned by the NPP_GetValue call that the browser makes into the plugin at the beginning of it's lifecycle; I'm fairly certain that this is consistent across all browsers, though to be honest I haven't done it in years so I could be wrong.

I would be interested to know the telemetry results, if you don't mind posting them here later on. I'll ask around in the FireBreath irc rooms and such and see if anyone is using this, but the more I think about it the more I kinda doubt it, since it would require customizing a JSAPI (FireBreath internal) object more than most people realize that you can.

Comment 7

[John Schoenick [:johns]]

According to telemetry the 99th percentile number of direct plugin calls is zero from the FF23 beta cycle -- if that is to be trusted it is at least not occurring in the major plugins.

Comment 8

[Benjamin Smedberg]

Given the data, we should remove it. bz do you want to bring this up in the spec list or would you like me to?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Go for it!

Comment 10

[Boris Zbarsky [:bzbarsky]]

Though it looks like there is _some_ telemetry indicating successful calls to this stuff.  About 6683 pings out of 79283787 on Fx 22.  And 27000 out of 265514690 on Fx23.

So approximately one in 10000 pings has hit this codepath.  Does that mean about 1 in 10000 users?

Comment 11

[Benjamin Smedberg]

If you ignore representative samples, yes.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK.  That's actually higher than I expected.  :(

Comment 13

[Boris Zbarsky [:bzbarsky]]

Note https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/MuxvaXiyBxU where their use counters are claiming that this is basically never used at least on <applet>.

Comment 14

[Boris Zbarsky [:bzbarsky]]

And looks like we don't support it on <applet> anyway.

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK.  We have zero hits on the use counter on current release (49), out of 113.95 million telemetry pings.  We have two hits on it out of 130.78 million telemetry pings in release 48.  We have zero hits out of 222.92 million telemetry pings in release 47.

Furthermore, we've just dropped support for non-Flash NPAPI plugins, and I'm pretty sure Flash doesn't end up using this.

I'm going to post a patch to remove this.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove the legacycaller from HTMLObjectElement/HTMLEmbedElement, since it's not used in the wild, other browsers don't seem to support it, and we're dropping non-Flash plugins anyway

Comment 17

[Benjamin Smedberg]

Comment on attachment 8804268 [details] [diff] [review]
Remove the legacycaller from HTMLObjectElement/HTMLEmbedElement, since it's not used in the wild, other browsers don't seem to support it, and we're dropping non-Flash plugins anyway

Might be worth waiting until 53, but definitely r+.

Comment 18

[Boris Zbarsky [:bzbarsky]]

OK, will wait for 53.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Benjamin, should I just remove the bits of test_npruntime_npninvokedefault.html that call plugin()?  Or is there something else those bits should be testing?

Comment 20

[Benjamin Smedberg]

I think we need to keep testing this, just not on the prototype object. Let me see if I can quickly alter this.

Comment 21

[Benjamin Smedberg]

Attachment: invokedefault-testfixup.patch

Here's a fixup for the test which I believe does the right thing.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8812260 [details] [diff] [review]
invokedefault-testfixup.patch

Thank you.  That looks right to me, and passes with this patch.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1a0df0334a1
Remove the legacycaller from HTMLObjectElement/HTMLEmbedElement, since it's not used in the wild, other browsers don't seem to support it, and we're dropping non-Flash plugins anyway.  r=bsmedberg

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5b77bd5388
Remove the legacycaller from HTMLObjectElement/HTMLEmbedElement, since it's not used in the wild, other browsers don't seem to support it, and we're dropping non-Flash plugins anyway: Update web-platform-tests. r=bustage-fix

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c1a0df0334a1
https://hg.mozilla.org/mozilla-central/rev/5d5b77bd5388

Comment 26

[Chris Mills [:cmills]]

We never mentioned the legacycaller anywhere in the MDN docs, so I've settled for just adding a note to 

https://developer.mozilla.org/en-US/Firefox/Releases/53#Other

Hope that's OK.