nsContentUtils::IsCallerChrome() returns false in the case of x-ray property access, which makes the check [1] incorrectly spawn a plugin from chrome CTP touching the object. In this case, just pressing "allow" on the CTP popup is synchronously spawning the plugin *in that event*. Ugh. [1] http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#2602 > 0 anonymous( <Failed to get argument while inspecting stack frame> > ) ["chrome://browser/content/browser.js":3441] > this = [object Object] > 1 PH_setPluginNotificationIcon(aBrowser = [object XULElement]) ["chrome://browser/content/browser.js":4385] > this = [object Object] > 2 PH_setPermissionForPlugins(aNotification = [object Object], aPluginInfo = [object Object], aNewState = "allownow") ["chrome://browser/content/browser.js":4246] > this = [object Object] > 3 _singleActivateNow() ["chrome://browser/content/urlbarBindings.xml":1801] > this = [object XULElement] > 4 _onButton(aButton = [object XULElement]) ["chrome://browser/content/urlbarBindings.xml":1794] > this = [object XULElement] > 5 oncommand(event = [object XULCommandEvent]) ["chrome://browser/content/browser.xul":1] > this = [object XULElement] > > #3 nsObjectLoadingContent::ScriptRequestPluginInstance (this=0x7fffd3876990, aCx=0x7fffd6444510, aResult=0x7fffffff1448) > #4 0x00007ffff13a05be in nsObjectLoadingContent::DoNewResolve (this=0x7fffd3876990, aCx=0x7fffd6444510, aObject=..., aId=..., aDesc=...) > #5 0x00007ffff0482597 in mozilla::dom::HTMLAppletElementBinding::ResolveOwnPropertyViaNewresolve (cx=0x7fffd6444510, wrapper=..., obj=..., id=..., desc=..., flags=0) > #6 0x00007ffff0a3a85f in mozilla::dom::XrayResolveOwnProperty (cx=0x7fffd6444510, wrapper=..., obj=..., id=..., desc=..., flags=0) > #7 0x00007ffff0d0b8b6 in xpc::DOMXrayTraits::resolveOwnProperty (this=0x7ffff6156120 <xpc::DOMXrayTraits::singleton>, cx=0x7fffd6444510, jsWrapper=..., wrapper=..., holder=..., id=..., desc=..., flags=0) > #8 0x00007ffff0d1585d in xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits>::getPropertyDescriptor (this=0x7ffff653cbc8 <xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits>::singleton>, cx=0x7fffd6444510, wrapper=..., id=..., desc=..., flags=0) > #9 0x00007ffff39c0f28 in js::BaseProxyHandler::get (this=0x7ffff653cbc8 <xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits>::singleton>, cx=0x7fffd6444510, proxy=..., receiver=..., id=..., vp=...) > #10 0x00007ffff0d17148 in xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits>::get (this=0x7ffff653cbc8 <xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits>::singleton>, cx=0x7fffd6444510, wrapper=..., receiver=..., id=..., vp=...) > #11 0x00007ffff39ccbd9 in js::Proxy::get (cx=0x7fffd6444510, proxy=..., receiver=..., id=..., vp=...) at /home/johns/moz/moz-git-build/js/src/../../js/src/jsproxy.cpp:2510 > #12 0x00007ffff39cefc5 in js::proxy_GetGeneric (cx=0x7fffd6444510, obj=..., receiver=..., id=..., vp=...) at /home/johns/moz/moz-git-build/js/src/../../js/src/jsproxy.cpp:2878 > #13 0x00007ffff340a6ea in JSObject::getGeneric (cx=0x7fffd6444510, obj=..., receiver=..., id=..., vp=...) at /home/johns/moz/moz-git-build/js/src/../../js/src/jsobj.h:1013 > #14 0x00007ffff3af63c2 in GetPropertyOperation (cx=0x7fffd6444510, fp=0x7fffe17ae318, script=..., pc=0x7fffd628ec30 "\270", lval=..., vp=...) > #15 0x00007ffff3ae0662 in Interpret (cx=0x7fffd6444510, state=...) at /home/johns/moz/moz-git-build/js/src/../../js/src/vm/Interpreter.cpp:2406 > #16 0x00007ffff3ad53e9 in js::RunScript (cx=0x7fffd6444510, state=...) at /home/johns/moz/moz-git-build/js/src/../../js/src/vm/Interpreter.cpp:423 > #17 0x00007ffff3aec22e in js::Invoke (cx=0x7fffd6444510, args=..., construct=js::NO_CONSTRUCT) at /home/johns/moz/moz-git-build/js/src/../../js/src/vm/Interpreter.cpp:485

This is presumably a regression from bug 945416. In that bug we changed resolve on an Xray to first resolve on the underlying content object to make it easier for Window to work Bobby, thoughts? We could do the resolve on underlying object only for Window or only if [Global] or something, I guess...

