870553 - A blocklist/whitelist for Shumway

Status: RESOLVED FIXED

(Firefox Graveyard :: Shumway, defect, P1)

Description

[Geoff Brown [:gbrown]]

The initial Shumway release will likely use a built-in, fixed list of supported sites: Sites serving Flash content that is known to be compatible with that version of Shumway. Over time, the list of supported sites will grow; a flexible, scalable solution is desired for determining which sites are, or perhaps which sites are not, supported. Ideally, the list could be updated independent of browser releases/updates.

Comment 1

[Geoff Brown [:gbrown]]

:jet -- Can you comment further on requirements?

Comment 2

[Jet Villegas (inactive)]

The list may include domains/sites and SWF file URL's.

Comment 3

[Geoff Brown [:gbrown]]

I think we can leverage nsBlocklistService.js and extend blocklist.xml to do this, similar to how bug 625160 extended the blocklist for graphics.

Comment 4

[Geoff Brown [:gbrown]]

Attachment: (1) Extend nsBlocklistService for Shumway

This patch extends nsBlocklistService to support a list of sites supported by Shumway. In the short-term, the Shumway project needs a "white-list" -- a relatively concise list of known web locations serving flash content that Shumway can successfully decode and display. In the long-term, a "blocklist" will likely be more appropriate: all content will be supported, with hopefully just a few exceptions.

The patch allows for a new section -- <shumItems> -- in the blocklist.xml. Each <shumItem> should contain a "uri" attribute which may be a simple string or a Javascript regular expression. By default, a match against a <shumItem> uri means that the site is blocked. To allow for convenient specification of a white-list, a "type" attribute may be present; if type="allow", a match indicates that the site is not blocked (supported by Shumway).

The patch adds a single method to nsIBlocklistService -- getShumwayBlocklistState(in AString uri). If the specified uri does not match any Shumway blocklist entries or matches an explicit "allow" entry, STATE_NOT_BLOCKED is returned, indicating that the site is supported by Shumway; if the specified uri matches a "deny" Shumway blocklist entry, STATE_BLOCKED is returned and Shumway should not attempt to decode swf files at this location.

Comment 5

[Geoff Brown [:gbrown]]

Attachment: (2) xpcshell tests for Shumway blocklist

Adds xpcshell tests to exercise the new api. Includes sample blocklist files demonstrating both a whitelist and a blocklist approach.

Comment 6

[Geoff Brown [:gbrown]]

Comment on attachment 761482 [details] [diff] [review]
(1) Extend nsBlocklistService for Shumway

:yury -- Does this seem reasonable? Any missing features? (Also have a look at the  xml files in the test patch to see blocklist examples.)

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Yury, feedback ping?

Comment 8

[Yury Delendik (:yury)]

Comment on attachment 761482 [details] [diff] [review]
(1) Extend nsBlocklistService for Shumway

Review of attachment 761482 [details] [diff] [review]:
-----------------------------------------------------------------

In overall the solution looks good.

I would introduce third state 'probably' for sites without information. So returned states will be:
- whitelisted (STATE_NOT_BLOCKED)
- blacklisted (STATE_BLOCKED)
- no info/probably (? STATE_SOFTBLOCKED)

There is missing functionality mentioned in comment 2. That will require adding second parameter (pageUrl?) to the getShumwayBlocklistState method.

