Closed Bug 1194488 Opened 4 years ago Closed 4 years ago

Firefox 40 breaks NPAPI Envysion Video Player Plugin with Asynchronous Plugin Initialization enabled

Categories

(Core :: Plug-ins, defect)

40 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla43
Tracking Status
firefox41 + wontfix
firefox42 --- verified
firefox43 --- verified

People

(Reporter: mail, Assigned: aklotz)

References

Details

(Keywords: regression)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.155 Safari/537.36

Steps to reproduce:

We have an NPAPI plugin that invokes JavaScript events using the DOM id of the embed element used to instantiate the plugin. The code that the plugin invokes looks something like this:

event = document.createEvent('UIEvents'); 
event.initEvent('event_name', true, false);
document.getElementById(elem_id).dispatchEvent(event);

We invoke this code using the evaluate NPN function. 

We obtain the value for elem_id in NPP_New, using this code snippet:

npnfuncs.getvalue(instance, NPNVPluginElementNPObject, &plugin_element);
NPIdentifier id_id = npnfuncs.getstringidentifier("id");
NPVariant v_id;
npnfuncs.getproperty(instance, plugin_element, id_id, &v_id);

if( NPVARIANT_IS_STRING(v_id) ) 
{ 
    elem_id = string( NPVARIANT_TO_STRING(v_id).UTF8Characters, 
                NPVARIANT_TO_STRING(v_id).UTF8Length);
        
    npnfuncs.releasevariantvalue(&v_id);
}


This code worked in Firefox 39, but now fails in Firefox 40. The elem_id variable is left uninitialized, and the getElementById() call fails, resulting in the JavaScript callback not being called.

We confirmed this on Windows and Mac OS X. Other platforms might be affected, but we haven't tested them.

Was there a recent change to NPAPI that can cause this?



Actual results:

We can't obtain the id of the embed element used to create the plugin.


Expected results:

The id should be available to the plugin instance.
Component: Untriaged → Plug-ins
Product: Firefox → Core
Does this still occur even if dom.ipc.plugins.asyncInit = false in about:config?
Flags: needinfo?(mail)
Setting dom.ipc.plugins.asyncInit to false makes this work as expected.
Flags: needinfo?(mail)
Keywords: regression
Ori, is it possible for you to attach a minimal version of your plugin as testcase to reproduce the regression?
Flags: needinfo?(mail)
The plugin itself is available here:

https://video.envysion.com/software/player

And a test page illustrating the problem can be found here:

http://software.envysion.com/firefox-bug-1194488

To make this page work, you need to enable the plugin when it first loads, and then reload the page. If everything works, the plugin should play some video. The bug is that the line under the plugin doesn't update when the video plays.

Also, the console shows these errors:

TypeError: document.getElementById(...) is null 
Empty string passed to getElementById().

