1361433 - Flash shouldn't be automatically blocked on invalid urls, if plugins.flashBlock.enabled is true

Status: RESOLVED WONTFIX

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

Description

[:Felipe Gomes (needinfo for replies!)]

Right now, the Flash classification that keeps things with unchanged behavior is FlashClassification::Unknown, and that's what's returned when the pref is off, or when the end of nsDocument::PrincipalFlashClassification is reached.

However, when there's a null principal, or an invalid URL is used, we are instead returning FlashClassification::Denied, http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/dom/base/nsDocument.cpp#13265

I'm trying to change plugins.flashEnabled.enabled to true by default, but that causes tests to fail, because when running various tests [1], these cases are being hit, and that causes the test plugin to be denied. One example is test_refresh_navigator_plugins.html: The denied plugin gets removed from navigator.plugins, and so the test fails [2]

I believe the right thing to do is to return Unknown in these cases.


[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ddf532ded6f21595fe1b88b02cb964994d2d30e2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=95690206

[2] http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/dom/plugins/test/mochitest/test_refresh_navigator_plugins.html#32

Comment 1

[Robin Steuber (they/them) [:bytesized]]

Without looking into it, I am not sure if it is safe to classify invalid URLs as Unknown, but I would like to caution against classifying null principals as Unknown. The reasons for this can be seen in this [1] comment by :bz. I will quote the especially relevant parts here:

> how do we want to handle URIs (coming from a principal) without a host?  In practice this could be any of:
> 
> a) file:// URI
> b) data: (or other host-less scheme) loaded directly from URL bar.
> c) iframe sandboxed without allow-same-origin
> d) A load that had resetPrincipalsToNullPrincipal() called on its loadinfo.

We chose this option :bz proposed:

> Null principal means deny, otherwise no host means treat as unlisted.  This would disallow cases (b), (c), (d) but allow (a).

I would prefer not to change this so that a Denied domain cannot use any of these methods to prevent its Flash from being blocked.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1307604#c46

Comment 2

[Boris Zbarsky [:bzbarsky]]

Per IRC discussion, the real problem here is system principal (because the tests involved are all chrome or browser tests), and the fact that we deny for it.  And the test changes in bug 1335475 are likely to make it a non-issue.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Indeed, I just cherry-picked the test changes from bug 1335475 and sent it to try together with the plugins.flashBlock.enabled pref flip. We'll see how it goes.

There's a change in behavior here: we'll start to block Flash on system principals, but that change in behavior is intentional, so I'll close this bug.