Closed Bug 1575788 Opened 1 year ago Closed 1 year ago

The TP ON/OFF switch will be incorrectly applied when quickly changing tabs

Categories

(Firefox :: Site Identity, defect, P1)

Unspecified
All
defect

Tracking

()

VERIFIED FIXED
Firefox 70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 --- verified

People

(Reporter: andrei.purice, Assigned: timhuang)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, Whiteboard: [privacy-panel][skyline])

Attachments

(2 files)

I have tested this issue on Windows 10 64-bit, Macos 10.11/10.14.06 and Linux 18.04.2 LTS on the lastest Nightly Build 70.0a1 (2019-08-21).
This issue doesn't occur on the Beta build.

Steps :

  1. Launch the Nightly Firefox build.
  2. Have at least 2 running tabs, i.e.: www.youtube.com and www.reddit.com
  3. On both of the 2 tabs have the Tracking Protection have the same state, either ON or OFF.
  4. Change the TP from on to off and quickly change to the other tab (or off to on).
  5. Notice that the changes are applied to the 2nd tab instead of the initial one where the TP was switched.

Expected result:
Any applied switches to the TP from on/off should be done in the tab they were initially made.

Actual result :
The TP change is applied to the 2nd tab instead of the one where it was switched.

Please check the attached file for more information.

I can see that the problem here is that it changes the tab within the 500ms delay of reloading page while toggling the TP switch. So, the new TP state will be applied to the wrong tab after the delay. I think we should cache the site where we need to apply and apply it after the delay.

Assignee: nobody → tihuang
Status: NEW → ASSIGNED
Component: Tracking Protection → Site Identity and Permission Panels
Whiteboard: [privacy-panel][skyline]
Priority: -- → P1

(In reply to Tim Huang[:timhuang] from comment #1)

I can see that the problem here is that it changes the tab within the 500ms delay of reloading page while toggling the TP switch. So, the new TP state will be applied to the wrong tab after the delay. I think we should cache the site where we need to apply and apply it after the delay.

The bug is caused because of the way the code is written, FWIW:

https://searchfox.org/mozilla-central/rev/597a69c70a5cce6f42f159eb54ad1ef6745f5432/browser/base/content/browser-siteProtections.js

Then we proceed to call disableForCurrentPage() or enableForCurrentPage(). Those functions however use the selectedBrowser property (here is an example) which causes the bug.

Caching the selectedBrowser property before doing the 500ms wait and passing the cached version to the ContentBlockingAllowList APIs should fix this bug.

(In reply to :Ehsan Akhgari from comment #2)

(In reply to Tim Huang[:timhuang] from comment #1)

I can see that the problem here is that it changes the tab within the 500ms delay of reloading page while toggling the TP switch. So, the new TP state will be applied to the wrong tab after the delay. I think we should cache the site where we need to apply and apply it after the delay.

The bug is caused because of the way the code is written, FWIW:

https://searchfox.org/mozilla-central/rev/597a69c70a5cce6f42f159eb54ad1ef6745f5432/browser/base/content/browser-siteProtections.js

Then we proceed to call disableForCurrentPage() or enableForCurrentPage(). Those functions however use the selectedBrowser property (here is an example) which causes the bug.

Caching the selectedBrowser property before doing the 500ms wait and passing the cached version to the ContentBlockingAllowList APIs should fix this bug.

Another option is to accept an optional boolean shouldReload = true in the disable/enable functions, and don't reload if false is passed. Then queue the reload in setTimeout. This would avoid having to update all the tests that use disable/enable and expect a reload.

This patch fixes the issue by updating the allow list as soon as the
switch been toggled. And the reload still happens after the 500ms delay.
We cache the target tab in order to reload the correct tab in case tabs
change and reload the target tab after the delay. In additon, we won't
reload the tab if is has been closed since it is totally unnecessary.

(In reply to Nihanth Subramanya [:nhnt11] from comment #3)

(In reply to :Ehsan Akhgari from comment #2)

(In reply to Tim Huang[:timhuang] from comment #1)

I can see that the problem here is that it changes the tab within the 500ms delay of reloading page while toggling the TP switch. So, the new TP state will be applied to the wrong tab after the delay. I think we should cache the site where we need to apply and apply it after the delay.

The bug is caused because of the way the code is written, FWIW:

https://searchfox.org/mozilla-central/rev/597a69c70a5cce6f42f159eb54ad1ef6745f5432/browser/base/content/browser-siteProtections.js

Then we proceed to call disableForCurrentPage() or enableForCurrentPage(). Those functions however use the selectedBrowser property (here is an example) which causes the bug.

Caching the selectedBrowser property before doing the 500ms wait and passing the cached version to the ContentBlockingAllowList APIs should fix this bug.

Another option is to accept an optional boolean shouldReload = true in the disable/enable functions, and don't reload if false is passed. Then queue the reload in setTimeout. This would avoid having to update all the tests that use disable/enable and expect a reload.

While reviewing I realized neither of these solutions will really fully fix the issue.

The deterministic way to handle this is to listen for TabSelect events and handle whichever happens first - TabSelect or 500ms timeout.

Pushed by tihuang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1994cc8fe32
Fix the issue that the TP state been applied wrongly if quickly change tabs after toggling the TP switch for protections panel. r=nhnt11
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

Hello,

I have re-tested this issue and it doesn't occur anymore on any of the platforms. Changing the status to Verified->Fixed for the latest Nightly 70.

Status: RESOLVED → VERIFIED

Hmm, this changed the user-visible behaviour of the panel. Before this patch we would close the pop-up immediately after the radio button was clicked, but now we wait 500ms and leave the pop-up open during that time, and then proceed to close the pop-up. (Obviously this was the direct result of the change of the flow of the code in this patch...)

Was that intentional?

Yes, I think this is the correct behavior. And in fact, we also implemented this ' wait 500ms and then close the panel' behavior for the old behavior. The 500ms waiting exists before this patch.

The old behavior is

  • Click the TP switch
  • Wait 500ms
  • Set the allow list.
  • Close the panel.
  • Reload the current page.

The new behavior is

  • Click The TP switch.
  • Set the allow list and cache the selected tab.
  • Wait 500ms or wait until 'TabSelect' event happens.
  • Close the panel.
  • Reload the cached tab.

To be honest, I have no clue what changes the user perception because they all do 'wait 500ms and then close the panel'. The major differences here are the timing of setting the allow list is different and new behavior caches tab and then reloads it instead of the current tab. Perhaps these two changes cause different user-visible behavior.

(In reply to Tim Huang[:timhuang] from comment #10)

Yes, I think this is the correct behavior. And in fact, we also implemented this ' wait 500ms and then close the panel' behavior for the old behavior. The 500ms waiting exists before this patch.

The old behavior is

  • Click the TP switch
  • Wait 500ms
  • Set the allow list.
  • Close the panel.
  • Reload the current page.

The new behavior is

  • Click The TP switch.
  • Set the allow list and cache the selected tab.
  • Wait 500ms or wait until 'TabSelect' event happens.
  • Close the panel.
  • Reload the cached tab.

To be honest, I have no clue what changes the user perception because they all do 'wait 500ms and then close the panel'. The major differences here are the timing of setting the allow list is different and new behavior caches tab and then reloads it instead of the current tab. Perhaps these two changes cause different user-visible behavior.

This seems accurate. I do not think there is any user-visible behavioral change as a result of this patch. The 500ms delay was indeed present since bug 1551902 I believe.

Regressions: 1576832

Yes, you're right. I think I was misunderstanding the previous behavior. Please ignore me. :-)

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression
You need to log in before you can comment on or make changes to this bug.