Few drive-by notes below.

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +390,5 @@
> +      this._loadBlocklist();
> +    return this._getShumwayBlocklistState(uri, this._shumwayEntries);
> +  },
> +
> +  _getShumwayBlocklistState: function Blocklist_getShumwayBlocklistState(uri, shumwayEntries) {

This function can be inlined into getShumwayBlocklistState

@@ +411,5 @@
> +    var closest = null;
> +    for (let entry of aShumwayEntries) {
> +      if (entry.matches instanceof RegExp &&
> +          entry.matches.test(aUri) &&
> +          (closest == null ||  

Nit: spaces at the end

@@ +685,5 @@
>  #      </pluginItems>
> +#      <shumwayItems>
> +#        <shumwayItem uri="some uri regex" type="allow">
> +#        </shumwayItem>
> +#      </shumwayItems>

Incorrect example: shumItems and shumItem must be used (I would prefer shumwayItems and shumayItem though)

@@ +836,5 @@
>    },
>  
> +  _handleShumwayItemNode: function Blocklist_handleShumwayItemNode(blocklistElement, result) {
> +    var blockEntry = {
> +      matches: {},

Avoid construction of the empty object since the field will be assigned below, e.g. by using |matches: null,|

::: xpcom/system/nsIBlocklistService.idl
@@ +92,5 @@
> +   *          A URI under consideration for Shumway redirection.
> +   * @returns The STATE constant.
> +   *          STATE_NOT_BLOCKED means that Shumway should decode this swf.
> +   */
> +  unsigned long getShumwayBlocklistState(in AString uri);

Update the uuid() class attribute above

Comment 9

[Geoff Brown [:gbrown]]

Attachment: (1) Extend nsBlocklistService for Shumway

Updated for :yury's feedback, except for:

> There is missing functionality mentioned in comment 2. That will require
> adding second parameter (pageUrl?) to the getShumwayBlocklistState method.

I had interpreted comment 2 differently. After discussing with yury on irc, I agree that a change is required -- another patch on the way.

Comment 10

[Geoff Brown [:gbrown]]

Attachment: (2) xpcshell tests for Shumway blocklist

Comment 11

[Geoff Brown [:gbrown]]

Attachment: (1) Extend nsBlocklistService for Shumway

In this version, each blocklist entry has 2 urls (which may be regular expressions): a host and a swf. nsIBlocklistService.getShumwayBlocklistState(host, swf) returns the blocked/soft-blocked/not-blocked state associated with the closest matching blocklist entry.

:yury -- Is this what you had in mind? Does Shumway still need this functionality?

Comment 12

[Geoff Brown [:gbrown]]

Attachment: (2) xpcshell tests for Shumway blocklist

Comment 13

[Geoff Brown [:gbrown]]

https://tbpl.mozilla.org/?tree=Try&rev=33afa88e6a5c

Comment 14

[Geoff Brown [:gbrown]]

Just tidying my dashboard -- happy to take this back if needed.

Comment 15

[David Chan [:dchan]]

From irc discussions with till and yury, there are 3 states to handle for the blocklist service

STATE_BLOCKED
Run default flashplayer if installed

STATE_NOT_BLOCKED
Run Shumway

STATE_SOFTBLOCKED
Run heuristics from bug 1038444 to determine whether to use Shumway or not

There is an open question on how to handle click-to-play. We can provide buttons for running either Flash or Shumway. Alternatively we could check against the service to see if any of the above states are true.

I will have to research how the blocklist service handles redirects. AFAIK the service has only been used on "static" items, plugins / extensions which can't easily mutate. A redirect from a whitelisted URL to another site for flash content may result in unexpected results. However the worst case should be that the SWF doesn't work.

Comment 16

[Chris Peterson [:cpeterson]]

We need whitelist support to identify the Facebook video player.

Comment 17

[Yury Delendik (:yury)]

Attachment: Adds whitelist to PlayPreview API

Extends functionality of the bug 839714: adds a whitelist of the page/object URIs. PlayPreview will be replaced soon, but this functionality is essential for first stages of the Shumway project. In the future this will be replaced.

The idea is to bypass PlayPreview (and Shumway) logic completely for page and object URLs that are not in the list. This will allow Shumway to not affect the performance of the sites it cannot handle yet.

The patch contains code that passes whitelist that is stored in "shumway.swf.whitelist" configuration variable and currently requires shumway/firefox to be restarted to be updated in PlayPreview (adding functionality to observe the changes in the configuration considered is out of scope of this bug and could be addressed later is necessary).

Comment 18

[Josh Aas]

Comment on attachment 8558140 [details] [diff] [review]
Adds whitelist to PlayPreview API

Review of attachment 8558140 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsObjectLoadingContent.cpp
@@ +3175,5 @@
> +      mBaseURI->GetSpec(baseSpec);
> +    }
> +    bool whitelisted;
> +    playPreviewInfo->CheckWhitelist(baseSpec, uriSpec, &whitelisted);
> +    isPlayPreviewSpecified = whitelisted;

Why don't you just put the result into isPlayPreviewSpecified in the first place, skip "whitelisted"?

::: dom/plugins/base/nsPluginPlayPreviewInfo.cpp
@@ +73,5 @@
> +
> +  // Parses whitelist as comma separated entries of
> +  //   [@page_url object_url|@page_url|object_url]
> +  // where page_url and object_url pattern matches for aPageURI
> +  // and aObjectURI, and performs matching as the same time.

Is there really no code we can re-use instead of adding more C++ string parsing?

@@ +80,5 @@
> +  mWhitelist.EndReading(end);
> +
> +  nsAutoCString pageURI(aPageURI);
> +  nsAutoCString objectURI(aObjectURI);
> +  nsACString::const_iterator p = start, q, r;

Better names for these variables would make this code easier to read. The brevity of a single character isn't worth it here, IMHO.

@@ +83,5 @@
> +  nsAutoCString objectURI(aObjectURI);
> +  nsACString::const_iterator p = start, q, r;
> +  int matchResult;
> +  while (p != end) {
> +    bool matched = true;

Surprises me to see matched equal true by default for a whitelist.

Comment 19

[Yury Delendik (:yury)]

Attachment: Patch for checkin

(In reply to Josh Aas (Mozilla Corporation) from comment #18)
> Comment on attachment 8558140 [details] [diff] [review]
> Adds whitelist to PlayPreview API
> 
> Review of attachment 8558140 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsObjectLoadingContent.cpp
> @@ +3175,5 @@
> > +      mBaseURI->GetSpec(baseSpec);
> > +    }
> > +    bool whitelisted;
> > +    playPreviewInfo->CheckWhitelist(baseSpec, uriSpec, &whitelisted);
> > +    isPlayPreviewSpecified = whitelisted;
> 
> Why don't you just put the result into isPlayPreviewSpecified in the first
> place, skip "whitelisted"?

Fixed.

> 
> ::: dom/plugins/base/nsPluginPlayPreviewInfo.cpp
> @@ +73,5 @@
> > +
> > +  // Parses whitelist as comma separated entries of
> > +  //   [@page_url object_url|@page_url|object_url]
> > +  // where page_url and object_url pattern matches for aPageURI
> > +  // and aObjectURI, and performs matching as the same time.
> 
> Is there really no code we can re-use instead of adding more C++ string
> parsing?
> 

Pattern has a custom syntax, and I could not find anything like in the m-c yet. Also it will give us better used memory control this way.

> @@ +80,5 @@
> > +  mWhitelist.EndReading(end);
> > +
> > +  nsAutoCString pageURI(aPageURI);
> > +  nsAutoCString objectURI(aObjectURI);
> > +  nsACString::const_iterator p = start, q, r;
> 
> Better names for these variables would make this code easier to read. The
> brevity of a single character isn't worth it here, IMHO.
> 

Better names added/changed.

> @@ +83,5 @@
> > +  nsAutoCString objectURI(aObjectURI);
> > +  nsACString::const_iterator p = start, q, r;
> > +  int matchResult;
> > +  while (p != end) {
> > +    bool matched = true;
> 
> Surprises me to see matched equal true by default for a whitelist.

Changed to ignore empty match entries.


https://treeherder.mozilla.org/#/jobs?repo=try&revision=997e95f2767b

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/6ffcd3990378

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And a followup to bump some UUIDs to hopefully fix all of the mass failed tests:
https://hg.mozilla.org/integration/fx-team/rev/19e519d72708

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ffcd3990378
https://hg.mozilla.org/mozilla-central/rev/19e519d72708