Since the plugin passes an empty string as the id of its embed element.
Flags: needinfo?(mail)
One other thing: It seems like turning off asyncInit doesn't make a difference anymore.
It would be good if you could fin a way to live without an NPAPI plugin at all there as we'll be deprecating those step by step in the future (as you might know, the first step already happened by making them all click-to-play except for a small whitelist).
This isn't a viable option right now, for a lot of reasons I can't get into. The web as a platform doesn't allow applications to do a lot of things that our customers need.
Summary: Firefox 40 breaks npapi plugin → Firefox 40 breaks NPAPI Envysion Video Player Plugin with Asynchronous Plugin Initialization enabled
(In reply to Ori Pessach from comment #5)
> One other thing: It seems like turning off asyncInit doesn't make a
> difference anymore.

Can you confirm this by turning off asyncInit, restarting the browser, and then trying again? Just flipping the pref while the browser is running gives us an incomplete picture.
Flags: needinfo?(mail)
Yes, it looks like setting dom.ipc.plugins.asyncInit to false no longer makes a difference. 

Where should I start looking if I want to debug this? Specifically, where's the code that calls NPP_New() in our plugin?
Flags: needinfo?(mail)
I'm sorry - I was wrong on dom.ipc.plugins.asyncInit no longer making a difference. I tested again and it DOES make a difference - the problem only appears if asyncInit it turned on.
Blocks: asyncplugininit-compat
No longer blocks: asyncplugininit
Why do you even do it like that?

You can get NPNVPluginElementNPObject when dispatching the event and then pass it to NPN_Evaluate as this. The eval-ed code would be:

(function(pluginElement) {
    var event = document.createEvent('UIEvents'); 
    event.initEvent('event_name', true, false);
    pluginElement.dispatchEvent(event);
})(this);

This way it is also going to work even if you don't set the ID attribute on the <embed>/<object> element.
Also you get the value of the ID attribute from argn/argv in NPP_New.
When I test this I get Error: Forbidden (303). Is there a perms issue here with some of the content on that test page?
Flags: needinfo?(mail)
Sorry about that - I fixed the test page. It should work now.
Flags: needinfo?(mail)
Here is the call stack of the property access for "id":

#0	0x0000000107b224e0 in js::CallResolveOp(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::MutableHandle<js::Shape*>, bool*) at /src/moz/js/src/vm/NativeObject-inl.h:382
#1	0x0000000107adc645 in bool js::LookupOwnPropertyInline<(js::AllowGC)1>(js::ExclusiveContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::MutableHandleType, bool*) at /src/moz/js/src/vm/NativeObject-inl.h:477
#2	0x0000000107ae05dc in bool NativeGetPropertyInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, IsNameLookup, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) at /src/moz/js/src/vm/NativeObject.cpp:1910
#3	0x0000000107ae04ed in js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) at /src/moz/js/src/vm/NativeObject.cpp:1954
#4	0x0000000107a30866 in js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) at /src/moz/js/src/vm/NativeObject.h:1417
#5	0x00000001081382a0 in JS_ForwardGetPropertyTo(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) at /src/moz/js/src/jsapi.cpp:2927
#6	0x0000000108138211 in JS_GetPropertyById(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) at /src/moz/js/src/jsapi.cpp:2916
#7	0x0000000104e9ce52 in GetProperty(JSContext*, JSObject*, void*, JS::MutableHandle<JS::Value>) at /src/moz/dom/plugins/base/nsJSNPRuntime.cpp:723
#8	0x0000000104e9aa73 in nsJSObjWrapper::NP_GetProperty(NPObject*, void*, _NPVariant*) at /src/moz/dom/plugins/base/nsJSNPRuntime.cpp:914
#9	0x0000000104e79c36 in mozilla::plugins::parent::_getproperty(_NPP*, NPObject*, void*, _NPVariant*) at /src/moz/dom/plugins/base/nsNPAPIPlugin.cpp:1439
#10	0x0000000104eff179 in mozilla::plugins::PluginScriptableObjectParent::AnswerGetParentProperty(mozilla::plugins::PluginIdentifier const&, mozilla::plugins::Variant*, bool*) at /src/moz/dom/plugins/ipc/PluginScriptableObjectParent.cpp:1048
#11	0x000000010216612d in mozilla::plugins::PPluginScriptableObjectParent::OnCallReceived(IPC::Message const&, IPC::Message*&) at /src/moz/obj-ff-prof-dbg/ipc/ipdl/./PPluginScriptableObjectParent.cpp:1087
#12	0x000000010212aff5 in mozilla::plugins::PPluginModuleParent::OnCallReceived(IPC::Message const&, IPC::Message*&) at /src/moz/obj-ff-prof-dbg/ipc/ipdl/./PPluginModuleParent.cpp:1407
#13	0x0000000101e2fe23 in mozilla::ipc::MessageChannel::DispatchInterruptMessage(IPC::Message const&, unsigned long) at /src/moz/ipc/glue/MessageChannel.cpp:1450
#14	0x0000000101e2f97e in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) at /src/moz/ipc/glue/MessageChannel.cpp:1300
#15	0x0000000101e2b64f in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() at /src/moz/ipc/glue/MessageChannel.cpp:1273
#16	0x0000000101e30045 in mozilla::ipc::MessageChannel::WaitForIncomingMessage() at /src/moz/ipc/glue/MessageChannel.cpp:1173
#17	0x0000000104ee415e in mozilla::plugins::PluginAsyncSurrogate::WaitForInit() at /src/moz/dom/plugins/ipc/PluginAsyncSurrogate.cpp:530
#18	0x0000000104ee6152 in mozilla::plugins::PluginAsyncSurrogate::ScriptableHasProperty(NPObject*, void*) at /src/moz/dom/plugins/ipc/PluginAsyncSurrogate.cpp:810
#19	0x0000000104ec03cd in NPObjWrapper_Resolve(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool*) at /src/moz/dom/plugins/base/nsJSNPRuntime.cpp:1679
#20	0x0000000107b22585 in js::CallResolveOp(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::MutableHandle<js::Shape*>, bool*) at /src/moz/js/src/vm/NativeObject-inl.h:388
#21	0x0000000107adc645 in bool js::LookupOwnPropertyInline<(js::AllowGC)1>(js::ExclusiveContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::MutableHandleType, bool*) at /src/moz/js/src/vm/NativeObject-inl.h:477
#22	0x0000000107ae05dc in bool NativeGetPropertyInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, IsNameLookup, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) at /src/moz/js/src/vm/NativeObject.cpp:1910
#23	0x0000000107ae04ed in js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) at /src/moz/js/src/vm/NativeObject.cpp:1954
#24	0x0000000107a30866 in js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) at /src/moz/js/src/vm/NativeObject.h:1417
#25	0x0000000107a2d8f4 in js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, js::PropertyName*, JS::MutableHandle<JS::Value>) at /src/moz/js/src/jsobj.h:830
#26	0x00000001079f6e3c in js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) at /src/moz/js/src/vm/Interpreter.cpp:4191
#27	0x0000000107a090d3 in GetPropertyOperation(JSContext*, js::InterpreterFrame*, JS::Handle<JSScript*>, unsigned char*, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) at /src/moz/js/src/vm/Interpreter.cpp:258
#28	0x00000001079e441f in Interpret(JSContext*, js::RunState&) at /src/moz/js/src/vm/Interpreter.cpp:2780
#29	0x00000001079d9217 in js::RunScript(JSContext*, js::RunState&) at /src/moz/js/src/vm/Interpreter.cpp:704
#30	0x00000001079cb636 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) at /src/moz/js/src/vm/Interpreter.cpp:781
#31	0x00000001079af263 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) at /src/moz/js/src/vm/Interpreter.cpp:818
#32	0x0000000107d73a4f in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) at /src/moz/js/src/jit/BaselineIC.cpp:9361
#33	0x00000001007d053b in 0x1007d053b ()


The problem is as follows:

1) NPP_New is running asynchronously;
2) The page is executing some JS that needs to resolve the id attribute on the plugin element;
3) JS is walking the prototype chain in search of the id attribute;
4) JS hits the NPAPI scriptable plugin object which needs to resolve;
5) Async plugin init blocks on NPP_New in order to resolve the scriptable plugin object for (3);
6) NPP_New does a getproperty on the plugin element to resolve the id attribute;
7) A recursion guard inside SpiderMonkey's property access code notices that the id attribute is already being resolved in step (2), which causes the property get from step (6) to fail, returning a void value to the plugin.

