Closed Bug 967694 Opened 6 years ago Closed 6 years ago

Chrome access can cause plugins to synchronously spawn via property access

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: johns, Assigned: johns)

References

Details

Attachments

(4 files, 2 obsolete files)

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...
Blocks: 945416
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(bobbyholley)
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?
Flags: needinfo?(jschoenick)
(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.
Flags: needinfo?(jschoenick)
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?
Flags: needinfo?(jschoenick)
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.
Flags: needinfo?(jschoenick)
> 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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
I guess we may want to spin the other patch off to a separate bug...
Attachment #8370542 - Flags: review?(bobbyholley) → review+
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' :( )
Attachment #8370359 - Flags: review?(benjamin)
Whiteboard: [leave open]
Assignee: bzbarsky → jschoenick
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.
Attachment #8370359 - Attachment is obsolete: true
Attachment #8370359 - Flags: review?(benjamin)
Attachment #8371155 - Flags: review?(benjamin)
Attached patch TestSplinter Review
Attachment #8371162 - Flags: review?(benjamin)
Try failed due to some mochitest-chrome tests that weren't waiting for plugin spawning

https://tbpl.mozilla.org/?tree=Try&rev=bfd0bce17f03
Attachment #8371938 - Flags: review?(benjamin)
Attachment #8371155 - Flags: review?(benjamin) → review+
Attachment #8371162 - Flags: review?(benjamin) → review+
Missed one in bc...
https://tbpl.mozilla.org/?tree=Try&rev=475d4444ea4c
Attachment #8371938 - Attachment is obsolete: true
Attachment #8371938 - Flags: review?(benjamin)
Attachment #8373611 - Flags: review?(benjamin)
Attachment #8373611 - Flags: review?(benjamin) → review+
You need to log in before you can comment on or make changes to this bug.