Opened 4 years ago

Closed 4 years ago

#3915 closed defect (fixed)

Options page sometimes not updated with newly added filters

Reported by: Ross Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-1.12-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: sebastian, greiner Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29339717/

Description (last modified by kzar)

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.

Attachments (1)

optionspage.mp4 (1.3 MB) - added by Ross 4 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 4 years ago by kzar

  • Cc sebastian added
  • Component changed from Unknown to Platform
  • Owner set to kzar
  • Priority changed from P3 to Unknown
  • Summary changed from Open options page is sometimes not updated to Options page sometimes not updated with newly added filters

comment:2 Changed 4 years ago by Ross

  • Description modified (diff)

comment:3 Changed 4 years ago by kzar

  • Description modified (diff)

Changed 4 years ago by Ross

comment:4 follow-up: Changed 4 years ago by kzar

  • Description modified (diff)
  • Priority changed from Unknown to P2
  • Ready set

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.

comment:5 Changed 4 years ago by kzar

  • Description modified (diff)

comment:6 Changed 4 years ago by kzar

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

comment:7 in reply to: ↑ 4 Changed 4 years ago by 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.

comment:8 Changed 4 years ago by 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.

comment:9 Changed 4 years ago by 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]);
    }
  );
}

comment:10 Changed 4 years ago by kzar

  • Cc greiner added
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

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.

comment:11 Changed 4 years ago by kzar

  • Review URL(s) modified (diff)

comment:12 Changed 4 years ago by kzar

  • Blocking 3917 added

comment:13 follow-up: Changed 4 years ago by 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.

Last edited 4 years ago by sebastian (previous) (diff)

comment:14 Changed 4 years ago by kzar

Perhaps, a better idea...

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

comment:15 Changed 4 years ago by kzar

  • Review URL(s) modified (diff)

comment:16 Changed 4 years ago by kzar

  • Review URL(s) modified (diff)

comment:17 Changed 4 years ago by kzar

  • Review URL(s) modified (diff)

comment:18 Changed 4 years ago by kzar

  • Description modified (diff)

comment:19 in reply to: ↑ 13 Changed 4 years ago by 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.

comment:20 Changed 4 years ago by kzar

  • Blocking 3917 removed
  • Description modified (diff)
  • Review URL(s) modified (diff)

comment:21 Changed 4 years ago by abpbot

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

comment:22 Changed 4 years ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.