I suppose it's possible to fix this in the NPAPI runtime but it would be really hacky. Basically we'd need to check to see if we're doing a property access from NPP_New on the NPAPI scriptable object, and if so, pass the resolution on to the NPAPI scriptable object wrapper's prototype.

By far the easiest way to fix this is to just grab the value of the id attribute directly from the argn and argv parameters to NPP_New as suggested by Milan Burda in comment 12. I realize that technically we are seeing a compatibility break, but what the plugin is doing now is an awfully roundabout way of obtaining information that it already has.
I am inclined to WONTFIX, but it's not my call. Oh, bsmedberg? ;-)

See comment 15 for context. If your NPP_New tries to get a property on the plugin element that has already been made available to you via argn and argv, it is possible that the property access will fail.

If we are to fix it, I see a couple of ways around this. One is to do the hack in comment 15, another is to have some kind of async init compatibility whitelist. Or another is to WONTFIX and make a compat note that plugins should be using argn and argv to obtain attributes in NPP_New. I'm open to other alternatives, of course :-)
Flags: needinfo?(benjamin)
I'm torn. But I think what we should do here is exempt this plugin from async init. Clearly this isn't a widely-used plugin that's going to affect most users performance.

We're pretty sure that this won't affect Flash/Java/Silverlight, because those plugins don't run arbitrarily-complex script from NPP_New: all of the setup code that they run happens later after the initial stream loads.
Flags: needinfo?(benjamin)
We're currently testing a fix to our plugin that removes the reliance on the id, which fixes the problem for us. Thanks for suggesting that modification.

