904222 - Plugins in display:none trees can trigger "PluginBindingAttached"

Status: RESOLVED WONTFIX

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

Attachment: testcase

When accessing the "clientTop" property for a plugin that is a child of a "display:none" element, "PluginBindingAttached" is fired.
That means that also the plugin notification will be shown. In the case of data: URLs [1] it stays around, while for a normal test-page (see test-case attachment) it is removed "a little" later (it's effectively only shortly visible in slow debug builds).

[1] data:text/html,<div style="display:none;"><object id="p" type="application/x-shockwave-flash"></object></div><a href="" onclick="javascript:document.getElementById('p').clientTop;">plugin.clientTop;</a>

Comment 1

[Georg Fritzsche [:gfritzsche]]

Callstack for the binding constructor from where "PluginBindingAttached" is fired [1]:
[...]
`js::CallJSNative(cx=0x0000000113869c60, native=0x0000000105cbcdf0, args=0x00007fff5fbf5d70)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 165 at jscntxtinlines.h:225
`js::Invoke(cx=0x0000000113869c60, args=(null), construct=NO_CONSTRUCT) + 1123 at Interpreter.cpp:486
`Interpret(cx=0x0000000113869c60, state=0x00007fff5fbf8448) + 24967 at Interpreter.cpp:2502
`js::RunScript(cx=0x0000000113869c60, state=0x00007fff5fbf8448) + 484 at Interpreter.cpp:443
`js::Invoke(cx=0x0000000113869c60, args=(null), construct=NO_CONSTRUCT) + 1394 at Interpreter.cpp:505
`js::Invoke(cx=0x0000000113869c60, thisv=0x00007fff5fbf8710, fval=0x00007fff5fbf87b0, argc=0, argv=0x0000000000000000, rval=(null)) + 656 at Interpreter.cpp:536
`JS_CallFunctionValue(cx=0x0000000113869c60, objArg=0x000000010ee83040, fval=(null), argc=0, argv=0x0000000000000000, rval=0x00007fff5fbf88f8) + 543 at jsapi.cpp:5369
`nsXBLProtoImplAnonymousMethod::Execute(this=0x0000000113413460, aBoundElement=0x0000000120f826e0) + 2142 at nsXBLProtoImplMethod.cpp:350
`nsXBLPrototypeBinding::BindingAttached(this=0x0000000120a77ca0, aBoundElement=0x0000000120f826e0) + 107 at nsXBLPrototypeBinding.cpp:269
`nsXBLBinding::ExecuteAttachedHandler(this=0x00000001110abc40) + 114 at nsXBLBinding.cpp:619
`mozilla::dom::Element::WrapObject(this=0x0000000120f826e0, aCx=0x0000000113869c60, aScope=(null)) + 699 at Element.cpp:424
`bool mozilla::dom::WrapNewBindingObject<mozilla::dom::Element>(cx=0x0000000113869c60, scope=(null), value=0x0000000120f826e0, rval=(null)) + 271 at BindingUtils.h:660
`mozilla::dom::DocumentBinding::getElementById(cx=0x0000000113869c60, obj=(null), self=0x000000011d644000, args=0x00007fff5fbf8e48) + 452 at DocumentBinding.cpp:419
`mozilla::dom::DocumentBinding::genericMethod(cx=0x0000000113869c60, argc=1, vp=0x000000010ef5a098) + 541 at DocumentBinding.cpp:7339
`js::CallJSNative(cx=0x0000000113869c60, native=0x000000010403fd10, args=0x00007fff5fbf9400)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 165 at jscntxtinlines.h:225
`js::Invoke(cx=0x0000000113869c60, args=(null), construct=NO_CONSTRUCT) + 1123 at Interpreter.cpp:486
`Interpret(cx=0x0000000113869c60, state=0x00007fff5fbfbad8) + 24967 at Interpreter.cpp:2502
`js::RunScript(cx=0x0000000113869c60, state=0x00007fff5fbfbad8) + 484 at Interpreter.cpp:443
`js::Invoke(cx=0x0000000113869c60, args=(null), construct=NO_CONSTRUCT) + 1394 at Interpreter.cpp:505
`js::Invoke(cx=0x0000000113869c60, thisv=0x00007fff5fbfbda0, fval=0x00007fff5fbfbe40, argc=1, argv=0x00007fff5fbfbf98, rval=(null)) + 656 at Interpreter.cpp:536
`JS_CallFunctionValue(cx=0x0000000113869c60, objArg=0x000000010ee39480, fval=(null), argc=1, argv=0x00007fff5fbfbf98, rval=0x00007fff5fbfc008) + 543 at jsapi.cpp:5369
`mozilla::dom::EventHandlerNonNull::Call(this=0x0000000110f0c420, cx=0x0000000113869c60, aThisObj=(null), event=0x000000011d5fe530, aRv=0x00007fff5fbfc2d0) + 751 at EventHandlerBinding.cpp:39
`JS::Value mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(this=0x0000000110f0c420, thisObj=0x00000001139c4e18, event=0x000000011d5fe530, aRv=0x00007fff5fbfc2d0, aExceptionHandling=eReportExceptions) + 446 at EventHandlerBinding.h:58
`nsJSEventListener::HandleEvent(this=0x00000001139c4e00, aEvent=0x000000011d5fe530) + 2198 at nsJSEventListener.cpp:249
[...]
`-[ChildView mouseUp:](self=0x000000011330cc00, _cmd=0x00007fff9522b3a1, theEvent=0x00000001139ce060) + 911 at nsChildView.mm:4436


mrbkap, does that help you?

[1] http://hg.mozilla.org/mozilla-central/annotate/9ff2fa888c04/toolkit/mozapps/plugins/content/pluginProblem.xml#l81

Comment 2

[John Schoenick [:johns]]

It doesn't look like this plugin is actually receiving a frame at any point, but is still getting an XBL binding. I don't know enough about XBL to say if this is correct or not, but if that is what's happening we need to change the XBL constructor to be smarter about sending the event.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

If there's a binding attached to a node, we have to initialize it when we touch the node in JS because the prototype needs to stay consistent even if the node moves in and out of display:none (at least, that's my understanding of the code and the bug history here). You should be able to make the binding stick around by holding a reference to the node.

It seems like the code that handles PluginBindingAttached needs to be resilient against this case.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Thanks for checking into this Blake.

Checking for the element being in a display:none tree still leaves the problem of tracking the display state for showing the notification later on.
Can we potentially delay "PluginBindingAttached" until the element has a frame?

Comment 5

[Georg Fritzsche [:gfritzsche]]

bz, do you maybe have an idea on a proper approach here?

Per comment 3 "PluginBindingAttached" (i.e. a plugin elements binding constructor) is not the right point to trigger the click-to-play code for doorhangers etc.

As-is, we need to trigger that click-to-play code whenever we have plugins instantiated on the page and after they have their binding.

I've been told that something like the binding-code listening to the ObjectLoadingContent sending a "i've got a frame" event (so it could avoid sending the event before having a frame) would be rather ugly.
Are there any good options here?

Comment 6

[Boris Zbarsky [:bzbarsky]]

We could have nsObjectLoadingContent dispatch a chrome-only event of some sort...  Or even call some method on the binding if we want (e.g. have the binding implement some XPCOM interface and have nsObjectLoadingContent QI to that and call a method).  Would that help?

Comment 7

[Georg Fritzsche [:gfritzsche]]

Thanks for the speedy response :)

(In reply to Boris Zbarsky [:bz] from comment #6)
> We could have nsObjectLoadingContent dispatch a chrome-only event of some
> sort... 

Could the binding listen to that, so it can fire a proper event when it was created prior to nsOLC having a frame?

> Or even call some method on the binding if we want (e.g. have the
> binding implement some XPCOM interface and have nsObjectLoadingContent QI to
> that and call a method).  Would that help?

I think calling it directly sounds preferable to routing events around? Or would it be more effort to do so for a binding?
Sketching it out this would mean:
* in it's constructor the pluginProblem binding checks if nsOLC already has a frame
* if it does, it fires the event
* when nsOLC gets a frame and has a binding it calls a method on the binding, which triggers the event if it wasn't fired already

Are there any examples for implementing an interface in a binding?
When nsOLC has a binding with that interface, could it just QI on |this| or what would it need to do?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Could the binding listen to that

Not easily, though chrome code could.

> I think calling it directly sounds preferable to routing events around?

It does to me, yes.

> Are there any examples for implementing an interface in a binding?

Sure.  For a simple example, see the "searchbar" binding in browser/components/search/content/search.xml

The relevant bits are:

1)  <implementation implements="nsIWhatever">
2)  Actually having a <method> for the relevant method.

> When nsOLC has a binding with that interface, could it just QI on |this

Yep.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Ok, i'll take this approach then. Thanks for the pointers bz.

Comment 10

[Georg Fritzsche [:gfritzsche]]

So, it turns out i misunderstood johns here... and the main problem would detecting when ObjLC gets a frame. Apparently ObjLC only gets notified of getting a frame when it's in plugin mode, so we'd miss it when it's in fallback mode :(

Suggestions by johns, barring better options:
* add something to XBL to fire an event at the binding when it gets a new frame, though that may be performance sensitive so it may need to be opt-in somehow
* add something to opt-in to frame notifications in XBL, and have the browser plugin code call it directly (e.g. nsISomeXBLService::DoesBindingHaveFrame() ::OnBindingReceivesFrame(runnable))

The main concern here would be layout being highly performance-sensitive.

bz, do you have ideas on that revised problem here?

Comment 11

[Boris Zbarsky [:bzbarsky]]

We could just notify <object>/<embed>/<applet> elements whenever they might have a new frame.  Would that help?  Basically have nsCSSFrameConstructor::FindObjectData post a runnable to notify the object that it might now have a frame.

Comment 12

[Georg Fritzsche [:gfritzsche]]

With the event-posting making this non-blocking that would get rid of the performance concern?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Which performance concern?  The event-posting is just to avoid running script during frame construction, when data structures are in an inconsistent state.  It could be run off a scriptblocker, or via the event loop, either way.

Comment 14

[Georg Fritzsche [:gfritzsche]]

I was told that this may be a performance concern for layout, but if you don't see one here that's great, thanks.

johns, do you see any more concerns here or do you think we can go with that plan?

Comment 15

[Boris Zbarsky [:bzbarsky]]

It's not a concern if you put it in FindObjectData, since that's only hit once we know we have an object/embed/applet.

Comment 16

[John Schoenick [:johns]]

So the plan would be have FindObjectData post an event to nsObjLC, which would check if it has an XBL binding, and fire PluginBindingAttached if so? That sounds like it would work, though it's possible for the frame to be re-created with the same XBL binding, so the front-end code should handle that.

This could also potentially just call ::HasNewFrame(), removing the nsObjectFrame callbacks that trigger it now, but if that's hairy it's optional.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> So the plan would be have FindObjectData post an event to nsObjLC,

Right.  That could either call HasNewFrame or whatever else we want.

> which would check if it has an XBL binding, and fire PluginBindingAttached if so? 

Or just call some method on the binding as discussed earlier in this bug.

Comment 18

[BMO Automation]

Resolving as wont fix, plugin support deprecated in Firefox 85.