Alternately, we could special-case doing a single resolve only for the plug-ins. We only have 3 things with this hook: Navigator, Window, and plug-ins. And we'd like to kill off the Navigator one. So deciding which is the special case is a bit hard.

I think we should special-case plugins. The side-effect of firing up a plugin during the resolve hook is clearly the special-case IMO.

So I was looking at this, and I'm not quite sure I understand the problem correctly anymore. In nsObjectLoadingContent::ScriptRequestPluginInstance the logic seems to be more or less along the lines of: if (callerIsContentJS) { // fire an event } else { SyncStartPluginInstance() } Bug 945416 made it so that callerIsContentJS is true for chrome accesses (at least for one of the two calls they do). How does that cause _more_ sync instantiation?

(In reply to Boris Zbarsky [:bz] from comment #4) > So I was looking at this, and I'm not quite sure I understand the problem > correctly anymore. > > In nsObjectLoadingContent::ScriptRequestPluginInstance the logic seems to be > more or less along the lines of: > > if (callerIsContentJS) { > // fire an event > } else { > SyncStartPluginInstance() > } > > Bug 945416 made it so that callerIsContentJS is true for chrome accesses (at > least for one of the two calls they do). How does that cause _more_ sync > instantiation? Sorry part 1 of this bug got lost when I was debugging it yesterday -- This chunk should be |else if (callerIsContentJS) { SyncStartPluginInstance }|. I have a patch for this in my queue and completely missed that this wasn't already the state of things when we were talking about this. So we need a solution for DoNewResolve to fix this, but it is not a regression (the missing check goes back to when I first landed ScriptRequestPluginInstance). There *is* a regression in that we will now essentially always fire PluginScripted (since CTP always touches the plugin during page setup), but IIRC this event isn't currently used by the new CTP code so it's not worth tracking.

The patch in question. So to clarify: - Chrome JS causing sync spawning is unintentional, due to this missing check - Bug 945416 additionally broke the callerIsContentJS check - The other use of the callerIsContentJS check is firing a currently-unused event, which is a regression, but not worth tracking.

Aha! Things make a lot more sense now. ;) I'll put up a codegen patch.

So actually, one more question, now that I remembered why comment 4 came up. If I read the new logic right, it looks like in the !callerIsContentJS case we just want DoNewResolve and GetOwnPropertyNames to be no-ops right?

As in, if I just pretend for purposes of Xrays to plugin-loading elements like those methods don't exist we'll have the right behavior.

(In reply to Boris Zbarsky [:bz] from comment #8) > So actually, one more question, now that I remembered why comment 4 came up. > > If I read the new logic right, it looks like in the !callerIsContentJS case > we just want DoNewResolve and GetOwnPropertyNames to be no-ops right? (In reply to Boris Zbarsky [:bz] from comment #9) > As in, if I just pretend for purposes of Xrays to plugin-loading elements > like those methods don't exist we'll have the right behavior. Assuming this also covers the XBL case (we attach XBL to plugin-tags all the time, and they touch themselves), this is correct. PluginScripted is a "I would've spawned the plugin if I were configured as a plugin" event, so should follow similar logic.

> Assuming this also covers the XBL case It actually might, but in any case we'd leave the IsCallerChrome/IsCallerXBL checks we have right now.

I guess we may want to spin the other patch off to a separate bug...

Comment on attachment 8370359 [details] [diff] [review] Only sync spawn plugins in response to content, not chrome, JS access With the DoNewResolve issue fixed, this prevents us from spawning plugins synchronously due to chrome touching their nodes. Currently, the CTP code appears to be inadvertently causing the sync spawning of plugins often (such as, inside the onclick handler for clicking 'allow now' :( )

Actually since bz's fix makes these not fire at all for xray this is probably not necessary, but makes the intent of the code clear. Added comments that these hooks don't fire in that case.

Try failed due to some mochitest-chrome tests that weren't waiting for plugin spawning https://tbpl.mozilla.org/?tree=Try&rev=bfd0bce17f03

Missed one in bc... https://tbpl.mozilla.org/?tree=Try&rev=475d4444ea4c

https://hg.mozilla.org/integration/mozilla-inbound/rev/49fb1ccb7a5e https://hg.mozilla.org/integration/mozilla-inbound/rev/301611f83b2b https://hg.mozilla.org/integration/mozilla-inbound/rev/5d4ffa16f845

And a followup to disable the newly added test on b2g-desktop: http://hg.mozilla.org/integration/mozilla-inbound/rev/28de6113cad8

https://hg.mozilla.org/mozilla-central/rev/49fb1ccb7a5e https://hg.mozilla.org/mozilla-central/rev/301611f83b2b https://hg.mozilla.org/mozilla-central/rev/5d4ffa16f845 https://hg.mozilla.org/mozilla-central/rev/28de6113cad8