The problem is that many of our customers aren't going to upgrade the plugin for a long time. Some of them will never upgrade. We do have many thousands of users, and our business relies on the plugin working correctly, so obviously we would like Firefox to stay compatible with our installed base if at all possible.

I do recognize that our plugin as it is currently written makes a "correct" fix difficult, if not impossible. Exempting our plugin from async init sounds like a great solution.
It would be great if we could blacklist plugins from async init based on an entry in the downloadable blocklist that we already dynamically serve the Firefox installations out there - and that blacklisting should ideally be able to include by add-on version or something like that, so e.g. in this case, the newer, fixed version could run with async init while the older unfixed version would not get async.
Hooking it up to the existing blocklist that originally was created for add-ons but is also used for gfx would also mean we could react fast to specific cases we find when we go to release again with this feature.
The question is how much engineering work would be needed to do that.
That is much too complex for this bug.
Attached patch Whitelist PatchSplinter Review
For now (and since we need to uplift to Beta) I'd like to land this simple patch that only enables async init for specific plugins.

Flash, Java, Silverlight, and Unity are whitelisted. The whitelist is effectively disabled on Nightly and Dev Edition to provide for a test platform. Going forward we can examine our telemetry and selectively enable additional plugins depending on their performance, popularity, and compatibility.
Assignee: nobody → aklotz
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8655596 - Flags: review?(jmathies)
Comment on attachment 8655596 [details] [diff] [review]
Whitelist Patch

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

