I've noticed that since Bug 880735 landed, activating CTP plugins on data: URLs no longer works when there are multiple CTP plugins on the page (yes, this is an edge case in some way :). To reproduce: 1. Load a data: URL with multiple CTP plugins, for example data:text/html,<object type="application/x-java-applet"></object><object type="application/x-shockwave-flash"></object> 2. Try to activate any CTP plugin 3. Observe this error in Browser Console: [15:11:07.090] NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.host] @ chrome://browser/content/browser.js:3791 That's this line in browser.js code: pluginInfo.pluginPermissionHost = browser.currentURI.host;

We can certainly wrap that line of code. I guess I'm wondering a couple things, though: * I presume that this data: URI was entered directly in the address bar, so it has a null principal. This means that we can't store permissions for the URL and the only real option for the user is "run all the plugins on the page right now" * The situation *should* change if the data: URI loads from a different page, and inherits that page's principal. In that case we should load plugins based on the principal of the original page, but I suspect that we don't do that correctly (either in objectloadingcontent, or in the frontend, or both). This seems like enough of an edge case that I'm not sure how much we should spend time making it perfect. Just fixing the JS exception so that the doorhanger works is probably important to fix for 24, though.

Same problem also exists with links to such data: URLs.

We do have a null-principal here both when the data: URL is directly copied into the nav-bar and when navigating to such an URL from another page. What would we to do about the host in the doorhanger message though ("Allow <host> to run <plugin>?").

That's interesting. I thought data: URIs were supposed to inherit their principal if you navigated from an existing page. In any case for the null principal I'm fine with "allow (null) to run <plugin>". Doing much more than that would require adding a new set of strings, which doesn't sound worth it.

Are you sure you have a null-principal? I think the problem is just that you're trying to get the host from the page URL rather than the principal.

