Options page sometimes not updated with newly added filters

[Ross]

Environment

Reproduced in:
ABP 1.11.0.1593
Safari 6.0 / OS X 10.8
Safari 7.1.8 / OS X 10.9.5
Safari 8.0.6 / OS X 10.10.3
Chrome 48 / Ubuntu 14.04

Could not reproduce in:
ABP 1.11.0.1592
(Same platforms as above)

Filters:
EasyList English

How to reproduce

1. Install Adblock Plus for Chrome development build 1593.
2. Open the options page.
3. Switch to the "Add your own filters" tab.
4. Add the filter test.com

Observed behaviour

The added filter does not appear in the list of filters. Switch to the "Filter lists" tab and observe there is a new subscription listed with no name. Note that if you close and re-open the options page the bug can no longer be reproduced.

(If you can't reproduce then uninstall and reinstall the extension before trying again.)

Expected behaviour

The added filter should immediately show in the list of filters. There should not be a subscription with a blank name added.

Notes

• This seems to be a regression caused by #3870 as the issue has not been reproduced in earlier builds (<= 1592).
• The problem is caused by the fact that sometimes subscriptions.added is dispatched instead of filters.added when the user adds a filter. We need to consistently dispatch filters.added and then make sure that the interface ignores special subscriptions that are created incidentally during the process of adding filters.
• See #3917 for the same problem with the new options page.

[kzar]

I have finally managed to reproduce this, it appears that sometimes (especially just after the extension is installed) added filters are wrongly considered to be subscriptions. When the options page is reloaded the problem goes away. I'm looking into this now.

[kzar]

When the bug happens the FilterNotifier listener in adblockplusui/messageResponder.js is firing for the subscription.added event instead of filters.added.

[sebastian]

That's not really wrong. It's just that when the very first custom filter is added, there is no SpecialSubscription for it yet. So instead of adding the filter to an existing subscription, a new subscription with that filter is added. Hence you get subscription.added instead of filter.added. This needs to be handled by the UI of course. It seems that this has to be fixed for the new options page as well.

[kzar]

Yea, just figured that out looking at the adblockpluscore/lib/filterStorage.js code, specifically the addFilter function.

Looks like the options page should check in the onSubscriptionMessage handler if the added subscription is a special one or not (how?) and if so it should instead fetch the associated filter and call the onFilterMessage function with that.

It seems that this has to be fixed for the new options page as well.

Yes, I'll file an issue shortly.

[sebastian]

Without further changes to the message responder, this is how it could be done:

if (subscription.url.indexOf("~user") == 0)
{
  ext.backgroundPage.sendMessage(
    {
      type: "filters.get",
      subscriptionUrl: subscription.url
    },
    function(filters)
    {
      for (var i = 0; i < filters.length; i++)
        onFilterMessage("added", filters[i]);
    }
  );
}

[kzar]

Without further changes to the message responder...

I think I'd rather include the downloadable and special properties when converting the subscriptions. That way it's more consistent with the subscriptions.get message.

[sebastian]

Perhaps, a better idea: Changing FilterStorage to first create an empty subscription and then add the filter to it, so that we always get a filters.added notification for each new custom filter. This would make the logic way simpler. And shouldn't break any existing code.

[kzar]

Perhaps, a better idea...

Yea, sounds good to me. Just having a look at it now.

[kzar]

Replying to sebastian:

Perhaps, a better idea: Changing FilterStorage to first create an empty subscription and then add the filter to it, so that we always get a filters.added notification for each new custom filter. This would make the logic way simpler. And shouldn't break any existing code.

​Wladimir didn't like that idea (well a similar one) and would rather we handled special subscriptions from the options page. I'll update the reviews shortly.

[abpbot]

A commit referencing this issue has landed:
​https://hg.adblockplus.org/adblockpluschrome/rev/9a24ba5f0b74