(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #21)
> Created attachment 8655596 [details] [diff] [review]
> Whitelist Patch
> 
> For now (and since we need to uplift to Beta) I'd like to land this simple
> patch that only enables async init for specific plugins.
> 
> Flash, Java, Silverlight, and Unity are whitelisted. The whitelist is
> effectively disabled on Nightly and Dev Edition to provide for a test
> platform. Going forward we can examine our telemetry and selectively enable
> additional plugins depending on their performance, popularity, and
> compatibility.

I think bsmedberg should sign off on this new policy, I can't make this call. I'll be happy to approve the code changes though.
Attachment #8655596 - Flags: review?(jmathies) → review+
Benjamin, I'd like to deal with this as a whitelisting patch, since there are a few other reports of relatively obscrure plugins with compat issues. jimm has r+'d a patch that whitelists Flash, Silverlight, Java, and Unity, being popular plugins that need to load an initial stream first. In the future we can look at telemetry and if necessary enable additional plugins on a case-by-base basis.

Are you willing to sign off on this?
Flags: needinfo?(benjamin)
Yes, do it.
Flags: needinfo?(benjamin)
Keywords: checkin-needed
Comment on attachment 8655596 [details] [diff] [review]
Whitelist Patch

Approval Request Comment
[Feature/regressing bug #]: async plugin init
[User impact if declined]: unexpected behavior when running some plugins
[Describe test coverage new/current, TreeHerder]: locally
[Risks and why]: Low, uses existing plugin type detection code to whitelist specific plugins for async init. Simple patch.
[String/UUID change made/needed]: None
Attachment #8655596 - Flags: approval-mozilla-beta?
Attachment #8655596 - Flags: approval-mozilla-aurora?
Blocks: 1195609
Tracked for 41.
Hi, this patch failed to apply:

patching file dom/plugins/base/nsNPAPIPlugin.cpp
Hunk #1 FAILED at 251
1 out of 1 hunks FAILED -- saving rejects to file dom/plugins/base/nsNPAPIPlugin.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh aiwhitelist.patch

could you take a look, thanks!
Flags: needinfo?(aklotz)
Keywords: checkin-needed
Comment on attachment 8655596 [details] [diff] [review]
Whitelist Patch

We need to whitelist some plugins. I also noticed that this patch enables async plugin init on Aurora and Release which is expected I believe. Let's uplift to Aurora42 and Beta41.

Please land patches after these have made into mozilla-central.
Attachment #8655596 - Flags: approval-mozilla-beta?
Attachment #8655596 - Flags: approval-mozilla-beta+
Attachment #8655596 - Flags: approval-mozilla-aurora?
Attachment #8655596 - Flags: approval-mozilla-aurora+
Rebased and landed on inbound.
Flags: needinfo?(aklotz)
https://hg.mozilla.org/mozilla-central/rev/a7d95d113ad6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
seems this cause problems when uplifting to beta 

merging dom/plugins/base/nsPluginTags.h ipc/PluginModuleParent.cpp                                                                                dom/plugins/ipc/PluginModuleParent.h 

can you take a look and maybe attach a beta patch ?
Flags: needinfo?(aklotz)
Flags: qe-verify+
QA Contact: petruta.rasa
Verified under Win 7 64-bit and Mac OS X 10.9.5.

Reproduced the issue with Firefox 41 beta 8.

Using Firefox 41 beta 9 with the same profile used for beta 8, the issue in no longer present.

On the other hand, with a clean profile, the issue still occurs. Refreshing the page doesn't help. Only after restarting the browser, the line below the video is updated and there are no console errors.

This seems only partially fixed.

Aaron, please let me know if there is something that I could further investigate.
Flags: needinfo?(aklotz)
Attached patch Patch part 2Splinter Review
Found an omission from the original patch that was messing up the whitelist. Here's the fix.
Flags: needinfo?(aklotz)
Attachment #8660167 - Flags: review?(jmathies)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8660167 - Flags: review?(jmathies) → review+
Comment on attachment 8660167 [details] [diff] [review]
Patch part 2

Approval Request Comment
[Feature/regressing bug #]: bug 1194488
[User impact if declined]: async init whitelist won't necessarily work correctly
[Describe test coverage new/current, TreeHerder]: Plugin test harness
[Risks and why]: None, trivial patch
[String/UUID change made/needed]: None
Attachment #8660167 - Flags: approval-mozilla-aurora?
Comment on attachment 8660167 [details] [diff] [review]
Patch part 2

Aaron, next time, please open a new bug for fallout. Otherwise, relman or sheriff could miss this kind of uplift.
Thanks
Attachment #8660167 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
http://software.envysion.com/firefox-bug-1194488 no longer shows the video and the text below it, only "Error: Forbidden(403)" is shown on the video area.

Ori, could you please reenable the testcase so that we can confirm it is fixed now? Thank you!

With Firefox 42 beta 1, the error messages in web console are not displayed anymore, but I would like to also check the text below the video.
Flags: needinfo?(mail)
Sorry about that - upgrades to the RTSP server's software happen from time to time, and they disable remote access. I just fixed that.
Flags: needinfo?(mail)
(In reply to Ori Pessach from comment #43)
> Sorry about that - upgrades to the RTSP server's software happen from time
> to time, and they disable remote access. I just fixed that.

No problem, thanks a lot! 

I verified this is now fixed using Firefox 42 beta 1 and Developer Edition 43.0a2 2015-09-23 under Win 7 64-bit, Mac OS X 10.9.5 and Ubuntu 12.04 32-bit.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.