Opened on 09/21/2017 at 04:12:30 PM
Closed on 10/05/2017 at 06:31:21 PM
Last modified on 11/02/2017 at 02:02:03 PM
#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): | |||
Description
Environment
Ubuntu 16.04
Chrome 61.0.3163.91
Adblock Plus 1.13.3.1838
How to reproduce
- Go to adblockplus.org/subscriptions
- Click on any of the links to subscribe to a filter list
- 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).
Attachments (0)
Change History (10)
comment:1 follow-up: ↓ 3 Changed on 09/25/2017 at 06:51:02 PM by sebastian
- Cc mjethani sebastian added
comment:2 Changed on 09/25/2017 at 07:31:33 PM by abpbot
A commit referencing this issue has landed:
Issue 5748 - Backed out changeset d81ef032a492
comment:3 in reply to: ↑ 1 ; follow-up: ↓ 4 Changed on 09/26/2017 at 11:38:09 AM 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 on 09/26/2017 at 12:05:33 PM 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 on 09/26/2017 at 02:39:50 PM by mjethani
- Owner set to mjethani
comment:6 follow-up: ↓ 7 Changed on 09/26/2017 at 08:29:37 PM 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.
comment:7 in reply to: ↑ 6 Changed on 09/26/2017 at 08:45:09 PM 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 on 09/27/2017 at 01:29:45 PM by mjethani
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:9 Changed on 10/05/2017 at 06:31:21 PM 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 on 11/02/2017 at 02:02:03 PM 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
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.