Opened 2 years ago

Closed 2 years ago

Last modified 23 months ago

#5748 closed defect (fixed)

No confirmation dialog shown in options page when using subscribe links

Reported by: greiner Assignee: mjethani
Priority: P1 Milestone: Adblock-Plus-3.0-for-Firefox
Module: User-Interface Keywords:
Cc: arthur, trev, mjethani, sebastian Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29536764/

Description

Environment

Ubuntu 16.04
Chrome 61.0.3163.91
Adblock Plus 1.13.3.1838

How to reproduce

  1. Go to adblockplus.org/subscriptions
  2. Click on any of the links to subscribe to a filter list
  3. Repeat the previous step a few times

Observed behaviour

Options page is opened in a new tab not showing a confirmation dialog for the selected filter list.

Expected behaviour

Options page is opened in a new tab showing a confirmation dialog for the selected filter list.

Further information

This regression can be traced back to d81ef032a492 (see #5574).

Change History (10)

comment:1 follow-up: Changed 2 years ago by sebastian

  • Cc mjethani sebastian added

I can reproduce it. However, I cannot see how this is directly related to the jQuery update. It rather seems to be a timing issue, which itself wouldn't be new. Whenever it is happening, the options page doesn't receive the app.respond message. It seem that is has been sent before the options page starts listening for messages. My best guess, why this cannot be reproduced before the jQuery update, is that the new version takes longer too load, or more likely to execute, since we don't sent the app.respond message before the document completed loading.

If my assumptions are correct, we should also keep that in mind for #5587, and it would be another argument for my suggestion there to make the options page request these information rather than pushing these messages to the options page. That might also fix this issue then.

comment:2 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 5748 - Backed out changeset d81ef032a492

comment:3 in reply to: ↑ 1 ; follow-up: Changed 2 years ago by greiner

Replying to sebastian:

If my assumptions are correct, we should also keep that in mind for #5587, and it would be another argument for my suggestion there to make the options page request these information rather than pushing these messages to the options page. That might also fix this issue then.

Fortunately, the UI already sends the "app.listen" message to start listening to those types of messages. So it'd be great if we made use of that to determine when it's safe to forward pending "app.respond" messages.

comment:4 in reply to: ↑ 3 Changed 2 years ago by mjethani

Replying to greiner:

Replying to sebastian:

If my assumptions are correct, we should also keep that in mind for #5587, and it would be another argument for my suggestion there to make the options page request these information rather than pushing these messages to the options page. That might also fix this issue then.

Fortunately, the UI already sends the "app.listen" message to start listening to those types of messages. So it'd be great if we made use of that to determine when it's safe to forward pending "app.respond" messages.

Noted.

Since the current fix for #5587 definitely forwards app.respond only after the inner frame's load event, this issue should not occur with that fix either. But I'll look into enhancing it based on Sebastian's suggestion.

comment:5 Changed 2 years ago by mjethani

  • Owner set to mjethani

comment:6 follow-up: Changed 2 years ago by sebastian

Well, currently we don't use any frames on the options page, and the background page waits until tab.status == "complete", which as far as I understand is the equivalent of waiting for the load event in a content script. So as far as I understand, if this issue no longer occurs with the current fix for #5587, this is for another reason. So my suspicion is that the the ready state might transition to complete before the the code registering the message listener has been executed.

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

comment:7 in reply to: ↑ 6 Changed 2 years ago by mjethani

Replying to sebastian:

Well, currently we don't use any frames on the options page, and the background page waits until tab.status == "complete", which as far as I understand is the equivalent of waiting for the load event in a content script. So as far as I understand, if this issue no longer occurs with the current fix for #5587, this is for another reason. So my suspicious is that the the ready state might transition to complete before the the code registering the message listener has been executed.

My bad, it did also occur on the options page with the frame-based solution.

The specific reason why this occurs is that the background page receives subscriptions.add before it has received app.listen from the options page. My guess as to why this happens with the jQuery update is that the way jQuery orders events/listeners has changed. Anyway, we shouldn't be relying on what jQuery does internally. I've uploaded another patch to #5587, which takes care of this bug as well.

comment:8 Changed 2 years ago by mjethani

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

comment:9 Changed 2 years ago by mjethani

  • Milestone set to Adblock-Plus-for-Chrome-Opera-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

Fixed in 3c76db9d2029.

comment:10 Changed 23 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Fixed. Adding many filter lists from subscription links worked as expected.

Firefox subscription links are covered by #5695.

ABP 1.13.4.1903
Chrome 49 / 62 / Windows 10
Opera 36 / 48 / Windows 10

Note: See TracTickets for help on using tickets.