Move the logic determining whether the nested frame should be whitelisted into js core

[sergz]

Let's say there is a page with the structure Root > FrameA > FrameB > FrameC. Currently if one of the frame in the frame hierarchy is whitelisted either by $document, by $elemhide or by sitekey then all nested frames are also whitelisted. Please check for example the implementation in Chrome extension.

  var isWhitelisted = exports.isWhitelisted = function(url, parentUrl, type, key)
  {
    var filter = defaultMatcher.matchesAny(stripFragmentFromURL(url), type || "DOCUMENT", extractHostFromURL(parentUrl || url), false, key);
    return filter instanceof WhitelistFilter ? filter : null;
  };
  var isFrameWhitelisted = exports.isFrameWhitelisted = function(page, frame, type)
  {
    for (; frame != null; frame = frame.parent)
    {
      var key = getKey(page, frame);
      if (isWhitelisted(frame.url, (frame.parent || {}).url, type, key))
      {
        return true;
      }
    }
    return false;
  };

which is called from

if (!isFrameWhitelisted(sender.page, sender.frame, "DOCUMENT") &&
    !isFrameWhitelisted(sender.page, sender.frame, "ELEMHIDE"))
{
  ....
  selectors = ElemHide.getSelectorsForDomain(host, false);

It's clear that such logic (with tests) should be shared by all platforms because it's not documented anywhere and raises a lot of questions. Designing the API please take into account that the best place to obtain the frame structure is the browser extension.

See also

#1933

[sebastian]

I don't see how such logic could be shared (in a reasonable way) as it is closely tied to platform-specific APIs, i.e. the page/frame abstraction in ext (which maps to the Chrome/WebExt API), in the example above. However, with the legacy Firefox extension being deprecated, and all browser extensions, now, using the same code, this seems to have become less relevant, anyway.

[sergz]

Replying to sebastian:

I don't see how such logic could be shared (in a reasonable way) as it is closely tied to platform-specific APIs, i.e. the page/frame abstraction in ext (which maps to the Chrome/WebExt API), in the example above.

In order to share this logic the core API should accept a list (can be a generator if it's convenient in web extensions) of parent frame URLs and keys. The latter should be obtained using platform-specific API. The rest, namely two calls with "DOCUMENT" and "ELEMHIDE" (BTW, which is now one call with logical OR) and isWhitelisted mentioned in the description (BTW, the current code changed since the creation of the issue but likely not in all projects based on the adblockpluscore) should be in core.

However, with the legacy Firefox extension being deprecated, and all browser extensions, now, using the same code, this seems to have become less relevant, anyway.

It's still relevant because there is also libadblockplus.

[sebastian]

Creating an array of the URLs of all frames up the hierarchy, just so that the core code can iterate over it seems to add some (overall) complexity. Also since this is a hotspot we might rather want to avoid creating unnecessary temporary objects here. And wouldn't it be a limitation of Chrome (which unfortunately got adopted by Edge and Firefox as well) that we have to respond synchronously when blocking requests, we would even retrieve frame information on demand using asynchronous APIs rather than keeping a copy of the frame hierarchy in our data structures.

But it seems what we should do at least is properly documenting how the core APIs are meant to be used, so that they are integrated consistently across libadblockplus and adblockpluschrome.

[sergz]

Replying to sebastian:

Creating an array of the URLs of all frames up the hierarchy, just so that the core code can iterate over it seems to add some (overall) complexity. Also since this is a hotspot we might rather want to avoid creating unnecessary temporary objects here. And wouldn't it be a limitation of Chrome (which unfortunately got adopted by Edge and Firefox as well) that we have to respond synchronously when blocking requests, we would even retrieve frame information on demand using asynchronous APIs rather than keeping a copy of the frame hierarchy in our data structures.

Sorry, I'm not sure that I understood it correctly. Let's the consider how it could be done with the current code of chrome extension.
Currently from the content script we send ​the asynchronous message ​`elemhide.getSelectors` to the background page which synchronously ​processes it by calling checkWhitelisted with different arguments and returning selectors. If we move the functionality of ​`checkWhitelisted` (and its dependencies like match) and of ​this piece into the core and pass a generator function (as well as some equivalent of getKey) which allows to iterate through frame parents then nothing really changes in the chrome extension. However it will keep the logic which is changing in the core and allow to re-use and keep updated this functionality on different platforms.

But it seems what we should do at least is properly documenting how the core APIs are meant to be used, so that they are integrated consistently across libadblockplus and adblockpluschrome.

That's for sure, e.g. we could document that one has to implement what is done in ​request blocker, namely to check that the page is not whitelisted and the way of obtaining the value of specificOnly which is BTW again a call of checkWhitelisted. If the implementation of request blocker gets more sophisticated with the time then I would say it also should be moved into the core, but for present I think there is no need in it.

[sebastian]

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the ​new issue tracker.