Closed
Bug 909656
Opened 11 years ago
Closed 8 years ago
Can we remove the legacycaller on <object>/<embed>?
Categories
(Core Graveyard :: Plug-ins, defect)
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)
7.20 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
9.92 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 2•11 years ago
|
||
> "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•11 years ago
|
||
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•11 years ago
|
||
Indeed, bz added the telemetry in bug 851917: http://hg.mozilla.org/mozilla-central/annotate/6ce16afcc8bd/content/base/src/nsObjectLoadingContent.cpp#l3199
Assignee | ||
Comment 5•11 years ago
|
||
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.
Comment 7•11 years ago
|
||
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•11 years ago
|
||
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)
Assignee | ||
Comment 10•11 years ago
|
||
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•11 years ago
|
||
If you ignore representative samples, yes.
Assignee | ||
Comment 12•11 years ago
|
||
OK. That's actually higher than I expected. :(
Assignee | ||
Comment 13•11 years ago
|
||
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>.
Assignee | ||
Comment 14•11 years ago
|
||
And looks like we don't support it on <applet> anyway.
Assignee | ||
Comment 15•8 years ago
|
||
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 | ||
Comment 16•8 years ago
|
||
Attachment #8804268 -
Flags: review?(benjamin)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment 17•8 years ago
|
||
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+
Assignee | ||
Comment 19•8 years ago
|
||
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)
Comment 20•8 years ago
|
||
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)
Comment 21•8 years ago
|
||
Here's a fixup for the test which I believe does the right thing.
Assignee | ||
Comment 22•8 years ago
|
||
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+
Comment 23•8 years ago
|
||
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
Comment 24•8 years ago
|
||
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
Comment 25•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c1a0df0334a1
https://hg.mozilla.org/mozilla-central/rev/5d5b77bd5388
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•8 years ago
|
Keywords: dev-doc-needed,
site-compat
Comment 26•8 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•