(In reply to Jesse Ruderman from comment #5) > Are you sure you have a null-principal? This principal here: http://hg.mozilla.org/mozilla-central/annotate/4ffb23062b3b/browser/base/content/browser-plugins.js#l624 ... has: * URI.scheme=="moz-nullprincipal" * origin=="moz-nullprincipal:{...}" ... and the permissionObject here has no host: http://hg.mozilla.org/mozilla-central/annotate/4ffb23062b3b/browser/base/content/browser-plugins.js#l650

To get the principal for the document rather than the document's URL, try replacing let principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(browser.currentURI); with let principal = contentWindow.document.nodePrincipal;

(I don't know if that changes things wrt navigation or frames, though.)

(In reply to Jesse Ruderman from comment #7) > let principal = > Services.scriptSecurityManager.getNoAppCodebasePrincipal(browser.currentURI); > > with > > let principal = contentWindow.document.nodePrincipal; We should definitely make this change regardless - getNoAppCodebasePrincipal(browser.currentURI) isn't a good idea.

Given this, it may be necessary to include both the principal (in order to later set the permission) and the domain (for display) in the pluginInfo object.

Navigation & frame handling seems fine from what i looked at. Jesse, could you give it a quick look if that matches what you thought of? Jaws, are you able to review this? Try run: https://tbpl.mozilla.org/?tree=Try&rev=106e276c10a7

Comment on attachment 770323 [details] [diff] [review] Use document.nodePrincipal, fix doorhanger host handling Review of attachment 770323 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-plugins.js @@ +329,5 @@ > return false; > > let pluginHost = Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost); > let permissionString = pluginHost.getPermissionStringForType(objLoadingContent.actualType); > + let principal = objLoadingContent.ownerDocument.defaultView.top.document.nodePrincipal; Why the top document rather than the document with the plugin? I guess it keeps the UI simple, but it's not really what I want for YouTube embeds. @@ +495,5 @@ > // not associated with any known plugin > if (!gPluginHandler.isKnownPlugin(objLoadingContent)) > return; > let permissionString = pluginHost.getPermissionStringForType(objLoadingContent.actualType); > + let principal = document.nodePrincipal; What is |document| here? I think you want doc.defaultView.top.document, like in the first hunk. @@ +616,5 @@ > } > }, > > + // Match the behaviour of nsPermissionManager > + _getHostFromPrincipal: function PH_getHostFromPrincipal(principal) { Trying to match the behavior of nsPermissionManager seems rather fragile. At least add a comment in the other place that points here, so if someone changes it, they'll know to change both. @@ +665,5 @@ > pluginInfo.pluginPermissionHost = permissionObj.host; > pluginInfo.pluginPermissionType = permissionObj.expireType; > } > else { > + pluginInfo.pluginPermissionHost = principalHost; Does this mean that enabling a plugin on an https page also enables it for the corresponding http page? :( What does this do when principalHost is "(null)"?

Comment on attachment 770323 [details] [diff] [review] Use document.nodePrincipal, fix doorhanger host handling r- for jesse's comments, but this looks like the right path!

(In reply to Jesse Ruderman from comment #13) > Why the top document rather than the document with the plugin? I guess it > keeps the UI simple, but it's not really what I want for YouTube embeds. From following the CTP re-spec, we want "activate plugins on this site". That the actual content may come from somewhere else is not really something that should matter for the user here (also, CDNs). > Trying to match the behavior of nsPermissionManager seems rather fragile. I'd rather inquire nsPermissionManager for the host, but currently there is nothing in public interfaces AFAICT. > Does this mean that enabling a plugin on an https page also enables it for > the corresponding http page? :( > > What does this do when principalHost is "(null)"? As is, browser-plugins delegates that decision to the permission manager (based on the top-document principal). |principalHost| is only for display purposes. The permissions are shared over http/https, same as before. E.g. copy-pasting a data-url in the nav bar gives a principalHost of "(null)": * "allow now"/"allow always" unblock the plugin * plugin still active after reload * plugin not active anymore when copy-pasting the url again ... which seems like sensible behavior.

Comment on attachment 770324 [details] [diff] [review] Test coverage for data URLs Review of attachment 770324 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/browser_pluginnotification.js @@ +819,5 @@ > setAndUpdateBlocklist(gHttpTestRoot + "blockNoPlugins.xml", > function() { > clearAllPluginPermissions(); > resetBlocklist(); > + prepareTest(test25a, "about:blank"); Let's move these new tests to a separate file, since browser_pluginnotification.js already runs pretty long and I don't want it to start timing out. @@ +868,5 @@ > +function test26a() { > + getTestPlugin().enabledState = Ci.nsIPluginTag.STATE_CLICKTOPLAY; > + getTestPlugin("Second Test Plug-in").enabledState = Ci.nsIPluginTag.STATE_CLICKTOPLAY; > + > + prepareTest(test26b, gHttpTestRoot + "plugin_data_url.html"); It looks like the beginning of test26a can be placed in test25d and the plugin_data_url.html page only needs to be loaded once here, instead of twice between these two functions.

Jesse, does comment 15 address your concerns?

Addressed review comments. Tests are now in browser_CTP_data_urls.js

(In reply to Georg Fritzsche [:gfritzsche] from comment #15) > From following the CTP re-spec, we want "activate plugins on this site". > That the actual content may come from somewhere else is not really something > that should matter for the user here (also, CDNs). This seems fishy. If the UI is adding permissions based on the outermost frame, the permissions checks need to match that. Is that the case? Doing permissions checks based on the outermost frame doesn't feel right and will likely lead to all sorts of weirdness.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #20) > If the UI is adding permissions based on the outermost > frame, the permissions checks need to match that. Is that the case? Yes, both permission setting and checking is baed on the top document (unless i'm missing something of course). Note that this patch should merely change to using the respective (node)principals instead of the URIs, fixing some behavior related to that.

So i guess the question becomes, can we move forward with this patch for now and follow-up with discussions on the validity of the approach later?

You didn't address or answer the second hunk of comment 15. Please file bugs for: * nsPermissionManager should expose the interface you need * nsPermissionManager should allow distinguishing between http and https

(In reply to Georg Fritzsche [:gfritzsche] from comment #21) > Yes, both permission setting and checking is baed on the top document > (unless i'm missing something of course). Can you point to the permission checking code?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #24) > (In reply to Georg Fritzsche [:gfritzsche] from comment #21) > > Yes, both permission setting and checking is baed on the top document > > (unless i'm missing something of course). > > Can you point to the permission checking code? That... > http://hg.mozilla.org/mozilla-central/annotate/17fe59f6c54a/content/base/src/ > nsObjectLoadingContent.cpp#l2974 ... and in the front-end the parts addressed by the patch (perms.addFromPrincipal() and perms.testPermissionFromPrincipal()). (In reply to Jesse Ruderman from comment #23) > You didn't address or answer the second hunk of comment 15. Right, i just fixed it with the updated patch. > Please file bugs for: > * nsPermissionManager should expose the interface you need > * nsPermissionManager should allow distinguishing between http and https Filed bug 891285 & bug 891281.

Comment on attachment 770749 [details] [diff] [review] Use document.nodePrincipal, fix doorhanger host handling, v2 Gavin, do yoiu want to review this then?

Comment on attachment 770749 [details] [diff] [review] Use document.nodePrincipal, fix doorhanger host handling, v2 I don't fully understand what notification.options.centerActions[0] is (and whether it's correct to get the host from it in the first urlbarBindings.xml hunk), and I haven't verified that this is complete, but the changes from currentURI->principal look sound (the .top thing still seems a bit odd, but I suppose it makes sense). Sorry for the delay.

Comment on attachment 770749 [details] [diff] [review] Use document.nodePrincipal, fix doorhanger host handling, v2 (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #28) > I don't fully understand what notification.options.centerActions[0] is (and > whether it's correct to get the host from it in the first urlbarBindings.xml > hunk) centerActions is used by browser-plugins.js to set the plugin data from which the code in urlbarBindings.xml builds it state. Benjamin, does the centerActions usage look proper to you?

Comment on attachment 770749 [details] [diff] [review] Use document.nodePrincipal, fix doorhanger host handling, v2 Well hrm. *Normally* the pluginPermissionHost will be the same for all the centerActions. But if there is a permission set for a superDomain, it theoretically could be different. It's an edge case, and you could solve it using gPluginHandler._getHostFromPrincipal(this.notification.browser.contentWindow.document.nodePrincipal); But it's also so weird that it's probably not worth it.

Rebased, also found it works fine with the improvement from comment 30 incorporated, so lets do this and not have to worry about edge-cases.

Rebased.

Checkin appreciated - the internet connection where i am today makes pulling the inbound updates a major pain.

Doesn't apply cleanly to inbound. Please rebase.

https://hg.mozilla.org/integration/mozilla-inbound/rev/43e5b71e1680 https://hg.mozilla.org/integration/mozilla-inbound/rev/0a1a5b7f6f8d

(In reply to Georg Fritzsche [:gfritzsche] from comment #29) > (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment > #28) > > I don't fully understand what notification.options.centerActions[0] is (and > > whether it's correct to get the host from it in the first urlbarBindings.xml > > hunk) > > centerActions is used by browser-plugins.js to set the plugin data from > which the code in urlbarBindings.xml builds it state. I don't understand the name "centerActions". Can we change it to something that makes sense?

(In reply to Dão Gottwald [:dao] from comment #36) > I don't understand the name "centerActions". Can we change it to something > that makes sense? Filed bug 896418.

https://hg.mozilla.org/mozilla-central/rev/43e5b71e1680 https://hg.mozilla.org/mozilla-central/rev/0a1a5b7f6f8d

Comment on attachment 778520 [details] [diff] [review] Use document.nodePrincipal, fix doorhanger host handling, v3 [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 880735, CtP doorhanger redesign. User impact if declined: CtP doorhanger not available at all on data: URLs, permission inheritance not working properly in other scenarios. Testing completed (on m-c, etc.): Baked on m-c for a bit, has test-coverage. Risk to taking this patch (and alternatives if risky): Slight behavior changes regarding permission inheritence, no major risk though AFAICT. String or IDL/UUID changes made by this patch: None.

Comment on attachment 778521 [details] [diff] [review] Test coverage for data URLs, v3 [Approval Request Comment] See comment 39, these are the tests.

https://hg.mozilla.org/releases/mozilla-aurora/rev/135008e6323f https://hg.mozilla.org/releases/mozilla-aurora/rev/44e40c775ff7

Georg, please don't resolve bugs until they land on m-c.

The CTP doorhanger is accessible now, but after "Allow now", the CTP overlay (Activate Java) doesn't go away only after refresh. Is it ok ?

This is not specific to data: URLs, filed bug 902882.

Thanks Georg. Verified fixed FF 24b1, 25.0a2 (2013-08-07)