Closed Bug 909656 Opened 11 years ago Closed 7 years ago

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

Categories

(Core Graveyard :: Plug-ins, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(2 files)

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?
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
> "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.
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.
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.
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.
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.
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?
Flags: needinfo?(bzbarsky)
Go for it!
Flags: needinfo?(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?
If you ignore representative samples, yes.
OK.  That's actually higher than I expected.  :(
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>.
And looks like we don't support it on <applet> anyway.
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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+.
Attachment #8804268 - Flags: review?(benjamin) → review+
OK, will wait for 53.
Flags: needinfo?(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?
Flags: needinfo?(bzbarsky) → needinfo?(benjamin)
I think we need to keep testing this, just not on the prototype object. Let me see if I can quickly alter this.
Flags: needinfo?(benjamin)
Here's a fixup for the test which I believe does the right thing.
Comment on attachment 8812260 [details] [diff] [review]
invokedefault-testfixup.patch

Thank you.  That looks right to me, and passes with this patch.
Attachment #8812260 - Flags: review+
Pushed by bzbarsky@mozilla.com:
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
Pushed by archaeopteryx@coole-files.de:
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
https://hg.mozilla.org/mozilla-central/rev/c1a0df0334a1
https://hg.mozilla.org/mozilla-central/rev/5d5b77bd5388
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.